fix(hierarchical): compress level values to eliminate gaps in cyclic graphs#2219
Conversation
|
Update: Fixed an edge case where Without this fallback, all nodes would be placed on level 0, which can cause the graph to render in an unintended direction. For example, if the user specifies direction: "LR", the result may visually resemble a "UD" layout due to all nodes being on the same level. |
Thomaash
left a comment
There was a problem hiding this comment.
Was your intent to only submit the // Added: Compress level values to eliminate gaps (for cyclic) part? If so, then just delete all the other changes around, address the 3 comments I have there (should be easy) and it can be merged.
The rest doesn't really make sense to me. If you wan't to keep that in the PR, I'll need a detailed explanation as to why it's useful.
Either way, thanks for your contribution, it's always great to see people trying to fix the issues they encounter.
| // Collect valid leaf nodes | ||
| const entryNodes: Node[] = []; | ||
| nodes.forEach((node) => { | ||
| // Pick only leaves (nodes without children). | ||
| const isLeaf = node.edges | ||
| // Take only visible nodes into account. | ||
| .filter((edge) => nodes.has(edge.toId)) | ||
| // Check that all edges lead to this node (leaf). | ||
| .every((edge) => edge.to === node); | ||
| if (isLeaf) { | ||
| entryNodes.push(node); | ||
| } | ||
| }); | ||
|
|
||
| if (entryNodes.length === 0) { | ||
| return fillLevelsByDirectionCyclic(nodes, {}); | ||
| } | ||
|
|
||
| return fillLevelsByDirection( | ||
| // Pick only leaves (nodes without children). | ||
| (node): boolean => | ||
| node.edges | ||
| // Take only visible nodes into account. | ||
| .filter((edge): boolean => nodes.has(edge.toId)) | ||
| // Check that all edges lead to this node (leaf). | ||
| .every((edge): boolean => edge.to === node), | ||
| (node): boolean => entryNodes.includes(node), |
There was a problem hiding this comment.
What is the purpose of this? Won't the condition at the end catch cyclic graphs every time this would? It seems to me like just added overhead for people who are not abusing this layout for cyclic graphs. Off course I may be missing something.
There was a problem hiding this comment.
Hi Tomáš,
I get your point about the fallback. My previous comment outlines the issue: fillLevelsByDirection would assign all nodes to level 0 if no clear entry points (roots or leaves) were found, which happens in cyclic graphs. This led to visually confusing layouts that didn't match the intended shakeTowards direction. For example, if the user specifies direction: "LR", the result may visually resemble a "UD" layout due to all nodes being on the same level."
Let me know your thoughts!
There was a problem hiding this comment.
Then the issue is most likely in https://github.com/visjs/vis-network/blob/master/lib/network/modules/layout-engine/index.ts#L21 unless the detection in https://github.com/visjs/vis-network/blob/master/lib/network/modules/layout-engine/index.ts#L21 fails.
| // Collect valid root nodes | ||
| const entryNodes: Node[] = []; | ||
| nodes.forEach((node) => { | ||
| // Pick only leaves (nodes without children). | ||
| const isRoot = node.edges | ||
| // Take only visible nodes into account. | ||
| .filter((edge) => nodes.has(edge.toId)) | ||
| // Check that all edges lead from this node (root). | ||
| .every((edge) => edge.from === node); | ||
| if (isRoot) { | ||
| entryNodes.push(node); | ||
| } | ||
| }); | ||
|
|
||
| if (entryNodes.length === 0) { | ||
| return fillLevelsByDirectionCyclic(nodes, {}); | ||
| } | ||
|
|
||
| return fillLevelsByDirection( | ||
| // Pick only roots (nodes without parents). | ||
| (node): boolean => | ||
| node.edges | ||
| // Take only visible nodes into account. | ||
| .filter((edge): boolean => nodes.has(edge.toId)) | ||
| // Check that all edges lead from this node (root). | ||
| .every((edge): boolean => edge.from === node), | ||
| (node): boolean => entryNodes.includes(node), |
There was a problem hiding this comment.
What is the purpose of this? Won't the condition at the end catch cyclic graphs every time this would? It seems to me like just added overhead for people who are not abusing this layout for cyclic graphs. Off course I may be missing something.
It's superfluous now. The new implementation sets min to zero and removes gaps in one go.
Thomaash
left a comment
There was a problem hiding this comment.
Thanks for you contribution.
PS: I made just some small changes in order not to rename the project and removed the now superfluous code.
|
🎉 This PR is included in version 9.1.12 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Proposal: Compact level values to improve layout consistency in cyclic graphs
When working with graphs that include cycles, the fallback layout (
fillLevelsByDirectionCyclic) can sometimes assign sparse or widely spaced level values (e.g., levels like 0, 1, 50). This causes nodes to be unevenly spaced in hierarchical mode, especially in partially cyclic structures.This PR introduces a small enhancement to
setMinLevelToZero():After the usual min-level normalization, it remaps the level values to a contiguous sequence (e.g., 0, 1, 50 → 0, 1, 2).
This helps produce a more balanced layout without affecting node relationships or acyclic graphs.
The change is minimal, and improves layout consistency in edge cases. Happy to discuss if there’s a better place for this logic.
Demo: https://jsfiddle.net/uvLcg98k/21/