From 400bfe6bd449772e6a8d92d310fa02b1bfa6e74b Mon Sep 17 00:00:00 2001 From: etienne Date: Tue, 24 Apr 2018 17:17:50 -0400 Subject: [PATCH 01/11] ping imagetest after having restarting nw.js - should fix some intermittent hangups when running the image tests locally --- tasks/baseline.js | 4 +--- tasks/test_export.js | 4 +--- tasks/test_image.js | 4 +--- tasks/util/container_commands.js | 16 +++++++++++----- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/tasks/baseline.js b/tasks/baseline.js index 707ce8a74be..2e6f666a883 100644 --- a/tasks/baseline.js +++ b/tasks/baseline.js @@ -14,6 +14,4 @@ var cmd = containerCommands.getRunCmd( ); console.log(msg); -common.execCmd(containerCommands.ping, function() { - common.execCmd(cmd); -}); +common.execCmd(cmd); diff --git a/tasks/test_export.js b/tasks/test_export.js index d747d818b6e..de08f97e7e7 100644 --- a/tasks/test_export.js +++ b/tasks/test_export.js @@ -14,6 +14,4 @@ var cmd = containerCommands.getRunCmd( ); console.log(msg); -common.execCmd(containerCommands.ping, function() { - common.execCmd(cmd); -}); +common.execCmd(cmd); diff --git a/tasks/test_image.js b/tasks/test_image.js index eed7ccd9148..8156cabaead 100644 --- a/tasks/test_image.js +++ b/tasks/test_image.js @@ -14,6 +14,4 @@ var cmd = containerCommands.getRunCmd( ); console.log(msg); -common.execCmd(containerCommands.ping, function() { - common.execCmd(cmd); -}); +common.execCmd(cmd); diff --git a/tasks/util/container_commands.js b/tasks/util/container_commands.js index fa011c5eaf2..8a6200ccf77 100644 --- a/tasks/util/container_commands.js +++ b/tasks/util/container_commands.js @@ -21,7 +21,8 @@ containerCommands.setup = [ containerCommands.cpIndex, containerCommands.injectEnv, containerCommands.restart, - 'sleep 1', + containerCommands.ping, + 'sleep 5' ].join(' && '); containerCommands.dockerRun = [ @@ -34,12 +35,17 @@ containerCommands.dockerRun = [ containerCommands.getRunCmd = function(isCI, commands) { var _commands = Array.isArray(commands) ? commands.slice() : [commands]; + var cmd; - if(isCI) return getRunCI(_commands); + if(isCI) { + _commands = [containerCommands.ping].concat(_commands); + cmd = getRunCI(_commands); + } else { + _commands = [containerCommands.setup].concat(_commands); + cmd = getRunLocal(_commands); + } - // add setup commands locally - _commands = [containerCommands.setup].concat(_commands); - return getRunLocal(_commands); + return cmd; }; function getRunLocal(commands) { From 284c206d89b8dfcdea6af2cb525c42872051bca5 Mon Sep 17 00:00:00 2001 From: etienne Date: Tue, 24 Apr 2018 17:29:06 -0400 Subject: [PATCH 02/11] introduce new strategy for cartesian trace module layer updates - make trace module layers 'data-driven', one layer per unique plot module. Use _module.layerName to denote reused plot methods. - pass layer to trace _module.plot (as 3rd arg) to not mutate node data and allow a given _module.plot method to trace in multiple layers - no need for previous `_plotMethods` stash attempt - rename 'imagelayer' -> 'heatmaplayer', 'maplayer' -> 'contourlayer' for consistency for other trace modules --- src/components/rangeslider/draw.js | 7 +- src/plot_api/subroutines.js | 11 +-- src/plots/cartesian/constants.js | 7 +- src/plots/cartesian/index.js | 96 +++++++++++++++++--------- src/traces/bar/plot.js | 11 ++- src/traces/box/plot.js | 5 +- src/traces/candlestick/index.js | 1 + src/traces/carpet/plot.js | 5 +- src/traces/contour/plot.js | 45 +++++------- src/traces/contourcarpet/plot.js | 4 +- src/traces/heatmap/plot.js | 5 +- src/traces/histogram/index.js | 1 + src/traces/histogram2d/index.js | 1 + src/traces/histogram2dcontour/index.js | 1 + src/traces/ohlc/plot.js | 4 +- src/traces/scatter/plot.js | 20 +++--- src/traces/scattercarpet/plot.js | 9 ++- src/traces/scatterpolar/plot.js | 4 +- src/traces/scatterternary/plot.js | 4 +- src/traces/violin/plot.js | 5 +- 20 files changed, 128 insertions(+), 118 deletions(-) diff --git a/src/components/rangeslider/draw.js b/src/components/rangeslider/draw.js index d90766439df..079667bd7e2 100644 --- a/src/components/rangeslider/draw.js +++ b/src/components/rangeslider/draw.js @@ -456,8 +456,7 @@ function drawRangePlot(rangeSlider, gd, axisOpts, opts) { plotgroup: plotgroup, xaxis: xa, yaxis: ya, - isRangePlot: true, - plotMethods: opts._plotMethods + isRangePlot: true }; if(isMainPlot) mainplotinfo = plotinfo; @@ -467,10 +466,6 @@ function drawRangePlot(rangeSlider, gd, axisOpts, opts) { } Cartesian.rangePlot(gd, plotinfo, filterRangePlotCalcData(calcData, id)); - - // stash list of plot methods on range-plot for later, - // so that they can be called to clear traces of 'gone' modules - opts._plotMethods = plotinfo.plotMethods; }); } diff --git a/src/plot_api/subroutines.js b/src/plot_api/subroutines.js index dfd76a1021f..494dea1fb75 100644 --- a/src/plot_api/subroutines.js +++ b/src/plot_api/subroutines.js @@ -21,7 +21,6 @@ var Titles = require('../components/titles'); var ModeBar = require('../components/modebar'); var Axes = require('../plots/cartesian/axes'); -var cartesianConstants = require('../plots/cartesian/constants'); var alignmentConstants = require('../constants/alignment'); var axisConstraints = require('../plots/cartesian/constraints'); var enforceAxisConstraints = axisConstraints.enforce; @@ -199,15 +198,9 @@ exports.lsInner = function(gd) { Drawing.setClipUrl(plotinfo.plot, plotClipId); - for(i = 0; i < cartesianConstants.traceLayerClasses.length; i++) { - var layer = cartesianConstants.traceLayerClasses[i]; - if(layer !== 'scatterlayer' && layer !== 'barlayer') { - plotinfo.plot.selectAll('g.' + layer).call(Drawing.setClipUrl, layerClipId); - } - } - // stash layer clipId value (null or same as clipId) - // to DRY up Drawing.setClipUrl calls downstream + // to DRY up Drawing.setClipUrl calls on trace-module and trace layers + // downstream plotinfo.layerClipId = layerClipId; // figure out extra axis line and tick positions as needed diff --git a/src/plots/cartesian/constants.js b/src/plots/cartesian/constants.js index 59ac6f1b87f..6a2cd12ce72 100644 --- a/src/plots/cartesian/constants.js +++ b/src/plots/cartesian/constants.js @@ -61,15 +61,16 @@ module.exports = { DFLTRANGEY: [-1, 4], // Layers to keep trace types in the right order + // N.B. each 'unique' plot method must have its own layer traceLayerClasses: [ - 'imagelayer', - 'maplayer', + 'heatmaplayer', + 'contourcarpetlayer', 'contourlayer', 'barlayer', 'carpetlayer', 'violinlayer', 'boxlayer', 'ohlclayer', - 'scatterlayer' + 'scattercarpetlayer', 'scatterlayer' ], layerValue2layerClass: { diff --git a/src/plots/cartesian/index.js b/src/plots/cartesian/index.js index c4de914f6ad..59b507c86ca 100644 --- a/src/plots/cartesian/index.js +++ b/src/plots/cartesian/index.js @@ -10,10 +10,13 @@ 'use strict'; var d3 = require('d3'); + +var Registry = require('../../registry'); var Lib = require('../../lib'); var Plots = require('../plots'); -var getModuleCalcData = require('../get_data').getModuleCalcData; +var Drawing = require('../../components/drawing'); +var getModuleCalcData = require('../get_data').getModuleCalcData; var axisIds = require('./axis_ids'); var constants = require('./constants'); var xmlnsNamespaces = require('../../constants/xmlns_namespaces'); @@ -179,40 +182,75 @@ exports.plot = function(gd, traces, transitionOpts, makeOnCompleteCallback) { }; function plotOne(gd, plotinfo, cdSubplot, transitionOpts, makeOnCompleteCallback) { + var traceLayerClasses = constants.traceLayerClasses; var fullLayout = gd._fullLayout; var modules = fullLayout._modules; - // list of plot methods to call (this list includes plot methods of 'gone' modules) - var plotMethodsToCall = plotinfo.plotMethods || []; - // list of plot methods of visible module on this subplot - var plotMethods = []; - var i, plotMethod; - - for(i = 0; i < modules.length; i++) { - var _module = modules[i]; - - if(_module.basePlotModule.name === 'cartesian') { - plotMethod = _module.plot; - Lib.pushUnique(plotMethodsToCall, plotMethod); - Lib.pushUnique(plotMethods, plotMethod); + var _module, cdModuleAndOthers, cdModule; + + var layerData = []; + + for(var i = 0; i < traceLayerClasses.length; i++) { + var className = traceLayerClasses[i]; + + for(var j = 0; j < modules.length; j++) { + _module = modules[j]; + + if(_module.basePlotModule.name === 'cartesian' && + (_module.layerName || _module.name + 'layer') === className + ) { + var plotMethod = _module.plot; + + // plot all traces of this type on this subplot at once + cdModuleAndOthers = getModuleCalcData(cdSubplot, plotMethod); + cdModule = cdModuleAndOthers[0]; + // don't need to search the found traces again - in fact we need to NOT + // so that if two modules share the same plotter we don't double-plot + cdSubplot = cdModuleAndOthers[1]; + + if(cdModule.length) { + layerData.push({ + className: className, + plotMethod: plotMethod, + cdModule: cdModule + }); + } + break; + } } } - for(i = 0; i < plotMethodsToCall.length; i++) { - plotMethod = plotMethodsToCall[i]; + var layers = plotinfo.plot.selectAll('g.mlayer') + .data(layerData, function(d) { return d.className; }); - // plot all traces of this type on this subplot at once - var cdModuleAndOthers = getModuleCalcData(cdSubplot, plotMethod); - var cdModule = cdModuleAndOthers[0]; - // don't need to search the found traces again - in fact we need to NOT - // so that if two modules share the same plotter we don't double-plot - cdSubplot = cdModuleAndOthers[1]; + layers.enter().append('g') + .attr('class', function(d) { return d.className; }) + .classed('mlayer', true); - plotMethod(gd, plotinfo, cdModule, transitionOpts, makeOnCompleteCallback); - } + layers.exit().remove(); + + layers.order(); + + layers.each(function(d) { + var sel = d3.select(this); + var className = d.className; - // save list of plot methods on subplot for later, - // so that they can be called to clear traces of 'gone' modules - plotinfo.plotMethods = plotMethods; + d.plotMethod( + gd, plotinfo, d.cdModule, sel, + transitionOpts, makeOnCompleteCallback + ); + + // layers that allow `cliponaxis: false` + if(className !== 'scatterlayer' && className !== 'barlayer') { + Drawing.setClipUrl(sel, plotinfo.layerClipId); + } + }); + + // call Scattergl.plot separately + if(fullLayout._has('scattergl')) { + _module = Registry.getModule('scattergl'); + cdModule = getModuleCalcData(cdSubplot, _module)[0]; + _module.plot(gd, plotinfo, cdModule); + } } exports.clean = function(newFullData, newFullLayout, oldFullData, oldFullLayout) { @@ -441,10 +479,6 @@ function makeSubplotLayer(gd, plotinfo) { ensureSingleAndAddDatum(plotinfo.gridlayer, 'g', plotinfo.xaxis._id); ensureSingleAndAddDatum(plotinfo.gridlayer, 'g', plotinfo.yaxis._id); plotinfo.gridlayer.selectAll('g').sort(axisIds.idSort); - - for(var i = 0; i < constants.traceLayerClasses.length; i++) { - ensureSingle(plotinfo.plot, 'g', constants.traceLayerClasses[i]); - } } plotinfo.xlines diff --git a/src/traces/bar/plot.js b/src/traces/bar/plot.js index 0acc8eecd8e..31de45c3b29 100644 --- a/src/traces/bar/plot.js +++ b/src/traces/bar/plot.js @@ -30,13 +30,12 @@ var attributes = require('./attributes'), // padding in pixels around text var TEXTPAD = 3; -module.exports = function plot(gd, plotinfo, cdbar) { - var xa = plotinfo.xaxis, - ya = plotinfo.yaxis, - fullLayout = gd._fullLayout; +module.exports = function plot(gd, plotinfo, cdbar, barLayer) { + var xa = plotinfo.xaxis; + var ya = plotinfo.yaxis; + var fullLayout = gd._fullLayout; - var bartraces = plotinfo.plot.select('.barlayer') - .selectAll('g.trace.bars') + var bartraces = barLayer.selectAll('g.trace.bars') .data(cdbar, function(d) { return d[0].trace.uid; }); bartraces.enter().append('g') diff --git a/src/traces/box/plot.js b/src/traces/box/plot.js index 7ca8763876d..ade346c6f93 100644 --- a/src/traces/box/plot.js +++ b/src/traces/box/plot.js @@ -17,13 +17,12 @@ var Drawing = require('../../components/drawing'); var JITTERCOUNT = 5; // points either side of this to include var JITTERSPREAD = 0.01; // fraction of IQR to count as "dense" -function plot(gd, plotinfo, cdbox) { +function plot(gd, plotinfo, cdbox, boxLayer) { var fullLayout = gd._fullLayout; var xa = plotinfo.xaxis; var ya = plotinfo.yaxis; - var boxtraces = plotinfo.plot.select('.boxlayer') - .selectAll('g.trace.boxes') + var boxtraces = boxLayer.selectAll('g.trace.boxes') .data(cdbox, function(d) { return d[0].trace.uid; }); boxtraces.enter().append('g') diff --git a/src/traces/candlestick/index.js b/src/traces/candlestick/index.js index 4a6372585b7..12911def4df 100644 --- a/src/traces/candlestick/index.js +++ b/src/traces/candlestick/index.js @@ -36,6 +36,7 @@ module.exports = { supplyDefaults: require('./defaults'), calc: require('./calc'), plot: require('../box/plot').plot, + layerName: 'boxlayer', style: require('../box/style'), hoverPoints: require('../ohlc/hover'), selectPoints: require('../ohlc/select') diff --git a/src/traces/carpet/plot.js b/src/traces/carpet/plot.js index c59366c9c0f..0e270726da1 100644 --- a/src/traces/carpet/plot.js +++ b/src/traces/carpet/plot.js @@ -30,7 +30,7 @@ module.exports = function plot(gd, plotinfo, cdcarpet) { } }; -function plotOne(gd, plotinfo, cd) { +function plotOne(gd, plotinfo, cd, carpetLayer) { var t = cd[0]; var trace = cd[0].trace, xa = plotinfo.xaxis, @@ -39,10 +39,9 @@ function plotOne(gd, plotinfo, cd) { bax = trace.baxis, fullLayout = gd._fullLayout; - var gridLayer = plotinfo.plot.select('.carpetlayer'); var clipLayer = fullLayout._clips; - var axisLayer = Lib.ensureSingle(gridLayer, 'g', 'carpet' + trace.uid).classed('trace', true); + var axisLayer = Lib.ensureSingle(carpetLayer, 'g', 'carpet' + trace.uid).classed('trace', true); var minorLayer = Lib.ensureSingle(axisLayer, 'g', 'minorlayer'); var majorLayer = Lib.ensureSingle(axisLayer, 'g', 'majorlayer'); var boundaryLayer = Lib.ensureSingle(axisLayer, 'g', 'boundarylayer'); diff --git a/src/traces/contour/plot.js b/src/traces/contour/plot.js index 325a54c83c7..66f5e708216 100644 --- a/src/traces/contour/plot.js +++ b/src/traces/contour/plot.js @@ -33,25 +33,20 @@ exports.plot = function plot(gd, plotinfo, cdcontours) { } }; -function plotOne(gd, plotinfo, cd) { - var trace = cd[0].trace, - x = cd[0].x, - y = cd[0].y, - contours = trace.contours, - uid = trace.uid, - xa = plotinfo.xaxis, - ya = plotinfo.yaxis, - fullLayout = gd._fullLayout, - id = 'contour' + uid, - pathinfo = emptyPathinfo(contours, plotinfo, cd[0]); - - if(trace.visible !== true) { - fullLayout._paper.selectAll('.' + id + ',.hm' + uid).remove(); - fullLayout._infolayer.selectAll('.cb' + uid).remove(); - return; - } +function plotOne(gd, plotinfo, cd, contourLayer) { + var trace = cd[0].trace; + var x = cd[0].x; + var y = cd[0].y; + var contours = trace.contours; + var id = 'contour' + trace.uid; + var xa = plotinfo.xaxis; + var ya = plotinfo.yaxis; + var fullLayout = gd._fullLayout; + var pathinfo = emptyPathinfo(contours, plotinfo, cd[0]); // use a heatmap to fill - draw it behind the lines + var heatmapColoringLayer = Lib.ensureSingle(contourLayer, 'g', 'heatmapcoloring'); + var cdheatmaps = []; if(contours.coloring === 'heatmap') { if(trace.zauto && (trace.autocontour === false)) { trace._input.zmin = trace.zmin = @@ -59,15 +54,9 @@ function plotOne(gd, plotinfo, cd) { trace._input.zmax = trace.zmax = trace.zmin + pathinfo.length * contours.size; } - - heatmapPlot(gd, plotinfo, [cd]); - } - // in case this used to be a heatmap (or have heatmap fill) - else { - fullLayout._paper.selectAll('.hm' + uid).remove(); - fullLayout._infolayer.selectAll('g.rangeslider-container') - .selectAll('.hm' + uid).remove(); + cdheatmaps = [cd]; } + heatmapPlot(gd, plotinfo, cdheatmaps, heatmapColoringLayer); makeCrossings(pathinfo); findAllPaths(pathinfo); @@ -90,15 +79,15 @@ function plotOne(gd, plotinfo, cd) { } // draw everything - var plotGroup = exports.makeContourGroup(plotinfo, cd, id); + var plotGroup = exports.makeContourGroup(contourLayer, cd, id); makeBackground(plotGroup, perimeter, contours); makeFills(plotGroup, fillPathinfo, perimeter, contours); makeLinesAndLabels(plotGroup, pathinfo, gd, cd[0], contours, perimeter); clipGaps(plotGroup, plotinfo, fullLayout._clips, cd[0], perimeter); } -exports.makeContourGroup = function(plotinfo, cd, id) { - var plotgroup = plotinfo.plot.select('.maplayer') +exports.makeContourGroup = function(layer, cd, id) { + var plotgroup = layer .selectAll('g.contour.' + id) .data(cd); diff --git a/src/traces/contourcarpet/plot.js b/src/traces/contourcarpet/plot.js index 8ebfd7057b4..e55b2ce15fa 100644 --- a/src/traces/contourcarpet/plot.js +++ b/src/traces/contourcarpet/plot.js @@ -32,7 +32,7 @@ module.exports = function plot(gd, plotinfo, cdcontours) { } }; -function plotOne(gd, plotinfo, cd) { +function plotOne(gd, plotinfo, cd, contourcarpetLayer) { var trace = cd[0].trace; var carpet = trace._carpetTrace = lookupCarpet(gd, trace); @@ -96,7 +96,7 @@ function plotOne(gd, plotinfo, cd) { mapPathinfo(pathinfo, ab2p); // draw everything - var plotGroup = contourPlot.makeContourGroup(plotinfo, cd, id); + var plotGroup = contourPlot.makeContourGroup(contourcarpetLayer, cd, id); // Compute the boundary path var seg, xp, yp, i; diff --git a/src/traces/heatmap/plot.js b/src/traces/heatmap/plot.js index 00212e4bd5e..8b7d30ab4d0 100644 --- a/src/traces/heatmap/plot.js +++ b/src/traces/heatmap/plot.js @@ -25,7 +25,7 @@ module.exports = function(gd, plotinfo, cdheatmaps) { } }; -function plotOne(gd, plotinfo, cd) { +function plotOne(gd, plotinfo, cd, heatmapLayer) { var cd0 = cd[0]; var trace = cd0.trace; var uid = trace.uid; @@ -137,8 +137,7 @@ function plotOne(gd, plotinfo, cd) { // if image is entirely off-screen, don't even draw it var isOffScreen = (imageWidth <= 0 || imageHeight <= 0); - var plotgroup = plotinfo.plot.select('.imagelayer') - .selectAll('g.hm.' + id) + var plotgroup = heatmapLayer.selectAll('g.hm.' + id) .data(isOffScreen ? [] : [0]); plotgroup.enter().append('g') diff --git a/src/traces/histogram/index.js b/src/traces/histogram/index.js index c0949a06447..563689b2a4b 100644 --- a/src/traces/histogram/index.js +++ b/src/traces/histogram/index.js @@ -32,6 +32,7 @@ Histogram.supplyLayoutDefaults = require('../bar/layout_defaults'); Histogram.calc = require('./calc'); Histogram.setPositions = require('../bar/set_positions'); Histogram.plot = require('../bar/plot'); +Histogram.layerName = 'barlayer'; Histogram.style = require('../bar/style'); Histogram.colorbar = require('../scatter/colorbar'); Histogram.hoverPoints = require('./hover'); diff --git a/src/traces/histogram2d/index.js b/src/traces/histogram2d/index.js index c8e9695b8dc..af2549a9c18 100644 --- a/src/traces/histogram2d/index.js +++ b/src/traces/histogram2d/index.js @@ -15,6 +15,7 @@ Histogram2D.attributes = require('./attributes'); Histogram2D.supplyDefaults = require('./defaults'); Histogram2D.calc = require('../heatmap/calc'); Histogram2D.plot = require('../heatmap/plot'); +Histogram2D.layerName = 'heatmaplayer'; Histogram2D.colorbar = require('../heatmap/colorbar'); Histogram2D.style = require('../heatmap/style'); Histogram2D.hoverPoints = require('./hover'); diff --git a/src/traces/histogram2dcontour/index.js b/src/traces/histogram2dcontour/index.js index 7953ff48354..ccd98cc8bd7 100644 --- a/src/traces/histogram2dcontour/index.js +++ b/src/traces/histogram2dcontour/index.js @@ -15,6 +15,7 @@ Histogram2dContour.attributes = require('./attributes'); Histogram2dContour.supplyDefaults = require('./defaults'); Histogram2dContour.calc = require('../contour/calc'); Histogram2dContour.plot = require('../contour/plot').plot; +Histogram2dContour.layerName = 'contourlayer'; Histogram2dContour.style = require('../contour/style'); Histogram2dContour.colorbar = require('../contour/colorbar'); Histogram2dContour.hoverPoints = require('../contour/hover'); diff --git a/src/traces/ohlc/plot.js b/src/traces/ohlc/plot.js index 6a54b48a347..f915612c957 100644 --- a/src/traces/ohlc/plot.js +++ b/src/traces/ohlc/plot.js @@ -12,12 +12,10 @@ var d3 = require('d3'); var Lib = require('../../lib'); -module.exports = function plot(gd, plotinfo, cdOHLC) { +module.exports = function plot(gd, plotinfo, cdOHLC, ohlcLayer) { var xa = plotinfo.xaxis; var ya = plotinfo.yaxis; - var ohlcLayer = plotinfo.plot.select('g.ohlclayer'); - var traces = ohlcLayer.selectAll('g.trace') .data(cdOHLC, function(d) { return d[0].trace.uid; }); diff --git a/src/traces/scatter/plot.js b/src/traces/scatter/plot.js index c9e92ee72e0..d75719bfa98 100644 --- a/src/traces/scatter/plot.js +++ b/src/traces/scatter/plot.js @@ -20,17 +20,15 @@ var linePoints = require('./line_points'); var linkTraces = require('./link_traces'); var polygonTester = require('../../lib/polygon').tester; -module.exports = function plot(gd, plotinfo, cdscatter, transitionOpts, makeOnCompleteCallback) { +module.exports = function plot(gd, plotinfo, cdscatter, scatterLayer, transitionOpts, makeOnCompleteCallback) { var i, uids, join, onComplete; - var scatterlayer = plotinfo.plot.select('g.scatterlayer'); - // If transition config is provided, then it is only a partial replot and traces not // updated are removed. var isFullReplot = !transitionOpts; var hasTransition = !!transitionOpts && transitionOpts.duration > 0; - join = scatterlayer.selectAll('g.trace') + join = scatterLayer.selectAll('g.trace') .data(cdscatter, function(d) { return d[0].trace.uid; }); // Append new traces: @@ -45,7 +43,7 @@ module.exports = function plot(gd, plotinfo, cdscatter, transitionOpts, makeOnCo // the z-order of fill layers is correct. linkTraces(gd, plotinfo, cdscatter); - createFills(gd, scatterlayer, plotinfo); + createFills(gd, scatterLayer, plotinfo); // Sort the traces, once created, so that the ordering is preserved even when traces // are shown and hidden. This is needed since we're not just wiping everything out @@ -54,7 +52,7 @@ module.exports = function plot(gd, plotinfo, cdscatter, transitionOpts, makeOnCo uids[cdscatter[i][0].trace.uid] = i; } - scatterlayer.selectAll('g.trace').sort(function(a, b) { + scatterLayer.selectAll('g.trace').sort(function(a, b) { var idx1 = uids[a[0].trace.uid]; var idx2 = uids[b[0].trace.uid]; return idx1 > idx2 ? 1 : -1; @@ -81,12 +79,12 @@ module.exports = function plot(gd, plotinfo, cdscatter, transitionOpts, makeOnCo transition.each(function() { // Must run the selection again since otherwise enters/updates get grouped together // and these get executed out of order. Except we need them in order! - scatterlayer.selectAll('g.trace').each(function(d, i) { + scatterLayer.selectAll('g.trace').each(function(d, i) { plotOne(gd, i, plotinfo, d, cdscatter, this, transitionOpts); }); }); } else { - scatterlayer.selectAll('g.trace').each(function(d, i) { + scatterLayer.selectAll('g.trace').each(function(d, i) { plotOne(gd, i, plotinfo, d, cdscatter, this, transitionOpts); }); } @@ -96,13 +94,13 @@ module.exports = function plot(gd, plotinfo, cdscatter, transitionOpts, makeOnCo } // remove paths that didn't get used - scatterlayer.selectAll('path:not([d])').remove(); + scatterLayer.selectAll('path:not([d])').remove(); }; -function createFills(gd, scatterlayer, plotinfo) { +function createFills(gd, scatterLayer, plotinfo) { var trace; - scatterlayer.selectAll('g.trace').each(function(d) { + scatterLayer.selectAll('g.trace').each(function(d) { var tr = d3.select(this); // Loop only over the traces being redrawn: diff --git a/src/traces/scattercarpet/plot.js b/src/traces/scattercarpet/plot.js index 6edb7a718c1..1b82fa59d95 100644 --- a/src/traces/scattercarpet/plot.js +++ b/src/traces/scattercarpet/plot.js @@ -13,26 +13,25 @@ var scatterPlot = require('../scatter/plot'); var Axes = require('../../plots/cartesian/axes'); var Drawing = require('../../components/drawing'); -module.exports = function plot(gd, plotinfoproxy, data) { +module.exports = function plot(gd, plotinfoproxy, data, layer) { var i, trace, node; var carpet = data[0][0].carpet; - // mimic cartesian plotinfo var plotinfo = { xaxis: Axes.getFromId(gd, carpet.xaxis || 'x'), yaxis: Axes.getFromId(gd, carpet.yaxis || 'y'), - plot: plotinfoproxy.plot + plot: plotinfoproxy.plot, }; - scatterPlot(gd, plotinfo, data); + scatterPlot(gd, plotinfo, data, layer); for(i = 0; i < data.length; i++) { trace = data[i][0].trace; // Note: .select is adequate but seems to mutate the node data, // which is at least a bit suprising and causes problems elsewhere - node = plotinfo.plot.selectAll('g.trace' + trace.uid + ' .js-line'); + node = layer.selectAll('g.trace' + trace.uid + ' .js-line'); // Note: it would be more efficient if this didn't need to be applied // separately to all scattercarpet traces, but that would require diff --git a/src/traces/scatterpolar/plot.js b/src/traces/scatterpolar/plot.js index 3e5cb62a30b..5ad3a019bef 100644 --- a/src/traces/scatterpolar/plot.js +++ b/src/traces/scatterpolar/plot.js @@ -60,5 +60,7 @@ module.exports = function plot(gd, subplot, moduleCalcData) { } } - scatterPlot(gd, plotinfo, moduleCalcData); + var scatterLayer = subplot.layers.frontplot.select('g.scatterlayer'); + + scatterPlot(gd, plotinfo, moduleCalcData, scatterLayer); }; diff --git a/src/traces/scatterternary/plot.js b/src/traces/scatterternary/plot.js index 26b323d306e..08ef636933a 100644 --- a/src/traces/scatterternary/plot.js +++ b/src/traces/scatterternary/plot.js @@ -25,5 +25,7 @@ module.exports = function plot(gd, ternary, moduleCalcData) { layerClipId: ternary._hasClipOnAxisFalse ? ternary.clipIdRelative : null }; - scatterPlot(gd, plotinfo, moduleCalcData); + var scatterLayer = ternary.layers.frontplot.select('g.scatterlayer'); + + scatterPlot(gd, plotinfo, moduleCalcData, scatterLayer); }; diff --git a/src/traces/violin/plot.js b/src/traces/violin/plot.js index 7bdeee3d659..99d70430fdc 100644 --- a/src/traces/violin/plot.js +++ b/src/traces/violin/plot.js @@ -15,7 +15,7 @@ var boxPlot = require('../box/plot'); var linePoints = require('../scatter/line_points'); var helpers = require('./helpers'); -module.exports = function plot(gd, plotinfo, cd) { +module.exports = function plot(gd, plotinfo, cd, violinLayer) { var fullLayout = gd._fullLayout; var xa = plotinfo.xaxis; var ya = plotinfo.yaxis; @@ -32,8 +32,7 @@ module.exports = function plot(gd, plotinfo, cd) { return Drawing.smoothopen(segments[0], 1); } - var traces = plotinfo.plot.select('.violinlayer') - .selectAll('g.trace.violins') + var traces = violinLayer.selectAll('g.trace.violins') .data(cd, function(d) { return d[0].trace.uid; }); traces.enter().append('g') From da63ed4fa9030caba7d7ecdbebc24e8728753d39 Mon Sep 17 00:00:00 2001 From: etienne Date: Tue, 24 Apr 2018 17:31:42 -0400 Subject: [PATCH 03/11] make heatmap/contour(carpet)/carpet plot method remove its own trace ... instead of relying on slow selectAll() during subroutines.drawData and Plots.cleanPlot. This can be a major perf improvement on graph with many DOM nodes (e.g. from many traces and/or many subplots) --- src/plot_api/subroutines.js | 27 +++------------------------ src/plots/plots.js | 25 ++++--------------------- src/traces/carpet/plot.js | 21 +++++++++++++-------- src/traces/contour/plot.js | 15 ++++++++++++--- src/traces/contourcarpet/plot.js | 21 ++++++++++++--------- src/traces/heatmap/plot.js | 31 ++++++++++++++----------------- 6 files changed, 58 insertions(+), 82 deletions(-) diff --git a/src/plot_api/subroutines.js b/src/plot_api/subroutines.js index 494dea1fb75..019072e20cf 100644 --- a/src/plot_api/subroutines.js +++ b/src/plot_api/subroutines.js @@ -488,34 +488,13 @@ exports.doCamera = function(gd) { exports.drawData = function(gd) { var fullLayout = gd._fullLayout; var calcdata = gd.calcdata; - var rangesliderContainers = fullLayout._infolayer.selectAll('g.rangeslider-container'); var i; - // in case of traces that were heatmaps or contour maps - // previously, remove them and their colorbars explicitly + // remove old colorbars explicitly for(i = 0; i < calcdata.length; i++) { var trace = calcdata[i][0].trace; - var isVisible = (trace.visible === true); - var uid = trace.uid; - - if(!isVisible || !Registry.traceIs(trace, '2dMap')) { - var query = ( - '.hm' + uid + - ',.contour' + uid + - ',#clip' + uid - ); - - fullLayout._paper - .selectAll(query) - .remove(); - - rangesliderContainers - .selectAll(query) - .remove(); - } - - if(!isVisible || !trace._module.colorbar) { - fullLayout._infolayer.selectAll('.cb' + uid).remove(); + if(trace.visible !== true || !trace._module.colorbar) { + fullLayout._infolayer.select('.cb' + trace.uid).remove(); } } diff --git a/src/plots/plots.js b/src/plots/plots.js index 072a1323949..1af954ec8b7 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -672,8 +672,6 @@ plots.cleanPlot = function(newFullData, newFullLayout, oldFullData, oldFullLayou } } - var hasPaper = !!oldFullLayout._paper; - var hasInfoLayer = !!oldFullLayout._infolayer; var hadGl = oldFullLayout._has && oldFullLayout._has('gl'); var hasGl = newFullLayout._has && newFullLayout._has('gl'); @@ -684,6 +682,8 @@ plots.cleanPlot = function(newFullData, newFullLayout, oldFullData, oldFullLayou } } + var hasInfoLayer = !!oldFullLayout._infolayer; + oldLoop: for(i = 0; i < oldFullData.length; i++) { var oldTrace = oldFullData[i], @@ -695,26 +695,9 @@ plots.cleanPlot = function(newFullData, newFullLayout, oldFullData, oldFullLayou if(oldUid === newTrace.uid) continue oldLoop; } - var query = ( - '.hm' + oldUid + - ',.contour' + oldUid + - ',.carpet' + oldUid + - ',#clip' + oldUid + - ',.trace' + oldUid - ); - - // clean old heatmap, contour traces and clip paths - // that rely on uid identifiers - if(hasPaper) { - oldFullLayout._paper.selectAll(query).remove(); - } - - // clean old colorbars and range slider plot + // clean old colorbars if(hasInfoLayer) { - oldFullLayout._infolayer.selectAll('.cb' + oldUid).remove(); - - oldFullLayout._infolayer.selectAll('g.rangeslider-container') - .selectAll(query).remove(); + oldFullLayout._infolayer.select('.cb' + oldUid).remove(); } } diff --git a/src/traces/carpet/plot.js b/src/traces/carpet/plot.js index 0e270726da1..5e9d5434bb5 100644 --- a/src/traces/carpet/plot.js +++ b/src/traces/carpet/plot.js @@ -18,15 +18,20 @@ var svgTextUtils = require('../../lib/svg_text_utils'); var Lib = require('../../lib'); var alignmentConstants = require('../../constants/alignment'); -module.exports = function plot(gd, plotinfo, cdcarpet) { - if(!cdcarpet.length) { - plotinfo.plot.select('.carpetlayer') - .selectAll('g.trace') - .remove(); - } +module.exports = function plot(gd, plotinfo, cdcarpet, carpetLayer) { + var i; + + carpetLayer.selectAll('g.trace').each(function() { + var classString = d3.select(this).attr('class'); + var oldUid = classString.split('carpet')[1].split(/\s/)[0]; + for(i = 0; i < cdcarpet.length; i++) { + if(oldUid === cdcarpet[i][0].trace.uid) return; + } + d3.select(this).remove(); + }); - for(var i = 0; i < cdcarpet.length; i++) { - plotOne(gd, plotinfo, cdcarpet[i]); + for(i = 0; i < cdcarpet.length; i++) { + plotOne(gd, plotinfo, cdcarpet[i], carpetLayer); } }; diff --git a/src/traces/contour/plot.js b/src/traces/contour/plot.js index 66f5e708216..f5534b7fcdf 100644 --- a/src/traces/contour/plot.js +++ b/src/traces/contour/plot.js @@ -27,9 +27,18 @@ var constants = require('./constants'); var costConstants = constants.LABELOPTIMIZER; -exports.plot = function plot(gd, plotinfo, cdcontours) { - for(var i = 0; i < cdcontours.length; i++) { - plotOne(gd, plotinfo, cdcontours[i]); +exports.plot = function plot(gd, plotinfo, cdcontours, contourLayer) { + var i; + + contourLayer.selectAll('g.contour').each(function(d) { + for(i = 0; i < cdcontours.length; i++) { + if(d.trace.uid === cdcontours[i][0].trace.uid) return; + } + d3.select(this).remove(); + }); + + for(i = 0; i < cdcontours.length; i++) { + plotOne(gd, plotinfo, cdcontours[i], contourLayer); } }; diff --git a/src/traces/contourcarpet/plot.js b/src/traces/contourcarpet/plot.js index e55b2ce15fa..ab773a8fc69 100644 --- a/src/traces/contourcarpet/plot.js +++ b/src/traces/contourcarpet/plot.js @@ -26,9 +26,18 @@ var lookupCarpet = require('../carpet/lookup_carpetid'); var closeBoundaries = require('../contour/close_boundaries'); -module.exports = function plot(gd, plotinfo, cdcontours) { - for(var i = 0; i < cdcontours.length; i++) { - plotOne(gd, plotinfo, cdcontours[i]); +module.exports = function plot(gd, plotinfo, cdcontours, contourcarpetLayer) { + var i; + + contourcarpetLayer.selectAll('g.contour').each(function(d) { + for(i = 0; i < cdcontours.length; i++) { + if(d.trace.uid === cdcontours[i][0].trace.uid) return; + } + d3.select(this).remove(); + }); + + for(i = 0; i < cdcontours.length; i++) { + plotOne(gd, plotinfo, cdcontours[i], contourcarpetLayer); } }; @@ -46,7 +55,6 @@ function plotOne(gd, plotinfo, cd, contourcarpetLayer) { var uid = trace.uid; var xa = plotinfo.xaxis; var ya = plotinfo.yaxis; - var fullLayout = gd._fullLayout; var id = 'contour' + uid; var pathinfo = emptyPathinfo(contours, plotinfo, cd[0]); var isConstraint = contours.type === 'constraint'; @@ -59,11 +67,6 @@ function plotOne(gd, plotinfo, cd, contourcarpetLayer) { return [xa.c2p(pt[0]), ya.c2p(pt[1])]; } - if(trace.visible !== true) { - fullLayout._infolayer.selectAll('.cb' + uid).remove(); - return; - } - // Define the perimeter in a/b coordinates: var perimeter = [ [a[0], b[b.length - 1]], diff --git a/src/traces/heatmap/plot.js b/src/traces/heatmap/plot.js index 8b7d30ab4d0..9b125db9eff 100644 --- a/src/traces/heatmap/plot.js +++ b/src/traces/heatmap/plot.js @@ -9,6 +9,7 @@ 'use strict'; +var d3 = require('d3'); var tinycolor = require('tinycolor2'); var Registry = require('../../registry'); @@ -18,32 +19,28 @@ var xmlnsNamespaces = require('../../constants/xmlns_namespaces'); var maxRowLength = require('./max_row_length'); +module.exports = function(gd, plotinfo, cdheatmaps, heatmapLayer) { + var i; -module.exports = function(gd, plotinfo, cdheatmaps) { - for(var i = 0; i < cdheatmaps.length; i++) { - plotOne(gd, plotinfo, cdheatmaps[i]); + heatmapLayer.selectAll('.hm > image').each(function(d) { + var oldTrace = d.trace || {}; + for(i = 0; i < cdheatmaps.length; i++) { + if(oldTrace.uid === cdheatmaps[i][0].trace.uid) return; + } + d3.select(this.parentNode).remove(); + }); + + for(i = 0; i < cdheatmaps.length; i++) { + plotOne(gd, plotinfo, cdheatmaps[i], heatmapLayer); } }; function plotOne(gd, plotinfo, cd, heatmapLayer) { var cd0 = cd[0]; var trace = cd0.trace; - var uid = trace.uid; var xa = plotinfo.xaxis; var ya = plotinfo.yaxis; - var fullLayout = gd._fullLayout; - var id = 'hm' + uid; - - // in case this used to be a contour map - fullLayout._paper.selectAll('.contour' + uid).remove(); - fullLayout._infolayer.selectAll('g.rangeslider-container') - .selectAll('.contour' + uid).remove(); - - if(trace.visible !== true) { - fullLayout._paper.selectAll('.' + id).remove(); - fullLayout._infolayer.selectAll('.cb' + uid).remove(); - return; - } + var id = 'hm' + trace.uid; var z = cd0.z; var x = cd0.x; From a1c76bcf36c14ba2658ba9634a47d0e252cea044 Mon Sep 17 00:00:00 2001 From: etienne Date: Tue, 24 Apr 2018 17:33:33 -0400 Subject: [PATCH 04/11] sub imagelayer->heatmaplayer, maplayer->contourlayer in tests --- test/jasmine/tests/range_slider_test.js | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/test/jasmine/tests/range_slider_test.js b/test/jasmine/tests/range_slider_test.js index abbad0f08f1..4c725d5526d 100644 --- a/test/jasmine/tests/range_slider_test.js +++ b/test/jasmine/tests/range_slider_test.js @@ -422,7 +422,6 @@ describe('Rangeslider visibility property', function() { }); it('should clear traces in range plot when needed', function(done) { - function count(query) { return d3.select(getRangeSlider()).selectAll(query).size(); } @@ -468,12 +467,12 @@ describe('Rangeslider visibility property', function() { }]); }) .then(function() { - expect(count('g.imagelayer > g.hm')).toEqual(1); + expect(count('g.heatmaplayer > g.hm')).toEqual(1); return Plotly.restyle(gd, 'visible', false); }) .then(function() { - expect(count('g.imagelayer > g.hm')).toEqual(0); + expect(count('g.heatmaplayer > g.hm')).toEqual(0); return Plotly.restyle(gd, { visible: true, @@ -481,25 +480,25 @@ describe('Rangeslider visibility property', function() { }); }) .then(function() { - expect(count('g.maplayer > g.contour')).toEqual(1); + expect(count('g.contourlayer > g.contour')).toEqual(1); return Plotly.restyle(gd, 'type', 'heatmap'); }) .then(function() { - expect(count('g.imagelayer > g.hm')).toEqual(1); - expect(count('g.maplayer > g.contour')).toEqual(0); + expect(count('g.heatmaplayer > g.hm')).toEqual(1); + expect(count('g.contourlayer > g.contour')).toEqual(0); return Plotly.restyle(gd, 'type', 'contour'); }) .then(function() { - expect(count('g.imagelayer > g.hm')).toEqual(0); - expect(count('g.maplayer > g.contour')).toEqual(1); + expect(count('g.heatmaplayer > g.hm')).toEqual(0); + expect(count('g.contourlayer > g.contour')).toEqual(1); return Plotly.deleteTraces(gd, [0]); }) .then(function() { - expect(count('g.imagelayer > g.hm')).toEqual(0); - expect(count('g.maplayer > g.contour')).toEqual(0); + expect(count('g.heatmaplayer > g.hm')).toEqual(0); + expect(count('g.contourlayer > g.contour')).toEqual(0); }) .catch(failTest) .then(done); From fa38ca5c7b677315f55d4c460f62908db7a8b074 Mon Sep 17 00:00:00 2001 From: etienne Date: Tue, 24 Apr 2018 17:34:19 -0400 Subject: [PATCH 05/11] update `cliponaxis: false` ... to take into account take now not all trace module layers are added to each subplot. --- test/jasmine/tests/bar_test.js | 24 +++++++++++++++++++----- test/jasmine/tests/scatter_test.js | 24 +++++++++++++++++++----- 2 files changed, 38 insertions(+), 10 deletions(-) diff --git a/test/jasmine/tests/bar_test.js b/test/jasmine/tests/bar_test.js index 29cfd175833..00e3d4f4712 100644 --- a/test/jasmine/tests/bar_test.js +++ b/test/jasmine/tests/bar_test.js @@ -1549,16 +1549,30 @@ describe('bar hover', function() { var fig = Lib.extendDeep({}, require('@mocks/bar_cliponaxis-false.json')); gd = createGraphDiv(); - // only show one trace + // only show one bar trace fig.data = [fig.data[0]]; + // add a non-bar trace to make sure its module layer gets clipped + fig.data.push({ + type: 'contour', + z: [[0, 0.5, 1], [0.5, 1, 3]] + }); + + function _assertClip(sel, exp, size, msg) { + if(exp === null) { + expect(sel.size()).toBe(0, msg + 'selection should not exist'); + } else { + assertClip(sel, exp, size, msg); + } + } + function _assert(layerClips, barDisplays, barTextDisplays, barClips) { var subplotLayer = d3.select('.plot'); var barLayer = subplotLayer.select('.barlayer'); - assertClip(subplotLayer, layerClips[0], 1, 'subplot layer'); - assertClip(subplotLayer.select('.maplayer'), layerClips[1], 1, 'some other trace layer'); - assertClip(barLayer, layerClips[2], 1, 'bar layer'); + _assertClip(subplotLayer, layerClips[0], 1, 'subplot layer'); + _assertClip(subplotLayer.select('.contourlayer'), layerClips[1], 1, 'some other trace layer'); + _assertClip(barLayer, layerClips[2], 1, 'bar layer'); assertNodeDisplay( barLayer.selectAll('.point'), @@ -1589,7 +1603,7 @@ describe('bar hover', function() { }) .then(function() { _assert( - [true, false, false], + [true, null, null], [], [], [false, 0] diff --git a/test/jasmine/tests/scatter_test.js b/test/jasmine/tests/scatter_test.js index ec295c7d9d9..cf574adedf1 100644 --- a/test/jasmine/tests/scatter_test.js +++ b/test/jasmine/tests/scatter_test.js @@ -1231,13 +1231,27 @@ describe('Test scatter *clipnaxis*:', function() { // add lines fig.data[0].mode = 'markers+lines+text'; + // add a non-scatter trace to make sure its module layer gets clipped + fig.data.push({ + type: 'contour', + z: [[0, 0.5, 1], [0.5, 1, 3]] + }); + + function _assertClip(sel, exp, size, msg) { + if(exp === null) { + expect(sel.size()).toBe(0, msg + 'selection should not exist'); + } else { + assertClip(sel, exp, size, msg); + } + } + function _assert(layerClips, nodeDisplays, errorBarClips, lineClips) { var subplotLayer = d3.select('.plot'); var scatterLayer = subplotLayer.select('.scatterlayer'); - assertClip(subplotLayer, layerClips[0], 1, 'subplot layer'); - assertClip(subplotLayer.select('.maplayer'), layerClips[1], 1, 'some other trace layer'); - assertClip(scatterLayer, layerClips[2], 1, 'scatter layer'); + _assertClip(subplotLayer, layerClips[0], 1, 'subplot layer'); + _assertClip(subplotLayer.select('.contourlayer'), layerClips[1], 1, 'some other trace layer'); + _assertClip(scatterLayer, layerClips[2], 1, 'scatter layer'); assertNodeDisplay( scatterLayer.selectAll('.point'), @@ -1274,7 +1288,7 @@ describe('Test scatter *clipnaxis*:', function() { }) .then(function() { _assert( - [true, false, false], + [true, null, null], [], [false, 0], [false, 0] @@ -1292,7 +1306,7 @@ describe('Test scatter *clipnaxis*:', function() { }) .then(function() { _assert( - [true, false, false], + [true, null, null], [], [false, 0], [false, 0] From 48ee0d87cbcc872cb816ace72336c35711b6a7a0 Mon Sep 17 00:00:00 2001 From: etienne Date: Tue, 24 Apr 2018 17:36:31 -0400 Subject: [PATCH 06/11] :lock: contour with heatmap coloring layer order - previously, Contour.plot called Heatmap.call for traces with 'contour.coloring': 'heatmap' which plotted a heatmap in always below the contours. Now as does not always exist, we plot the heatmap fill inside in a ensureSingle layer. --- test/jasmine/tests/contour_test.js | 51 ++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/test/jasmine/tests/contour_test.js b/test/jasmine/tests/contour_test.js index dea87bd6869..98b50bd77be 100644 --- a/test/jasmine/tests/contour_test.js +++ b/test/jasmine/tests/contour_test.js @@ -407,4 +407,55 @@ describe('contour plotting and editing', function() { .catch(fail) .then(done); }); + + it('should always draw heatmap coloring layer below contour lines', function(done) { + var cnt = 0; + + function _assert(exp) { + var msg = ' index in (call #' + cnt + ')'; + var contourLayer = gd.querySelector('.xy > .plot > .contourlayer'); + var hmIndex = -1; + var contoursIndex = -1; + + for(var i in contourLayer.children) { + var child = contourLayer.children[i]; + if(child.querySelector) { + if(child.querySelector('.hm')) hmIndex = +i; + else if(child.querySelector('.contourlevel')) contoursIndex = +i; + } + } + + expect(hmIndex).toBe(exp.hmIndex, 'heatmap' + msg); + expect(contoursIndex).toBe(exp.contoursIndex, 'contours' + msg); + cnt++; + } + + Plotly.newPlot(gd, [{ + type: 'contour', + z: [[1, 2, 3], [1, 3, 0]], + contours: {coloring: 'heatmap'} + }]) + .then(function() { + _assert({ + hmIndex: 0, + contoursIndex: 1 + }); + return Plotly.restyle(gd, 'contours.coloring', 'lines'); + }) + .then(function() { + _assert({ + hmIndex: -1, + contoursIndex: 1 + }); + return Plotly.restyle(gd, 'contours.coloring', 'heatmap'); + }) + .then(function() { + _assert({ + hmIndex: 0, + contoursIndex: 1 + }); + }) + .catch(fail) + .then(done); + }); }); From 362da1fe1bb78cb28f4e23fc2cba57cd1756d95b Mon Sep 17 00:00:00 2001 From: etienne Date: Tue, 24 Apr 2018 17:36:47 -0400 Subject: [PATCH 07/11] :lock: scattercarpet + scatter coexistence --- test/jasmine/tests/carpet_test.js | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/test/jasmine/tests/carpet_test.js b/test/jasmine/tests/carpet_test.js index 1bae596789d..db602897759 100644 --- a/test/jasmine/tests/carpet_test.js +++ b/test/jasmine/tests/carpet_test.js @@ -511,6 +511,32 @@ describe('Test carpet interactions:', function() { .catch(fail) .then(done); }); + + it('scattercarpet should be able to coexist with scatter traces', function(done) { + var mock = Lib.extendDeep({}, require('@mocks/scattercarpet.json')); + + function _assert(exp) { + expect(d3.selectAll('.point').size()) + .toBe(exp, 'number of scatter pts on graph'); + } + + Plotly.newPlot(gd, mock).then(function() { + _assert(12); + + return Plotly.addTraces(gd, { + y: [1, 2, 1] + }); + }) + .then(function() { + _assert(15); + return Plotly.deleteTraces(gd, [0]); + }) + .then(function() { + _assert(3); + }) + .catch(fail) + .then(done); + }); }); describe('scattercarpet array attributes', function() { From 3bef9a99c2439e74d582c028a2c1e02c70739ffd Mon Sep 17 00:00:00 2001 From: etienne Date: Mon, 30 Apr 2018 11:03:23 -0400 Subject: [PATCH 08/11] add getUidsFromCalcData helper --- src/plots/get_data.js | 17 +++++++++++++++++ src/traces/carpet/plot.js | 11 ++++++----- src/traces/contour/plot.js | 11 +++++------ src/traces/contourcarpet/plot.js | 11 +++++------ src/traces/heatmap/plot.js | 11 ++++++----- 5 files changed, 39 insertions(+), 22 deletions(-) diff --git a/src/plots/get_data.js b/src/plots/get_data.js index 93b83f6c4d9..8d6bceb5b20 100644 --- a/src/plots/get_data.js +++ b/src/plots/get_data.js @@ -123,3 +123,20 @@ exports.getSubplotData = function getSubplotData(data, type, subplotId) { return subplotData; }; + +/** + * Get a lookup object of trace uids corresponding in a given calcdata array. + * + * @param {array} calcdata: as in gd.calcdata (or a subset) + * @return {object} lookup object of uids (`uid: 1`) + */ +exports.getUidsFromCalcData = function(calcdata) { + var out = {}; + + for(var i = 0; i < calcdata.length; i++) { + var trace = calcdata[i][0].trace; + out[trace.uid] = 1; + } + + return out; +}; diff --git a/src/traces/carpet/plot.js b/src/traces/carpet/plot.js index 5e9d5434bb5..c1a04ba4087 100644 --- a/src/traces/carpet/plot.js +++ b/src/traces/carpet/plot.js @@ -17,20 +17,21 @@ var orientText = require('./orient_text'); var svgTextUtils = require('../../lib/svg_text_utils'); var Lib = require('../../lib'); var alignmentConstants = require('../../constants/alignment'); +var getUidsFromCalcData = require('../../plots/get_data').getUidsFromCalcData; module.exports = function plot(gd, plotinfo, cdcarpet, carpetLayer) { - var i; + var uidLookup = getUidsFromCalcData(cdcarpet); carpetLayer.selectAll('g.trace').each(function() { var classString = d3.select(this).attr('class'); var oldUid = classString.split('carpet')[1].split(/\s/)[0]; - for(i = 0; i < cdcarpet.length; i++) { - if(oldUid === cdcarpet[i][0].trace.uid) return; + + if(!uidLookup[oldUid]) { + d3.select(this).remove(); } - d3.select(this).remove(); }); - for(i = 0; i < cdcarpet.length; i++) { + for(var i = 0; i < cdcarpet.length; i++) { plotOne(gd, plotinfo, cdcarpet[i], carpetLayer); } }; diff --git a/src/traces/contour/plot.js b/src/traces/contour/plot.js index f5534b7fcdf..cc391889854 100644 --- a/src/traces/contour/plot.js +++ b/src/traces/contour/plot.js @@ -16,6 +16,7 @@ var Drawing = require('../../components/drawing'); var svgTextUtils = require('../../lib/svg_text_utils'); var Axes = require('../../plots/cartesian/axes'); var setConvert = require('../../plots/cartesian/set_convert'); +var getUidsFromCalcData = require('../../plots/get_data').getUidsFromCalcData; var heatmapPlot = require('../heatmap/plot'); var makeCrossings = require('./make_crossings'); @@ -26,18 +27,16 @@ var closeBoundaries = require('./close_boundaries'); var constants = require('./constants'); var costConstants = constants.LABELOPTIMIZER; - exports.plot = function plot(gd, plotinfo, cdcontours, contourLayer) { - var i; + var uidLookup = getUidsFromCalcData(cdcontours); contourLayer.selectAll('g.contour').each(function(d) { - for(i = 0; i < cdcontours.length; i++) { - if(d.trace.uid === cdcontours[i][0].trace.uid) return; + if(!uidLookup[d.trace.uid]) { + d3.select(this).remove(); } - d3.select(this).remove(); }); - for(i = 0; i < cdcontours.length; i++) { + for(var i = 0; i < cdcontours.length; i++) { plotOne(gd, plotinfo, cdcontours[i], contourLayer); } }; diff --git a/src/traces/contourcarpet/plot.js b/src/traces/contourcarpet/plot.js index ab773a8fc69..16689efd848 100644 --- a/src/traces/contourcarpet/plot.js +++ b/src/traces/contourcarpet/plot.js @@ -13,6 +13,7 @@ var map1dArray = require('../carpet/map_1d_array'); var makepath = require('../carpet/makepath'); var Drawing = require('../../components/drawing'); var Lib = require('../../lib'); +var getUidsFromCalcData = require('../../plots/get_data').getUidsFromCalcData; var makeCrossings = require('../contour/make_crossings'); var findAllPaths = require('../contour/find_all_paths'); @@ -25,18 +26,16 @@ var mapPathinfo = require('./map_pathinfo'); var lookupCarpet = require('../carpet/lookup_carpetid'); var closeBoundaries = require('../contour/close_boundaries'); - module.exports = function plot(gd, plotinfo, cdcontours, contourcarpetLayer) { - var i; + var uidLookup = getUidsFromCalcData(cdcontours); contourcarpetLayer.selectAll('g.contour').each(function(d) { - for(i = 0; i < cdcontours.length; i++) { - if(d.trace.uid === cdcontours[i][0].trace.uid) return; + if(!uidLookup[d.trace.uid]) { + d3.select(this).remove(); } - d3.select(this).remove(); }); - for(i = 0; i < cdcontours.length; i++) { + for(var i = 0; i < cdcontours.length; i++) { plotOne(gd, plotinfo, cdcontours[i], contourcarpetLayer); } }; diff --git a/src/traces/heatmap/plot.js b/src/traces/heatmap/plot.js index 9b125db9eff..3d5f73828f4 100644 --- a/src/traces/heatmap/plot.js +++ b/src/traces/heatmap/plot.js @@ -16,21 +16,22 @@ var Registry = require('../../registry'); var Lib = require('../../lib'); var Colorscale = require('../../components/colorscale'); var xmlnsNamespaces = require('../../constants/xmlns_namespaces'); +var getUidsFromCalcData = require('../../plots/get_data').getUidsFromCalcData; var maxRowLength = require('./max_row_length'); module.exports = function(gd, plotinfo, cdheatmaps, heatmapLayer) { - var i; + var uidLookup = getUidsFromCalcData(cdheatmaps); heatmapLayer.selectAll('.hm > image').each(function(d) { var oldTrace = d.trace || {}; - for(i = 0; i < cdheatmaps.length; i++) { - if(oldTrace.uid === cdheatmaps[i][0].trace.uid) return; + + if(!uidLookup[oldTrace.uid]) { + d3.select(this.parentNode).remove(); } - d3.select(this.parentNode).remove(); }); - for(i = 0; i < cdheatmaps.length; i++) { + for(var i = 0; i < cdheatmaps.length; i++) { plotOne(gd, plotinfo, cdheatmaps[i], heatmapLayer); } }; From d47276f969f2cbcfbd5f3383247e23c71ff960de Mon Sep 17 00:00:00 2001 From: etienne Date: Mon, 30 Apr 2018 11:06:26 -0400 Subject: [PATCH 09/11] make visible trace module -> layer data a 1D loop w/ + indexOf + sort - I don't see the need to optimize here using e.g a lookup object. traceLayerClasses.length ~ 10, and fullLayout._modules.length < 20, most often 1 or 2. --- src/plots/cartesian/index.js | 50 +++++++++++++++++------------------- 1 file changed, 24 insertions(+), 26 deletions(-) diff --git a/src/plots/cartesian/index.js b/src/plots/cartesian/index.js index 59b507c86ca..7bf7e43ba57 100644 --- a/src/plots/cartesian/index.js +++ b/src/plots/cartesian/index.js @@ -189,36 +189,34 @@ function plotOne(gd, plotinfo, cdSubplot, transitionOpts, makeOnCompleteCallback var layerData = []; - for(var i = 0; i < traceLayerClasses.length; i++) { - var className = traceLayerClasses[i]; - - for(var j = 0; j < modules.length; j++) { - _module = modules[j]; - - if(_module.basePlotModule.name === 'cartesian' && - (_module.layerName || _module.name + 'layer') === className - ) { - var plotMethod = _module.plot; - - // plot all traces of this type on this subplot at once - cdModuleAndOthers = getModuleCalcData(cdSubplot, plotMethod); - cdModule = cdModuleAndOthers[0]; - // don't need to search the found traces again - in fact we need to NOT - // so that if two modules share the same plotter we don't double-plot - cdSubplot = cdModuleAndOthers[1]; - - if(cdModule.length) { - layerData.push({ - className: className, - plotMethod: plotMethod, - cdModule: cdModule - }); - } - break; + for(var i = 0; i < modules.length; i++) { + _module = modules[i]; + var name = _module.name; + + if(Registry.modules[name].categories.svg) { + var className = (_module.layerName || name + 'layer'); + var plotMethod = _module.plot; + + // plot all traces of this type on this subplot at once + cdModuleAndOthers = getModuleCalcData(cdSubplot, plotMethod); + cdModule = cdModuleAndOthers[0]; + // don't need to search the found traces again - in fact we need to NOT + // so that if two modules share the same plotter we don't double-plot + cdSubplot = cdModuleAndOthers[1]; + + if(cdModule.length) { + layerData.push({ + i: traceLayerClasses.indexOf(className), + className: className, + plotMethod: plotMethod, + cdModule: cdModule + }); } } } + layerData.sort(function(a, b) { return a.i - b.i; }); + var layers = plotinfo.plot.selectAll('g.mlayer') .data(layerData, function(d) { return d.className; }); From 83422f7ce39abfa571e43abb8c3e628216e91c9b Mon Sep 17 00:00:00 2001 From: etienne Date: Mon, 30 Apr 2018 11:20:25 -0400 Subject: [PATCH 10/11] skip gl tags in flaky-tagged jasmine test cmd --- .circleci/test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/test.sh b/.circleci/test.sh index ea9b64140e4..656d8705148 100755 --- a/.circleci/test.sh +++ b/.circleci/test.sh @@ -34,7 +34,7 @@ case $1 in jasmine2) retry npm run test-jasmine -- --tags=gl --skip-tags=noCI,flaky - retry npm run test-jasmine -- --tags=flaky --skip-tags=noCI + retry npm run test-jasmine -- --tags=flaky --skip-tags=noCI,gl npm run test-bundle || EXIT_STATE=$? exit $EXIT_STATE ;; From 8c9a7bf660f3e02fc7ba14ffc9cfe5b953c6a3f3 Mon Sep 17 00:00:00 2001 From: etienne Date: Mon, 30 Apr 2018 11:43:58 -0400 Subject: [PATCH 11/11] Revert "skip gl tags in flaky-tagged jasmine test cmd" - so that we cover tests that are tagged @gl and @flaky This reverts commit 83422f7ce39abfa571e43abb8c3e628216e91c9b. --- .circleci/test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/test.sh b/.circleci/test.sh index 656d8705148..ea9b64140e4 100755 --- a/.circleci/test.sh +++ b/.circleci/test.sh @@ -34,7 +34,7 @@ case $1 in jasmine2) retry npm run test-jasmine -- --tags=gl --skip-tags=noCI,flaky - retry npm run test-jasmine -- --tags=flaky --skip-tags=noCI,gl + retry npm run test-jasmine -- --tags=flaky --skip-tags=noCI npm run test-bundle || EXIT_STATE=$? exit $EXIT_STATE ;;