From eb235ba999bd1d50cff3d12a9dc8878cc97c3623 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Vy=C4=8D=C3=ADtal?= Date: Sat, 29 Aug 2020 09:11:38 +0200 Subject: [PATCH] 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 --- ...ayout-hierarchical-directed-levels.spec.ts | 93 +++++++++++++++++++ cypress/support/commands/index.ts | 1 + .../commands/vis-simple-canvas-snapshot.ts | 34 ++----- .../commands/vis-snapshot-opened-page.ts | 50 ++++++++++ lib/network/modules/LayoutEngine.js | 5 +- lib/network/modules/layout-engine/index.ts | 36 +++---- 6 files changed, 171 insertions(+), 48 deletions(-) create mode 100644 cypress/integration/visual/layout-hierarchical-directed-levels.spec.ts create mode 100644 cypress/support/commands/vis-snapshot-opened-page.ts diff --git a/cypress/integration/visual/layout-hierarchical-directed-levels.spec.ts b/cypress/integration/visual/layout-hierarchical-directed-levels.spec.ts new file mode 100644 index 0000000000..9ed2d9a397 --- /dev/null +++ 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 index d02967f9a0..66f0bbb3d7 100644 --- 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 index 0c1c5f9a00..578dd44a19 100644 --- 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; } } } -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( - { - 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 new file mode 100644 index 0000000000..6d2891ff8b --- /dev/null +++ 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 { + /** + * 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; + } + } +} + +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( + { + 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 index abfdf8dacb..de92f1373b 100644 --- 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 index 7484c53a9d..63aa0b65db 100644 --- 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, - levels: Levels = Object.create(null) -): Levels { +export function fillLevelsByDirectionLeaves(nodes: Map): Levels { return fillLevelsByDirection( // Pick only leaves (nodes without children). (node): boolean => @@ -73,8 +69,7 @@ export function fillLevelsByDirectionLeaves( (newLevel, oldLevel): boolean => oldLevel > newLevel, // Go against the direction of the edges. "from", - nodes, - levels + nodes ); } @@ -82,14 +77,10 @@ export function fillLevelsByDirectionLeaves( * 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, - levels: Levels = Object.create(null) -): Levels { +export function fillLevelsByDirectionRoots(nodes: Map): 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,7 +104,6 @@ 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. */ @@ -122,10 +111,21 @@ function fillLevelsByDirection( isEntryNode: (node: Node) => boolean, shouldLevelBeReplaced: (newLevel: number, oldLevel: number) => boolean, direction: "to" | "from", - nodes: Map, - levels: Levels + nodes: Map ): 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( + (acc, node): number => acc + 1 + node.edges.length, + 0 + ); + const edgeIdProp: "fromId" | "toId" = (direction + "Id") as "fromId" | "toId"; const newLevelDiff = direction === "to" ? 1 : -1;