Fix canvas sizing in InterconnectionLayoutEngine for waypoints#27
Merged
Conversation
InterconnectionLayoutEngine.Place() computed TotalWidth/TotalHeight purely from placed node rects (and, for the single-component fast path, the column geometry), never accounting for the actual routed connector waypoints. A reversed (back) edge's wrap-around approach in LayeredCorridorRouter can route a bend point beyond the far edge of the last node it passes, so the reported canvas silently clipped that connector's rendered path. Fix: after computing the node-derived totals, fold every routed waypoint's coordinates (plus the existing Padding) into TotalWidth/TotalHeight via Math.Max, so the canvas can only ever grow to cover what is actually drawn and never shrinks below the existing floor. Reproduced the clipping with a 5-node cycle plus a long edge and a much taller node (waypoint Y=200 exceeded a computed TotalHeight of ~198.5), confirmed the fix resolves it, and added a permanent regression test (Place_CyclicGraphWithTallNode_AllWaypointsWithinCanvasBounds) that fails with the fix reverted. The fix also changes results for the frozen legacy-oracle equivalence suite on any graph whose back-edge routing would otherwise clip (including the existing 'longedge' and 'cycle' named topologies and one random-seed case), since the legacy oracle still has this bug. Documented this as a third intentional divergence (HasWaypointBeyondNodeBounds), matching the existing isolated-node and component-packing divergence pattern, and moved 'longedge'/'cycle' to a dedicated test asserting the new no-clip behavior directly. Updated the InterconnectionLayoutEngine design doc to describe the waypoint-based canvas widening. Regenerated the gallery to confirm no committed diagram currently triggers this clipping case (no real content diff after filtering line-ending noise). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
This pull request addresses a subtle but important issue in the layout engine: previously, the canvas size was only based on the node rectangles, which could result in routed connector waypoints being clipped if they extended beyond these bounds—especially in graphs with reversed (back) edges. The update ensures the canvas is always large enough to contain all routed geometry by widening it to cover every connector waypoint. The test suite and documentation have been updated to reflect this new, intentional behavior difference from the legacy engine.
Canvas Sizing and Routing Fixes:
InterconnectionLayoutEngine.Placemethod now ensures the canvas (TotalWidth/TotalHeight) always covers all routed connector waypoints, not just the node rectangles, by folding in every waypoint's coordinates. This prevents connectors from being clipped when they extend beyond the node bounds, especially for reversed (back) edges.interconnection-layout-engine.md) has been updated to explain the new canvas-sizing logic and its impact, clarifying when and why the totals may diverge from the legacy oracle.Test Suite and Equivalence Logic Updates:
Place_CyclicGraphWithTallNode_AllWaypointsWithinCanvasBounds) to directly assert that all connector waypoints are within canvas bounds for a cyclic graph with a tall node.Place_LongEdgeAndCycleTopologies_NoWaypointClipsCanvas) that asserts no waypoint clips the canvas, instead of requiring bit-for-bit equivalence with the legacy oracle.HasWaypointBeyondNodeBoundshelper to detect when the new canvas-widening logic changes the output versus the legacy engine.