Repository navigation
Conversation
06c4493 to
55977f5
Compare
There was a problem hiding this comment.
Found one compatibility regression for existing Go API callers: omitting the newly added option changes edge spacing from 50 to 0 and causes unrelated edge segments to overlap. Please preserve the previous default for callers that do not explicitly configure the new option.
Validation at 55977f5: native ELK and CLI test suites passed, selected rendering tests passed (sanity, stable, root, unicode, and themes), and targeted WASM option/profile tests passed. The regression was confirmed by running the same Go program against the PR head and the merge-base layout implementation.
- sent from alixander's Codex
| Algorithm: opts.Algorithm, | ||
| NodeSpacing: opts.NodeSpacing, | ||
| EdgeNodeSpacing: opts.EdgeNodeSpacing, | ||
| EdgeEdgeSpacing: opts.EdgeEdgeSpacing, |
There was a problem hiding this comment.
[P2] Preserve the default for existing options literals
Existing Go callers that construct a ConfigurableOpts literal with the five previously available fields now implicitly pass EdgeEdgeSpacing == 0. Copying that value here (and in newContainerLayoutOptions) replaces the former hardcoded spacing of 50, even though the caller has not opted into changing it.
I verified this with all five previous fields set to their defaults and the graph a -> x; a -> y; b -> x; b -> y; c -> x; c -> y: the base uses horizontal routing lanes at y=92,142,192,242, while the PR head places every horizontal segment at y=92, causing unrelated links to overlap.
Please distinguish an unset option from an explicitly requested zero and resolve unset values to 50 in both root and container options. A regression test using an options literal that omits the new field would cover this compatibility case.
- sent from alixander's Codex
There was a problem hiding this comment.
Made the new value an int pointer to handle fallback behaviour while still allowing explicit zero-values. nil means unset and gets 50.
There was a problem hiding this comment.
Thanks, this resolves the original compatibility issue: omitted values retain 50, explicit zero is preserved, and the original example now matches the pre-PR output.
There is one small follow-up before clearing the review: [P2] Copy the spacing value before layout. In edgeEdgeSpacingOrDefault, returning opts.EdgeEdgeSpacing directly shares the caller's storage (including the global DefaultOpts pointer). Layout later unmarshals ELK output into that same pointer, so concurrent DefaultLayout calls race even when their graphs are independent.
Please change the non-nil branch to:
return go2.Pointer(*opts.EdgeEdgeSpacing)The nil branch already creates independent storage. I verified this on 745a239 with the existing parallel sanity tests:
go test -race ./e2etests -run '^TestE2E$/^sanity$' -count=1They fail with a data race on the current revision and pass with just the pointer-copy change applied through a local Go overlay. The same tests also pass against the pre-PR layout implementation. The native ELK, CLI, and targeted WASM tests otherwise pass.
- sent from alixander's Codex
EdgeEdgeSpacing is a *int, so nil means unset and resolves to the 50 D2 sent before the option existed, while an explicit 0 still reaches ELK.
Returning the caller's pointer shared storage with DefaultOpts, and Layout unmarshals ELK output back through it, racing across concurrent layouts.
ELK's
edgeEdgeBetweenLayersspacing is hardcoded to 50, so there's no way to change it.Every edge between two layers gets its own routing slot, and each slot adds this much space. So the more links you have between two layers, the further apart ELK pushes them. Lowering
--elk-nodeNodeBetweenLayersdoesn't help, because it only sets a minimum. Especially noticeable in tall charts like #1221 (comment).This adds a
--elk-edgeEdgeBetweenLayersflag, next to the other four spacing flags. The default stays 50, so nothing changes unless you pass it. ELK's own default is 10, for reference.Related to #1221.