From 1ec51911d9cf437012cc99ba584a328f38b7e557 Mon Sep 17 00:00:00 2001 From: wimrijnders Date: Sat, 16 Sep 2017 18:48:50 +0200 Subject: [PATCH] Network: Fixes sorting on Layout, refactoring. (#3411) * Network: Fixes sorting on Layout sorting, refactoring. Fix for #3403 Chromium has divergent behaviour on sorting of `undefined` values, a check has been added for these. The fix itself is small, it adds a check on `undefined` in the sorting function(s). In addition: - Fixed off-by-one error on tree index - Refactored away checks on visibility by replacing `_isVisible()` with a Strategy pattern for direction The latter removes a long-standing eyesore in `LayoutManager`; it's probably also faster. There is some hope that these fixes will improve the hierarchical layout initialization, which has had problems for quite a while. * reformat code block * Add next attempt to fix Travis unit test bug * Fix unit test - timeout * Adjustments for comment reviews * Moved direction strategies to separate module; added fix for 'window is undefined' --- lib/network/modules/CanvasRenderer.js | 37 +-- lib/network/modules/LayoutEngine.js | 186 ++++--------- .../modules/components/DirectionStrategy.js | 244 ++++++++++++++++++ test/Network.test.js | 2 +- 4 files changed, 309 insertions(+), 160 deletions(-) create mode 100644 lib/network/modules/components/DirectionStrategy.js diff --git a/lib/network/modules/CanvasRenderer.js b/lib/network/modules/CanvasRenderer.js index 2a0a6b3b..b317cfec 100644 --- a/lib/network/modules/CanvasRenderer.js +++ b/lib/network/modules/CanvasRenderer.js @@ -149,31 +149,18 @@ class CanvasRenderer { // This is not something that will happen in normal operation, but we still need // to take it into account. // - let timer; - - try { - if(typeof window === 'undefined') { // (window === undefined) doesn't work here! - return; - } - - var myWindow = window; // Grab a reference to reduce the possibility that 'window' is reset - // while running this method. - - if (this.requiresTimeout === true) { - // wait given number of milliseconds and perform the animation step function - timer = myWindow.setTimeout(callback, delay); - } else { - if (myWindow.requestAnimationFrame) { - timer = myWindow.requestAnimationFrame(callback); - } - } - } catch(e) { - var msg = e.message; - //console.warn("Got exception with message: '" + msg + "'"); - if (msg === 'window is not defined') { - console.warn("CanvasRenderer._requestNextFrame: error '" + msg + "' happened again"); - } else { - throw e; + if (typeof window === 'undefined') return; // Note: `if (window === undefined)` doesn't work + + var myWindow = window; // Grab a reference to reduce the possibility that 'window' is reset + // while running this method. + let timer; + + if (this.requiresTimeout === true) { + // wait given number of milliseconds and perform the animation step function + timer = myWindow.setTimeout(callback, delay); + } else { + if (myWindow.requestAnimationFrame) { + timer = myWindow.requestAnimationFrame(callback); } } diff --git a/lib/network/modules/LayoutEngine.js b/lib/network/modules/LayoutEngine.js index a25d40e7..296d04ba 100644 --- a/lib/network/modules/LayoutEngine.js +++ b/lib/network/modules/LayoutEngine.js @@ -32,6 +32,7 @@ */ let util = require('../../util'); var NetworkUtil = require('../NetworkUtil').default; +var {HorizontalStrategy, VerticalStrategy} = require('./components/DirectionStrategy.js'); /** @@ -109,6 +110,8 @@ class HierarchicalStatus { * @param {string|number} treeId */ setTreeIndex(node, treeId) { + if (treeId === undefined) return; // Don't bother + if (this.trees[node.id] === undefined) { this.trees[node.id] = treeId; this.treeIndex = Math.max(treeId, this.treeIndex); @@ -367,7 +370,7 @@ class LayoutEngine { return; } // get the type of static smooth curve in case it is required - let type = this.getStaticType(); + let type = this.direction.curveType(); // force all edges into static smooth curves. this.body.emitter.emit('_forceDisableDynamicCurves', type, false); @@ -406,6 +409,8 @@ class LayoutEngine { } } + this.setDirectionStrategy(); + this.body.emitter.emit('_resetHierarchicalLayout'); // because the hierarchical system needs it's own physics and smooth curve settings, // we adapt the other options if needed. @@ -451,7 +456,7 @@ class LayoutEngine { } // get the type of static smooth curve in case it is required - let type = this.getStaticType(); + let type = this.direction.curveType(); // disable smooth curves if nothing is defined. If smooth curves have been turned on, // turn them into static smooth curves. @@ -770,29 +775,17 @@ class LayoutEngine { for (let nodeId in trees) { if (trees.hasOwnProperty(nodeId)) { if (trees[nodeId] === index) { - let node = this.body.nodes[nodeId]; - let pos = this._getPositionForHierarchy(node); - this._setPositionForHierarchy(node, pos + offset, undefined, true); + this.direction.shift(nodeId, offset); } } } }; - // get the width of a tree - let getTreeSize = (index) => { - let res = this.hierarchical.getTreeSize(this.body.nodes, index); - if (this._isVertical()) { - return {min: res.min_x, max: res.max_x}; - } else { - return {min: res.min_y, max: res.max_y}; - } - }; - // get the width of all trees let getTreeSizes = () => { let treeWidths = []; - for (let i = 0; i <= this.hierarchical.numTrees(); i++) { - treeWidths.push(getTreeSize(i)); + for (let i = 0; i < this.hierarchical.numTrees(); i++) { + treeWidths.push(this.direction.getTreeSize(i)); } return treeWidths; }; @@ -825,7 +818,7 @@ class LayoutEngine { if (branchMap.hasOwnProperty(branchNode)) { let node = this.body.nodes[branchNode]; let level = this.hierarchical.levels[node.id]; - let position = this._getPositionForHierarchy(node); + let position = this.direction.getPosition(node); // get the space around the node. let [minSpaceNode, maxSpaceNode] = this._getSpaceAroundNode(node,branchMap); @@ -884,8 +877,8 @@ class LayoutEngine { // callback for shifting branches let branchShiftCallback = (node1, node2, centerParent = false) => { //window.CALLBACKS.push(() => { - let pos1 = this._getPositionForHierarchy(node1); - let pos2 = this._getPositionForHierarchy(node2); + let pos1 = this.direction.getPosition(node1); + let pos2 = this.direction.getPosition(node2); let diffAbs = Math.abs(pos2 - pos1); let nodeSpacing = this.options.hierarchical.nodeSpacing; //console.log("NOW CHECKING:", node1.id, node2.id, diffAbs); @@ -954,7 +947,7 @@ class LayoutEngine { let sum = 0; for (let i = 0; i < edges.length; i++) { if (referenceNodes[edges[i].id] !== undefined) { - let a = this._getPositionForHierarchy(referenceNodes[edges[i].id]) - point; + let a = this.direction.getPosition(referenceNodes[edges[i].id]) - point; sum += a / Math.sqrt(a * a + C2); } } @@ -966,7 +959,7 @@ class LayoutEngine { let sum = 0; for (let i = 0; i < edges.length; i++) { if (referenceNodes[edges[i].id] !== undefined) { - let a = this._getPositionForHierarchy(referenceNodes[edges[i].id]) - point; + let a = this.direction.getPosition(referenceNodes[edges[i].id]) - point; sum -= (C2 * Math.pow(a * a + C2, -1.5)); } } @@ -974,7 +967,7 @@ class LayoutEngine { }; let getGuess = (iterations, edges) => { - let guess = this._getPositionForHierarchy(node); + let guess = this.direction.getPosition(node); // Newton's method for optimization let guessMap = {}; for (let i = 0; i < iterations; i++) { @@ -996,7 +989,7 @@ class LayoutEngine { let moveBranch = (guess) => { // position node if there is space - let nodePosition = this._getPositionForHierarchy(node); + let nodePosition = this.direction.getPosition(node); // check movable area of the branch if (branches[node.id] === undefined) { @@ -1028,7 +1021,7 @@ class LayoutEngine { }; let moveNode = (guess) => { - let nodePosition = this._getPositionForHierarchy(node); + let nodePosition = this.direction.getPosition(node); // position node if there is space let [minSpace, maxSpace] = this._getSpaceAroundNode(node); @@ -1044,7 +1037,7 @@ class LayoutEngine { if (newPosition !== nodePosition) { //console.log("moving Node:",diff, minSpace, maxSpace); - this._setPositionForHierarchy(node, newPosition, undefined, true); + this.direction.setPosition(node, newPosition); //this.body.emitter.emit("_redraw"); stillShifting = true; } @@ -1146,14 +1139,14 @@ class LayoutEngine { let level = this.hierarchical.levels[node.id]; if (level !== undefined) { let index = this.hierarchical.distributionIndex[node.id]; - let position = this._getPositionForHierarchy(node); + let position = this.direction.getPosition(node); let ordering = this.hierarchical.distributionOrdering[level]; let minSpace = 1e9; let maxSpace = 1e9; if (index !== 0) { let prevNode = ordering[index - 1]; if ((useMap === true && map[prevNode.id] === undefined) || useMap === false) { - let prevPos = this._getPositionForHierarchy(prevNode); + let prevPos = this.direction.getPosition(prevNode); minSpace = position - prevPos; } } @@ -1161,7 +1154,7 @@ class LayoutEngine { if (index != ordering.length - 1) { let nextNode = ordering[index + 1]; if ((useMap === true && map[nextNode.id] === undefined) || useMap === false) { - let nextPos = this._getPositionForHierarchy(nextNode); + let nextPos = this.direction.getPosition(nextNode); maxSpace = Math.min(maxSpace, nextPos - position); } } @@ -1191,12 +1184,12 @@ class LayoutEngine { // get the range of the children let newPosition = this._getCenterPosition(children); - let position = this._getPositionForHierarchy(parentNode); + let position = this.direction.getPosition(parentNode); let [minSpace, maxSpace] = this._getSpaceAroundNode(parentNode); let diff = position - newPosition; if ((diff < 0 && Math.abs(diff) < maxSpace - this.options.hierarchical.nodeSpacing) || (diff > 0 && Math.abs(diff) < minSpace - this.options.hierarchical.nodeSpacing)) { - this._setPositionForHierarchy(parentNode, newPosition, undefined, true); + this.direction.setPosition(parentNode, newPosition); } } } @@ -1218,7 +1211,7 @@ class LayoutEngine { // sort nodes in level by position: let nodeArray = Object.keys(distribution[level]); nodeArray = this._indexArrayToNodes(nodeArray); - this._sortNodeArray(nodeArray); + this.direction.sort(nodeArray); let handledNodeCount = 0; for (let i = 0; i < nodeArray.length; i++) { @@ -1229,9 +1222,9 @@ class LayoutEngine { // We get the X or Y values we need and store them in pos and previousPos. // The get and set make sure we get X or Y if (handledNodeCount > 0) { - pos = this._getPositionForHierarchy(nodeArray[i-1]) + spacing; + pos = this.direction.getPosition(nodeArray[i-1]) + spacing; } - this._setPositionForHierarchy(node, pos, level); + this.direction.setPosition(node, pos, level); this._validatePositionAndContinue(node, level, pos); handledNodeCount++; @@ -1265,7 +1258,7 @@ class LayoutEngine { } // use the positions to order the nodes. - this._sortNodeArray(childNodes); + this.direction.sort(childNodes); // position the childNodes for (let i = 0; i < childNodes.length; i++) { @@ -1279,9 +1272,12 @@ class LayoutEngine { // we get the X or Y values we need and store them in pos and previousPos. // The get and set make sure we get X or Y - if (i === 0) {pos = this._getPositionForHierarchy(this.body.nodes[parentId]);} - else {pos = this._getPositionForHierarchy(childNodes[i-1]) + spacing;} - this._setPositionForHierarchy(childNode, pos, childNodeLevel); + if (i === 0) { + pos = this.direction.getPosition(this.body.nodes[parentId]); + } else { + pos = this.direction.getPosition(childNodes[i-1]) + spacing; + } + this.direction.setPosition(childNode, pos, childNodeLevel); this._validatePositionAndContinue(childNode, childNodeLevel, pos); } else { @@ -1291,7 +1287,7 @@ class LayoutEngine { // center the parent nodes. let center = this._getCenterPosition(childNodes); - this._setPositionForHierarchy(this.body.nodes[parentId], center, parentLevel); + this.direction.setPosition(this.body.nodes[parentId], center, parentLevel); } @@ -1310,7 +1306,7 @@ class LayoutEngine { // if overlap has been detected, we shift the branch if (this.lastNodeOnLevel[level] !== undefined) { - let previousPos = this._getPositionForHierarchy(this.body.nodes[this.lastNodeOnLevel[level]]); + let previousPos = this.direction.getPosition(this.body.nodes[this.lastNodeOnLevel[level]]); if (pos - previousPos < this.options.hierarchical.nodeSpacing) { let diff = (previousPos + this.options.hierarchical.nodeSpacing) - pos; let sharedParent = this._findCommonParent(this.lastNodeOnLevel[level], node.id); @@ -1354,14 +1350,7 @@ class LayoutEngine { if (this.body.nodes.hasOwnProperty(nodeId)) { node = this.body.nodes[nodeId]; let level = this.hierarchical.levels[nodeId] === undefined ? 0 : this.hierarchical.levels[nodeId]; - if(this._isVertical()) { - node.y = this.options.hierarchical.levelSeparation * level; - node.options.fixed.y = true; - } - else { - node.x = this.options.hierarchical.levelSeparation * level; - node.options.fixed.x = true; - } + this.direction.fix(node, level); if (distribution[level] === undefined) { distribution[level] = {}; } @@ -1624,12 +1613,7 @@ class LayoutEngine { return; } progress[parentId] = true; - if(this._isVertical()) { - this.body.nodes[parentId].x += diff; - } - else { - this.body.nodes[parentId].y += diff; - } + this.direction.shift(parentId, diff); let childRef = this.hierarchical.childrenReference[parentId]; if (childRef !== undefined) { @@ -1682,93 +1666,27 @@ class LayoutEngine { return findParent(parents, childB); } + /** - * Abstract the getting of the position so we won't have to repeat the check for direction all the time + * Set the strategy pattern for handling the coordinates given the current direction. + * + * The individual instances contain all the operations and data specific to a layout direction. + * * @param {Node} node * @param {{x: number, y: number}} position * @param {number} level * @param {boolean} [doNotUpdate=false] * @private */ - _setPositionForHierarchy(node, position, level, doNotUpdate = false) { - //console.log('_setPositionForHierarchy',node.id, position) - if (doNotUpdate !== true) { - this.hierarchical.addToOrdering(node, level); - } - - if(this._isVertical()) { - node.x = position; + setDirectionStrategy() { + var isVertical = (this.options.hierarchical.direction === 'UD' + || this.options.hierarchical.direction === 'DU'); + + if(isVertical) { + this.direction = new VerticalStrategy(this); + } else { + this.direction = new HorizontalStrategy(this); } - else { - node.y = position; - } - } - - - /** - * Utility function to cut down on typing this all the time. - * - * TODO: use this in all applicable situations in this class. - * @returns {boolean} - * @private - */ - _isVertical() { - return (this.options.hierarchical.direction === 'UD' || this.options.hierarchical.direction === 'DU'); - } - - /** - * Abstract the getting of the position of a node so we do not have to repeat the direction check all the time. - * @param {Node} node - * @returns {number} - * @private - */ - _getPositionForHierarchy(node) { - if(this._isVertical()) { - return node.x; - } - else { - return node.y; - } - } - - /** - * Use the x or y value to sort the array, allowing users to specify order. - * - * @param {Array.} nodeArray - * @private - */ - _sortNodeArray(nodeArray) { - if (nodeArray.length > 1) { - if(this._isVertical()) { - nodeArray.sort(function (a, b) { - return a.x - b.x; - }) - } - else { - nodeArray.sort(function (a, b) { - return a.y - b.y; - }) - } - } - } - - - /** - * Get the type of static smooth curve in case it is required. - * - * The return value is the type to use to translate dynamic curves to - * another type, in the case of hierarchical layout. Dynamic curves do - * not work for that layout type. - * @returns {'horizontal'|'vertical'} - */ - getStaticType() { - // Node that 'type' is the edge type, and therefore 'orthogonal' to the layout type. - let type = 'horizontal'; - if (!this._isVertical()) { - type = 'vertical'; - } - - return type; } @@ -1793,7 +1711,7 @@ class LayoutEngine { childNode = this.body.nodes[childNodeId]; } - let position = this._getPositionForHierarchy(childNode); + let position = this.direction.getPosition(childNode); minPos = Math.min(minPos, position); maxPos = Math.max(maxPos, position); } diff --git a/lib/network/modules/components/DirectionStrategy.js b/lib/network/modules/components/DirectionStrategy.js new file mode 100644 index 00000000..b72e127e --- /dev/null +++ b/lib/network/modules/components/DirectionStrategy.js @@ -0,0 +1,244 @@ +/** + * Helper classes for LayoutEngine. + * + * Strategy pattern for usage of direction methods for hierarchical layouts. + */ + + +/** + * Interface definition for direction strategy classes. + * + * This class describes the interface for the Strategy + * pattern classes used to differentiate horizontal and vertical + * direction of hierarchical results. + * + * For a given direction, one coordinate will be 'fixed', meaning that it is + * determined by level. + * The other coordinate is 'unfixed', meaning that the nodes on a given level + * can still move along that coordinate. So: + * + * - `vertical` layout: `x` unfixed, `y` fixed per level + * - `horizontal` layout: `x` fixed per level, `y` unfixed + * + * The local methods are stubs and should be regarded as abstract. + * Derived classes **must** implement all the methods themselves. + * + * @private + */ +class DirectionInterface { + /** @ignore **/ + abstract() { + throw new Error("Can't instantiate abstract class!"); + } + + /** + * This is a dummy call which is used to suppress the jsdoc errors of type: + * + * "'param' is assigned a value but never used" + * + * @ignore + **/ + fake_use() { + // Do nothing special + } + + /** + * Type to use to translate dynamic curves to, in the case of hierarchical layout. + * Dynamic curves do not work for these. + * + * The value should be perpendicular to the actual direction of the layout. + * + * @return {string} Direction, either 'vertical' or 'horizontal' + */ + curveType() { return this.abstract(); } + + + /** + * Return the value of the coordinate that is not fixed for this direction. + * + * @param {Node} node The node to read + * @return {number} Value of the unfixed coordinate + */ + getPosition(node) { this.fake_use(node); return this.abstract(); } + + + /** + * Set the value of the coordinate that is not fixed for this direction. + * + * @param {Node} node The node to adjust + * @param {number} position + * @param {number} [level] if specified, the hierarchy level that this node should be fixed to + */ + setPosition(node, position, level = undefined) { this.fake_use(node, position, level); this.abstract(); } + + + /** + * Get the width of a tree. + * + * A `tree` here is a subset of nodes within the network which are not connected to other nodes, + * only among themselves. In essence, it is a sub-network. + * + * @param {number} index The index number of a tree + * @return {number} the width of a tree in the view coordinates + */ + getTreeSize(index) { this.fake_use(index); return this.abstract(); } + + + /** + * Sort array of nodes on the unfixed coordinates. + * + * @param {Array.} nodeArray array of nodes to sort + */ + sort(nodeArray) { this.fake_use(nodeArray); this.abstract(); } + + + /** + * Assign the fixed coordinate of the node to the given level + * + * @param {Node} node The node to adjust + * @param {number} level The level to fix to + */ + fix(node, level) { this.fake_use(node, level); this.abstract(); } + + + /** + * Add an offset to the unfixed coordinate of the given node. + * + * @param {NodeId} nodeId Id of the node to adjust + * @param {number} diff Offset to add to the unfixed coordinate + */ + shift(nodeId, diff) { this.fake_use(nodeId, diff); this.abstract(); } +} + + +/** + * Vertical Strategy + * + * Coordinate `y` is fixed on levels, coordinate `x` is unfixed. + * + * @extends DirectionInterface + * @private + */ +class VerticalStrategy extends DirectionInterface { + /** + * Constructor + * + * @param {Object} layout reference to the parent LayoutEngine instance. + */ + constructor(layout) { + super(); + this.layout = layout; + } + + /** @inheritdoc */ + curveType() { + return 'horizontal'; + } + + /** @inheritdoc */ + getPosition(node) { + return node.x; + } + + /** @inheritdoc */ + setPosition(node, position, level = undefined) { + if (level !== undefined) { + this.layout.hierarchical.addToOrdering(node, level); + } + node.x = position; + } + + /** @inheritdoc */ + getTreeSize(index) { + let res = this.layout.hierarchical.getTreeSize(this.layout.body.nodes, index); + return {min: res.min_x, max: res.max_x}; + } + + /** @inheritdoc */ + sort(nodeArray) { + nodeArray.sort(function(a, b) { + // Test on 'undefined' takes care of divergent behaviour in chrome + if (a.x === undefined || b.x === undefined) return 0; // THIS HAPPENS + return a.x - b.x; + }); + } + + /** @inheritdoc */ + fix(node, level) { + node.y = this.layout.options.hierarchical.levelSeparation * level; + node.options.fixed.y = true; + } + + /** @inheritdoc */ + shift(nodeId, diff) { + this.layout.body.nodes[nodeId].x += diff; + } +} + + +/** + * Horizontal Strategy + * + * Coordinate `x` is fixed on levels, coordinate `y` is unfixed. + * + * @extends DirectionInterface + * @private + */ +class HorizontalStrategy extends DirectionInterface { + /** + * Constructor + * + * @param {Object} layout reference to the parent LayoutEngine instance. + */ + constructor(layout) { + super(); + this.layout = layout; + } + + /** @inheritdoc */ + curveType() { + return 'vertical'; + } + + /** @inheritdoc */ + getPosition(node) { + return node.y; + } + + /** @inheritdoc */ + setPosition(node, position, level = undefined) { + if (level !== undefined) { + this.layout.hierarchical.addToOrdering(node, level); + } + node.y = position; + } + + /** @inheritdoc */ + getTreeSize(index) { + let res = this.layout.hierarchical.getTreeSize(this.layout.body.nodes, index); + return {min: res.min_y, max: res.max_y}; + } + + /** @inheritdoc */ + sort(nodeArray) { + nodeArray.sort(function(a, b) { + // Test on 'undefined' takes care of divergent behaviour in chrome + if (a.y === undefined || b.y === undefined) return 0; // THIS HAPPENS + return a.y - b.y; + }); + } + + /** @inheritdoc */ + fix(node, level) { + node.x = this.layout.options.hierarchical.levelSeparation * level; + node.options.fixed.x = true; + } + + /** @inheritdoc */ + shift(nodeId, diff) { + this.layout.body.nodes[nodeId].y += diff; + } +} + + +export {HorizontalStrategy, VerticalStrategy}; diff --git a/test/Network.test.js b/test/Network.test.js index 2bfccda0..2b450a15 100644 --- a/test/Network.test.js +++ b/test/Network.test.js @@ -1160,7 +1160,7 @@ describe('runs example ', function () { // Count in following also contains the helper nodes for dynamic edges assert.equal(Object.keys(network.body.nodes).length, 9964); assert.equal(Object.keys(network.body.edges).length, 9228); - done(); + done(); });