Skip to content

Even better cartesian trace updates and removal #2579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Apr 30, 2018
+335 −230
Merged

Even better cartesian trace updates and removal #2579

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
400bfe6
ping imagetest after having restarting nw.js
etpinard Apr 24, 2018
284c206
introduce new strategy for cartesian trace module layer updates
etpinard Apr 24, 2018
da63ed4
make heatmap/contour(carpet)/carpet plot method remove its own trace
etpinard Apr 24, 2018
a1c76bc
sub imagelayer->heatmaplayer, maplayer->contourlayer in tests
etpinard Apr 24, 2018
fa38ca5
update `cliponaxis: false`
etpinard Apr 24, 2018
48ee0d8
:lock: contour with heatmap coloring layer order
etpinard Apr 24, 2018
362da1f
:lock: scattercarpet + scatter coexistence
etpinard Apr 24, 2018
3bef9a9
add getUidsFromCalcData helper
etpinard Apr 30, 2018
d47276f
make visible trace module -> layer data a 1D loop w/ + indexOf + sort
etpinard Apr 30, 2018
83422f7
skip gl tags in flaky-tagged jasmine test cmd
etpinard Apr 30, 2018
8c9a7bf
Revert "skip gl tags in flaky-tagged jasmine test cmd"
etpinard Apr 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions src/components/rangeslider/draw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
});
}

Expand Down
38 changes: 5 additions & 33 deletions src/plot_api/subroutines.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -495,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();
}
}

Expand Down
7 changes: 4 additions & 3 deletions src/plots/cartesian/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

N.B. On the other hand, trace modules (e.g. scattercarpet) that only wrap another plot method need a separate layer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason some of the new ones share lines?

Copy link
Contributor Author

@etpinard etpinard Apr 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put modules that almost have the same plot method on the same line. I thought that would make it easier to track. I can change that of course.

],

layerValue2layerClass: {
Expand Down
96 changes: 65 additions & 31 deletions src/plots/cartesian/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be a 2D loop? Could we just loop over modules, include the layer index in layerData, and sort by that at the end? I guess we'd also need to track which layers we've already included, to ensure uniqueness. But still seems faster than looping over all combinations of className and _module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid sorting for 🐎 but you're probably right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in d47276f

}
}
}

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) {
Expand Down Expand Up @@ -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
Expand Down
25 changes: 4 additions & 21 deletions src/plots/plots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand All @@ -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],
Expand All @@ -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();
}
}

Expand Down
11 changes: 5 additions & 6 deletions src/traces/bar/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
5 changes: 2 additions & 3 deletions src/traces/box/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
1 change: 1 addition & 0 deletions src/traces/candlestick/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
26 changes: 15 additions & 11 deletions src/traces/carpet/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,24 @@ 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;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌴 🐎 perhaps make a common helper getUidsFromCalcdata or something, that can be called once up front and returns a hash {uid0: 1, uid1: 1, ...}, then if(!uidHash[oldUid]) d3.select(this).remove();?
Looks like the same helper would work for all modules in this commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in d47276f

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);
}
};

function plotOne(gd, plotinfo, cd) {
function plotOne(gd, plotinfo, cd, carpetLayer) {
var t = cd[0];
var trace = cd[0].trace,
xa = plotinfo.xaxis,
Expand All @@ -39,10 +44,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');
Expand Down
Loading