Add cycle diagram shape with true border-clipped arcs#2749
Add cycle diagram shape with true border-clipped arcs#2749ilostmyhandle wants to merge 2 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8f565a598
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| size := math.Hypot(obj.Box.Width/2, obj.Box.Height/2) | ||
| maxSize = math.Max(maxSize, size) | ||
| } | ||
| minRadius := (maxSize + PADDING) / math.Sin(math.Pi/numObjects) |
There was a problem hiding this comment.
Guard the radius calculation for len(objects)==1: with one node, math.Sin(math.Pi/numObjects) evaluates to ~0, so minRadius becomes astronomically large and the only node is placed at extreme coordinates. A cycle diagram like shape: cycle with a single child will therefore produce an unusable canvas size/positioning instead of a normal centered placement.
Useful? React with 👍 / 👎.
| objects := g.Root.ChildrenArray | ||
| if len(objects) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| for _, obj := range g.Objects { | ||
| positionLabelsIcons(obj) | ||
| } | ||
|
|
||
| radius := calculateRadius(objects) | ||
| positionObjects(objects, radius) |
There was a problem hiding this comment.
This layout only repositions g.Root.ChildrenArray and never invokes the provided layout function for descendants, so objects nested inside a cycle node keep default coordinates (typically (0,0)) and overlap instead of being arranged within their parent. Any cycle diagram containing container nodes with internal content will render broken child placement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe32e730e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| angle := angleOffset + (2 * math.Pi * float64(i) / numObjects) | ||
| x := radius * math.Cos(angle) | ||
| y := radius * math.Sin(angle) | ||
| obj.MoveWithDescendantsTo(x-obj.Box.Width/2, y-obj.Box.Height/2) |
There was a problem hiding this comment.
After layout(ctx, g) computes routes, positionObjects repositions each root child with MoveWithDescendantsTo, which only moves object coordinates and does not move any connected edge routes. In cycle diagrams where a node contains nested edges, those nested routes stay at their pre-cycle coordinates and render disconnected from their endpoints (the new cycle-diagram dagre fixture already shows this for 6.a.(x -> y)[0], whose route is far from 6.a.x/6.a.y). Repositioning needs to update descendant edge paths too (or rerun routing for non-cycle edges) to keep internal connections attached.
Useful? React with 👍 / 👎.
1 participant
Summary
shape: cycleas a nested diagram typeTraceToShapeBorderso non-rectangular children stop on the visible shape outline, not just the bounding boxThis is a focused AI-assisted submission for the #1578 bounty, based on the current main-compatible cycle-layout direction and covering the non-rectangular border-clipping gap called out on another active attempt.
Tests
go test ./d2layouts/d2cycle ./d2layouts ./d2target ./d2graphTA=1 go test ./e2etests -run TestE2E/txtar/cycle-diagram -vgit diff --cached --check/claim #1578