fix(layout): directed hierarchical for acyclic graphs (#996)

The infinite cycle prevention used to cut off when there were multiple one way paths between nodes, this is fixed now. Leftover levels from previous runs are also now cleared (I'm not sure if it caused any issues as all the visible node levels were always overwritten but it was definitely a little memory leak for people adding and removing nodes without reusing ids, not a good thing either way). Co-authored-by: Yotam Berkowitz <[email protected]>
visjs · Aug 29, 2020 · eb235ba · eb235ba
1 parent 84adea1
commit eb235ba
Show file tree

Hide file tree

Showing 6 changed files with 171 additions and 48 deletions.
diff --git a/cypress/integration/visual/layout-hierarchical-directed-levels.spec.ts b/cypress/integration/visual/layout-hierarchical-directed-levels.spec.ts
@@ -0,0 +1,93 @@
+const CONFIG = {
+  nodes: [
+    { id: "730", label: "730" },
+    { id: "748", label: "748" },
+    { id: "755", label: "755" },
+    { id: "757", label: "757" },
+    { id: "758", label: "758" },
+    { id: "759", label: "759" },
+    { id: "762", label: "762" },
+    { id: "780", label: "780" },
+    { id: "782", label: "782" },
+    { id: "794", label: "794" },
+    { id: "796", label: "796" },
+    { id: "813c", label: "813c" },
+    { id: "814c", label: "814c" },
+    { id: "824c", label: "824c" },
+    { id: "828c", label: "828c" },
+    { id: "838c", label: "838c" },
+    { id: "839c", label: "839c" },
+    { id: "950", label: "950" },
+    { id: "968", label: "968" },
+  ],
+  edges: [
+    { id: "730-762", from: "730", to: "762" },
+    { id: "813c-824c", from: "813c", to: "824c" },
+    { id: "730-757", from: "730", to: "757" },
+    { id: "730-759", from: "730", to: "759" },
+    { id: "814c-839c", from: "814c", to: "839c" },
+    { id: "828c-950", from: "828c", to: "950" },
+    { id: "758-780", from: "758", to: "780" },
+    { id: "762-782", from: "762", to: "782" },
+    { id: "757-796", from: "757", to: "796" },
+    { id: "755-796", from: "755", to: "796" },
+    { id: "730-755", from: "730", to: "755" },
+    { id: "814c-838c", from: "814c", to: "838c" },
+    { id: "730-758", from: "730", to: "758" },
+    { id: "839c-968", from: "839c", to: "968" },
+    { id: "824c-968", from: "824c", to: "968" },
+    { id: "828c-968", from: "828c", to: "968" },
+    { id: "838c-968", from: "838c", to: "968" },
+    { id: "796-814c", from: "796", to: "814c" },
+    { id: "796-813c", from: "796", to: "813c" },
+    { id: "757-794", from: "757", to: "794" },
+    { id: "813c-828c", from: "813c", to: "828c" },
+    { id: "759-748", from: "759", to: "748" },
+    { id: "968-748", from: "968", to: "748" },
+    { id: "782-748", from: "782", to: "748" },
+    { id: "780-748", from: "780", to: "748" },
+    { id: "950-748", from: "950", to: "748" },
+    { id: "794-748", from: "794", to: "748" },
+  ],
+  options: {
+    edges: {
+      arrows: "to",
+    },
+    layout: {
+      improvedLayout: true,
+      hierarchical: {
+        sortMethod: "directed",
+      },
+    },
+  },
+};
+
+context("Hierarchical layout directed levels", (): void => {
+  it("Without clusters", (): void => {
+    cy.visVisitUniversal(CONFIG);
+    cy.visSnapshotOpenedPage(
+      "layout-hierarchical-directed-levels-without-clusters"
+    );
+  });
+
+  it("With clusters", (): void => {
+    cy.visVisitUniversal(CONFIG, { requireNewerVersionThan: "8.2.0" });
+    cy.visRun(({ network }): void => {
+      const clusterOptionsByData = {
+        joinCondition(childOptions: { id: string | number }) {
+          return ("" + childOptions.id).endsWith("c");
+        },
+        clusterNodeProperties: {
+          id: "cluster",
+          label: "cluster",
+          borderWidth: 3,
+          shape: "database",
+        },
+      };
+      network.cluster(clusterOptionsByData);
+    });
+    cy.visSnapshotOpenedPage(
+      "layout-hierarchical-directed-levels-with-clusters"
+    );
+  });
+});
diff --git a/cypress/support/commands/index.ts b/cypress/support/commands/index.ts
@@ -13,6 +13,7 @@ export * from "./vis-place-node";
 export * from "./vis-run";
 export * from "./vis-run-with-window";
 export * from "./vis-simple-canvas-snapshot";
+export * from "./vis-snapshot-opened-page";
 export * from "./vis-stabilize-and-fit";
 export * from "./vis-stabilize-fit-and-run";
 export * from "./vis-visit-universal";
diff --git a/cypress/support/commands/vis-simple-canvas-snapshot.ts b/cypress/support/commands/vis-simple-canvas-snapshot.ts
@@ -1,6 +1,5 @@
-import { UniversalNetworkConfig, MoveToOptions } from "./types";
-import { deepObjectAssign } from "vis-util";
-import { VisVisitPageOptions } from "./vis-visit-universal";
+import { UniversalNetworkConfig } from "./types";
+import { VisSnapshotOpenedPageOptions } from "./vis-snapshot-opened-page";
 
 declare global {
   // eslint-disable-next-line no-redeclare
@@ -12,44 +11,25 @@ declare global {
        *
        * @param label - Snapshot file label. Numbers will be padded by zeros.
        * @param config - Passed to cy.visVisitUniversal.
-       * @param options - Passed to cy.visVisitUniversal.
+       * @param options - Passed to cy.visVisitUniversal and
+       * cy.visSnapshotOpenedPage.
        */
       visSimpleCanvasSnapshot(
         label: number | string,
         config?: UniversalNetworkConfig,
-        options?: VisSimpleCanvasSnapshotOptions
+        options?: VisSnapshotOpenedPageOptions
       ): Chainable<Subject>;
     }
   }
 }
 
-export interface VisSimpleCanvasSnapshotOptions extends VisVisitPageOptions {
-  moveTo?: {
-    position?: { x?: number; y?: number };
-    scale?: number;
-  };
-}
-
 // eslint-disable-next-line require-jsdoc
 export function visSimpleCanvasSnapshot(
   label: number | string,
   config: UniversalNetworkConfig = {},
-  options: VisSimpleCanvasSnapshotOptions = {}
+  options: VisSnapshotOpenedPageOptions = {}
 ): void {
   cy.visVisitUniversal(config, options);
-  cy.visRun(({ network }): void => {
-    network.moveTo(
-      deepObjectAssign<MoveToOptions>(
-        {
-          position: { x: 0, y: 0 },
-          scale: 1,
-        },
-        options.moveTo ?? {}
-      )
-    );
-  });
-  cy.get("#mynetwork canvas").compareSnapshot(
-    typeof label === "string" ? label : ("" + label).padStart(3, "0")
-  );
+  cy.visSnapshotOpenedPage(label, options);
 }
 Cypress.Commands.add("visSimpleCanvasSnapshot", visSimpleCanvasSnapshot);
diff --git a/cypress/support/commands/vis-snapshot-opened-page.ts b/cypress/support/commands/vis-snapshot-opened-page.ts
@@ -0,0 +1,50 @@
+import { MoveToOptions } from "./types";
+import { deepObjectAssign } from "vis-util";
+import { VisVisitPageOptions } from "./vis-visit-universal";
+
+declare global {
+  // eslint-disable-next-line no-redeclare
+  namespace Cypress {
+    interface Chainable<Subject> {
+      /**
+       * Take a screenshot of the canvas of opened page.
+       *
+       * @param label - Snapshot file label. Numbers will be padded by zeros.
+       * @param options - Passed to cy.visVisitUniversal.
+       */
+      visSnapshotOpenedPage(
+        label: number | string,
+        options?: VisSnapshotOpenedPageOptions
+      ): Chainable<Subject>;
+    }
+  }
+}
+
+export interface VisSnapshotOpenedPageOptions extends VisVisitPageOptions {
+  moveTo?: {
+    position?: { x?: number; y?: number };
+    scale?: number;
+  };
+}
+
+// eslint-disable-next-line require-jsdoc
+export function visSnapshotOpenedPage(
+  label: number | string,
+  options: VisSnapshotOpenedPageOptions = {}
+): void {
+  cy.visRun(({ network }): void => {
+    network.moveTo(
+      deepObjectAssign<MoveToOptions>(
+        {
+          position: { x: 0, y: 0 },
+          scale: 1,
+        },
+        options.moveTo ?? {}
+      )
+    );
+  });
+  cy.get("#mynetwork canvas").compareSnapshot(
+    typeof label === "string" ? label : ("" + label).padStart(3, "0")
+  );
+}
+Cypress.Commands.add("visSnapshotOpenedPage", visSnapshotOpenedPage);
diff --git a/lib/network/modules/LayoutEngine.js b/lib/network/modules/LayoutEngine.js
@@ -1521,12 +1521,11 @@ class LayoutEngine {
       acc.set(id, this.body.nodes[id]);
       return acc;
     }, new Map());
