Fix bugs in InterconnectionViewLayoutStrategy and improve layout algorithms#40
Merged
Merged
Conversation
… fix arbitrary root selection
InterconnectionViewLayoutStrategy had two bugs, both surfaced while testing
asInterconnectionDiagram against VersionMark's real (namespace-split) model:
1. CollectParts re-applied the view's expose-scope namespace-prefix filter
unchanged at every recursion depth when descending into a nested container
part's own interior. Since composition structure and namespace/file
organization are independent in SysML v2, a subsystem declared in a
different namespace than the root's exposed subject had its own nested
parts incorrectly filtered out, rendering as an empty box. Fixed by only
applying the scope filter at depth == 0 (the root's own direct children);
deeper recursion now always shows full nested structure, matching SysML v2
spec section 9.2.20.2.6 ("nested features as nested nodes").
2. FindRoot picked among scope-relevant part def candidates purely by
"most specific" (deepest qualified name), which degenerates to an
essentially arbitrary pick once a broad expose (e.g. an entire namespace
recursively) makes every definition in it scope-relevant. Fixed by
preferring the composition-graph root: a candidate not referenced as any
other candidate's own part-feature type, and that itself composes at
least one part - falling back to the prior heuristic when no such unique
root exists.
Extracted a shared ResolveByTyping helper (feature typing -> definition, by
qualified then simple name) used by both the pre-existing TryResolveContainer
and the new FindRoot composition-graph logic.
Added two regression tests reproducing the exact shapes that broke against
the real VersionMark model: a root exposed alone with a nested subsystem in a
different namespace, and a broad namespace expose alongside an unrelated
deep-qualified orphan definition.
Verified end-to-end against VersionMark's SoftwareStructureView model: a
recursive expose of just VersionMarkSystem now correctly selects it as root
(previously picked an arbitrary leaf subsystem) and every subsystem's own
nested units (e.g. CliSubsystem's Program/Context) now render instead of
empty boxes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes a silent parallel-edge routing degeneration under MergeParallelEdges (both the true/cycle case and the false/auto-split case), where a collapsed or dropped edge fell back to a raw straight line between node centres with no warning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ActionFlowViewLayoutStrategy and StateTransitionViewLayoutStrategy both index LayeredPlacement.Place's EdgePolylines by the original input edge index. That was only ever valid by accident: with MergeParallelEdges left at its default true, a cyclic pair (a back edge sharing a node pair with a forward edge) collapses onto one routed line, so the returned EdgePolylines is shorter than the input edges list. A latent Rendering-library bug previously masked this by always emitting a (garbage, straight-line) entry per input edge even when collapsed; now that Rendering (0.1.0-beta.12) emits exactly one entry per surviving edge as documented, the blind edgePolylines[e] indexing throws IndexOutOfRangeException for any cyclic/parallel-edge graph. Fixed by passing mergeParallelEdges: false at both call sites, matching InterconnectionViewLayoutStrategy's existing usage: every input edge (including cycle back edges) is now always independently routed and keeps its own line, which is also the more correct rendering for a flow/state diagram where every transition should stay visually distinct. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GeneralViewLayoutStrategy's root scope now resolves its algorithm via "auto" instead of always pinning ContainmentLayoutAlgorithm: it classifies root-level folders/definitions by connectivity, routing connected groups through the layered algorithm and packing disconnected/singleton nodes by reading order, rather than treating every root node as an unconnected peer regardless of its actual edges. InterconnectionViewLayoutStrategy's LayOutInterior already delegated to "auto" via LayeredPlacement.PlaceWithPorts; removed the now-dead LayOutInteriorAsPeers/ComputeContentWidth fallback path (and its now-unused PeerPartSpacing/PackedCanvasAspectRatio constants) since "auto" already packs disconnected parts via containment internally, covered by the existing InterconnectionView_BuildLayout_UnconnectedPeerParts_PacksIntoCompactGridNotSingleColumn regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…and scope-depth-0 code Addresses a Medium finding from the formal review of this branch: docs still described the root graph as being placed with a hardcoded HierarchicalLayoutAlgorithm.Apply/ContainmentLayoutAlgorithm call, and described the Interconnection view's expose scope as threaded through every recursive LayOutInterior call, neither of which matches the current implementation (root placement now delegates to "auto" via AutoLayoutAlgorithm; scope filtering only applies at depth 0, with nested container recursion always passing scope: null). 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 updates the layout strategies and dependency versions for the SysML2Tools core, focusing on improving the selection and application of layout algorithms and enhancing the root selection logic for interconnection views. The changes modernize how layouts are computed, making them more adaptive to graph connectivity, and refine the logic for identifying the main definition to display in interconnection diagrams.
Layout Algorithm Modernization:
Replaces direct calls to
HierarchicalLayoutAlgorithm.Applywith the newLayoutEngine.Layoutusing the"auto"meta-algorithm (AutoLayoutAlgorithm) throughout the codebase and documentation. This allows the layout engine to automatically select between layered and containment algorithms based on node connectivity, improving flexibility and correctness in diagram layouts. [1] [2] [3] [4] [5] [6]Updates references and descriptions in documentation and XML comments to reflect the new layout algorithm approach, ensuring clarity for future maintainers and users. [1] [2] [3] [4]
Dependency and API Updates:
Upgrades the
DemaConsulting.Rendering.Layoutpackage from version0.1.0-beta.8to0.1.0-beta.12in the project file, ensuring compatibility with the new layout engine features and bug fixes.Updates usage of the
LayeredPlacement.Placemethod inActionFlowViewLayoutStrategyto explicitly disable merging of parallel edges, preserving 1:1 correspondence between input successions and routed lines.Interconnection View Improvements:
Refines the root selection logic in
InterconnectionViewLayoutStrategyto prefer the composition-graph root (a candidate not referenced as another candidate's nested part), falling back to candidates with the most connections or part usages when necessary. This leads to more intuitive and accurate diagram roots, especially when broad scopes are exposed. [1] [2] [3]Changes the application of namespace-prefix filtering for interconnection views: filters are now only applied at the root's direct children, ensuring that nested containers always show their full composition structure, regardless of namespace.
Refactors the layout of parts in interconnection views to use the "auto" layout algorithm, improving the arrangement of connected and disconnected parts within containers. [1] [2]
Code Organization:
References: [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14]