fix(layout): charge multiline widgets their measured height, not a widget row - #887
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughMultiline widgets now contribute renderer-sized height estimates. Counts match widget names, tolerate missing port options, and are clamped to widget totals. New-node planning and persisted node creation use the same multiline sizing terms. ChangesMultiline widget sizing
Sequence Diagram(s)sequenceDiagram
participant CatalogMetadata
participant assign_positions
participant layout
participant add_node
participant PersistedWorkflow
CatalogMetadata->>assign_positions: provide widget names and port metadata
assign_positions->>layout: count multiline widgets
layout-->>assign_positions: return estimated node size
add_node->>layout: count multiline widgets
layout-->>add_node: return persisted node size
add_node->>PersistedWorkflow: save node dimensions
Priority: ⬇️ Low Unblocks: 1 PR Merge Risk: ⚪ Minimal · up to Multiline widget heights are now included consistently in planned and saved node sizes, with no current PR-introduced issue remaining. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
|
release-blocking rather than a follow-up, and I have numbers now rather than an argument. I scored three realistic agent-built graphs by taking the positions the placer actually chooses and measuring them against the rendered geometry (the 12-class fit: base 6, slot 20, ordinary widget 24, multiline 166, block pad 8, title 30) instead of against the CLI's own model. Scoring against the model is what hides this — the model believes it left a gap.
On The reason #882 and #883 did not close it: #882 fixed the width model and the title band, #883 fixed ordering and collision between new nodes. Both are correct and both were necessary. Neither changes the node's modelled height, and the height error is 142px per prompt node — larger than the vertical gap the placer leaves.
|
e7cf4e5 to
298fdae
Compare
c8ce3bb to
773acde
Compare
|
correcting my own numbers above, and the direction of the correction matters. that table used measured node heights but the CLI's estimated widths. the widths are wrong too. measured in the same browser pass, all 8 classes:
the pattern is clean: every widget-bearing node renders at exactly 270, multiline at 400, and nodes with no widgets are content-derived. the CLI derives width from label text for all of them, which is the wrong model rather than a mistuned constant. re-scored with measured widths and heights:
so this PR is necessary and large, but not sufficient on txt2img. the residual is nothing about this PR changes — the multiline height number is measured and the fix is right. flagging it because my earlier "all three go to zero" was wrong, and because the width error is the same root cause (multiline widgets modelled as ordinary rows) showing up in the other axis. caveat on the width rule: the 270 floor is 5 independent classes and solid; the 400 is one class. worth more samples before encoding it. happy to open that as a third PR if wanted. |
|
exact-head Fallow audit for
A repository-wide |
Swarmhost agentic reviewThe detailed evaluation is available to employees in the internal Slack review thread. Updated by Swarmhost's agentic review process. |
skishore23
left a comment
skishore23
left a comment
There was a problem hiding this comment.
The multiline term only reaches the planner. The size that ends up saved on the node is still the old estimate.
assign_positions passes n_multiline, but workflow_ops.add_node calls estimate_size a second time without it (around workflow_ops.py:551) and writes that result into the node's size. apply_specs goes through the same add_node, so batched builds hit this too. The next call's collision check reads that saved size.
Repro at the #888 head: two CLIPTextEncode in one apply_specs, then one more in a second call.
planner in the batch: a at y=60, b at y=366 (correct, uses 400x236)
saved node size: [270, 94] (no multiline term)
next call places c at: x=390 (a really spans 40..440, so ~50px overlap)
Passing it in add_node fixes it. The saved size becomes [400, 236] and c lands at x=520:
size = layout.estimate_size(
len([p for p in m.inputs if p.is_link]),
len(m.outputs),
len(_widget_names),
n_multiline=sum(
1 for p in m.inputs
if getattr(getattr(p, "options", None), "multiline", False) and p.name in _widget_names
),
...Better still, move the count into one helper in layout.py that both callers use, and add a test that checks the saved size after apply_specs. The current tests only exercise assign_positions, which is why this got through.
Nit: test_multiline_count_cannot_exceed_widget_count accepts _widgets_height(1, 5) charging five multiline areas to a one-widget node. Clamping n_multiline to n_widgets seems more honest than codifying that.
|
you were right, and the bug was worse than a missed argument.
Took the nit too. New One thing I did not do. Your two-batches-then-one-more overlap repro does not go red on this branch — the height is fixed here but width is still text-derived (249.9 for |
…ender The width model under-estimates, and under-estimating width is what produces the overlap users report: the placer puts the next column a node-width away and the real node reaches past it. LiteGraph's own formula is NODE_WIDTH * (1.5 if widgets else 1.0) = 210, which no widget-bearing node actually renders at. Measured across 27 classes in a real browser: every node carrying a widget renders at 270 or wider, every node carrying a MULTILINE widget at 400 or wider. Fourteen of fourteen non-multiline widget classes land at exactly 270 when their content is narrower, and seven of seven multiline classes at exactly 400 -- the shape of a floor, not a fixed width. KSamplerAdvanced 312, ControlNetApply 317.9 and CheckpointLoader 396.9 exceed it on content, which is what confirms it. Before this, on the eight classes in the release fixtures, five were under-estimated: CLIPTextEncode by 150, EmptyLatentImage and CheckpointLoaderSimple by 60, LatentUpscale by 48, SaveImage by 40. After, zero are. The three over-estimates (KSampler +19, ConditioningCombine +48, VAEDecode +6) are left alone: over-spacing wastes canvas, which nobody has filed. Effect on the release fixtures, scoring placer output against measured width AND height: txt2img 16,123 -> 0 px^2, four-prompts 0 -> 0. #886 and #887 fixed the height term; this is the width term of the same defect, and txt2img needs all three. Known over-estimate, accepted: Note and MarkdownNote carry a multiline widget but render at 140 because they are annotation nodes with no slots. They get floored to 400 and will be over-spaced. Left deliberately -- the direction argument above applies, and special-casing them on "no inputs and no outputs" is a guess I have not measured. 17 parametrized cases assert the floor never exceeds what a class renders, plus four behavioural tests. All verified red on the parent.
|
exact-head Fallow recheck for
|
c787298 to
e1da0ca
Compare
…ender The width model under-estimates, and under-estimating width is what produces the overlap users report: the placer puts the next column a node-width away and the real node reaches past it. LiteGraph's own formula is NODE_WIDTH * (1.5 if widgets else 1.0) = 210, which no widget-bearing node actually renders at. Measured across 27 classes in a real browser: every node carrying a widget renders at 270 or wider, every node carrying a MULTILINE widget at 400 or wider. Fourteen of fourteen non-multiline widget classes land at exactly 270 when their content is narrower, and seven of seven multiline classes at exactly 400 -- the shape of a floor, not a fixed width. KSamplerAdvanced 312, ControlNetApply 317.9 and CheckpointLoader 396.9 exceed it on content, which is what confirms it. Before this, on the eight classes in the release fixtures, five were under-estimated: CLIPTextEncode by 150, EmptyLatentImage and CheckpointLoaderSimple by 60, LatentUpscale by 48, SaveImage by 40. After, zero are. The three over-estimates (KSampler +19, ConditioningCombine +48, VAEDecode +6) are left alone: over-spacing wastes canvas, which nobody has filed. Effect on the release fixtures, scoring placer output against measured width AND height: txt2img 16,123 -> 0 px^2, four-prompts 0 -> 0. #886 and #887 fixed the height term; this is the width term of the same defect, and txt2img needs all three. Known over-estimate, accepted: Note and MarkdownNote carry a multiline widget but render at 140 because they are annotation nodes with no slots. They get floored to 400 and will be over-spaced. Left deliberately -- the direction argument above applies, and special-casing them on "no inputs and no outputs" is a guess I have not measured. 17 parametrized cases assert the floor never exceeds what a class renders, plus four behavioural tests. All verified red on the parent.
…dget row
A multiline text box is a text AREA, not a widget ROW, and the difference is
most of a node. estimate_size charged every widget the same 24px, which
under-measures a CLIPTextEncode by 142px -- the dominant term in the overlap
the browser harness reproduces on the batched recording.
The number is measured, not guessed. Twelve core node classes were created in a
real browser, sized by the real renderer, and their heights fitted to
H = 6 + 20 * max(link_inputs, outputs) + (widget heights + 8)
That fits ALL TWELVE exactly with an ordinary widget at 24 and a multiline one
at 166. The fixture and the fit are in the test file, so the model can be
re-checked without a browser.
Two things the measurement settled beyond the headline:
- It independently confirms the 24 and the 8 from the per-row-gap fix. Those
were derived by reading computeSize; this arrives at the same values from
rendered pixels.
- The fit needs TRUE link inputs. KSampler has seven widgets but only six
widget-backed inputs, because control_after_generate is a widget with no
input at all, so deriving links as len(inputs) - len(widgets) is off by a
row. An earlier fit that did exactly that matched 11 of 12 and looked close
enough to accept.
object_info already marks multiline inputs and the catalog parses it into
PortOptions.multiline, so no new input is needed. Matching is by name because
widget_order is the render order, not the declaration order.
The attribute guard covers `options` itself rather than the field on it: a port
carrying no options is not hypothetical -- every test double is one, and so is
any catalog entry from an object_info that omitted the block. Guarding only the
inner field raises AttributeError before the default can apply.
Additive: estimate_size keeps its signature and n_multiline defaults to 0, so
every existing caller is unchanged. 17 tests, all verified red on the parent.
… the node Review was right, and the bug was worse than a missed argument. add_node calls estimate_size a SECOND time and writes that result into the node's `size`. That saved size is what every later collision check reads, so the planner was correct and the persisted state was not: a batch placed cleanly, then the next call placed a node against a record of the first that was 142px too short. Both callers now derive the count through one helper, layout.count_multiline, which was the reviewer's preferred shape. Having two call sites each compute it their own way is what allowed them to disagree in the first place. Also takes the nit: _widgets_height clamps n_multiline to n_widgets. The old test asserted that a one-widget node could be charged five multiline areas, codifying a 700px over-measure as intended. It only arises from a bad catalog or a caller bug and silently over-measuring hides both. New tests assert the SAVED artefact rather than the planner's return value -- every existing layout test exercised assign_positions, which is exactly why this got through. Two are verified red with the add_node argument removed. NOT included: the reviewer's two-batches-then-one-more overlap repro. On this branch the height is fixed but width is still text-derived (249.9 for CLIPTextEncode), so the third node lands clear and the case cannot go red -- he reproduced it at the #888 head, where the 400px width floor makes it overlap. It goes there. Asserting it here would have passed for the wrong reason, which I only caught by checking that it failed without the fix.
e1da0ca to
ca451e7
Compare
Dismissing as addressed, not disagreed with. The finding was correct and is fixed in 2492267/ca451e7: the multiline count now lives in one helper, layout.count_multiline, and workflow_ops.add_node passes it into estimate_size at line 560, so the size written onto the node matches the one the planner used. tests/comfy_cli/test_persisted_node_size.py asserts the saved size after apply_specs rather than the planner's return, which is the gap you identified. The nit is in too: _widgets_height clamps n_multiline to n_widgets. Merging for the release gate; happy to follow up in a new PR if any of it reads differently to you.
4 participants
Stacked on #886. Closes the second half of the under-measure that ComfyUI_frontend #17901 reproduces.
What was wrong
A multiline text box is a text area, not a widget row.
estimate_sizecharged every widget the same 24px, so aCLIPTextEncodecame out 142px shorter than it draws — the dominant term in the overlap the browser harness catches.The number is measured, not guessed
That distinction is the whole point of this change. I created twelve core node classes in a real browser, let the real renderer size them, and fitted their heights to:
All twelve fit exactly with an ordinary widget at 24 and a multiline one at 166:
The fixture and the fit live in the test file, so the model is re-checkable without a browser.
Two things the measurement settled beyond the headline
It independently confirms #886. The 24 and the 8 there were derived by reading
computeSize; this arrives at the same two values from rendered pixels, by a completely different route.The fit needs true link inputs.
KSamplerhas seven widgets but only six widget-backed inputs, becausecontrol_after_generateis a widget with no input at all. Deriving links aslen(inputs) - len(widgets)is therefore off by a row — and an earlier fit that did exactly that matched 11 of 12 and looked close enough to accept. Worth knowing before anyone re-derives this.Implementation
object_infoalready marks multiline inputs and the catalog parses it intoPortOptions.multiline, so no new input is needed. Matching is by name, becausewidget_orderis the render order rather than the declaration order.The attribute guard covers
optionsitself rather than the field on it. A port carrying no options is not hypothetical — every test double is one, and so is any catalog entry from anobject_infothat omitted the block; guarding only the inner field raisesAttributeErrorbefore the default can apply. That was a real crash I introduced and caught here.Additive:
estimate_sizekeeps its signature andn_multilinedefaults to 0, so every existing caller is unchanged.Tests
17 new, including one parametrized case per measured class asserting the widget term reproduces rendered geometry. All verified red on the parent. Full layout suite: 103 passing.