shape: cycle - trace arcs to shape borders (close #1578)#2742
Open
yasumorishima wants to merge 6 commits into
Open
shape: cycle - trace arcs to shape borders (close #1578)#2742yasumorishima wants to merge 6 commits into
yasumorishima wants to merge 6 commits into
Conversation
…ction Closes terrastruct#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.
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
2 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.
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