Clarify CoreOptions documentation and implement NodeSpacing#11
Merged
Conversation
…ry README - CoreOptions.NodeSpacing/LayerSpacing doc comments now say 'minimum spacing' (matching ELK's spacing.nodeNode/spacing.nodeNodeBetweenLayers semantics) instead of the misleading 'desired spacing', and note the routing-derived floor LayerSpacing would need to respect. - ROADMAP.md now ranks the three unimplemented CoreOptions behaviors by actual effort instead of treating them uniformly: NodeSpacing (easy, isolated constant), LayerSpacing (needs a design decision around the routing-derived CorridorMinWidth floor), and HierarchyHandling (hardest, requires a genuinely new layout mode). - Renamed docs/gallery/gallery.md to docs/gallery/README.md so GitHub renders it by default when browsing the docs/gallery folder. Updated the GalleryIndex generator, its doc comments, gallery.ps1, and all references in README.md and docs/user_guide/introduction.md accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Thread a resolved node-spacing value from CoreOptions.NodeSpacing through LayeredLayoutAlgorithm -> InterconnectionLayoutEngine.Place -> LayeredGraph -> BrandesKopfPlacer, replacing the previously-hardcoded LayeredLayoutMetrics.NodeSpacing constant used inside BrandesKopfPlacer's horizontal compaction step. Resolution follows the same graph-then-options-then-default precedence already used for CoreOptions.Direction. Also corrects CoreOptions.NodeSpacing.DefaultValue from 20.0 to 30.0 to match both ELK's own real-world default and the engine's pre-existing hardcoded constant, so implementing the read-path does not change any existing diagram's default rendering. Adds regression, monotonicity, and graph-vs-options precedence tests at both the public LayeredLayoutAlgorithm level and the internal InterconnectionLayoutEngine facade level. Adds new reqstream requirements (Rendering-Layout-LayeredAlgorithm-NodeSpacing, Rendering-Layout-InterconnectionEngine-NodeSpacing) and updates the corresponding design docs. Updates ROADMAP.md to move NodeSpacing out of the unimplemented-CoreOptions list. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request makes the bundled layered layout honor CoreOptions.NodeSpacing by threading a resolved node-spacing value from LayeredLayoutAlgorithm into InterconnectionLayoutEngine and down to the Brandes–Köpf compaction stage, while updating documentation/requirements and aligning gallery index naming to docs/gallery/README.md.
Changes:
- Implement
CoreOptions.NodeSpacingsupport end-to-end (option resolution → engine facade → layered pipeline placement). - Update the
CoreOptions.NodeSpacingdefault to30.0to preserve legacy engine output when unset. - Refresh design docs, ReqStream requirements, tests, and gallery index references (switching the gallery index to
README.md).
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/DemaConsulting.Rendering.Layout.Tests/LayeredLayoutAlgorithmTests.cs | Adds regression tests for node spacing resolution and behavior in the public layered algorithm. |
| test/DemaConsulting.Rendering.Layout.Tests/Engine/InterconnectionLayoutEngineTests.cs | Adds engine-level tests for default node spacing and widening behavior. |
| test/DemaConsulting.Rendering.Gallery/GalleryOutput.cs | Updates remarks to reflect that the gallery index is README.md. |
| test/DemaConsulting.Rendering.Gallery/GalleryIndexTests.cs | Updates test documentation to reference README.md as the generated index. |
| test/DemaConsulting.Rendering.Gallery/GalleryIndex.cs | Changes the index generator to write README.md instead of gallery.md. |
| test/DemaConsulting.Rendering.Gallery/GalleryCatalog.cs | Updates XML docs to reference README.md as the index. |
| src/DemaConsulting.Rendering/Options/CoreOptions.cs | Clarifies NodeSpacing semantics and updates its default to 30.0; refines LayerSpacing docs. |
| src/DemaConsulting.Rendering.Layout/LayeredLayoutAlgorithm.cs | Resolves CoreOptions.NodeSpacing and passes it into the engine placement call. |
| src/DemaConsulting.Rendering.Layout/Engine/Layered/LayeredGraph.cs | Adds NodeSpacing property to carry spacing through the pipeline. |
| src/DemaConsulting.Rendering.Layout/Engine/Layered/BrandesKopfPlacer.cs | Threads nodeSpacing into the Brandes–Köpf compaction logic (replacing the fixed constant). |
| src/DemaConsulting.Rendering.Layout/Engine/InterconnectionLayoutEngine.cs | Extends Place API with optional nodeSpacing and stores it on the LayeredGraph. |
| ROADMAP.md | Updates status notes to reflect NodeSpacing is now implemented; expands LayerSpacing discussion. |
| README.md | Updates gallery link to point to docs/gallery/README.md. |
| gallery.ps1 | Updates script comments to reference docs/gallery/README.md. |
| docs/user_guide/introduction.md | Updates gallery references/links to docs/gallery/README.md. |
| docs/reqstream/rendering-layout/layered-layout-algorithm.yaml | Adds requirements/tests for layered algorithm honoring node spacing with correct precedence and defaulting. |
| docs/reqstream/rendering-layout/engine/interconnection-layout-engine.yaml | Adds requirements/tests for engine nodeSpacing parameter and default behavior. |
| docs/gallery/README.md | Adds/commits a browsable gallery index at the new location/name. |
| docs/design/rendering-layout/layered-layout-algorithm.md | Updates design steps to include node-spacing resolution and threading. |
| docs/design/rendering-layout/engine/layered-pipeline.md | Documents NodeSpacing as a layered pipeline parameter affecting compaction. |
| docs/design/rendering-layout/engine/interconnection-layout-engine.md | Documents the updated Place(..., nodeSpacing) contract and behavior. |
Comments suppressed due to low confidence (1)
src/DemaConsulting.Rendering.Layout/Engine/InterconnectionLayoutEngine.cs:143
- InterconnectionLayoutEngine.Place accepts nodeSpacing but does not validate it. Negative/NaN/Infinity values can break the "minimum gap" contract and can propagate invalid geometry through the pipeline (e.g., overlapping nodes or NaN coordinates).
public static LayerResult Place(
IReadOnlyList<LayerNode> nodes,
IReadOnlyList<LayerEdge> edges,
LayoutDirection direction = LayoutDirection.Right,
double nodeSpacing = Layered.LayeredLayoutMetrics.NodeSpacing)
{
ArgumentNullException.ThrowIfNull(nodes);
ArgumentNullException.ThrowIfNull(edges);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+553
to
+559
| private static double SiblingGap(LayoutTree tree) | ||
| { | ||
| var boxes = tree.Nodes.OfType<LayoutBox>().OrderBy(b => b.Y).ToList(); | ||
| var sibling1 = boxes[1]; | ||
| var sibling2 = boxes[2]; | ||
| return sibling2.Y - (sibling1.Y + sibling1.Height); | ||
| } |
Comment on lines
+499
to
+504
| public void Apply_DefaultNodeSpacing_MatchesPriorEngineBehavior() | ||
| { | ||
| var tree = new LayeredLayoutAlgorithm().Apply(BuildFanOut(), new LayoutOptions()); | ||
|
|
||
| Assert.Equal(-5.0, SiblingGap(tree), 6); | ||
| } |
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 implements support for the
CoreOptions.NodeSpacingproperty in the bundledlayeredlayout algorithm and the underlying engine, allowing callers to control the minimum gap between sibling nodes in the same layer. The default value matches the engine's previous fixed constant, so existing diagrams are unaffected unless the option is explicitly set. The documentation, requirements, and user guide are updated to reflect this new behavior. Additionally, several references to the gallery documentation are updated for consistency.Node spacing support in layout engine and algorithm:
src/DemaConsulting.Rendering.Layout/Engine/InterconnectionLayoutEngine.cs,src/DemaConsulting.Rendering.Layout/Engine/Layered/BrandesKopfPlacer.cs: Added anodeSpacingparameter to the engine and pipeline, threading it to the placement stage so the minimum gap between same-layer nodes is now configurable viaCoreOptions.NodeSpacing. [1] [2] [3] [4] [5] [6] [7] [8]docs/design/rendering-layout/layered-layout-algorithm.md,docs/design/rendering-layout/engine/interconnection-layout-engine.md,docs/design/rendering-layout/engine/layered-pipeline.md: Updated documentation to describe how the resolved node spacing is now used by the algorithm and engine. [1] [2] [3] [4] [5]docs/reqstream/rendering-layout/layered-layout-algorithm.yaml,docs/reqstream/rendering-layout/engine/interconnection-layout-engine.yaml: Added requirements and tests specifying that node spacing is honored and defaults to the prior engine behavior. [1] [2]ROADMAP.md: Updated to note thatCoreOptions.NodeSpacingis now implemented and honored by the bundled algorithm.Gallery and documentation reference updates:
README.md,docs/user_guide/introduction.md,gallery.ps1: Updated links and references to the gallery index to point toREADME.mdinstead ofgallery.mdfor consistency. [1] [2] [3] [4]These changes make node spacing configurable for layered layouts, improving flexibility for callers while preserving backward compatibility.