d2cycle: trace arcs to shape borders (close #1578)#1
Open
yasumorishima wants to merge 4 commits into
Open
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReworks circular-arc edge routing to sample arcs on a computed layout circle and clip endpoints to shape outlines; introduces segment–circle intersection support; updates e2e fixture route coordinates to match the new routing outputs. ChangesCircular arc routing & geometry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@d2layouts/d2cycle/layout.go`:
- Around line 163-184: The problem is boxCircleIntersectionAngles uses boxEdges
(via boxEdges) so intersections are computed against the bounding box instead of
the actual node outline; update boxCircleIntersectionAngles to accept or derive
the real shape boundary and compute intersections against that boundary (e.g.,
call a shape-specific IntersectCircle or build edges from the node's actual path
instead of geo.Box), and remove/replace boxEdges usage; change or add a helper
(instead of boxEdges) that returns the true shape segments for
circles/ovals/polygons (or use an existing shape.IntersectCircle method) so arc
endpoints lie on the rendered shape outline rather than the bounding rectangle.
In `@lib/geo/segment.go`:
- Around line 145-159: The quadratic root loop can append the same intersection
twice when disc == 0 (tangent); modify the logic in the intersection code that
computes disc, sqrtDisc and iterates the two t values so duplicate roots are
deduplicated before appending to pts (e.g., only consider each t once or skip
appending if a previously appended point with the same coordinates already
exists within a small epsilon); keep references to the existing symbols (disc,
sqrtDisc, pts, NewPoint, s.Start, dx, dy) and ensure IntersectCircle returns
only unique intersection points.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16518446-a1e4-4dfe-89c1-237617d7b906
⛔ Files ignored due to path filters (2)
e2etests/testdata/txtar/cycle-diagram/dagre/sketch.exp.svgis excluded by!**/*.svge2etests/testdata/txtar/cycle-diagram/elk/sketch.exp.svgis excluded by!**/*.svg
📒 Files selected for processing (5)
d2layouts/d2cycle/layout.goe2etests/testdata/txtar/cycle-diagram/dagre/board.exp.jsone2etests/testdata/txtar/cycle-diagram/elk/board.exp.jsonlib/geo/segment.golib/geo/segment_test.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.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@d2layouts/d2cycle/layout.go`:
- Around line 88-103: The fallback logic uses node centers (srcCenter/dstCenter)
when nextBoundaryAngle misses, causing edges to originate at centers and
breaking TraceToShapeBorder; instead, compute border-clipped points using the
existing shape-border helper (e.g., TraceToShapeBorder or the same border
clipping used elsewhere) for both source and destination whenever startAngle or
endAngle fall back to srcAngle/dstAngle or when sweep <= 0 or the end<=start
case; update the branches around nextBoundaryAngle and the calls in
fallbackStraightRoute to return points produced by tracing/clipping to the shape
border (using the edge.Src.Box and edge.Dst.Box with the respective angle or
center-to-circle intersection) rather than returning srcCenter/dstCenter so all
three failing cases preserve shape-border endpoints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 525dd801-bf6c-4ad7-af6e-d9e96941a86b
📒 Files selected for processing (3)
d2layouts/d2cycle/layout.golib/geo/segment.golib/geo/segment_test.go
✅ Files skipped from review due to trivial changes (1)
- lib/geo/segment.go
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/geo/segment_test.go
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.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
1 participant
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.
Fork-only Draft PR for CodeRabbit / CodeQL review of the d2cycle border-tracing fix on top of alixander/d2:cycle (upstream PR terrastruct#2362).
Scope of this PR
This branch was forked from
alixander/d2:cycle(the head of the open upstream PR terrastruct#2362), then a single new commit was added:So the diff against
mastercontains:Reviewers should focus on the latest commit (
d2cycle: trace arcs to shape borders ...).Problem
In upstream PR terrastruct#2362, cycle edges were sampled as a 30-point arc between source and destination centers, then handed to
Edge.TraceToShapeto clip to 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, not on the border.The bounty (terrastruct#1578, $50) asks for the curves to start and stop exactly on the source / destination shape borders.
Fix
Solve the layout circle against each rectangle's four edges analytically (
Segment.IntersectCircle, new helper inlib/geo/segment.go) and limit the arc to the angles where it crosses the source and destination borders. The arc's first and last sample then sit exactly on the shape boundaries.Tests
e2etests/testdata/txtar/cycle-diagram/{dagre,elk}/{board.exp.json,sketch.exp.svg}withTA=1.d2layouts/...andlib/geo/...unit tests pass.Why this PR is on a fork only
Summary by CodeRabbit
Bug Fixes
Tests