shape: cycle - trace arcs to shape borders (close #1578) - #2742
yasumorishima wants to merge 21 commits into
Conversation
…ction Closes d2lang#1578. The previous cycle layout sampled an arc between shape centers and relied on TraceToShape to clip the route. With small shapes sitting on a large radius, every arc sample near the source center fell inside the rectangle, so the segment-based clipper never found an edge crossing and left the curve starting at the shape center. Now the layout solves the layout circle against each rectangle's four edges analytically and limits the arc to the angles where it crosses the source and destination borders, so every connection starts and ends exactly on a shape boundary. Also adds Segment.IntersectCircle to lib/geo and removes the unused clamp helpers from d2cycle/layout.go.
Addresses CodeRabbit review feedback on #1. Major: my analytic intersection only used `geo.Box` edges, which is the shape's bounding box. For non-rectangular shapes (circle, oval, hexagon, cloud, ...) the bounding box border is not the actual shape outline, so connections were landing on the bounding box rather than on the shape. Fixed by passing each arc endpoint through `shape.TraceToShapeBorder`, which walks the line from the shape center to the box-border point and returns the intersection with the shape's perimeter (a no-op for rectangular shapes). Minor: `Segment.IntersectCircle` appended both quadratic roots even when the discriminant was zero (a tangent contact), so a graze was reported as two identical points. Fixed by emitting the second root only when the discriminant is strictly positive, and added a tangent regression test case.
Addresses CodeRabbit second-review feedback on #1. When `nextBoundaryAngle` could not find an arc-range crossing (very narrow sweep, degenerate geometry, ...) the previous fallbacks let `startAngle = srcAngle` / `endAngle = dstAngle` and emitted a route whose first/last point coincided with the shape center, since the center lies on the layout circle. That regressed the very fix the PR introduces and also fed `TraceToShapeBorder` a center-as-rectBorder input. Now the createCircularArc flow falls through to the straight-line fallback whenever either boundary angle is missing or the resulting range is empty, and `fallbackStraightRoute` itself runs each endpoint through a new `clipToShapeBorder` helper. The helper extends a ray from the shape center toward the other endpoint, intersects the bounding box, and refines via `TraceToShapeBorder` for non-rectangular shapes. Both fallbacks therefore emit shape-border endpoints, matching the arc-success path. The cycle-diagram fixture is unchanged because rectangle shapes never hit the fallback in the existing test cases.
Point.Equals compares float64 exactly, so the chord case asserted y == 4 against a computed 3.9999999999999982 and failed.
172fada to
2030067
Compare
Adopt the github.com/d2lang/d2 module path (org migration) in the cycle layout files and regenerate the cycle-diagram sketch fixtures for the upstream SVG rendering changes.
The only conflict was in pathData(): this branch carried the WIP rewrite of that function from the original cycle draft, while master has since reformatted the same code with svg.FormatFloat. The WIP version is not needed for border tracing (the arc route has ARC_STEPS+1 = 31 points, so master final-curve handling applies, and the straight fallback route sets IsCurve=false), so take master version verbatim and drop the churn. Regenerate the cycle-diagram sketch fixtures: the arc geometry is unchanged (same M/C coordinates), the diff is svg.FormatFloat trimming trailing zeros plus the unused-CSS pruning master added.
master added a raster/scene path that draws each shape from an explicit type switch, with a default branch that asks lib/shape for path commands. Square-family shapes have no path commands, so the cycle container fell through and failed with "unsupported typed geometry for cycle" in TestTargetShapePixels. Handle cycle next to sequence_diagram and hierarchy in the four square-family switches, add it to the shape lists the two scene builder tests enumerate, and regenerate the target-shapes golden.
The scene builder commit made the cycle container visible for the first time, which showed that the container box does not sit on the board it wraps: d2layouts fits the container to the nested board and then places it at (0, 0), and PositionNested offsets the contents by the container position alone, so a nested board is expected to start at the origin. positionObjects centers the ring on (0, 0) instead, leaving the upper left half of the board negative, so the container was drawn offset by half the ring. Normalise the board at the end of the layout, including the arcs in the bounds so the container encloses them, and record the size on the root for FitToGraph. The shift is a translation applied to the shapes and the routes together, so the arc endpoints stay on the shape borders: the first arc keeps x = 28.476319 + 227 = 255.476319 exactly. Handle cycle in the SVG rectangle case as well, next to sequence_diagram and hierarchy, so SVG and raster agree on the container.
|
Merged master in and resolved the one conflict, which was in Two follow-ups came out of the merge. They change what a cycle container looks like, so I want to flag them rather than bury them in the diff. 1. The cycle shape was not drawn by either renderer. 2. Once the container was visible, it was not sitting on the board it wraps. Before (containers absent) and after:
Also added the changelog entry, which I had missed.
If you would rather cycle containers stayed invisible, I am happy to take the rendering back out and find another way to satisfy the pixel-coverage test — I just did not want to leave SVG and PNG disagreeing with each other. |
550fe38 to
f1828a0
Compare
…-tracing # Conflicts: # ci/release/changelogs/next.md
v0.9.0 vendored TALA into the repository, so the txtar suite now runs every case under dagre, elk and tala. The cycle-diagram case had no tala goldens and failed with a missing board. The layout is unchanged; these are the recorded output of the engine that ships with the tree.
The exporter truncates positions and sizes to integers, and a circle leaves shapes on half pixels, so the container could be written one pixel narrower than the child sitting on its right edge and its border was drawn through that child. It showed up only under tala, where the board lands on a fractional offset: container 1 ended at 699 while b ended at 700. Sweeping all 162 txtar goldens, that was the only case in the suite of a child outside its container that is not a sequence diagram lifeline. Rounding the board size up makes the box contain its children under all three engines, and the three now agree.
|
Rebased onto master after v0.9.0, and two things in the push are worth a note because they move goldens. TALA now ships in the tree, so the txtar suite runs every case under dagre, elk and tala. This case had no The cycle board is rounded up to whole pixels ( It only showed under tala, where the board lands on a fractional offset — dagre and elk happened to land flush. Sweeping all 162 txtar goldens for a child outside its container, this was the only case in the suite that is not a sequence diagram lifeline. Rounding the board size up makes the box contain its children under every engine, which is why all three goldens moved: the three now agree on the same relative geometry, where before they differed by a pixel each way.
|
A self edge shares one shape between its source and its destination, so the arc geometry degenerates: the two centres coincide, the sweep is zero, and the fallback clips both endpoints to the same point. The route was left as two identical points, which the exporter wrote out as d="M NaN NaN L NaN NaN" and which, now that the render target is validated, fails the whole render with "route[0:1] must have a finite, non-zero segment length". Give the self edge a loop of its own instead: a circle tangent to the shape border on the far side from the centre of the ring, sampled the same way the arcs are. Shapes that sit on the centre have no outward direction, so their loop goes straight up. The txtar case is a new entry rather than a fourth board on cycle-diagram, because adding a board there moves the other three and rewrites goldens that have nothing to do with this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0174tmEsVYhznCbNSYEZWett
calculateRadius spaces the shapes around the ring by sin(pi/n), and sin(pi/1) is 1.2e-16 rather than 0, so a cycle holding a single shape came out with a radius of about 5.7e17. At that magnitude the gap between representable doubles is wider than the shapes themselves, so anything computed on the ring collapses. It was visible before this branch as a board written 4.3e17 pixels tall; fitting the container to its contents hid that, but the arithmetic was still wrong underneath, and the self loop of a lone shape came out flattened onto y = 0 while still passing validation. One shape needs no room for neighbours, so return the minimum radius. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0174tmEsVYhznCbNSYEZWett
This branch has not been deployed
1 participant
Summary
Closes #1578.
This is a continuation of #2362 by @alixander. That PR introduced the
shape: cyclelayout but the curves did not start or stop on shape borders, which the bounty on #1578 asks to fix. This branch is based onalixander:cycle(the head of #2362) and adds the missing border-tracing on top, plus tests and a couple of small geometry fixes.If preferred, I am happy to either land this as the supersede PR or rebase the four new commits onto a fresh branch off master. Glad to follow whichever workflow suits the maintainers best.
Problem
In #2362 the cycle edges were sampled as a 30-point arc between source and destination shape centers, then handed to
Edge.TraceToShapeto clip the route to the shape borders. With small shapes on a layout circle, every arc sample near the source center fell inside the source rectangle, so the segment-edge clipper found no crossing and left the curve starting at the shape center instead of on the border.Fix
Solve the layout circle against each shape analytically and limit the arc to the angles where it crosses the source and destination borders, then refine the endpoints onto the actual shape outline:
Segment.IntersectCircle(center, radius)helper inlib/geosolves a quadratic for segment-circle crossings (single point on a tangent contact).d2cycle.createCircularArcnow intersects the layout circle with the four edges of each shape's bounding box, picks the angles where the arc enters / exits, and samples between them.shape.TraceToShapeBorderso non-rectangular shapes (circle, oval, hexagon, cloud, ...) end on their actual outline rather than on the bounding box border. For rectangles this is a no-op.fallbackStraightRouteproduces a straight connection whose endpoints are also clipped to the shape border via the same helper, instead of returning the shape centers.The renderer side (cubic-bezier emission for
IsCurve) is left exactly as in #2362, so the visual style of the curves matches that PR.Tests
e2etests/testdata/txtar/cycle-diagram/{dagre,elk}/{board.exp.json,sketch.exp.svg}withTA=1. The new SVGs render every cycle connection starting and ending on a shape border.TestSegmentIntersectCircleinlib/geo/segment_test.gocovers eight cases (chord, tangent, off-origin circle, endpoint on circle, both endpoints inside / outside, vertical chord, degenerate zero-length segment).go test ./d2layouts/... ./lib/geo/...all green.go test ./e2etests/...was run; only failures are the existing Go 1.26url.URLJSON field-order shifts that already affectmaster(e.g.icon-label,cycle-order,sql-table-reserved) and are unrelated to this change.Self-review
Both findings from CodeRabbit on the equivalent fork PR were addressed:
shape.TraceToShapeBorder.IntersectCircleno longer reports a tangent contact twice (disc > 0guard added with regression test).Bounty
/claim #1578