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.