Skip to content

#3976 - PNG/SVG: draw brackets and labels of copolymers, components, mixtures and formulations - #4053

Open
konstantin-mordovskih wants to merge 9 commits into
masterfrom
3976-render-sgroup-brackets
Open

konstantin-mordovskih wants to merge 9 commits into
masterfrom
3976-render-sgroup-brackets

Conversation

@konstantin-mordovskih

@konstantin-mordovskih konstantin-mordovskih commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator

Copolymers, components, mixtures and formulations are drawn with their brackets and a label, and every bracketed S-group is drawn by the same rules in PNG, SVG, PDF and EMF, with no option.

  • Where: SGroupRenderer with one subclass per type (core/render2d/render_sgroups.h) replaces the type branches of _initSGroups. The base class draws by the rules of the ticket, a subclass overrides what differs; SGroupCanvas is all a drawer sees of the molecule drawing, SGroupBrackets holds the bracket rules in page coordinates.
  • How: brackets come from the structure. A group without a pair (none, one, or a zero-length one) gets one from SGroupLayout on the renderer's copy of the molecule, measured in the drawn bond and at the font of the drawing. Hooks are BRACKET_HOOK (0.15 bond lengths) and turn towards the atoms; in a pair a hook is at most a third of the gap, so a narrow pair stays open. The label stands at the lower end of the closing bracket, found by position for any direction, LABEL_OFFSET (0.05) to the right; the repeat pattern (hh, eu) at its upper end. Labels: n or the file's, co/alt/ran/blk, c/cN, mix, f, the multiplier of a multiple group.
  • Kept as before: the brackets of generic groups and superatoms (the renderer's own pair when the file has fewer than two, hooks 0.15 of the bracket), the superatom label, the data values, the atom colouring by render-atom-color-property.
  • Changed on purpose: labels and repeat patterns stand outside the bracket ends, as the ticket says. A bracketed group with one bracket in the file gets a pair. A generic group whose pair runs the other way turns its hooks towards its atoms, like every group. A repeating unit with an empty label (KET "subscript": "") shows n. Generic and multiple groups with three or more brackets draw all of them. An unnamed superatom gets no empty text item, so its picture is a few pixels smaller.
  • Also fixed: a data S-group without a field name crashed the renderer when render-atom-color-property was set.
  • Tests: rendering/sgroup_brackets.py, 47 SVGs compared with compare_diff: the 30 structures of layout/sgroup_brackets.py; 9 MOL variants (subtypes, no SCN, swapped, horizontal, turned, three brackets, one bracket); CXSMILES without brackets at bond lengths 1 and 2, at render-font-size 80, nested, of one unlabelled atom, and with ids after an atom is removed; a multiple group; the unnamed data group. The PNG references of 19 pictures with repeating units or multiple groups are updated for macOS and Linux.
  • Not covered: brackets placed on load (MOL or KET without bracket coordinates) stay in molecule units at the default font, as Layout: compute S-group brackets after layout, clean-up and KET loading #3974 places them. Where the renderer stretches the bonds, hooks and offsets keep the size of the font. Nothing keeps a label clear of a neighbouring bracket, by the ticket.

Related: #3974, #3975.

Generic request

  • PR name follows the pattern #1234 - issue name
  • branch name does not contain '#'
  • base branch (master or release/xx) is correct
  • PR is linked with the issue
  • task status changed to "In Review"
  • code follows product standards
  • regression tests updated
  • documentation updated

@NikolaiBalabanov NikolaiBalabanov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — moving the S-group drawing into one class per type made the rules easy to check, and they hold for brackets that come from the file: closing bracket by position for swapped, turned, horizontal and three-bracket input, hooks towards the atoms, labels at the lower and repeat patterns at the upper end, all pinned by SVG references. I built master and this solution ran both:

  • integration harness on this solution: all test passed on this solution and the new test crashes master on the unnamed data group with render-atom-color-property — the crash this solution fixes;
  • core, C API and C++ GTest suites pass;
  • an expanded superatom and a generic group without brackets draw exactly as on master; a SMILES without coordinates gets its brackets after the renderer's layout;
  • mutating the renderer's rules is caught by the tests in 17 of 19 cases (the gap is in the comment on the test).

What I'd like changed: every length this solution adds to the picture is measured in the input's coordinates rather than in drawn bonds, so the hooks, the label offset and the brackets the renderer places depend on the bond length of the file — on a CDXML in the repository the hooks are twice the ticket's value, and on a CXSMILES with bonds of 2.0, or at twice the default render-font-size, the brackets run through the atom labels. A repeating unit with one bracket in the file has lost its second one, and the copy of the molecule now taken for every group without brackets shows renumbered bond ids. The renderer's own bracket placement is covered by a single picture.

Two behaviour changes that look right by the ticket but are not in the description: a generic group whose stored pair runs the other way used to have its hooks outwards and now has them towards its atoms; a repeating unit whose label is an empty string (KET "subscript": "") used to be drawn without a label and now shows n.

One question for the ticket rather than the code: a repeating unit of a single atom without a label, e.g. *C*|(0,0,;0.866,0.5,;1.732,0,),Sg:n:1:n:ht|, gets brackets 0.2 bond apart with hooks of 0.15 turned towards each other, so they overlap and draw a closed box. The .memory-bank records it as a limitation; since poly(methylene) –[CH2]n– is often drawn that way, it deserves a decision in #3976 and a fixture either way.

Comment thread core/render2d/src/render_internal.cpp Resolved
Comment thread core/render2d/src/render_sgroups.cpp Resolved
Comment thread core/render2d/src/render_sgroups.cpp Resolved
Comment thread core/render2d/src/render_internal.cpp Resolved
Comment thread core/render2d/src/render_sgroups.cpp Resolved

@NikolaiBalabanov NikolaiBalabanov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working through every thread — I rebuilt this solution and checked each fix with its original input; everything looks promising and reliable. The pair the renderer places now follows the drawn bond and render-font-size, hooks and offsets are in drawn bonds, a single bracket becomes a pair, the one-atom unit stays open, ids survive the copy, and the duplicates and tracker references are gone. Integration harness and the core, C API and C++ GTest suites pass.

Still open:

  • the third-of-the-gap cap also shortens the hooks of generic groups and superatoms, against "kept as before" and with no test either way;
  • the multiple group's fallback pair has no test;
  • the Windows PNG references were not updated.

The requirements of #3976 now also say "Change layout bbox function to take into account "*" atoms" and "Change HORIZONTAL_BRACKET_SPACE = 0.4 * BOND_LENGTH". Neither is in this solution — SGroupLayout::HORIZONTAL_SPACE is still 0.3 and the atom box covers the group's own atoms only:

  • the wider space now has its own ticket, #4059. It's okay to omit question related to wider space.
  • the * atoms in the box have neither code nor a ticket — please either implement it here or move it out the same way.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PNG references are updated for Linux and macOS (18 files each), but on Windows only mol-with-indices.png and the four 3283_exp_svg.* were updated, and the latter in the first round, before this round redrew them. The other 13 — ind-296-*, ind-297-*, ind-300-*, reactions_with_brackets_2801, sa-mono, sgroups.mol_q — are still master's pictures. The Windows job stays green only because the PNG comparison is a coarse 32×32 hash; the references no longer show what is drawn, and a later change in these pictures would pass unseen there.


print("*** A multiple group keeps its brackets and its multiplier ***")
mol = indigo.loadMoleculeFromFile(
joinPathPy("../formats/molecules/smul.mol", __file__)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description promises the renderer's own pair for a multiple group "when the file has fewer than two" brackets, but no fixture has such a multiple group: smul.mol carries its brackets. Making MultipleGroupRenderer::_brackets return the stored brackets only — so such a group loses its brackets — still leaves the whole rendering suite green. The generic-group side is pinned by gen-sup-zero-next-to-real; the multiple-group side is not.

for (const BracketLine& bracket : brackets.all())
{
const float hook = _hookLength(bracket);
canvas.addBracket(bracket, SGroupBrackets::hookDirection(bracket, centre), pair_gap ? std::min(hook, *pair_gap * LONGEST_HOOK_SHARE_OF_GAP) : hook);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cap is applied in the base draw, so it reaches generic groups and superatoms as well, whose hooks are 0.15 of the bracket length. A tall narrow group shows it — a vertical chain of five carbons with a generic group, or an expanded superatom, on the middle three, without brackets in the file:

master this solution
generic group, hook 37.6 px (0.15 × 250.7) 27.6 px (82.7 / 3)
expanded superatom, hook 37.6 px 27.6 px

The description says the brackets of generic groups and superatoms are "kept as before … hooks 0.15 of the bracket", and the ticket keeps them "as today", with the third-of-the-gap rule written under the hooks of bracketed and multiple groups; the .memory-bank says the cap applies to every type. Either way no test holds it: skipping the cap for generic groups leaves the whole rendering suite green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PNG/SVG: draw brackets and labels of copolymers, components, mixtures and formulations

2 participants