Repository navigation
Conversation
Brings the extension API (LayoutValidationExtension, checkLayout, isSoftIssueType) and nine additional rules across, split into hard and soft, and adds layoutCost.ts as their shared cost model. Four swimlanes expectations moved because the new rules find real defects in those layouts rather than because the rules are wrong; the domus branch had already made exactly these spec adaptations, so they are taken as-is. swimlanes/14-messy-layout is pinned in a KNOWN_INVALID set that ratchets in both directions. The swimlanes aggregate moves 25498 -> 24574.6. Domus's unrelated swimlanes algorithm changes (postProcessing.ts, cornerAttachmentRepair.ts) are deliberately left out of this port.
DDLT replaced the measure step with applyFixture*Sizes, which reproduced its numbers but not its side effects. The one that mattered is node.intersect: every shape assigns it inside its own draw function, so skipping the draw left it undefined on every node. computeNodeIntersection in the ELK adapter is `tryNodeIntersect(...) ?? fallbackIntersection(...)`, so each endpoint silently fell through to a fallback that clips along an arbitrary interior line instead of the shape outline. The harness was therefore grading geometry no browser produces: 251 hard issues over the ELK edge-case corpus against the browser's 36. It also invalidates the conclusion drawn from it — that a 26-variant ELK config sweep "could not move the baseline". The sweep was hill-climbing a number that measured the harness. measureLayoutWithFixture runs the real defaultMeasureLayout under JSDOM with getBBox/getBoundingClientRect/getComputedTextLength backed by the fixture, so the actual shape handlers run and attach their own intersect. Boxes are attributed by DOM ancestry: nearest [data-id] is an edge label, otherwise the nearest id naming a known node, taking the label box when the read is inside that node's g.label and the node box otherwise, plus a downward search for link nodes whose <a> wrapper sits outside the shape group. Reads that match no fixture entry throw rather than quietly returning 0x0; auxiliary text metrics, which only steer wrapping, are answered with an estimate. Supporting changes: - labelHelper records node.labelBBox for leaf nodes, as measureGroupLabel already did for clusters. A shape's outline is a function of the label box and is not recoverable from the node's outer size, because padding differs per shape and per look. - Capture contract v2: per-node labelBBox, and theme/look in metadata. - assertSizesFixtureFresh takes a version RANGE rather than requiring equality, since versions only ever add optional fields; consumers that need more pass minCaptureVersion. It also asserts expectedTheme/ expectedLook — both change measured sizes AND shape outlines, so a re-capture at another configuration silently moves every score. - parseToLayoutData gains siteConfig, applied with setSiteConfig rather than saveConfigFromInitialize; the latter never reaches siteConfig, so reset() dropped it and the parse silently ran at the default look. - The ELK sweep asserts that every .mmd in the corpus is discovered. Discovery SKIPS a fixture whose freshness check throws, so an edited .mmd shrank the corpus with nothing failing. ELK fixtures re-captured at theme=redux-color look=neo. Sweep aggregate moves 1878.5 -> 5748.5, invalid 11/13 -> 7/13.
DDLT is only worth optimizing against if it grades what the browser renders, and for a while it did not — the harness skipped the measure step, and an entire 26-variant ELK configuration study ran against the resulting number before anyone compared a single edge to a real render. This makes that comparison cheap and repeatable. Compares edge endpoints, not whole polylines: painted paths round their corners into quadratic curves, so intermediate anchors legitimately differ, while the endpoints are the attachment points every endpoint-related validateLayout rule reads. The paint-time marker inset is applied with the same production function the painter uses rather than re-derived. Which function that is depends on the curve, and the two do not agree to the pixel: a rounded edge is painted via generateRoundedPath(applyMarkerOffsetsToPoints(...)), and everything else by a d3 line using getLineFunctionsWithOffset. Needs a dev server, so it is gated behind DDLT_PARITY and skipped in a normal run. 11 of 13 fixtures match exactly. The other two carry a bounded, recorded divergence of 1-2px that is in ELK's own node placement rather than in endpoint logic; the cause is not yet isolated, so it is capped per fixture instead of loosening the whole check.
intersectLine comes from Graphics Gems, where coordinates were integers
and `denom / 2` was added to the numerator so the integer division
rounded instead of truncating. JavaScript division does neither, so the
term was never a rounding correction here: `(num + denom / 2) / denom`
is `num / denom + 0.5`. Every intersection came back displaced half a
unit on both axes.
intersectPolygon is how every non-rectangular shape finds its edge
attachment — diamond, stadium, hexagon, trapezoid, subroutine — so the
displacement moved the attachment off the axis the query ray travelled
along. A vertical ray leaving a node's bottom border returned a point
half a pixel to the right and half a pixel below it, which is enough to
give an otherwise orthogonal edge a tiny diagonal opening segment, and,
when the ray pointed the other way, to put the attachment just inside
the node it was meant to touch.
question.ts had been subtracting the 0.5 back off for diamonds
("Adjusted result"), which is the same bug diagnosed locally once
before. That compensation is removed with the cause.
Found via the ELK DDLT sweep, where it accounted for the bulk of both
edge-non-orthogonal and edge-endpoint-inside-node. The aggregate moves
5748.5 -> 6738.5 and Render-stadium-shape becomes valid outright, so it
leaves KNOWN_INVALID. All 13 DDLT-to-browser parity checks still pass,
i.e. the renderer and the harness moved together.
Rendered output changes by half a pixel wherever a polygon shape
terminates an edge; visual snapshots will show it.
ELK routes to ports on a node's BOUNDING BOX and always leaves one perpendicular to the side it sits on. For a rectangle that port is the attachment point. For anything else the outline is inside the box, so the attachment has to move inwards — and the direction it moves in decides whether the edge stays orthogonal. The adapter moved it along the ray from the node centre, which is what `intersect` does on its own. That lands on the outline at a different offset along the side than the port, so the opening segment came out diagonal and the edge visibly kinked as it left the shape. On the `diamond-intersections` fixture, ELK's own route was already clean — port (129.48, 454.87), first bend (144.48, 454.87), due east — and the adapter prepended (111.9, 446.08) in front of it. outlineAttachPoint walks the outline along the departure axis instead, giving (103.11, 454.87): collinear with ELK's stub, so the edge leaves the outline, crosses the box and carries on in one straight line. It works through `intersect` alone, with no per-shape knowledge — a probe is inside exactly when it is no further from the centre than the crossing `intersect` reports for it, which holds for any outline that is star-shaped about its centre. For a rectangle it returns the port unchanged, so nothing about rect nodes moves. That alone only traded one hard issue for another, because two checks treat every node as its bounding box: - edge-endpoint-inside-node: attaching correctly to a diamond PUTS the endpoint inside the box. - edge-intersects-obstacle: the terminal segment runs from the outline out across the box by construction, and L_ATTACH is a fixed 8 which cannot cover an inset that large. Both now exempt the node the endpoint attaches to, and only when its outline is not its box. The obstacle exemption is limited to the terminal segment, so a path that later re-enters the node is still reported — the loop-back case the comment there was protecting. Sweep aggregate 6738.5 to 9128.5, invalid 6/13 to 3/13; diamond-intersections, flowchart-edge-styling and non-rect-intersections leave KNOWN_INVALID. All 13 DDLT-to-browser parity checks still pass. What remains is unrelated to shapes: bends too close to an endpoint, and group frames with more empty space than content.
The `@ts-expect-error` directives were unused — those modules resolve with types — and `intersectLine` returns undefined when the segments miss, so the result needs asserting before it is read. Surfaced by the typedoc pass in `docs:build`, which is stricter than the test run.
The best variant in the ELK configuration sweep, now that the sweep measures the layout rather than the harness. Over the edge-case corpus: hard issues 9 to 7, aggregate 9128.5 to 10082.8, invalid 3/13 to 2/13. The change is concentrated rather than broad, which is the reassuring part for a default that affects every ELK diagram. Nine of thirteen fixtures are byte-identical. styled-edge-labels goes from invalid to 934, full-class-diagram gains 35 and non-rect-intersections 10; one fixture, should-render-edge-labels-correctly-when-flowchart-htmlLabels- is-false, loses 25 on soft issues alone. Set via the schema and defaultConfig together, since the elk block is spelled out in both. config.type.ts is unchanged: the enum already allowed the value, only the default moved. nodePlacementAlignment stays as it is — it is Brandes-Koepf specific and now inert unless the strategy is set back. Thirteen fixtures is a narrow sample for a default every consumer gets; the visual e2e suite is the real check. All 13 DDLT-to-browser parity checks pass. Changesets also cover the two rendering fixes on this branch, which had none.
…s one Sharing a handle on a node, or a lane for a stretch, was a hard violation however it arose. That is right for the case these checks were written for — an edge arriving exactly where another leaves, so the handle no longer says which way anything goes — and wrong for a fan: one trunk that splits, which a reader can follow to the split. Because `isSoftIssueType` keys off the type alone, a type cannot be conditionally soft, so bundling gets its own two: `edge-bundled-attachment-point` and `edge-bundled-subpath`. They are soft — a bundle still hides how many edges are inside it, so it should cost, and a layout that bundles gratuitously should score below one that does not — but they no longer zero the score. The test is the same on both checks: the two edges play the same role at the node they meet on, and travel the shared geometry the same way. `direction` is already measured AWAY from the node for both terminals, so equal roles plus equal directions is exactly "both leaving together" or "both arriving together". For a shared lane the edges must also meet at a node at all — two unrelated edges in one lane are ambiguous however they travel — and `normalizePolyline` keeps its segments in polyline order, so `a -> b` still gives the direction of travel. Motivated by evaluating edge merging. With `elk.mergeEdges: true` over the ELK edge-case corpus, 532 of 540 hard issues were these three checks alone; the layout was otherwise no worse than baseline. After this the same run scores 52 hard, and the 44 that remain are real: ELK does merge edges of differing role or opposing travel, which is the specific thing that makes merged diagrams hard to read. The baseline corpus is unchanged at 7 hard issues, so this does not quietly flatter layouts that never bundle.
The graph is `A --> B & C & D`, plus `C --> A`, `F --> A`, `D --> A`, with `elk.mergeEdges` on. There is no edge between B and A, but merging collapses A's whole fan onto a single handle carrying a double-headed arrow, so the rendering reads as though there is one. Three wrong readings at once: A to C is bidirectional and true, A to D is bidirectional and true but indistinguishable from the rest, and A to B is a relationship the source never declared. That is a correctness failure rather than an untidy layout — the picture asserts something the graph does not say. Added as a counterexample, not as debt, so KNOWN_INVALID grows by one on purpose. It must STAY invalid until merging is fixed; the ratchet works in both directions, so whoever makes it pass gets told to remove it, and that is the signal that merging became safe. It also pins where the bundle classification draws its line. Every pair producing the false B-A reading is mixed-role — one edge arriving where another leaves — and stays hard; the same-role fans in the same diagram are soft bundles. The browser-parity check covers it too, so the merged geometry being graded is the geometry a browser renders.
…e thing Re-capturing a whole directory is the normal response to editing one diagram — and it is now required, since the sweep fails when a fixture's source hash drifts. But the endpoint stamped a fresh `capturedAt` onto every sibling regardless, so re-capturing 14 fixtures to refresh one produced a diff touching all 14, hiding the only file that actually changed. The timestamp is now carried over when the measurements are identical. A full re-capture of an unchanged directory leaves the working tree clean.
`elk.mergeEdges` was handed straight to ELK's `elk.layered.mergeEdges`, which collapses a node's edges onto one handle per side — arriving and leaving alike. The raw sections for the merge-edge-ambiguity fixture showed A's three outgoing edges and two incoming ones all attached at (133.90625, 127): a single handle under a double-headed arrow. There is no edge between B and A, but B hung off that handle, so the diagram asserted a relationship its source never declared. Bundling now gives each node a port for arriving edges and another for leaving ones, and points edges at those. Edges sharing a port still share a trunk — the reason to enable the option is unchanged — but ELK has to place the two ports separately, so the roles no longer collide. On the same fixture A's handles come out 15px apart and the false reading is gone; the fixture goes from invalid to 732, valid, carrying only soft bundle penalties, and leaves KNOWN_INVALID. `elk.layered.mergeEdges` is now pinned off at the root and in subgraphs, so `elk.mergeEdges: true` means safe bundling rather than ELK's. The existing specs asserting it propagated are updated to assert the opposite, with the reason. Known trade-off: one handle per role means an edge arriving from the far side of a node can take a longer route than it did before. Per-side buckets would fix that, and need the side to be known before layout. All 14 browser-parity checks pass, so the bundled geometry being graded is what a browser renders.
…nection" This reverts commit ad4d60b. Routing every arriving edge through a single port per node produces worse layouts than it was worth: a node has one handle per role regardless of which side an edge approaches from, so edges coming from the far side detour around the node to reach it. The ambiguity it fixed is real, but this is not the right shape of fix. `elk.mergeEdges` goes back to ELK's own `elk.layered.mergeEdges`, and merge-edge-ambiguity returns to KNOWN_INVALID as the pinned counterexample it was — still failing, still describing the defect, and still the thing a proper fix has to make pass.
…t point An edge unrelated to node N, running through the exact spot where some other edge attaches to N, is read as leaving N: a reader cannot tell a line that touches a node from one that starts there. The diagram shows a connection its source never declared. This is what makes bundled layouts untrustworthy, and it is the class the soft bundle classification was blind to. Those checks ask whether the bundled edges share a source and travel the same way, and stop there — so a trunk that runs to a node, drops one member off at it and carries the rest onward scored as a benign bundle. Reproduced by hand: two edges from one source, one terminating on A's border and the other turning away at that same point, came back valid at 932 while showing an edge from A that does not exist. Narrow on purpose. It is not about passing near a node — that is edge-border-hugging — but about coinciding with a real attachment point, within EPS_SHARED_ATTACH, the tolerance two attachment points must already differ by. An edge's own endpoints are exempt, and groups are skipped since their frames contain edges by design. No fixture in the ELK, domus or swimlanes corpora trips it, so it adds no debt; it exists so the next attempt at edge bundling can be judged rather than taken on trust.
Every label in this fixture carries explicit <br> breaks, so a node is as wide as its longest hard-coded line and the wrap width only binds above that. The widest natural label measures about 144, which is why the default 200 and an intermediate 150 both leave the diagram untouched — node widths are byte-identical at either setting. 125 is below that, so the wide labels clamp to a common width: the widest node goes from 175.95 to 157 (125 plus neo's 32px of horizontal padding), five nodes now share exactly that, and distinct widths drop from 25 to 21. Within a subgraph the wide nodes line up instead of shifting about, which is the point. Sizes re-captured, since the source hash changed. The fixture stays in KNOWN_INVALID and the aggregate is unchanged; edge-border-hugging drops off its issue list.
Three options settle where a node ends up, and they live in different phases of the layout: layering picks the column, node placement the coordinate within it, and cycle breaking decides which edges are reversed and therefore which ones detour. Picking them well means knowing all three interact, so this names the combinations worth using. default NETWORK_SIMPLEX LINEAR_SEGMENTS GREEDY_MODEL_ORDER legacy NETWORK_SIMPLEX BRANDES_KOEPF GREEDY depthFirst NETWORK_SIMPLEX LINEAR_SEGMENTS DEPTH_FIRST An explicit `layeringStrategy` / `nodePlacementStrategy` / `cycleBreakingStrategy` beats the preset for that one option and leaves the rest of it in place. That is why `defaultConfig` now leaves all three undefined rather than giving them values: `??` is what tells "the user chose this" from "nobody did", and concrete defaults would have made every preset unreachable. They stay listed so `configKeys` still finds them. `legacy` uses GREEDY, not the GREEDY_MODEL_ORDER the schema advertised. That default was declared in the schema but never listed in the shipped defaults, so it reached ELK as undefined and ELK's own default applied. Measured rather than assumed: with the key absent, app->billing on the many-subgraphs fixture came out at 425, identical to explicit GREEDY, where GREEDY_MODEL_ORDER gives 2408. The preset reproduces what develop renders, not what its schema claimed, and the stale `default:` lines are removed so the schema stops advertising values it cannot deliver. The default placement strategy moves from NETWORK_SIMPLEX to LINEAR_SEGMENTS, so existing ELK diagrams lay out differently. `preset: legacy` restores the previous behaviour. Also moves the scratch option blocks out of render.ts into elkOptionCatalogue.ts, which nothing imports. It is a reference to copy from — every ELK option that affects layout, each value listed with one line on what it does — and it carries the two traps that cost time working this out: root options do not reach containers, and an entry naming a key wired to `config.elk.*` silently disables that config.
Line hops are applied after paint — the crossings are only known once every edge has been emitted — so an `afterPaint` hook is the only place to draw them. A layout inside this package can reach into `rendering-util` for that; `@mermaid-js/layout-elk` ships separately and cannot. Required rather than cosmetic: `layout-elk/src/lineHops.ts` imports it from `mermaid`, so the ELK line-hop support does not build without it. Exported next to the other common-renderer pieces, with `EdgeGeom` and `LineJumpConfig` since they are its argument types.
ELK spreads an edge's port evenly along a node's side, then routes the edge down an inter-layer channel whose row rarely matches that port. The leftover is a staircase right at the border: leave the port, run a few pixels, step perpendicular onto the channel, carry on. Rounded corners put the two micro-bends on top of each other, which is what reads as jagged. On knsv2 the infra->B edge stepped 3.25px and painted as M193,116.25 L216.375,116.25 Q218,116.25 218,117.875 L218,117.875 Q218,119.5 219.625,119.5 L279.5,119.5 L315.5,119.5 — two quadratic corners either side of a zero-length segment. It now paints as M193,119.5 L218,119.5 L279.5,119.5 L315.5,119.5. Three guards keep it safe, and they are the whole design: the step must be small (16 max), the route must continue the same way afterwards so a real turn is never collapsed, and the moved terminal must stay within the node's border span so the edge cannot detach. A step this close to the border can only be the port-to-channel connector — an obstacle dodge bends much further out — so moving it cannot introduce an overlap. Over the ELK corpus this clears 10 of 24 such steps, on 6 of the 14 edges that had one. What remains is mostly mid-route rather than terminal, where collapsing could push a line onto a neighbour; that is why this is deliberately scoped to the ends. Gated on `elk.straightenEdges`, default true. Opt-out rather than opt-in: the step it removes is never intentional. Based on a patch from a colleague. Their `terminalAnchor` half is not included — it fixes the anchor fed into clipping so the stub stays collinear with the routed segment, which `outlineAttachPoint` already does here; adding it moved the jog count by zero.
Adds the missing half of the ELK line-hop support: `render.ts` already registers `afterPaint: applyElkLineJumps`, but the module it imports was never committed, so the branch did not build from a clean checkout. My mistake — it was written alongside the wiring and then left untracked through several commits that touched render.ts. The crossing detection, arc and gap styles all already existed and were in use by swimlanes; this is the adapter that lets an external layout reach them. Reads `elk.lineHops` (true | false | 'arc' | 'gap'), defaulting to on, and hands the routed polylines to `applyLineJumpsToSvg` after paint, which is the only point where every edge has been emitted and the crossings are known. ELK's curves are compatible either way: `applyElkEdgeLayout` sets `rounded` for a routed edge and `linear` for its straight-line fallback, and `curveSupportsLineHops` accepts both. An edge that takes a hop gives up its corner rounding in exchange, which is the trade the line-jump module documents.
The premise for collapsing a terminal step is that a jog against the border can only be the port-to-channel connector — anything else bends further out. The code asserted that in a comment but never checked it: it bounded the step's HEIGHT at 16 and said nothing about its distance from the node. A small step a long way down the route was collapsed too, dragging the port along with it. Reported from a real diagram: app -> analytics turned 48 from the border, and collapsing it slid the port 15.1px onto a row another edge already occupied. The result was a crossing that had not been there, complete with a line hop over it — strictly worse than the jog it removed. Measured across the corpus, every genuine connector turns 20-25 from the border; the ones worth leaving alone turn at 48, 112 and 173. TERMINAL_RUN_MAX is 30. Costs two of the sixteen steps previously cleared, both from the far end of long routes, and one fewer port moved. Hard issues stay at 16 and the aggregate at 9921.6, so the guard removes the harmful class for free.
The straightening pass measured everything with the shared `EPS` of 1. That value exists for "is this point on a border" and is far too coarse for a step: ELK routinely leaves under a pixel between the port row and the channel row, and at `EPS` those segments are not classified as axis-aligned at all, so the pattern never matched and the step was rejected as degenerate before any guard ran. Sub-pixel does not mean invisible. An infra -> auth edge stepped 0.858 and painted as L1204.071,883.5 Q1204.5,883.5 1204.5,883.929 L1204.5,883.929 Q1204.5,884.358 1204.929,884.358 — two rounded corners stacked on each other around a zero-length segment, the same artefact as the larger steps. It now paints as M1179.5,884.358 L1204.5,884.358 L1266,884.358. `JOG_EPS` is 0.01, used for classifying a segment's axis and for the minimum step size. The border-span checks keep the shared `EPS`, where a pixel of slack is the right tolerance. One more step cleared over the corpus (15 from 16 remaining), the only sub-pixel one; hard issues stay at 16 and the aggregate at 9921.6. The smallest port move is now 0.60 rather than 2.69.
NIKOLOV, DUMMYNODE and NODECOUNT are `elk.layered.layering.nodePromotion.strategy` values listed in elkOptionCatalogue.ts, which is already committed — the spellcheck fails without them. Three additions only. The file had also picked up an unrelated rewrite that shredded its two header comments into individual words (`be`, `coding`, `contains`, `file`, `It`) and reordered every entry; that is reverted here rather than carried along.
The step was being removed by sliding the port onto the channel's row. That leaves the attachment somewhere ELK did not choose, and a node whose other edges are still at their spread positions then looks lopsided — reported on app -> analytics, where the port slid 5.4px while its neighbours stayed put. Now the channel moves onto the port's row instead. Both ports stay exactly where the layout placed them, and the edge still draws as one straight line. Three consequences: - The WHOLE run moves, not just its first segment. The channel carries on past the step until the route turns, and shifting only part of it left a diagonal where the moved and unmoved halves met. - An edge is skipped when the run ends at the far terminal, since moving it would drag the other end's port and recreate the problem there. - Straightening became a post-pass over the finished layout rather than a per-edge step, because displacing a run can land it in a lane something else occupies. Each candidate is counted against every other edge and dropped if it would add a crossing. The cost is real and worth stating: this clears 2 of 24 steps where moving the port cleared 16, and hard issues stay at 25 rather than dropping to 16. Most routes have their run ending at the far port, so the pass declines. Aggregate 9867 -> 9917, no ports moved, no crossings added. The crossing check is currently defensive — nothing in the corpus trips it — but the displacement it guards is real and untested territory without it.
Reported on design-system -> mermaid-chart-app in many-subgraphs-and-edges, where the hop rendered as: Q430.625,180.086 437.696,180.086 L437.696,180.086 <- zero-length A2.929,2.929 0 0 1 443.554,180.086 The edge turns east at x=430.6 and is crossed 10px later. The rounded corner eats 7.07px of that, so the clamp fitted the hop into the remaining 2.9px — under half the requested 6 — and opened the arc exactly on the corner's tangent point. At that radius the arc does not clear the stroke it is hopping, so the lines still touch, and the corner's quadratic runs straight into the arc with no straight run between them. The two curves read as one malformed squiggle. 10px is SUBGRAPH_EDGE_LANE_SPACING: ELK stacks subgraph-internal edges that far apart, and a 10px lane cannot hold a 7.07px corner cut plus a 6px hop. So this is not a tuning miss, it is a case where no correct hop exists, and the clamp had no way to say so. Two changes to how the radius is clamped: - Keep 2px of straight line between a bend and a hop, so an arc never opens on a corner's tangent point. Radius gives way, not the clearance. - Drop a hop that would still have to shrink below 60% of the requested radius. An undrawn hop is an ordinary crossing, which is what every diagram looked like before hops existed, and a much better failure than a broken-looking one. Dropping happens before the neighbour-spacing pass so a hop squeezed out by a corner does not also shrink the hops either side of it. Selective in practice: of the 13 crossings in that fixture, the 2 in narrow lanes are dropped and the other 11 keep their full 6px radius.
Reported on knsv2.html, on the crossing between org -> platform and design -> app. The hop rendered as: L236.071,1033.625 L243,1033.625 A6,6 0 0 1 255,1033.625 L269,... which looks well-formed, and from the hopping edge's side it is: its own bend is 20px back, so there is room for a full 6px arc. The problem is the edge being hopped. org -> platform turns south at (249, 1031.102) and design -> app crosses that vertical at y=1033.625 — 2.5px below the turn, inside the 7.07px the corner's quadratic takes to rejoin the line. At that y, org -> platform is not on x=249 at all; it is still curving. The arc arched over blank paper while the two strokes carried on touching beside it. Crossings are found on polylines, but generateRoundedPath does not draw a rounded edge as its polyline: it departs the line up to cutLen before each bend and rejoins it cutLen after. Inside that stretch the polyline says the stroke is somewhere it is not, so a crossing computed there is mislocated at best and fictional at worst. findEdgeIntersections now drops a crossing that sits inside a rounded corner on EITHER edge. Only `rounded` edges lie this way; every other supported curve is drawn as the polyline it describes, so they are unaffected. This is a separate defect from the corner-clearance fix in the previous commit, and neither subsumes the other: that one is about the hopping edge having no room for the arc, this one is about the arc having nothing real to hop. On knsv2.html the bad hop goes and the other 17 stay at full radius.
A subgraph could carry 74px of padding on one side and 24px everywhere else, which reads as a mistake because nothing visible occupies it. The space is a routing lane, held open for an edge that runs against the flow of the layout and has to be routed back around the outside. On knsv2.html only design -> app and infra -> app do that, and they are exactly the two groups that were lopsided: platform on its right, where they leave, and product on its left, where they arrive. The lane's width came from spacing.baseValue, which was doing two jobs. Every unset ELK spacing derives from it, so it had to stay large enough that an edge got a straight run before the node it enters — below about 40 the approach came out shorter than the 10px arrowhead and the turn read as happening underneath it. But an edge routed down the inside of a frame claims a lane the same width, so paying for the approach out of the base value also pushed groups clear of their own borders. Split them: base value down to 24, and the approach run, node separation and edge separation set explicitly. Two things worth recording, because both cost time: - The key that buys the approach is elk.layered.spacing.edgeNodeBetweenLayers. A previous attempt used elk.layered.spacing.edgeEdgeBetweenLayers, which is edge-to-edge and a different quantity, and the note left behind concluded that ELK ignored edge-node spacing "in every key form". It does not. Corrected in place so the next person does not re-derive it. - Node separation derives from the base value too. Lowering it alone pulled sibling nodes together until three fixtures tripped the validator's node-node-padding rule and the aggregate fell to 9294. spacing.nodeNode is now set on its own, spelled the same way as the rectpacking override so a container cannot end up holding two values for one option, and clearContainerAlgorithmOptions restores it alongside the base value. Subgraph nodes are also placed with NETWORK_SIMPLEX and PORT_POSITION flexibility, which keeps a group's nodes aligned with one another rather than drifting, and lets a node shift so an edge leaves straight instead of bending off the port. Measured on knsv2.html: platform and product both back to 24/24, and the shortest approach run over all 28 edges is 30px with none under 15, against 7 under 15 when the base value alone was lowered. DDLT aggregate 9916.6 -> 9985.9 over 14 fixtures, still one invalid and it is the deliberate merge-edge-ambiguity counterexample. Re-captured many-subgraphs-and-edges, whose .mmd had gained a lineHops frontmatter key and so was being skipped by the sweep. Resolves #8150
🦋 Changeset detectedLatest commit: 80fab04 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
❌ Deploy Preview for mermaid-js failed.
|
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
knsv-bot
changed the title
Collaborator
Author
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8151 +/- ##
============================================
- Coverage 79.45% 57.23% -22.23%
============================================
Files 607 419 -188
Lines 83390 34369 -49021
Branches 16881 5496 -11385
============================================
- Hits 66258 19670 -46588
+ Misses 16086 12300 -3786
- Partials 1046 2399 +1353
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #8150
The issue says the current ELK config works but has side effects worth avoiding. This branch chases those side effects down one at a time. Each commit states what was measured, and the DDLT aggregate over the ELK fixture corpus moved 1878.5 → 9985.9 across the branch.
The config itself
Subgraph padding was uneven. A group could carry 74px on one side and 24px everywhere else, with nothing visible in the gap. It is a routing lane, held open for an edge that runs against the flow of the layout and has to be routed back around the outside — so only groups containing such an edge were affected, which is why it looked arbitrary.
The lane width came from
spacing.baseValue, which was doing two jobs. Every unset ELK spacing derives from it, so it had to stay large enough that an edge got a straight run before the node it enters; below about 40 the approach came out shorter than the 10px arrowhead and the turn read as happening underneath it. But an edge routed inside a frame claims a lane the same width, so paying for the approach out of the base value also pushed groups clear of their own borders.Those are now bought separately: base value down to 24, with the approach run, node separation and edge separation set explicitly. Padding is even again and edges keep their run — measured on a 28-edge diagram, the shortest approach is 30px with none under 15.
Two traps found on the way, both now recorded in the source:
elk.layered.spacing.edgeNodeBetweenLayers. An earlier attempt usededgeEdgeBetweenLayers— edge-to-edge, a different quantity — and concluded from its failure that ELK ignored edge-node spacing "in every key form". It does not.node-node-paddingand the aggregate fell to 9294.Also:
elk.preset(default/legacy/depthFirst) names the node-placement combinations, with explicit config beating the preset.Fixes underneath it
Most of the branch is defects the configuration work exposed:
intersectLinedisplaced every result by half a pixel. Graphics Gems addeddenom / 2so an integer division would round; in floating point that is just+0.5on both axes. It is how every non-rectangular shape finds its edge attachment. Removing it clearedRender-stadium-shapeoutright.elk.lineHops, plus two rendering defects in them: a hop squeezed into whatever room a bend left over (2.9px against a requested 6), and a crossing found inside the stretch where an edge is rounding a corner — where the polyline is not where the stroke is, so the arc hopped blank paper.Testing
validateLayoutwas ported from the domus branch and the ELK corpus now runs through DDLT, so every claim above is a number rather than an impression. The harness itself needed fixing first: it was skipping the measure step, leavingnode.intersectundefined and grading a fallback the browser never takes — 251 hard issues against the browser's 36. A 26-variant config sweep run against that baseline concluded nothing could move it; those results were void.1267 unit tests pass. One fixture is still invalid and is meant to be:
merge-edge-ambiguitypins the case where edge merging fabricates a relationship the source never declared.Reviewing
elk.mergeEdgeswas tried and reverted (ad4d60ba3/23a19c4c6) — merging surfaced ambiguities where a reader could deduce an edge that does not exist.This changes rendered output for existing ELK diagrams, subgraph spacing and crossings especially. Worth looking at the Argos diffs rather than the code alone.