Fix parallel edge routing issues in MergeParallelEdges#24
Merged
Conversation
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>
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
1 participant
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 important bug fixes and structural improvements to the layered graph layout engine, primarily addressing issues with the handling of parallel edges in multi-component graphs and ensuring robust propagation of caller-configured options. The changes ensure that all relevant input options are consistently copied when creating per-component subgraphs, preventing silent reversion to defaults and fixing cases where parallel edges were incorrectly collapsed or emitted.
Key improvements and fixes:
Core logic and bug fixes
LayeredGraph.CreateChild, a single method to create per-component subgraphs that copies all caller-configured input options (BackEdgeEntryApproach,NodeSpacing,MergeParallelEdges) from the parent, preventing silent loss of settings when laying out disconnected components. (src/DemaConsulting.Rendering.Layout/Engine/Layered/LayeredGraph.cs, src/DemaConsulting.Rendering.Layout/Engine/Layered/LayeredGraph.csR187-R224)ComponentPackerto useLayeredGraph.CreateChildinstead of manually constructing subgraphs, ensuring all options are inherited and fixing a bug whereMergeParallelEdgesandNodeSpacingwere ignored for components. (src/DemaConsulting.Rendering.Layout/Engine/Layered/ComponentPacker.cs, src/DemaConsulting.Rendering.Layout/Engine/Layered/ComponentPacker.csL367-R371)LayeredLayoutAlgorithm: the emission logic now uses undirected node pairs for grouping and checks the actual set of surviving routed edges, ensuring only genuinely surviving connectors are emitted and that collapsed edges (including those with opposite directions) are never spuriously rendered. (src/DemaConsulting.Rendering.Layout/LayeredLayoutAlgorithm.cs, [1] [2] [3] [4]Testing and safeguards
MergeParallelEdges, even in multi-component graphs. (test/DemaConsulting.Rendering.Layout.Tests/Engine/Layered/ComponentPackerTests.cs, [1];test/DemaConsulting.Rendering.Layout.Tests/LayeredLayoutAlgorithmTests.cs, [2]LayeredGraphare always copied byCreateChild, with a reflection-driven test to catch future omissions. (test/DemaConsulting.Rendering.Layout.Tests/Engine/Layered/LayeredGraphTests.cs, test/DemaConsulting.Rendering.Layout.Tests/Engine/Layered/LayeredGraphTests.csR65-R143)These changes significantly improve the reliability and maintainability of the layout engine, ensuring that option propagation is robust and that parallel edge handling is consistent and correct across all scenarios.