From 01c9b0363c04e62dfd2670e059381ab2a939b422 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Wed, 15 Jul 2026 21:18:01 -0400 Subject: [PATCH] Fix silently-dropped parallel-edge routes under MergeParallelEdges Fixes two independent bugs that both manifested as a parallel edge silently falling back to a raw straight line between two node centres instead of a properly-anchored orthogonal connector, with no warning: 1. MergeParallelEdges=true (default): two edges declared in opposite directions between the same node pair (e.g. A->B and B->A) collapsed inconsistently. CycleBreaker normalizes direction before deciding which edges collapse together, so it correctly dropped one of them, but LayeredLayoutAlgorithm's emission loop compared the raw, un-normalized declared direction and did not recognize the pair as collapsed, so it still tried to emit the dropped edge (which had no computed route). Fixed by using the routes dictionary itself as the ground truth for which edges survived, instead of a second, direction-blind duplicate rule. Also fixed the adjacent collapsed-label-suppression check to key on the undirected node pair for the same reason. 2. MergeParallelEdges=false: when the input graph split into 2+ connected components, ComponentPacker built each component's child LayeredGraph inline and only copied BackEdgeEntryApproach, silently losing the caller's MergeParallelEdges=false (and NodeSpacing) override for every component. Fixed by introducing LayeredGraph.CreateChild, a single factory that copies every caller-configured input option, backed by a reflection-driven completeness test (LayeredGraph_CreateChild_CopiesEveryKnownInputOption) that fails the build if a future input option is added without being classified as either copied or a computed output - structurally preventing this class of bug from recurring. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Engine/Layered/ComponentPacker.cs | 12 +-- .../Engine/Layered/LayeredGraph.cs | 38 ++++++++ .../LayeredLayoutAlgorithm.cs | 40 +++++--- .../Engine/Layered/ComponentPackerTests.cs | 41 ++++++++ .../Engine/Layered/LayeredGraphTests.cs | 80 ++++++++++++++++ .../LayeredLayoutAlgorithmTests.cs | 94 +++++++++++++++++++ 6 files changed, 285 insertions(+), 20 deletions(-) diff --git a/src/DemaConsulting.Rendering.Layout/Engine/Layered/ComponentPacker.cs b/src/DemaConsulting.Rendering.Layout/Engine/Layered/ComponentPacker.cs index b047728..57ab5bd 100644 --- a/src/DemaConsulting.Rendering.Layout/Engine/Layered/ComponentPacker.cs +++ b/src/DemaConsulting.Rendering.Layout/Engine/Layered/ComponentPacker.cs @@ -364,13 +364,11 @@ private ComponentLayout LayoutComponent(LayeredGraph graph, List members) } } - // Lay out the component sub-graph with the wrapped inner stages. Propagate the parent's - // BackEdgeEntryApproach so a caller-customized reversed-edge clearance is honored for packed - // disconnected components instead of silently reverting to the LayeredGraph default. - var child = new LayeredGraph(localNodes, localEdges, graph.Direction) - { - BackEdgeEntryApproach = graph.BackEdgeEntryApproach, - }; + // Lay out the component sub-graph with the wrapped inner stages. LayeredGraph.CreateChild + // copies every one of the parent's caller-configured input options (BackEdgeEntryApproach, + // NodeSpacing, MergeParallelEdges) in one place, so a component sub-graph is never laid out + // under silently-reverted-to-default settings. + var child = LayeredGraph.CreateChild(localNodes, localEdges, graph); RunInner(child); // Map the child's acyclic edges (local indices) back to original node indices so the merged diff --git a/src/DemaConsulting.Rendering.Layout/Engine/Layered/LayeredGraph.cs b/src/DemaConsulting.Rendering.Layout/Engine/Layered/LayeredGraph.cs index b4a86e7..dc1dc8f 100644 --- a/src/DemaConsulting.Rendering.Layout/Engine/Layered/LayeredGraph.cs +++ b/src/DemaConsulting.Rendering.Layout/Engine/Layered/LayeredGraph.cs @@ -184,6 +184,44 @@ public LayeredGraph( /// public bool MergeParallelEdges { get; set; } = true; + /// + /// Creates a fresh child for one connected component of + /// (used by ), copying every caller-configured + /// input option — , , and + /// — from , so a component sub-graph is + /// laid out under exactly the same caller-requested settings as the whole graph. + /// + /// + /// This is the single place a component's child graph is assembled, specifically so a future caller + /// option never again has to be remembered at each construction call site: add the new property + /// next to the three above, add its copy line here, and + /// LayeredGraphCreateChildTests.CreateChild_CopiesEveryKnownInputOption (a reflection-driven + /// completeness test enumerating every public settable property on this class against a hardcoded + /// computed-output allowlist) will fail the build if the new property is left out of both this + /// method and that allowlist — it cannot be silently forgotten. This directly fixes a real bug where + /// previously built each component's child graph inline and copied only + /// , silently losing a caller's = + /// (and ) override for any graph with 2+ connected + /// components. + /// + /// The component's real nodes, remapped to dense local indices. + /// The component's edges, remapped to the same local indices. + /// The whole graph this component was split from. + /// A fresh child graph carrying every one of 's input options. + public static LayeredGraph CreateChild( + IReadOnlyList nodes, + IReadOnlyList edges, + LayeredGraph parent) + { + ArgumentNullException.ThrowIfNull(parent); + return new LayeredGraph(nodes, edges, parent.Direction) + { + BackEdgeEntryApproach = parent.BackEdgeEntryApproach, + NodeSpacing = parent.NodeSpacing, + MergeParallelEdges = parent.MergeParallelEdges, + }; + } + /// Gets or sets the acyclic edge set after cycle breaking. public List Acyclic { get; set; } = []; diff --git a/src/DemaConsulting.Rendering.Layout/LayeredLayoutAlgorithm.cs b/src/DemaConsulting.Rendering.Layout/LayeredLayoutAlgorithm.cs index 5e63e79..4437d3f 100644 --- a/src/DemaConsulting.Rendering.Layout/LayeredLayoutAlgorithm.cs +++ b/src/DemaConsulting.Rendering.Layout/LayeredLayoutAlgorithm.cs @@ -130,17 +130,21 @@ protected internal override LayoutTree ApplyCore(LayoutGraph graph, LayoutOption var nodeSpacing = ResolveNodeSpacing(graph, options); var mergeParallelEdges = ResolveMergeParallelEdges(graph, options); - // Count how many raw input edges share each (source, target) pair, so the final connector - // emission loop can tell whether an emitted line is a genuine single edge or the survivor of - // 2+ collapsed parallel edges. Only needed when MergeParallelEdges is true (when it is false, - // every edge is emitted independently and always keeps its own label). Independent of node - // sizes, so it is computed once and unaffected by the Fix 5 auto-grow re-pass below. - var pairCounts = new Dictionary<(int Source, int Target), int>(); + // Count how many raw input edges share each node pair, so the final connector emission loop + // can tell whether an emitted line is a genuine single edge or the survivor of 2+ collapsed + // parallel edges. Keyed by the *undirected* pair (not the raw declared direction) because + // CycleBreaker's own merge/de-duplication normalizes direction (reversing cycle-causing back edges) + // before deciding which edges collapse together, so two edges declared as A->B and B->A can + // still collapse into a single survivor; grouping here must match that or this count silently + // misses direction-mixed parallel groups. Only needed when MergeParallelEdges is true (when it + // is false, every edge is emitted independently and always keeps its own label). Independent + // of node sizes, so it is computed once and unaffected by the Fix 5 auto-grow re-pass below. + var pairCounts = new Dictionary<(int Low, int High), int>(); if (mergeParallelEdges) { foreach (var edge in engineEdges) { - var key = (edge.Source, edge.Target); + var key = (Math.Min(edge.Source, edge.Target), Math.Max(edge.Source, edge.Target)); pairCounts[key] = pairCounts.TryGetValue(key, out var existing) ? existing + 1 : 1; } } @@ -168,9 +172,12 @@ protected internal override LayoutTree ApplyCore(LayoutGraph graph, LayoutOption // Build an engine-edge-index -> (waypoints, reversed) lookup from the acyclic edge set the // engine routed. When MergeParallelEdges is true, CycleBreaker has already collapsed - // parallel edges into a single acyclic edge per node pair, so only the surviving engine - // edge resolves here; the emission loop below independently skips the non-surviving - // duplicates using the same first-occurrence rule, so the two decisions always agree. + // parallel edges (including ones declared in opposite directions between the same node + // pair, which it normalizes via its own back-edge reversal before comparing) into a single + // acyclic edge per node pair, so only the surviving engine edge resolves here. The + // emission loop below treats "no entry in this dictionary" as the ground truth for "this + // edge was collapsed away", rather than re-deriving its own (necessarily direction-blind) + // duplicate rule, so the two stages can never disagree about which edges survived. var routes = new Dictionary Waypoints, bool Reversed)>(); for (var k = 0; k < passResult.AcyclicEdges.Count; k++) { @@ -183,7 +190,6 @@ protected internal override LayoutTree ApplyCore(LayoutGraph graph, LayoutOption // Decide which edges are emitted (honoring MergeParallelEdges) and resolve each emitted // edge's normalized (source -> target) waypoints, so port sides/labels can be aggregated // per node before the boxes (which need the resulting content insets) are built. - var emittedPairsLocal = new HashSet<(int Source, int Target)>(); var emissionsLocal = new List<( LayoutGraphEdge Edge, int Source, @@ -197,7 +203,13 @@ protected internal override LayoutTree ApplyCore(LayoutGraph graph, LayoutOption var s = engineEdges[i].Source; var t = engineEdges[i].Target; - if (mergeParallelEdges && !emittedPairsLocal.Add((s, t))) + // A self-loop (s == t) is always dropped by CycleBreaker regardless of + // MergeParallelEdges, yet must still be emitted (via ResolveRoute's centre-to-centre + // fallback below) so the connector remains visible. A non-self-loop edge with no + // surviving route was genuinely collapsed into another parallel edge — possibly one + // declared in the opposite direction — by CycleBreaker's merge/de-duplication, so it is skipped + // here rather than falling back to a spurious straight line through both node centres. + if (mergeParallelEdges && s != t && !routes.ContainsKey(i)) { continue; } @@ -542,7 +554,9 @@ void RecordAnchor(int nodeIndex, Point2D anchor, string? label) foreach (var emission in emissions) { var collapsed = mergeParallelEdges && - pairCounts.TryGetValue((emission.Source, emission.Target), out var pairCount) && + pairCounts.TryGetValue( + (Math.Min(emission.Source, emission.Target), Math.Max(emission.Source, emission.Target)), + out var pairCount) && pairCount > 1; nodes.Add(new LayoutLine( diff --git a/test/DemaConsulting.Rendering.Layout.Tests/Engine/Layered/ComponentPackerTests.cs b/test/DemaConsulting.Rendering.Layout.Tests/Engine/Layered/ComponentPackerTests.cs index 8b25382..ca5af33 100644 --- a/test/DemaConsulting.Rendering.Layout.Tests/Engine/Layered/ComponentPackerTests.cs +++ b/test/DemaConsulting.Rendering.Layout.Tests/Engine/Layered/ComponentPackerTests.cs @@ -112,6 +112,47 @@ public void ComponentPacker_Apply_SingleComponent_EqualsDefaultPipeline() } } + /// + /// Regression test for a bug where a component's per-connected-component child graph (built by + /// when the input graph splits into 2+ connected components) was + /// constructed inline and silently forgot to copy the parent's + /// (and ) override, so each component's child graph + /// silently reverted to the defaults regardless of what the caller + /// requested on the parent. With MergeParallelEdges = false requested on the parent, two + /// parallel edges within one component used to collapse down to one acyclic edge inside that + /// component's child graph (as if merging had been requested), silently dropping the second + /// edge's route entirely and leaving it to a crude fallback line at the outer + /// LayeredLayoutAlgorithm layer. Proves both parallel edges now survive as their own + /// acyclic entries even when a second, entirely disconnected component forces the multi-component + /// packing path. + /// + [Fact] + public void ComponentPacker_Apply_MergeParallelEdgesFalse_MultiComponentGraph_RetainsEveryParallelEdge() + { + // Arrange: two nodes {0,1} joined by two parallel edges, plus an entirely disconnected pair + // {2,3} forming a second connected component, so the multi-component packing path runs. + var nodes = new List + { + new(NodeWidth, NodeHeight), + new(NodeWidth, NodeHeight), + new(NodeWidth, NodeHeight), + new(NodeWidth, NodeHeight), + }; + var edges = new List { new(0, 1), new(0, 1), new(2, 3) }; + var graph = new LayeredGraph(nodes, edges, LayoutDirection.Right) { MergeParallelEdges = false }; + + // Act. + ComponentPacker.WithDefaultStages().Apply(graph); + + // Assert: both parallel edges between nodes 0 and 1 survived as their own acyclic entries + // (original edge indices 0 and 1), each with a route computed — not collapsed to one. + var parallelEdgeOriginalIndices = graph.AcyclicOriginalIndex + .Where(idx => idx is 0 or 1) + .OrderBy(idx => idx) + .ToList(); + Assert.Equal([0, 1], parallelEdgeOriginalIndices); + } + /// An empty graph is laid out as a no-op without throwing. [Fact] public void ComponentPacker_Apply_EmptyGraph_IsNoOp() diff --git a/test/DemaConsulting.Rendering.Layout.Tests/Engine/Layered/LayeredGraphTests.cs b/test/DemaConsulting.Rendering.Layout.Tests/Engine/Layered/LayeredGraphTests.cs index 060598a..7ba466f 100644 --- a/test/DemaConsulting.Rendering.Layout.Tests/Engine/Layered/LayeredGraphTests.cs +++ b/test/DemaConsulting.Rendering.Layout.Tests/Engine/Layered/LayeredGraphTests.cs @@ -2,6 +2,7 @@ // Copyright (c) DemaConsulting. All rights reserved. // +using System.Reflection; using DemaConsulting.Rendering.Layout.Engine; using DemaConsulting.Rendering.Layout.Engine.Layered; @@ -61,4 +62,83 @@ public void LayeredGraph_Constructor_ValidInput_StoresNodesEdgesDirectionAndCoun Assert.Equal(LayoutDirection.Right, graph.Direction); Assert.Equal(3, graph.N); } + + /// + /// Every public settable property on is either a pipeline-computed + /// output (listed in the local allowlist below, since a fresh child must compute its own) or is + /// copied by . This is a structural safeguard, not just a + /// regression test for one bug: it fails the build the moment a future caller-configurable + /// option is added to without a conscious decision being made about + /// whether 's per-component child graphs should inherit it — exactly + /// the class of bug this test was written to catch (a real one: a component's child graph used + /// to be built inline and silently forgot to copy MergeParallelEdges and + /// NodeSpacing, so any graph split into 2+ connected components silently reverted those + /// settings to their defaults for every component). + /// + [Fact] + public void LayeredGraph_CreateChild_CopiesEveryKnownInputOption() + { + // Every public settable property that CreateChild is NOT expected to copy, because it is + // pipeline-computed output state a fresh child graph must derive for itself, not a + // caller-configured input option. + string[] computedOutputAllowlist = + [ + nameof(LayeredGraph.InputAxesNormalized), + nameof(LayeredGraph.Acyclic), + nameof(LayeredGraph.AcyclicOriginalIndex), + nameof(LayeredGraph.AcyclicReversed), + nameof(LayeredGraph.NodeLayers), + nameof(LayeredGraph.AugNodes), + nameof(LayeredGraph.AugEdges), + nameof(LayeredGraph.Groups), + nameof(LayeredGraph.AugX), + nameof(LayeredGraph.AugY), + nameof(LayeredGraph.ColumnX), + nameof(LayeredGraph.MaxColWidth), + nameof(LayeredGraph.AugPortYSrc), + nameof(LayeredGraph.AugPortYTgt), + nameof(LayeredGraph.AugBendPoints), + nameof(LayeredGraph.Waypoints), + ]; + + var settableProperties = typeof(LayeredGraph) + .GetProperties(BindingFlags.Public | BindingFlags.Instance) + .Where(p => p.CanWrite) + .ToList(); + + var inputOptionProperties = settableProperties + .Where(p => !computedOutputAllowlist.Contains(p.Name)) + .ToList(); + + // Fail loudly (naming the property) if a new settable property shows up that this test does + // not yet know how to classify, rather than silently skipping it. + Assert.True( + inputOptionProperties.Count > 0, + "Expected at least one caller-configured input option property on LayeredGraph."); + + // Arrange: a parent graph with every known input option set to a distinctive non-default value. + var parent = new LayeredGraph([], [], LayoutDirection.Down) + { + BackEdgeEntryApproach = 123.5, + NodeSpacing = 456.5, + MergeParallelEdges = false, + }; + + // Act + var child = LayeredGraph.CreateChild([], [], parent); + + // Assert: every input-option property was copied onto the child, and the allowlisted + // computed-output properties were left at their own fresh defaults (not the parent's). + foreach (var property in inputOptionProperties) + { + var expected = property.GetValue(parent); + var actual = property.GetValue(child); + Assert.Equal(expected, actual); + } + + // Direction is derived from the parent (not itself in the allowlist above, since it is neither + // a mutable input option nor copied by field-initializer — CreateChild takes it from the + // parent's own Direction property directly), confirming the child was built for the same flow. + Assert.Equal(parent.Direction, child.Direction); + } } diff --git a/test/DemaConsulting.Rendering.Layout.Tests/LayeredLayoutAlgorithmTests.cs b/test/DemaConsulting.Rendering.Layout.Tests/LayeredLayoutAlgorithmTests.cs index 96b55a9..d54d9dd 100644 --- a/test/DemaConsulting.Rendering.Layout.Tests/LayeredLayoutAlgorithmTests.cs +++ b/test/DemaConsulting.Rendering.Layout.Tests/LayeredLayoutAlgorithmTests.cs @@ -668,6 +668,100 @@ public void Apply_SingleEdge_MergeParallelEdgesDefaultTrue_KeepsOwnLabel() Assert.Equal("only", line.MidpointLabel); } + /// + /// Regression test for a bug where two edges declared in opposite directions between the same + /// node pair (e.g. A->B and B->A), with the default + /// (), collapsed inconsistently: the engine's cycle-breaking stage + /// normalizes direction (reversing cycle-causing back edges) before deciding which edges + /// collapse together, so it correctly recognized the two edges as parallel and dropped one — + /// but the emission loop's own duplicate rule compared the raw, un-normalized declared + /// direction, so it did not recognize them as a collapsed pair and still tried to emit the + /// dropped edge. That edge had no route computed for it, so it fell back to a raw straight + /// line between the two node centres — cutting through both boxes instead of a proper + /// orthogonal connector. Proves exactly one properly-anchored line survives instead. + /// + [Fact] + public void Apply_OppositeDirectionParallelEdges_MergeParallelEdgesDefaultTrue_CollapsesToOneProperlyRoutedLine() + { + // Arrange: two nodes joined by edges declared in opposite directions. + var graph = new LayoutGraph(); + var a = graph.AddNode("a", 80, 40); + a.Label = "a"; + var b = graph.AddNode("b", 80, 40); + b.Label = "b"; + graph.AddEdge("e1", a, b); + graph.AddEdge("e2", b, a); + + // Act + var tree = new LayeredLayoutAlgorithm().ApplyCore(graph, new LayoutOptions()); + + // Assert: only one connector survives, and it is a genuinely-anchored route (not the + // centre-to-centre fallback used for an unresolved/dropped edge). + var line = Assert.Single(tree.Nodes.OfType()); + var boxes = tree.Nodes.OfType().ToList(); + var boxA = boxes.Single(box => box.Label == "a"); + var boxB = boxes.Single(box => box.Label == "b"); + var centreA = new Point2D(boxA.X + (boxA.Width / 2.0), boxA.Y + (boxA.Height / 2.0)); + var centreB = new Point2D(boxB.X + (boxB.Width / 2.0), boxB.Y + (boxB.Height / 2.0)); + Assert.NotEqual(centreA, line.Waypoints[0]); + Assert.NotEqual(centreB, line.Waypoints[^1]); + } + + /// + /// Regression test for a bug where set to + /// was silently ignored for any node pair that ended up inside a + /// connected component alongside 2+ total components: the internal engine's per-component + /// packing stage built each component's child graph without propagating the caller's + /// MergeParallelEdges override, so it silently reverted to the engine's own default + /// () and collapsed the parallel edges anyway — leaving one of them with + /// no computed route, which fell back to a crude straight line between the two node centres. + /// Proves both parallel edges now survive as their own properly-routed, independently-labeled + /// connectors even when an unrelated, entirely disconnected second component is also present. + /// + [Fact] + public void Apply_MergeParallelEdgesFalse_MultiComponentGraph_RetainsEveryParallelEdge() + { + // Arrange: two nodes {a,b} joined by two parallel edges (mirroring a "connect" + + // "dependency" pair between the same two boxes), plus an entirely unrelated, disconnected + // pair {c,d} forming a second connected component so the multi-component packing path runs. + var graph = new LayoutGraph(); + graph.Set(CoreOptions.MergeParallelEdges, false); + + var a = graph.AddNode("a", 80, 40); + a.Label = "a"; + var b = graph.AddNode("b", 80, 40); + b.Label = "b"; + graph.AddEdge("connect", a, b).Label = "connect"; + graph.AddEdge("dependency", a, b).Label = "dependency"; + + var c = graph.AddNode("c", 80, 40); + var d = graph.AddNode("d", 80, 40); + graph.AddEdge("cd", c, d); + + // Act + var tree = new LayeredLayoutAlgorithm().ApplyCore(graph, new LayoutOptions()); + + // Assert: both parallel edges between a and b survive, each keeping its own label, and + // neither degenerates to the centre-to-centre fallback used for an unresolved/dropped edge. + var boxes = tree.Nodes.OfType().ToList(); + var boxA = boxes.Single(box => box.Label == "a"); + var boxB = boxes.Single(box => box.Label == "b"); + var centreA = new Point2D(boxA.X + (boxA.Width / 2.0), boxA.Y + (boxA.Height / 2.0)); + var centreB = new Point2D(boxB.X + (boxB.Width / 2.0), boxB.Y + (boxB.Height / 2.0)); + + var abLines = tree.Nodes.OfType() + .Where(l => l.MidpointLabel is "connect" or "dependency") + .ToList(); + Assert.Equal(2, abLines.Count); + Assert.Equal(["connect", "dependency"], abLines.Select(l => l.MidpointLabel).OrderBy(l => l).ToList()); + + foreach (var line in abLines) + { + Assert.NotEqual(centreA, line.Waypoints[0]); + Assert.NotEqual(centreB, line.Waypoints[^1]); + } + } + /// /// Proves that a per-graph override wins over an /// options-scope value, consistent with every other resolved option in this algorithm.