From 8f22d58cab1e03d885db30bb55c4c940993360cf Mon Sep 17 00:00:00 2001 From: Filip Barl Date: Fri, 7 Jul 2017 19:22:04 +0200 Subject: [PATCH] Last line of defense against overlapping nodes in graph layout (#2688) * Refresh the layout once more at the end if previous heuristics cause an overlap. * Added mixpanel tracking. --- .../charts/__tests__/node-layout-test.js | 31 ++ client/app/scripts/charts/nodes-layout.js | 374 +++++++++--------- .../utils/__tests__/math-utils-test.js | 23 ++ client/app/scripts/utils/math-utils.js | 23 ++ 4 files changed, 269 insertions(+), 182 deletions(-) diff --git a/client/app/scripts/charts/__tests__/node-layout-test.js b/client/app/scripts/charts/__tests__/node-layout-test.js index b37a11c99..47ac7ab2a 100644 --- a/client/app/scripts/charts/__tests__/node-layout-test.js +++ b/client/app/scripts/charts/__tests__/node-layout-test.js @@ -407,4 +407,35 @@ describe('NodesLayout', () => { expect(nodes.n6.x).toBeGreaterThan(nodes.n3.x); expect(nodes.n6.y).toEqual(nodes.n3.y); }); + + it('rerenders the nodes completely after the coordinates have been messed up', () => { + // Take an initial setting + let result = NodesLayout.doLayout( + nodeSets.rank4.nodes, + nodeSets.rank4.edges, + ); + + // Cache the result layout + options.cachedLayout = result; + options.nodeCache = options.nodeCache.merge(result.nodes); + options.edgeCache = options.edgeCache.merge(result.edge); + + // Shrink the coordinates of all the notes 2x to make them closer to one another + options.nodeCache = options.nodeCache.update(cache => cache.map(node => node.merge({ + x: node.get('x') / 2, + y: node.get('y') / 2, + }))); + + // Rerun the initial layout to get a trivial diff and skip all the advanced layouting logic. + result = NodesLayout.doLayout( + nodeSets.rank4.nodes, + nodeSets.rank4.edges, + options + ); + + // The layout should have updated by running into our last 'integration testing' criterion + coords = getNodeCoordinates(options.nodeCache); + resultCoords = getNodeCoordinates(result.nodes); + expect(resultCoords).not.toEqual(coords); + }); }); diff --git a/client/app/scripts/charts/nodes-layout.js b/client/app/scripts/charts/nodes-layout.js index f061f8315..a67b18840 100644 --- a/client/app/scripts/charts/nodes-layout.js +++ b/client/app/scripts/charts/nodes-layout.js @@ -4,8 +4,10 @@ import { fromJS, Map as makeMap, Set as ImmSet } from 'immutable'; import { NODE_BASE_SIZE, EDGE_WAYPOINTS_CAP } from '../constants/styles'; import { EDGE_ID_SEPARATOR } from '../constants/naming'; +import { trackMixpanelEvent } from '../utils/tracking-utils'; import { featureIsEnabledAny } from '../utils/feature-utils'; import { buildTopologyCacheId, updateNodeDegrees } from '../utils/topology-utils'; +import { minEuclideanDistanceBetweenPoints } from '../utils/math-utils'; import { uniformSelect } from '../utils/array-utils'; const log = debug('scope:nodes-layout'); @@ -17,6 +19,7 @@ export const DEFAULT_MARGINS = { top: 0, left: 0 }; const NODE_SIZE_FACTOR = 1.5 * NODE_BASE_SIZE; const NODE_SEPARATION_FACTOR = 1 * NODE_BASE_SIZE; const RANK_SEPARATION_FACTOR = 2 * NODE_BASE_SIZE; +const NODE_CENTERS_SEPARATION_FACTOR = NODE_SIZE_FACTOR + NODE_SEPARATION_FACTOR; let layoutRuns = 0; let layoutRunsTrivial = 0; @@ -64,159 +67,6 @@ function correctedEdgePath(waypoints, source, target) { return waypoints; } -/** - * Layout engine runner - * After the layout engine run nodes and edges have x-y-coordinates. Engine is - * not run if the number of nodes is bigger than `MAX_NODES`. - * @param {Object} graph dagre graph instance - * @param {Map} imNodes new node set - * @param {Map} imEdges new edge set - * @return {Object} Layout with nodes, edges, dimensions - */ -function runLayoutEngine(graph, imNodes, imEdges) { - let nodes = imNodes; - let edges = imEdges; - - const ranksep = RANK_SEPARATION_FACTOR; - const nodesep = NODE_SEPARATION_FACTOR; - const nodeWidth = NODE_SIZE_FACTOR; - const nodeHeight = NODE_SIZE_FACTOR; - - // configure node margins - graph.setGraph({ - nodesep, - ranksep - }); - - // add nodes to the graph if not already there - nodes.forEach((node) => { - const gNodeId = graphNodeId(node.get('id')); - if (!graph.hasNode(gNodeId)) { - graph.setNode(gNodeId, { - width: nodeWidth, - height: nodeHeight - }); - } - }); - - // remove nodes that are no longer there or are 0-degree nodes - graph.nodes().forEach((gNodeId) => { - const nodeId = fromGraphNodeId(gNodeId); - if (!nodes.has(nodeId) || nodes.get(nodeId).get('degree') === 0) { - graph.removeNode(gNodeId); - } - }); - - // add edges to the graph if not already there - edges.forEach((edge) => { - const s = graphNodeId(edge.get('source')); - const t = graphNodeId(edge.get('target')); - if (!graph.hasEdge(s, t)) { - const virtualNodes = s === t ? 1 : 0; - graph.setEdge(s, t, {id: edge.get('id'), minlen: virtualNodes}); - } - }); - - // remove edges that are no longer there - graph.edges().forEach((edgeObj) => { - const edge = [fromGraphNodeId(edgeObj.v), fromGraphNodeId(edgeObj.w)]; - const edgeId = edge.join(EDGE_ID_SEPARATOR); - if (!edges.has(edgeId)) { - graph.removeEdge(edgeObj.v, edgeObj.w); - } - }); - - dagre.layout(graph, { debugTiming: false }); - const layout = graph.graph(); - - // apply coordinates to nodes and edges - - graph.nodes().forEach((gNodeId) => { - const graphNode = graph.node(gNodeId); - const nodeId = fromGraphNodeId(gNodeId); - nodes = nodes.setIn([nodeId, 'x'], graphNode.x); - nodes = nodes.setIn([nodeId, 'y'], graphNode.y); - }); - - graph.edges().forEach((graphEdge) => { - const graphEdgeMeta = graph.edge(graphEdge); - const edge = edges.get(graphEdgeMeta.id); - - const source = nodes.get(fromGraphNodeId(edge.get('source'))); - const target = nodes.get(fromGraphNodeId(edge.get('target'))); - const waypoints = correctedEdgePath(fromJS(graphEdgeMeta.points), source, target); - - edges = edges.setIn([graphEdgeMeta.id, 'points'], waypoints); - }); - - // return object with the width and height of layout - return { - graphWidth: layout.width, - graphHeight: layout.height, - width: layout.width, - height: layout.height, - nodes, - edges - }; -} - -/** - * Adds `points` array to edge based on location of source and target - * @param {Map} edge new edge - * @param {Map} nodeCache all nodes - * @returns {Map} modified edge - */ -function setSimpleEdgePoints(edge, nodeCache) { - const source = nodeCache.get(edge.get('source')); - const target = nodeCache.get(edge.get('target')); - return edge.set('points', fromJS([ - {x: source.get('x'), y: source.get('y')}, - {x: target.get('x'), y: target.get('y')} - ])); -} - -/** - * Layout nodes that have rank that already exists. - * Relies on only nodes being added that have a connection to an existing node - * while having a rank of an existing node. They will be laid out in the same - * line as the latter, with a direct connection between the existing and the new node. - * @param {object} layout Layout with nodes and edges - * @param {Map} nodeCache previous nodes - * @param {object} opts Options - * @return {object} new layout object - */ -export function doLayoutNewNodesOfExistingRank(layout, nodeCache) { - const result = Object.assign({}, layout); - const nodesep = NODE_SEPARATION_FACTOR; - const nodeWidth = NODE_SIZE_FACTOR; - - // determine new nodes - const oldNodes = ImmSet.fromKeys(nodeCache); - const newNodes = ImmSet.fromKeys(layout.nodes.filter(n => n.get('degree') > 0)) - .subtract(oldNodes); - result.nodes = layout.nodes.map((n) => { - if (newNodes.contains(n.get('id'))) { - const nodesSameRank = nodeCache.filter(nn => nn.get('rank') === n.get('rank')); - if (nodesSameRank.size > 0) { - const y = nodesSameRank.first().get('y'); - const x = nodesSameRank.maxBy(nn => nn.get('x')).get('x') + nodesep + nodeWidth; - return n.merge({ x, y }); - } - return n; - } - return n; - }); - - result.edges = layout.edges.map((edge) => { - if (!edge.has('points')) { - return setSimpleEdgePoints(edge, layout.nodes); - } - return edge; - }); - - return result; -} - /** * Add coordinates to 0-degree nodes using a square layout * Depending on the previous layout run's graph aspect ratio, the square will be @@ -299,6 +149,163 @@ function layoutSingleNodes(layout, opts) { return result; } +/** + * Layout engine runner + * After the layout engine run nodes and edges have x-y-coordinates. Engine is + * not run if the number of nodes is bigger than `MAX_NODES`. + * @param {Object} graph dagre graph instance + * @param {Map} imNodes new node set + * @param {Map} imEdges new edge set + * @param {Object} opts Options with nodes layout + * @return {Object} Layout with nodes, edges, dimensions + */ +function runLayoutEngine(graph, imNodes, imEdges, opts) { + let nodes = imNodes; + let edges = imEdges; + + const ranksep = RANK_SEPARATION_FACTOR; + const nodesep = NODE_SEPARATION_FACTOR; + const nodeWidth = NODE_SIZE_FACTOR; + const nodeHeight = NODE_SIZE_FACTOR; + + // configure node margins + graph.setGraph({ + nodesep, + ranksep + }); + + // add nodes to the graph if not already there + nodes.forEach((node) => { + const gNodeId = graphNodeId(node.get('id')); + if (!graph.hasNode(gNodeId)) { + graph.setNode(gNodeId, { + width: nodeWidth, + height: nodeHeight + }); + } + }); + + // remove nodes that are no longer there or are 0-degree nodes + graph.nodes().forEach((gNodeId) => { + const nodeId = fromGraphNodeId(gNodeId); + if (!nodes.has(nodeId) || nodes.get(nodeId).get('degree') === 0) { + graph.removeNode(gNodeId); + } + }); + + // add edges to the graph if not already there + edges.forEach((edge) => { + const s = graphNodeId(edge.get('source')); + const t = graphNodeId(edge.get('target')); + if (!graph.hasEdge(s, t)) { + const virtualNodes = s === t ? 1 : 0; + graph.setEdge(s, t, {id: edge.get('id'), minlen: virtualNodes}); + } + }); + + // remove edges that are no longer there + graph.edges().forEach((edgeObj) => { + const edge = [fromGraphNodeId(edgeObj.v), fromGraphNodeId(edgeObj.w)]; + const edgeId = edge.join(EDGE_ID_SEPARATOR); + if (!edges.has(edgeId)) { + graph.removeEdge(edgeObj.v, edgeObj.w); + } + }); + + dagre.layout(graph, { debugTiming: false }); + + // apply coordinates to nodes and edges + graph.nodes().forEach((gNodeId) => { + const graphNode = graph.node(gNodeId); + const nodeId = fromGraphNodeId(gNodeId); + nodes = nodes.setIn([nodeId, 'x'], graphNode.x); + nodes = nodes.setIn([nodeId, 'y'], graphNode.y); + }); + graph.edges().forEach((graphEdge) => { + const graphEdgeMeta = graph.edge(graphEdge); + const edge = edges.get(graphEdgeMeta.id); + + const source = nodes.get(fromGraphNodeId(edge.get('source'))); + const target = nodes.get(fromGraphNodeId(edge.get('target'))); + const waypoints = correctedEdgePath(fromJS(graphEdgeMeta.points), source, target); + + edges = edges.setIn([graphEdgeMeta.id, 'points'], waypoints); + }); + + const { width, height } = graph.graph(); + let layout = { + graphWidth: width, + graphHeight: height, + width, + height, + nodes, + edges + }; + + // layout the single nodes + layout = layoutSingleNodes(layout, opts); + + // return object with the width and height of layout + return layout; +} + +/** + * Adds `points` array to edge based on location of source and target + * @param {Map} edge new edge + * @param {Map} nodeCache all nodes + * @returns {Map} modified edge + */ +function setSimpleEdgePoints(edge, nodeCache) { + const source = nodeCache.get(edge.get('source')); + const target = nodeCache.get(edge.get('target')); + return edge.set('points', fromJS([ + {x: source.get('x'), y: source.get('y')}, + {x: target.get('x'), y: target.get('y')} + ])); +} + +/** + * Layout nodes that have rank that already exists. + * Relies on only nodes being added that have a connection to an existing node + * while having a rank of an existing node. They will be laid out in the same + * line as the latter, with a direct connection between the existing and the new node. + * @param {object} layout Layout with nodes and edges + * @param {Map} nodeCache previous nodes + * @param {object} opts Options + * @return {object} new layout object + */ +export function doLayoutNewNodesOfExistingRank(layout, nodeCache) { + const result = Object.assign({}, layout); + const nodesep = NODE_SEPARATION_FACTOR; + const nodeWidth = NODE_SIZE_FACTOR; + + // determine new nodes + const oldNodes = ImmSet.fromKeys(nodeCache); + const newNodes = ImmSet.fromKeys(layout.nodes.filter(n => n.get('degree') > 0)) + .subtract(oldNodes); + result.nodes = layout.nodes.map((n) => { + if (newNodes.contains(n.get('id'))) { + const nodesSameRank = nodeCache.filter(nn => nn.get('rank') === n.get('rank')); + if (nodesSameRank.size > 0) { + const y = nodesSameRank.first().get('y'); + const x = nodesSameRank.maxBy(nn => nn.get('x')).get('x') + nodesep + nodeWidth; + return n.merge({ x, y }); + } + return n; + } + return n; + }); + + result.edges = layout.edges.map((edge) => { + if (!edge.has('points')) { + return setSimpleEdgePoints(edge, layout.nodes); + } + return edge; + }); + + return result; +} + /** * Determine if nodes were added between node sets * @param {Map} nodes new Map of nodes @@ -408,6 +415,7 @@ function copyLayoutProperties(layout, nodeCache, edgeCache) { return result; } + /** * Layout of nodes and edges * If a previous layout was given and not too much changed, the previous layout @@ -435,47 +443,49 @@ export function doLayout(immNodes, immEdges, opts) { const nodeCache = options.nodeCache || cache.nodeCache; const edgeCache = options.edgeCache || cache.edgeCache; const useCache = !options.forceRelayout && cachedLayout && nodeCache && edgeCache; + const nodesWithDegrees = updateNodeDegrees(immNodes, immEdges); let layout; layoutRuns += 1; if (useCache && !hasUnseenNodes(immNodes, nodeCache)) { layoutRunsTrivial += 1; + // trivial case: no new nodes have been added log('skip layout, trivial adjustment', layoutRunsTrivial, layoutRuns); layout = cloneLayout(cachedLayout, immNodes, immEdges); - // copy old properties, works also if nodes get re-added layout = copyLayoutProperties(layout, nodeCache, edgeCache); - } else { - const nodesWithDegrees = updateNodeDegrees(immNodes, immEdges); - if (useCache - && featureIsEnabledAny('layout-dance', 'layout-dance-single') - && hasNewSingleNode(nodesWithDegrees, nodeCache)) { - // special case: new nodes are 0-degree nodes, no need for layout run, - // they will be laid out further below - log('skip layout, only 0-degree node(s) added'); - layout = cloneLayout(cachedLayout, nodesWithDegrees, immEdges); - layout = copyLayoutProperties(layout, nodeCache, edgeCache); - } else if (useCache - && featureIsEnabledAny('layout-dance', 'layout-dance-rank') - && hasNewNodesOfExistingRank(nodesWithDegrees, immEdges, nodeCache)) { - // special case: few new nodes were added, no need for layout run, - // they will inserted according to ranks - log('skip layout, used rank-based insertion'); - layout = cloneLayout(cachedLayout, nodesWithDegrees, immEdges); - layout = copyLayoutProperties(layout, nodeCache, edgeCache); - layout = doLayoutNewNodesOfExistingRank(layout, nodeCache); - } else { - const graph = cache.graph; - layout = runLayoutEngine(graph, nodesWithDegrees, immEdges); - if (!layout) { - return layout; - } - } - + } else if (useCache + && featureIsEnabledAny('layout-dance', 'layout-dance-single') + && hasNewSingleNode(nodesWithDegrees, nodeCache)) { + // special case: new nodes are 0-degree nodes, no need for layout run, + // they will be laid out further below + log('skip layout, only 0-degree node(s) added'); + layout = cloneLayout(cachedLayout, nodesWithDegrees, immEdges); + layout = copyLayoutProperties(layout, nodeCache, edgeCache); layout = layoutSingleNodes(layout, opts); + } else if (useCache + && featureIsEnabledAny('layout-dance', 'layout-dance-rank') + && hasNewNodesOfExistingRank(nodesWithDegrees, immEdges, nodeCache)) { + // special case: few new nodes were added, no need for layout run, + // they will inserted according to ranks + log('skip layout, used rank-based insertion'); + layout = cloneLayout(cachedLayout, nodesWithDegrees, immEdges); + layout = copyLayoutProperties(layout, nodeCache, edgeCache); + layout = doLayoutNewNodesOfExistingRank(layout, nodeCache); + layout = layoutSingleNodes(layout, opts); + } else { + // default case: the new layout is too different and refreshing is required + layout = runLayoutEngine(cache.graph, nodesWithDegrees, immEdges, opts); } - // cache results + if (layout) { + // Last line of defense - re-render everything if two nodes are too close to one another. + if (minEuclideanDistanceBetweenPoints(layout.nodes) < NODE_CENTERS_SEPARATION_FACTOR) { + layout = runLayoutEngine(cache.graph, nodesWithDegrees, immEdges, opts); + trackMixpanelEvent('scope.layout.graph.overlap'); + } + + // cache results cache.cachedLayout = layout; cache.nodeCache = cache.nodeCache.merge(layout.nodes); cache.edgeCache = cache.edgeCache.merge(layout.edges); diff --git a/client/app/scripts/utils/__tests__/math-utils-test.js b/client/app/scripts/utils/__tests__/math-utils-test.js index ab46b1a96..75697dcfc 100644 --- a/client/app/scripts/utils/__tests__/math-utils-test.js +++ b/client/app/scripts/utils/__tests__/math-utils-test.js @@ -1,3 +1,5 @@ +import { fromJS } from 'immutable'; + describe('MathUtils', () => { const MathUtils = require('../math-utils'); @@ -19,4 +21,25 @@ describe('MathUtils', () => { expect(f(-5, 5)).toBe(0); }); }); + + describe('minEuclideanDistanceBetweenPoints', () => { + const f = MathUtils.minEuclideanDistanceBetweenPoints; + const entryA = { pointA: { x: 0, y: 0 } }; + const entryB = { pointB: { x: 30, y: 0 } }; + const entryC = { pointC: { x: 0, y: -40 } }; + const entryD = { pointD: { x: -1000, y: 567 } }; + const entryE = { pointE: { x: -999, y: 567 } }; + const entryF = { pointF: { x: 30, y: 0 } }; + + it('it should return the minimal distance between any two points in the collection', () => { + expect(f(fromJS({}))).toBe(0); + expect(f(fromJS({...entryA}))).toBe(0); + expect(f(fromJS({...entryA, ...entryB}))).toBe(30); + expect(f(fromJS({...entryA, ...entryC}))).toBe(40); + expect(f(fromJS({...entryB, ...entryC}))).toBe(50); + expect(f(fromJS({...entryA, ...entryB, ...entryC, ...entryD}))).toBe(30); + expect(f(fromJS({...entryA, ...entryB, ...entryC, ...entryD, ...entryE}))).toBe(1); + expect(f(fromJS({...entryA, ...entryB, ...entryC, ...entryD, ...entryF}))).toBe(0); + }); + }); }); diff --git a/client/app/scripts/utils/math-utils.js b/client/app/scripts/utils/math-utils.js index 578d83760..37ff46a94 100644 --- a/client/app/scripts/utils/math-utils.js +++ b/client/app/scripts/utils/math-utils.js @@ -18,3 +18,26 @@ export function modulo(i, n) { return ((i % n) + n) % n; } + +function euclideanDistance(pointA, pointB) { + const dx = pointA.get('x') - pointB.get('x'); + const dy = pointA.get('y') - pointB.get('y'); + return Math.sqrt((dx * dx) + (dy * dy)); +} + +// This could be solved in O(N log N) (see https://en.wikipedia.org/wiki/Closest_pair_of_points_problem), +// but this brute-force O(N^2) should be good enough for a reasonable number of nodes. +export function minEuclideanDistanceBetweenPoints(points) { + let minDistance = 0; + let foundPair = false; + points.forEach((pointA, idA) => { + points.forEach((pointB, idB) => { + const distance = euclideanDistance(pointA, pointB); + if (idA !== idB && (distance < minDistance || !foundPair)) { + minDistance = distance; + foundPair = true; + } + }); + }); + return minDistance; +}