-    const levels = this.hierarchical.levels;
 
     if (this.options.hierarchical.shakeTowards === "roots") {
-      this.hierarchical.levels = fillLevelsByDirectionRoots(nodes, levels);
+      this.hierarchical.levels = fillLevelsByDirectionRoots(nodes);
     } else {
-      this.hierarchical.levels = fillLevelsByDirectionLeaves(nodes, levels);
+      this.hierarchical.levels = fillLevelsByDirectionLeaves(nodes);
     }
 
     this.hierarchical.setMinLevelToZero(this.body.nodes);

diff --git a/lib/network/modules/layout-engine/index.ts b/lib/network/modules/layout-engine/index.ts
@@ -53,14 +53,10 @@ function fillLevelsByDirectionCyclic(
  * Assign levels to nodes according to their positions in the hierarchy. Leaves will be lined up at the bottom and all other nodes as close to their children as possible.
  *
  * @param nodes - Visible nodes of the graph.
- * @param levels - If present levels will be added to it, if not a new object will be created.
  *
  * @returns Populated node levels.
  */
-export function fillLevelsByDirectionLeaves(
-  nodes: Map<Id, Node>,
-  levels: Levels = Object.create(null)
-): Levels {
+export function fillLevelsByDirectionLeaves(nodes: Map<Id, Node>): Levels {
   return fillLevelsByDirection(
     // Pick only leaves (nodes without children).
     (node): boolean =>
@@ -73,23 +69,18 @@ export function fillLevelsByDirectionLeaves(
     (newLevel, oldLevel): boolean => oldLevel > newLevel,
     // Go against the direction of the edges.
     "from",
-    nodes,
-    levels
+    nodes
   );
 }
 
 /**
  * Assign levels to nodes according to their positions in the hierarchy. Roots will be lined up at the top and all nodes as close to their parents as possible.
  *
  * @param nodes - Visible nodes of the graph.
- * @param levels - If present levels will be added to it, if not a new object will be created.
  *
  * @returns Populated node levels.
  */
-export function fillLevelsByDirectionRoots(
-  nodes: Map<Id, Node>,
-  levels: Levels = Object.create(null)
-): Levels {
+export function fillLevelsByDirectionRoots(nodes: Map<Id, Node>): Levels {
   return fillLevelsByDirection(
     // Pick only roots (nodes without parents).
     (node): boolean =>
@@ -102,8 +93,7 @@ export function fillLevelsByDirectionRoots(
     (newLevel, oldLevel): boolean => oldLevel < newLevel,
     // Go in the direction of the edges.
     "to",
-    nodes,
-    levels
+    nodes
   );
 }
 
@@ -114,18 +104,28 @@ export function fillLevelsByDirectionRoots(
  * @param shouldLevelBeReplaced - Checks and returns true if the level of given node should be updated to the new value.
  * @param direction - Wheter the graph should be traversed in the direction of the edges `"to"` or in the other way `"from"`.
  * @param nodes - Visible nodes of the graph.
- * @param levels - If present levels will be added to it, if not a new object will be created.
  *
  * @returns Populated node levels.
  */
 function fillLevelsByDirection(
   isEntryNode: (node: Node) => boolean,
   shouldLevelBeReplaced: (newLevel: number, oldLevel: number) => boolean,
   direction: "to" | "from",
-  nodes: Map<Id, Node>,
-  levels: Levels
+  nodes: Map<Id, Node>
 ): Levels {
-  const limit = nodes.size;
+  const levels = Object.create(null);
+
+  // If acyclic, the graph can be walked through with (most likely way) fewer
+  // steps than the number bellow. The exact value isn't too important as long
+  // as it's quick to compute (doesn't impact acyclic graphs too much), is
+  // higher than the number of steps actually needed (doesn't cut off before
+  // acyclic graph is walked through) and prevents infinite loops (cuts off for
+  // cyclic graphs).
+  const limit = [...nodes.values()].reduce<number>(
+    (acc, node): number => acc + 1 + node.edges.length,
+    0
+  );
+
   const edgeIdProp: "fromId" | "toId" = (direction + "Id") as "fromId" | "toId";
   const newLevelDiff = direction === "to" ? 1 : -1;