Skip to content

Splom zoom perf #2527

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 14 commits into from
Apr 11, 2018
Merged

Splom zoom perf #2527

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
02ed2eb
move doAutoRangeAndConstraints, drawData and finalDraw to subroutines
etpinard Apr 5, 2018
3ad1eaa
add 'axrange' editType
etpinard Apr 5, 2018
bb02281
speed up doModeBar subroutines for cartesian subplots
etpinard Apr 5, 2018
d07ae70
improve fullLayout._has()
etpinard Apr 5, 2018
468119e
speed up dragbox
etpinard Apr 5, 2018
0979272
clean splom drag logic
etpinard Apr 5, 2018
ebb35ce
lint in plot_api
etpinard Apr 5, 2018
f9090b7
(fixup) add draggedPts to scattercarpet categories
etpinard Apr 6, 2018
b2ee736
Merge branch 'splom-feature' into splom-zoom-perf
etpinard Apr 9, 2018
a0b2574
Merge branch 'splom-feature' into splom-zoom-perf
etpinard Apr 9, 2018
d3fe40d
replace selectAll with for(k in _plots) loop
etpinard Apr 10, 2018
cc1b3de
fixup and :lock: dragbox with linked y axes
etpinard Apr 10, 2018
980855c
Merge branch 'splom-feature' into splom-zoom-perf
etpinard Apr 11, 2018
f7d637d
:books: link TODOs to gh issues
etpinard Apr 11, 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
6 changes: 5 additions & 1 deletion src/components/shapes/draw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@ function draw(gd) {
// Remove previous shapes before drawing new in shapes in fullLayout.shapes
fullLayout._shapeUpperLayer.selectAll('path').remove();
fullLayout._shapeLowerLayer.selectAll('path').remove();
fullLayout._shapeSubplotLayers.selectAll('path').remove();

for(var k in fullLayout._plots) {
var shapelayer = fullLayout._plots[k].shapelayer;
if(shapelayer) shapelayer.selectAll('path').remove();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting - why is this better? Wasn't fullLayout._shapeSubplotLayers essentially just an array of all the shapelayer nodes? Or is the win actually below in subroutines.js that you don't have to do a selectAll to find these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or is the win actually below in subroutines.js that you don't have to do a selectAll to find these?

That. selectAll are bad.

}

for(var i = 0; i < fullLayout.shapes.length; i++) {
if(fullLayout.shapes[i].visible) {
Expand Down
3 changes: 2 additions & 1 deletion src/plot_api/edit_types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ var layoutOpts = {
valType: 'flaglist',
extras: ['none'],
flags: [
'calc', 'calcIfAutorange', 'plot', 'legend', 'ticks', 'margins',
'calc', 'calcIfAutorange', 'plot', 'legend', 'ticks', 'axrange', 'margins',
'layoutstyle', 'modebar', 'camera', 'arraydraw'
],
description: [
Expand All @@ -48,6 +48,7 @@ var layoutOpts = {
'*legend* only redraws the legend.',
'*ticks* only redraws axis ticks, labels, and gridlines.',
'*margins* recomputes ticklabel automargins.',
'*axrange* minimal sequence when updating axis ranges.',
'*layoutstyle* reapplies global and SVG cartesian axis styles.',
'*modebar* just updates the modebar.',
'*camera* just updates the camera settings for gl3d scenes.',
Expand Down
164 changes: 58 additions & 106 deletions src/plot_api/plot_api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,26 +22,21 @@ var Registry = require('../registry');
var PlotSchema = require('./plot_schema');
var Plots = require('../plots/plots');
var Polar = require('../plots/polar/legacy');
var initInteractions = require('../plots/cartesian/graph_interact');

var Axes = require('../plots/cartesian/axes');
var Drawing = require('../components/drawing');
var Color = require('../components/color');
var initInteractions = require('../plots/cartesian/graph_interact').initInteractions;
var xmlnsNamespaces = require('../constants/xmlns_namespaces');
var svgTextUtils = require('../lib/svg_text_utils');
var clearGlCanvases = require('../lib/clear_gl_canvases');

var defaultConfig = require('./plot_config');
var manageArrays = require('./manage_arrays');
var helpers = require('./helpers');
var subroutines = require('./subroutines');
var editTypes = require('./edit_types');

var cartesianConstants = require('../plots/cartesian/constants');
var axisConstraints = require('../plots/cartesian/constraints');
var enforceAxisConstraints = axisConstraints.enforce;
var cleanAxisConstraints = axisConstraints.clean;
var doAutoRange = require('../plots/cartesian/autorange').doAutoRange;
var AX_NAME_PATTERN = require('../plots/cartesian/constants').AX_NAME_PATTERN;

var numericNameWarningCount = 0;
var numericNameWarningCountLimit = 5;
Expand Down Expand Up @@ -331,15 +326,7 @@ exports.plot = function(gd, data, layout, config) {
function doAutoRangeAndConstraints() {
if(gd._transitioning) return;

var axList = Axes.list(gd, '', true);
for(var i = 0; i < axList.length; i++) {
var ax = axList[i];
cleanAxisConstraints(gd, ax);

doAutoRange(ax);
}

enforceAxisConstraints(gd);
subroutines.doAutoRangeAndConstraints(gd);

// store initial ranges *after* enforcing constraints, otherwise
// we will never look like we're at the initial ranges
Expand All @@ -351,83 +338,6 @@ exports.plot = function(gd, data, layout, config) {
return Axes.doTicks(gd, graphWasEmpty ? '' : 'redraw');
}

// Now plot the data
function drawData() {
var calcdata = gd.calcdata,
i,
rangesliderContainers = fullLayout._infolayer.selectAll('g.rangeslider-container');

// in case of traces that were heatmaps or contour maps
// previously, remove them and their colorbars explicitly
for(i = 0; i < calcdata.length; i++) {
var trace = calcdata[i][0].trace,
isVisible = (trace.visible === true),
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();
}
}

// TODO does this break or slow down parcoords??
clearGlCanvases(gd);

// loop over the base plot modules present on graph
var basePlotModules = fullLayout._basePlotModules;
for(i = 0; i < basePlotModules.length; i++) {
basePlotModules[i].plot(gd);
}

// keep reference to shape layers in subplots
var layerSubplot = fullLayout._paper.selectAll('.layer-subplot');
fullLayout._shapeSubplotLayers = layerSubplot.selectAll('.shapelayer');

// styling separate from drawing
Plots.style(gd);

// show annotations and shapes
Registry.getComponentMethod('shapes', 'draw')(gd);
Registry.getComponentMethod('annotations', 'draw')(gd);

// source links
Plots.addLinks(gd);

// Mark the first render as complete
fullLayout._replotting = false;

return Plots.previousPromises(gd);
}

// An initial paint must be completed before these components can be
// correctly sized and the whole plot re-margined. fullLayout._replotting must
// be set to false before these will work properly.
function finalDraw() {
Registry.getComponentMethod('shapes', 'draw')(gd);
Registry.getComponentMethod('images', 'draw')(gd);
Registry.getComponentMethod('annotations', 'draw')(gd);
Registry.getComponentMethod('legend', 'draw')(gd);
Registry.getComponentMethod('rangeslider', 'draw')(gd);
Registry.getComponentMethod('rangeselector', 'draw')(gd);
Registry.getComponentMethod('sliders', 'draw')(gd);
Registry.getComponentMethod('updatemenus', 'draw')(gd);
}

var seq = [
Plots.previousPromises,
addFrames,
Expand All @@ -439,9 +349,10 @@ exports.plot = function(gd, data, layout, config) {
seq.push(subroutines.layoutStyles);
if(hasCartesian) seq.push(drawAxes);
seq.push(
drawData,
finalDraw,
subroutines.drawData,
subroutines.finalDraw,
initInteractions,
Plots.addLinks,
Plots.rehover,
Plots.previousPromises
);
Expand Down Expand Up @@ -1385,8 +1296,8 @@ exports.restyle = function restyle(gd, astr, val, _traces) {

var traces = helpers.coerceTraceIndices(gd, _traces);

var specs = _restyle(gd, aobj, traces),
flags = specs.flags;
var specs = _restyle(gd, aobj, traces);
var flags = specs.flags;

// clear calcdata and/or axis types if required so they get regenerated
if(flags.clearCalc) gd.calcdata = undefined;
Expand Down Expand Up @@ -1750,8 +1661,8 @@ exports.relayout = function relayout(gd, astr, val) {

if(Object.keys(aobj).length) gd.changed = true;

var specs = _relayout(gd, aobj),
flags = specs.flags;
var specs = _relayout(gd, aobj);
var flags = specs.flags;

// clear calcdata if required
if(flags.calc) gd.calcdata = undefined;
Expand All @@ -1772,6 +1683,30 @@ exports.relayout = function relayout(gd, astr, val) {

if(flags.legend) seq.push(subroutines.doLegend);
if(flags.layoutstyle) seq.push(subroutines.layoutStyles);

if(flags.axrange) {
// N.B. leave as sequence of subroutines (for now) instead of
// subroutine of its own so that finalDraw always gets
// executed after drawData
seq.push(
// TODO
// no test fail when commenting out doAutoRangeAndConstraints,
// but I think we do need this (maybe just the enforce part?)
// Am I right?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh, I'd have thought so... perhaps worth spending a little time looking for something that does fail if this is removed, so it can be 🔒 down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcjohnson Tests are currently passing even when subroutines.doAutoRangeAndConstraints is commented out, because of this block:

plotly.js/src/plot_api/plot_api.js

Lines 2088 to 2107 in 2a25820

// figure out if we need to recalculate axis constraints
var constraints = fullLayout._axisConstraintGroups || [];
for(axId in rangesAltered) {
for(i = 0; i < constraints.length; i++) {
var group = constraints[i];
if(group[axId]) {
// Always recalc if we're changing constrained ranges.
// Otherwise it's possible to violate the constraints by
// specifying arbitrary ranges for all axes in the group.
// this way some ranges may expand beyond what's specified,
// as they do at first draw, to satisfy the constraints.
flags.calc = true;
for(var groupAxId in group) {
if(!rangesAltered[groupAxId]) {
Axes.getFromId(gd, groupAxId)._constraintShrinkable = true;
}
}
}
}
}

which makes axis range relayout calls altering constrained axes go though the calc: true edit pathway which (of course) calls doAutoRangeAndConstraints itself.

Interestingly, commenting out that block which sets flags.calc = true does not make any test fails when subroutines.doAutoRangeAndConstraints is called during axrange edits. Perhaps we don't need?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't look to me as though I wrote all that great test coverage for this section... the two key points of it are:

  • You can relayout a constrained axis, particularly to a smaller range, and have the other axes in the group update (expanding or shrinking) to continue to meet the constraints (note that this won't work with react, there you'd have to manually update all axes in the constraint group)
  • If you relayout (or react) two or more axes in a group in such a way as to explicitly violate the constraints, one or more of those axes will expand (never shrink) its range to fit the constraints.

So if you want to clean this up we should first add a few tests explicitly for ^^. I see some gui tests and one restyle test that uses constraints, but unfortunately no relayout or react tests of these edge cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok if I open up an issue about this and defer this to a later PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok if I open up an issue about this and defer this to a later PR?

Yep for sure - that was the "if you want to clean this up" clause

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to -> #2540

// More info in:
// https://github.com/plotly/plotly.js/issues/2540
subroutines.doAutoRangeAndConstraints,
// TODO
// can target specific axes,
// do not have to redraw all axes here
// See:
// https://github.com/plotly/plotly.js/issues/2547
subroutines.doTicksRelayout,
subroutines.drawData,
subroutines.finalDraw
);
}

if(flags.ticks) seq.push(subroutines.doTicksRelayout);
if(flags.modebar) seq.push(subroutines.doModeBar);
if(flags.camera) seq.push(subroutines.doCamera);
Expand Down Expand Up @@ -1992,7 +1927,7 @@ function _relayout(gd, aobj) {
}
Lib.nestedProperty(fullLayout, ptrunk + '._inputRange').set(null);
}
else if(pleaf.match(cartesianConstants.AX_NAME_PATTERN)) {
else if(pleaf.match(AX_NAME_PATTERN)) {
var fullProp = Lib.nestedProperty(fullLayout, ai).get(),
newType = (vi || {}).type;

Expand Down Expand Up @@ -2045,8 +1980,9 @@ function _relayout(gd, aobj) {
if(checkForAutorange && (refAutorange(gd, objToAutorange, 'x') || refAutorange(gd, objToAutorange, 'y'))) {
flags.calc = true;
}
else editTypes.update(flags, updateValObject);

else {
editTypes.update(flags, updateValObject);
}

// prepare the edits object we'll send to applyContainerArrayChanges
if(!arrayEdits[arrayStr]) arrayEdits[arrayStr] = {};
Expand Down Expand Up @@ -2197,11 +2133,11 @@ exports.update = function update(gd, traceUpdate, layoutUpdate, _traces) {

var traces = helpers.coerceTraceIndices(gd, _traces);

var restyleSpecs = _restyle(gd, Lib.extendFlat({}, traceUpdate), traces),
restyleFlags = restyleSpecs.flags;
var restyleSpecs = _restyle(gd, Lib.extendFlat({}, traceUpdate), traces);
var restyleFlags = restyleSpecs.flags;

var relayoutSpecs = _relayout(gd, Lib.extendFlat({}, layoutUpdate)),
relayoutFlags = relayoutSpecs.flags;
var relayoutSpecs = _relayout(gd, Lib.extendFlat({}, layoutUpdate));
var relayoutFlags = relayoutSpecs.flags;

// clear calcdata and/or axis types if required
if(restyleFlags.clearCalc || relayoutFlags.calc) gd.calcdata = undefined;
Expand Down Expand Up @@ -2236,6 +2172,14 @@ exports.update = function update(gd, traceUpdate, layoutUpdate, _traces) {
if(restyleFlags.colorbars) seq.push(subroutines.doColorBars);
if(relayoutFlags.legend) seq.push(subroutines.doLegend);
if(relayoutFlags.layoutstyle) seq.push(subroutines.layoutStyles);
if(relayoutFlags.axrange) {
seq.push(
subroutines.doAutoRangeAndConstraints,
subroutines.doTicksRelayout,
subroutines.drawData,
subroutines.finalDraw
);
}
if(relayoutFlags.ticks) seq.push(subroutines.doTicksRelayout);
if(relayoutFlags.modebar) seq.push(subroutines.doModeBar);
if(relayoutFlags.camera) seq.push(subroutines.doCamera);
Expand Down Expand Up @@ -2388,6 +2332,14 @@ exports.react = function(gd, data, layout, config) {
if(restyleFlags.colorbars) seq.push(subroutines.doColorBars);
if(relayoutFlags.legend) seq.push(subroutines.doLegend);
if(relayoutFlags.layoutstyle) seq.push(subroutines.layoutStyles);
if(relayoutFlags.axrange) {
seq.push(
subroutines.doAutoRangeAndConstraints,
subroutines.doTicksRelayout,
subroutines.drawData,
subroutines.finalDraw
);
}
if(relayoutFlags.ticks) seq.push(subroutines.doTicksRelayout);
if(relayoutFlags.modebar) seq.push(subroutines.doModeBar);
if(relayoutFlags.camera) seq.push(subroutines.doCamera);
Expand Down
Loading