Fix title centering, port label collisions, and layout alignment#16
Merged
Merged
Conversation
…ions - Add explicit CenterTitle flag to LayoutBox, replacing fragile Children.Count==0 leaf inference, fixing container root-box titles rendering dead-center over connector routing corridors. - Reserve title space against left/right ports in the flat layered engine (LayeredLayoutMetrics.ResolveTitleReserveTop, PortDistributor, LayeredLayoutAlgorithm growth floor) so named port labels no longer render underneath box titles. Scoped to the flat engine only; the hierarchical/merge-region pipeline explicitly opts out for now (assumedFontSize: 0.0) pending a follow-up. - Gate CenterTitle off only when a box has a named left/right port (not any anonymous edge anchor), preserving centered titles for ordinary flow-diagram boxes. - Add BoxMetrics.ResolveTitleVsSidePortContentInsetTop, a public helper letting callers who construct raw LayoutBox/LayoutPort geometry outside LayoutGraph (e.g. SysML2Tools' InterconnectionViewLayoutStrategy) opt into the same title/port collision protection. - Update ROADMAP.md: document the two resolved items above and the new public helper, prioritize widening port spacing for same-face crowding given real-world evidence, and add a deferred item for a defensive title/connector exclusion zone. - Regenerate gallery images. Fixes issues 1-4 raised against the GantryInterconnectionView regression report (SysML2Tools integration). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…face crowding - Gate TitleReserveTop on Node.HasPorts so plain unlabeled edges no longer get their connector anchor offset by title reservation meant for named side ports (regression from earlier title/port collision fix). - Compensate box height for SvgRenderer.EmitPortLabel's asymmetric downward label offset so a Left/Right port's ExternalLabel no longer scrapes the box's bottom edge. - Fix the parallel-edge label-spacing floor to also recognize named port ExternalLabels (not just an edge's own mid-line Label), so two or more labeled ports sharing one face grow the box enough to avoid crowding/overlapping each other and the box title. - Add PortsShowcaseMultiConnectorHorizontal/Vertical gallery diagrams exercising two named, differently-sized ports per face, to catch same-face crowding regressions visually going forward. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Ports sharing a face were being placed at a fixed 10px inset from the box edges regardless of face size or count, so on Top/Bottom faces (where labels render centered on the port X) the first/last port's label text could overflow past the box corners with only 2 ports. Replace the fixed-inset formula with an equal-area-center distribution: divide the face into count equal slices and center each port within its own slice (y = nodeTop + (k + 0.5) * height / count). This gives every port a margin from its neighbors and the face edges that is proportional to the slice size, matching ELK's equal-spacing convention, and naturally reproduces the expected 0.25/0.5/0.25 margin/gap/margin layout for two ports. Updated in lockstep across all independent copies of this formula: - PortDistributor.DistributePorts (production port placement) - BrandesKopfPlacer.BkPreprocess (alignment-heuristic port preview, whose prior mismatch with PortDistributor caused two genuine TotalHeight/TotalWidth regressions in the equivalence tests) - LegacyInterconnectionLayoutEngineOracle (test-only equivalence oracle, three inlined copies) Also updated LayeredLayoutAlgorithm's port-label growth-floor formulas (Left/Right height floor, Top/Bottom width floor) to use the new count-based spacing multiplier and a Math.Max floor instead of an additive corner-margin term. Regenerated the gallery; ports-showcase-multi-connector-horizontal/ vertical now show properly inset ports with no label overflow, and spot-checked other affected diagrams (layered-pipeline, parallel-edges-preserved, boundary-ports-showcase-vertical) for regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…xation Classic Brandes-Kopf balanced layout computes each node's Y as the median average of 4 directional passes. For a 'hub' node (a real node with 2+ differently-positioned ports on one face, each connecting to a different sibling node in an adjacent layer), each individual pass can only align the hub with ONE of its same-face neighbors, so the neighbor nodes each get only about half the correction needed - producing a systematic offset that biases all connectors toward one side (visible in the ports-showcase multi-connector-vertical gallery diagram, where the Hub box was offset from the Monitor/Control rows, forcing every connector to bend). Adds a bounded, convergent Gauss-Seidel relaxation pass after the classic balanced layout: each sweep recomputes every node's Y as the average of its neighbors' implied ideal positions (processing layers in sequential order, alternating direction each sweep for faster convergence across long layer chains), then re-projects each layer back onto its order-preserving, minimum-spacing sequence via isotonic regression (pool-adjacent-violators). Iteration stops early on convergence or after a fixed sweep cap. The relaxation is a no-op for ordinary chains and simple forks (already optimal under classic BK) and only meaningfully moves nodes for genuine hub shapes. Ported the same algorithm into LegacyInterconnectionLayoutEngineOracle.cs so the pipeline-equivalence tests remain meaningful. Verified with a hand-derived minimal reproduction and confirmed the ports-showcase-multi-connector-vertical gallery diagram now shows fully straight, hub-aligned connectors. Full test suite passes; gallery regenerated with only the expected two diagrams changed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Code review (change-review agent, gpt-5.4-mini) found that RelaxPortCentroids could shift a layout's whole Y range so some nodes landed at negative Y (relaxation only nudges positions relative to neighbors, with no absolute anchor), while downstream canvas sizing in InterconnectionLayoutEngine.Place only accounts for the far (max) extent plus padding, not the near (min) extent. This clipped the top edge of affected diagrams' boxes off the rendered SVG/PNG canvas - confirmed via pixel inspection of docs/gallery/layered-pipeline.png, where the 'Transform' box's top border was missing entirely. Fixed by re-normalizing the relaxed Y array at the end of RelaxPortCentroids, shifting the whole result so its minimum is exactly Padding again - restoring the same invariant BkBalancedLayout already established before relaxation ran. Applied identically to BrandesKopfPlacer.cs and its LegacyInterconnectionLayoutEngineOracle.cs counterpart. Verified: full test suite passes (277 layout tests x3 TFMs, plus all other projects); gallery regenerated (layered-pipeline.svg height restored to its correct 170, boundary-ports-showcase-vertical.svg and ports-showcase-multi-connector-vertical.svg re-verified visually with no clipping and dead-straight hub connectors). Also ran a formal review of the Rendering-Layout-Engine-LayeredPipeline review-set (gpt-5.4-mini) - approved with zero findings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request updates the layered layout + rendering pipeline to better handle title placement and port/label spacing, and adds gallery + test coverage demonstrating the fixes (notably for SysML2Tools-style diagrams).
Changes:
- Updated port distribution to an “equal-slice center” formula and introduced title-reserve exclusion for left/right port placement to prevent title/port-label collisions.
- Added iterative post-processing to Brandes–Köpf placement to relax residual port-centroid error for hub-like nodes (and mirrored this in the legacy oracle).
- Introduced
LayoutBox.CenterTitle(set by layout algorithms) and a new publicBoxMetrics.ResolveTitleVsSidePortContentInsetTophelper; updated docs/tests/gallery artifacts accordingly.
Reviewed changes
Copilot reviewed 20 out of 36 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| test/DemaConsulting.Rendering.Layout.Tests/Engine/Layered/PortDistributorTests.cs | Updates expected port distribution formula in tests. |
| test/DemaConsulting.Rendering.Layout.Tests/Engine/Layered/LegacyInterconnectionLayoutEngineOracle.cs | Keeps legacy oracle in lockstep (port distribution + BK relaxation). |
| test/DemaConsulting.Rendering.Gallery/GalleryShowcaseTests.cs | Adds new gallery showcase render tests for multi-connector port crowding. |
| test/DemaConsulting.Rendering.Gallery/GalleryDiagrams.cs | Adds new multi-connector showcase diagrams to exercise crowding/collision protection. |
| test/DemaConsulting.Rendering.Gallery/GalleryCatalog.cs | Registers new gallery images and descriptions. |
| test/DemaConsulting.Rendering.Abstractions.Tests/BoxMetricsTests.cs | Adds tests for the new ResolveTitleVsSidePortContentInsetTop helper. |
| src/DemaConsulting.Rendering/LayoutTree/LayoutBox.cs | Adds CenterTitle flag to control render-time title centering behavior. |
| src/DemaConsulting.Rendering.Layout/LayeredLayoutAlgorithm.cs | Threads title reserve + min-size adjustments and sets CenterTitle per emitted box. |
| src/DemaConsulting.Rendering.Layout/HierarchicalLayoutAlgorithm.cs | Threads assumed font size parameter through recursive pipeline (currently opting out). |
| src/DemaConsulting.Rendering.Layout/Engine/Layered/PortDistributor.cs | Applies title-band exclusion and switches to equal-slice-center distribution. |
| src/DemaConsulting.Rendering.Layout/Engine/Layered/MergeRegionGraphAssembler.cs | Adds assumed-font-size parameter to support title reserve in recursive graphs. |
| src/DemaConsulting.Rendering.Layout/Engine/Layered/LayeredLayoutPipeline.cs | Plumbs assumed-font-size into merge-region graph construction. |
| src/DemaConsulting.Rendering.Layout/Engine/Layered/LayeredLayoutMetrics.cs | Adds shared constants + title-reserve-top computation helper. |
| src/DemaConsulting.Rendering.Layout/Engine/Layered/BrandesKopfPlacer.cs | Adds port-centroid relaxation + updates preprocessing port distribution to match pipeline. |
| src/DemaConsulting.Rendering.Layout/Engine/InterconnectionLayoutEngine.cs | Extends LayerNode with TitleReserveTop metadata. |
| src/DemaConsulting.Rendering.Layout/ContainmentLayoutAlgorithm.cs | Sets CenterTitle for leaf boxes emitted by containment algorithm. |
| src/DemaConsulting.Rendering.Abstractions/BoxMetrics.cs | Adds public inset helper and shifts title centering to use LayoutBox.CenterTitle. |
| ROADMAP.md | Documents motivation, fixes, and follow-up items (connector/title defensive routing). |
| docs/gallery/README.md | Documents new gallery items. |
| docs/gallery/ports-showcase-multi-connector-horizontal.svg | Adds new rendered example SVG. |
| docs/gallery/ports-showcase-multi-connector-vertical.svg | Adds new rendered example SVG. |
| docs/gallery/ports-showcase-horizontal.svg | Updates existing rendered SVG output. |
| docs/gallery/parallel-edges-preserved.svg | Updates existing rendered SVG output. |
| docs/gallery/parallel-edges-preserved-vertical.svg | Updates existing rendered SVG output. |
| docs/gallery/layered-pipeline.svg | Updates existing rendered SVG output. |
| docs/gallery/direction-right.svg | Updates existing rendered SVG output. |
| docs/gallery/direction-down.svg | Updates existing rendered SVG output. |
| docs/gallery/boundary-ports-showcase-vertical.svg | Updates existing rendered SVG output. |
| .cspell.yaml | Adds “Seidel” to spelling dictionary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+128
to
+131
| private static double TitleReserveFor(LayoutDirection direction, LayerNode node) => | ||
| direction is LayoutDirection.Right or LayoutDirection.Left | ||
| ? Math.Min(node.TitleReserveTop, node.Height) | ||
| : 0.0; |
Comment on lines
+133
to
137
| /// <summary> | ||
| /// Distributes port Y positions along a node face by dividing it into <c>count</c> equal-width | ||
| /// areas and centering each port within its own area (ELK-style "equal spacing" convention). | ||
| /// </summary> | ||
| /// <remarks> |
Comment on lines
+180
to
+181
| // Assert: each recorded port Y matches the equal-area-center formula exactly — port k centred | ||
| // within the k-th of `count` equal slices of the face. |
Comment on lines
+309
to
+314
| /// <summary> | ||
| /// Renders the multi-connector horizontal ports showcase to SVG, proving two independently | ||
| /// labelled <see cref="LayoutGraphPort"/>s sharing a single left or right face spread evenly | ||
| /// without collapsing onto one row, and that the hub's title stays clear of both stacked port | ||
| /// rows on either side. | ||
| /// </summary> |
Comment on lines
+324
to
+329
| /// <summary> | ||
| /// Renders the multi-connector vertical ports showcase to SVG — the companion to | ||
| /// <see cref="Gallery_PortsShowcaseMultiConnectorHorizontal_RendersSvg"/> — proving the same | ||
| /// same-face crowding and title-collision protection when PortDistributor spreads anchors | ||
| /// horizontally along a top/bottom face instead of vertically along a left/right one. | ||
| /// </summary> |
Comment on lines
+423
to
+426
| /// <summary> | ||
| /// Renders the multi-connector vertical ports showcase to PNG, proving the raster path | ||
| /// handles two labelled ports sharing a single top or bottom face. | ||
| /// </summary> |
Comment on lines
+483
to
+491
| /// <summary> | ||
| /// A five-node left-to-right chain exercising <em>two</em> named <see cref="LayoutGraphPort"/>s | ||
| /// on each of the middle node's left and right sides — the multi-connector companion to | ||
| /// <see cref="PortsShowcaseHorizontal"/>'s single-port-per-side case, demonstrating | ||
| /// same-face crowding: two independently-labelled ports sharing one face, one of them | ||
| /// deliberately long, so the box's <see cref="LayoutBox.ContentInsetLeft"/>/Right auto-grow | ||
| /// from the widest same-side label and PortDistributor's even spacing between the two anchors | ||
| /// never collapses onto a single row. | ||
| /// </summary> |
Comment on lines
+529
to
+535
| /// <summary> | ||
| /// A five-node top-to-bottom chain exercising <em>two</em> named <see cref="LayoutGraphPort"/>s | ||
| /// on each of the middle node's top and bottom sides — the vertical companion to | ||
| /// <see cref="PortsShowcaseMultiConnectorHorizontal"/>, proving the same same-face crowding and | ||
| /// title-collision protection when PortDistributor spreads anchors horizontally along a | ||
| /// top/bottom face instead of vertically along a left/right one. | ||
| /// </summary> |
Comment on lines
1813
to
+1817
| /// <summary> | ||
| /// Evenly distributes port Y positions along a node face, with | ||
| /// <see cref="ConnectorClearance"/> inset from the top and bottom edges. | ||
| /// Distributes port Y positions along a node face by dividing it into <c>count</c> equal-width | ||
| /// areas and centering each port within its own area (kept in lockstep with | ||
| /// <c>PortDistributor.DistributePorts</c> so this legacy oracle remains a valid equivalence | ||
| /// baseline for the refactored pipeline). |
Comment on lines
112
to
+116
| double ContentInsetLeft = 0.0, | ||
| double ContentInsetRight = 0.0, | ||
| double ContentInsetTop = 0.0, | ||
| double ContentInsetBottom = 0.0) : LayoutNode; | ||
| double ContentInsetBottom = 0.0, | ||
| bool CenterTitle = false) : LayoutNode; |
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
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 introduces several improvements and fixes to the box layout and rendering logic, particularly around title and port label placement, to address real-world issues surfaced by SysML2Tools diagrams. The main themes are: (1) improved handling of title/port label collisions, (2) more robust detection of leaf boxes for title centering, (3) new public helpers for external layout strategies, and (4) documentation and test updates reflecting these changes.
Title and Port Label Collision Prevention:
PortDistributorand the layout algorithms reserve space at the top of the box for the title when left/right ports are present, ensuring clear separation.Leaf Box Title Centering Logic:
CenterTitleflag set by layout algorithms at box creation. This avoids mis-centering titles in special cases (e.g., root boxes with flat siblings), ensuring only true leaf boxes have centered titles. [1] [2] [3] [4]Public Helper for External Layouts:
BoxMetrics.ResolveTitleVsSidePortContentInsetTop, a public helper for callers that constructLayoutBox/port geometry directly (outside the standard layout algorithms), so they can opt into the same title-vs-side-port protection. [1] [2]Documentation and Roadmap Updates:
ROADMAP.mdto document the real-world motivation for these changes, the new logic, and remaining open concerns (such as defensive connector routing around titles, which is deferred for now). [1] [2] [3]Other Minor Changes:
LayerNodestruct to include aTitleReserveTopfield, documenting and tracking the reserved title area for each node. [1] [2]