Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Splom zoom perf #2527

Merged
merged 14 commits into from
Apr 11, 2018
Merged

Splom zoom perf #2527

Show file tree
Hide file tree
Changes from 2 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
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
133 changes: 40 additions & 93 deletions src/plot_api/plot_api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ var Drawing = require('../components/drawing');
var Color = require('../components/color');
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');
Expand All @@ -38,10 +37,6 @@ 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 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 @@ -1772,6 +1683,26 @@ 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

subroutines.doAutoRangeAndConstraints,
// TODO
// can target specific axes,
// do not have to redraw all axes here
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 @@ -2236,6 +2167,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 +2327,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
92 changes: 92 additions & 0 deletions src/plot_api/subroutines.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@
var d3 = require('d3');
var Registry = require('../registry');
var Plots = require('../plots/plots');

var Lib = require('../lib');
var clearGlCanvases = require('../lib/clear_gl_canvases');

var Color = require('../components/color');
var Drawing = require('../components/drawing');
Expand All @@ -21,6 +23,10 @@ var Axes = require('../plots/cartesian/axes');
var initInteractions = require('../plots/cartesian/graph_interact');
var cartesianConstants = require('../plots/cartesian/constants');
var alignmentConstants = require('../constants/alignment');
var axisConstraints = require('../plots/cartesian/constraints');
var enforceAxisConstraints = axisConstraints.enforce;
var cleanAxisConstraints = axisConstraints.clean;
var doAutoRange = require('../plots/cartesian/autorange').doAutoRange;

exports.layoutStyles = function(gd) {
return Lib.syncOrAsync([Plots.doAutoMargin, exports.lsInner], gd);
Expand Down Expand Up @@ -480,3 +486,89 @@ exports.doCamera = function(gd) {
scene.setCamera(sceneLayout.camera);
}
};

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

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

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

return Plots.previousPromises(gd);
};

exports.doAutoRangeAndConstraints = function(gd) {
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);
};

// 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.
exports.finalDraw = function(gd) {
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);
};
2 changes: 1 addition & 1 deletion src/plots/cartesian/axes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2188,7 +2188,7 @@ axes.doTicks = function(gd, axid, skipTitle) {
}
drawTicks(mainPlotinfo[axLetter + 'axislayer'], tickpath);

tickSubplots = Object.keys(ax._linepositions);
tickSubplots = Object.keys(ax._linepositions || {});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunate, but important as relinkPrivateKeys does relink empty objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... does not relink

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does not relink empty objects.

}

tickSubplots.map(function(subplot) {
Expand Down
6 changes: 3 additions & 3 deletions src/plots/cartesian/layout_attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,10 @@ module.exports = {
valType: 'info_array',
role: 'info',
items: [
{valType: 'any', editType: 'plot+margins', impliedEdits: {'^autorange': false}},
{valType: 'any', editType: 'plot+margins', impliedEdits: {'^autorange': false}}
{valType: 'any', editType: 'axrange+margins', impliedEdits: {'^autorange': false}},
{valType: 'any', editType: 'axrange+margins', impliedEdits: {'^autorange': false}}
],
editType: 'plot+margins',
editType: 'axrange+margins',
impliedEdits: {'autorange': false},
description: [
'Sets the range of this axis.',
Expand Down
45 changes: 44 additions & 1 deletion test/jasmine/tests/plot_api_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,10 @@ describe('Test plot api', function() {
'layoutStyles',
'doTicksRelayout',
'doModeBar',
'doCamera'
'doCamera',
'doAutoRangeAndConstraints',
'drawData',
'finalDraw'
];

var gd;
Expand Down Expand Up @@ -613,6 +616,46 @@ describe('Test plot api', function() {
expectReplot(attr);
}
});

it('should trigger minimal sequence for cartesian axis range updates', function() {
var seq = ['doAutoRangeAndConstraints', 'doTicksRelayout', 'drawData', 'finalDraw'];

function _assert(msg) {
expect(gd.calcdata).toBeDefined();
mockedMethods.forEach(function(m) {
expect(subroutines[m].calls.count()).toBe(
seq.indexOf(m) === -1 ? 0 : 1,
'# of ' + m + ' calls - ' + msg
);
});
}

var trace = {y: [1, 2, 1]};

var specs = [
['relayout', ['xaxis.range[0]', 0]],
['relayout', ['xaxis.range[1]', 3]],
['relayout', ['xaxis.range', [-1, 5]]],
['update', [{}, {'xaxis.range': [-1, 10]}]],
['react', [[trace], {xaxis: {range: [0, 1]}}]]
];

specs.forEach(function(s) {
// create 'real' div for Plotly.react to work
gd = createGraphDiv();
Plotly.plot(gd, [trace], {xaxis: {range: [1, 2]}});
mock(gd);

Plotly[s[0]](gd, s[1][0], s[1][1]);

_assert([
'Plotly.', s[0],
'(gd, ', JSON.stringify(s[1][0]), ', ', JSON.stringify(s[1][1]), ')'
].join(''));

destroyGraphDiv();
});
});
});

describe('Plotly.restyle subroutines switchboard', function() {
Expand Down