Fix parallel edge routing issues in MergeParallelEdges

src/DemaConsulting.Rendering.Layout/Engine/Layered/ComponentPacker.cs

@@ -364,13 +364,11 @@ private ComponentLayout LayoutComponent(LayeredGraph graph, List<int> 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

src/DemaConsulting.Rendering.Layout/Engine/Layered/LayeredGraph.cs

@@ -184,6 +184,44 @@ public LayeredGraph(
     /// </remarks>
     public bool MergeParallelEdges { get; set; } = true;
 
+    /// <summary>
+    /// Creates a fresh child <see cref="LayeredGraph"/> for one connected component of
+    /// <paramref name="parent"/> (used by <see cref="ComponentPacker"/>), copying every caller-configured
+    /// input option — <see cref="BackEdgeEntryApproach"/>, <see cref="NodeSpacing"/>, and
+    /// <see cref="MergeParallelEdges"/> — from <paramref name="parent"/>, so a component sub-graph is
+    /// laid out under exactly the same caller-requested settings as the whole graph.
+    /// </summary>
+    /// <remarks>
+    /// 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
+    /// <c>LayeredGraphCreateChildTests.CreateChild_CopiesEveryKnownInputOption</c> (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
+    /// <see cref="ComponentPacker"/> previously built each component's child graph inline and copied only
+    /// <see cref="BackEdgeEntryApproach"/>, silently losing a caller's <see cref="MergeParallelEdges"/> =
+    /// <see langword="false"/> (and <see cref="NodeSpacing"/>) override for any graph with 2+ connected
+    /// components.
+    /// </remarks>
+    /// <param name="nodes">The component's real nodes, remapped to dense local indices.</param>
+    /// <param name="edges">The component's edges, remapped to the same local indices.</param>
+    /// <param name="parent">The whole graph this component was split from.</param>
+    /// <returns>A fresh child graph carrying every one of <paramref name="parent"/>'s input options.</returns>
+    public static LayeredGraph CreateChild(
+        IReadOnlyList<LayerNode> nodes,
+        IReadOnlyList<LayerEdge> edges,
+        LayeredGraph parent)
+    {
+        ArgumentNullException.ThrowIfNull(parent);
+        return new LayeredGraph(nodes, edges, parent.Direction)
+        {
+            BackEdgeEntryApproach = parent.BackEdgeEntryApproach,
+            NodeSpacing = parent.NodeSpacing,
+            MergeParallelEdges = parent.MergeParallelEdges,
+        };
+    }
+
     /// <summary>Gets or sets the acyclic edge set after cycle breaking.</summary>
     public List<LayerEdge> Acyclic { get; set; } = [];
 

src/DemaConsulting.Rendering.Layout/LayeredLayoutAlgorithm.cs

@@ -53,7 +53,7 @@
     public override string Id => AlgorithmId;
 
     /// <inheritdoc/>
     protected internal override LayoutTree ApplyCore(LayoutGraph graph, LayoutOptions options)
     {
         ArgumentNullException.ThrowIfNull(graph);
         ArgumentNullException.ThrowIfNull(options);
@@ -130,17 +130,21 @@
         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 @@
 
             // 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<int, (IReadOnlyList<Point2D> Waypoints, bool Reversed)>();
             for (var k = 0; k < passResult.AcyclicEdges.Count; k++)
             {
@@ -183,7 +190,6 @@
             // 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 @@
                 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 @@
         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(

test/DemaConsulting.Rendering.Layout.Tests/Engine/Layered/ComponentPackerTests.cs

@@ -112,6 +112,47 @@ public void ComponentPacker_Apply_SingleComponent_EqualsDefaultPipeline()
         }
     }
 
+    /// <summary>
+    ///     Regression test for a bug where a component's per-connected-component child graph (built by
+    ///     <see cref="ComponentPacker"/> when the input graph splits into 2+ connected components) was
+    ///     constructed inline and silently forgot to copy the parent's <see cref="LayeredGraph.MergeParallelEdges"/>
+    ///     (and <see cref="LayeredGraph.NodeSpacing"/>) override, so each component's child graph
+    ///     silently reverted to the <see cref="LayeredGraph"/> defaults regardless of what the caller
+    ///     requested on the parent. With <c>MergeParallelEdges = false</c> 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
+    ///     <c>LayeredLayoutAlgorithm</c> 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.
+    /// </summary>
+    [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<LayerNode>
+        {
+            new(NodeWidth, NodeHeight),
+            new(NodeWidth, NodeHeight),
+            new(NodeWidth, NodeHeight),
+            new(NodeWidth, NodeHeight),
+        };
+        var edges = new List<LayerEdge> { 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);
+    }
+
     /// <summary>An empty graph is laid out as a no-op without throwing.</summary>
     [Fact]
     public void ComponentPacker_Apply_EmptyGraph_IsNoOp()

test/DemaConsulting.Rendering.Layout.Tests/Engine/Layered/LayeredGraphTests.cs

@@ -2,6 +2,7 @@
 // Copyright (c) DemaConsulting. All rights reserved.
 // </copyright>
 
+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);
     }
+
+    /// <summary>
+    ///     Every public settable property on <see cref="LayeredGraph"/> is either a pipeline-computed
+    ///     output (listed in the local allowlist below, since a fresh child must compute its own) or is
+    ///     copied by <see cref="LayeredGraph.CreateChild"/>. 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 <see cref="LayeredGraph"/> without a conscious decision being made about
+    ///     whether <see cref="ComponentPacker"/>'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 <c>MergeParallelEdges</c> and
+    ///     <c>NodeSpacing</c>, so any graph split into 2+ connected components silently reverted those
+    ///     settings to their defaults for every component).
+    /// </summary>
+    [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);
+    }
 }