Skip to content

Faster svg pan #2623

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.

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 4 commits into from
May 10, 2018
Merged
Changes from all commits
Commits
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
35 changes: 14 additions & 21 deletions src/components/drawing/index.js
Original file line number Diff line number Diff line change
@@ -100,18 +100,17 @@ drawing.hideOutsideRangePoint = function(d, sel, xa, ya, xcalendar, ycalendar) {
);
};

drawing.hideOutsideRangePoints = function(traceGroups, subplot, selector) {
drawing.hideOutsideRangePoints = function(traceGroups, subplot) {
if(!subplot._hasClipOnAxisFalse) return;

selector = selector || '.point,.textpoint';

var xa = subplot.xaxis;
var ya = subplot.yaxis;

traceGroups.each(function(d) {
var trace = d[0].trace;
var xcalendar = trace.xcalendar;
var ycalendar = trace.ycalendar;
var selector = trace.type === 'bar' ? '.bartext' : '.point,.textpoint';

traceGroups.selectAll(selector).each(function(d) {
drawing.hideOutsideRangePoint(d, d3.select(this), xa, ya, xcalendar, ycalendar);
@@ -1057,38 +1056,32 @@ drawing.setScale = function(element, x, y) {
return transform;
};

drawing.setPointGroupScale = function(selection, x, y) {
var t, scale, re;
var SCALE_RE = /\s*sc.*/;

x = x || 1;
y = y || 1;
drawing.setPointGroupScale = function(selection, xScale, yScale) {
xScale = xScale || 1;
yScale = yScale || 1;

if(x === 1 && y === 1) {
scale = '';
} else {
// The same scale transform for every point:
scale = ' scale(' + x + ',' + y + ')';
}
if(!selection) return;

// A regex to strip any existing scale:
re = /\s*sc.*/;
// The same scale transform for every point:
var scale = (xScale === 1 && yScale === 1) ?
'' :
' scale(' + xScale + ',' + yScale + ')';

selection.each(function() {
// Get the transform:
t = (this.getAttribute('transform') || '').replace(re, '');
var t = (this.getAttribute('transform') || '').replace(SCALE_RE, '');
t += scale;
t = t.trim();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know whether the string manipulations here are a meaningful contribution to the performance... but this could all be simplified a bit (indexOf & .substr instead of a regex & .replace, no .trim) if we could omit the space before the word scale. It seems to work fine in my browsers, though in principle it's not supposed to be allowed: "The individual transform definitions are separated by whitespace and/or a comma."

Alternatively, we could imagine stashing the pre-scaled transform as perhaps a data-initialtransform attribute on each element and avoid all this processing...

Not necessary, just a possibility if you think there would be a benefit.

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 haven't benchmarked this particular block yet, but I know that Drawing.setTextPointsScale is 4-5 times slower than this routine here Drawing.setPointGroupScale.

Spending time on making these routines faster would help improve scroll performance. So yeah, thanks for writing this down. I just opened a new ticket about this -> #2625


// Append the scale transform
this.setAttribute('transform', t);
});

return scale;
};

var TEXT_POINT_LAST_TRANSLATION_RE = /translate\([^)]*\)\s*$/;

drawing.setTextPointsScale = function(selection, xScale, yScale) {
if(!selection) return;

selection.each(function() {
var transforms;
var el = d3.select(this);
52 changes: 25 additions & 27 deletions src/plots/cartesian/dragbox.js
Original file line number Diff line number Diff line change
@@ -82,6 +82,8 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
var isSubplotConstrained;
// do we need to edit x/y ranges?
var editX, editY;
// graph-wide optimization flags
var hasScatterGl, hasOnlyLargeSploms, hasSplom, hasSVG;

function recomputeAxisLists() {
xa0 = plotinfo.xaxis;
@@ -117,6 +119,12 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
isSubplotConstrained = links.isSubplotConstrained;
editX = ew || isSubplotConstrained;
editY = ns || isSubplotConstrained;

var fullLayout = gd._fullLayout;
hasScatterGl = fullLayout._has('scattergl');
hasOnlyLargeSploms = fullLayout._hasOnlyLargeSploms;
hasSplom = hasOnlyLargeSploms || fullLayout._has('splom');
hasSVG = fullLayout._has('svg');
}

recomputeAxisLists();
@@ -688,6 +696,10 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
], gd);
}

// x/y scaleFactor stash,
// minimizes number of per-point DOM updates in updateSubplots below
var xScaleFactorOld, yScaleFactorOld;

// updateSubplots - find all plot viewboxes that should be
// affected by this drag, and update them. look for all plots
// sharing an affected axis (including the one being dragged),
@@ -696,14 +708,6 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
var fullLayout = gd._fullLayout;
var plotinfos = fullLayout._plots;
var subplots = fullLayout._subplots.cartesian;

// TODO can we move these to outer scope?
var hasScatterGl = fullLayout._has('scattergl');
var hasOnlyLargeSploms = fullLayout._hasOnlyLargeSploms;
var hasSplom = hasOnlyLargeSploms || fullLayout._has('splom');
var hasSVG = fullLayout._has('svg');
var hasDraggedPts = fullLayout._has('draggedPts');

var i, sp, xa, ya;

if(hasSplom || hasScatterGl) {
@@ -788,26 +792,20 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
.call(Drawing.setTranslate, plotDx, plotDy)
.call(Drawing.setScale, 1 / xScaleFactor2, 1 / yScaleFactor2);

// TODO move these selectAll calls out of here
// and stash them somewhere nice, see:
// https://github.com/plotly/plotly.js/issues/2548
if(hasDraggedPts) {
var traceGroups = sp.plot
.selectAll('.scatterlayer .trace, .boxlayer .trace, .violinlayer .trace');

// This is specifically directed at marker points in scatter, box and violin traces,
// applying an inverse scale to individual points to counteract
// the scale of the trace as a whole:
traceGroups.selectAll('.point')
.call(Drawing.setPointGroupScale, xScaleFactor2, yScaleFactor2);
traceGroups.selectAll('.textpoint')
.call(Drawing.setTextPointsScale, xScaleFactor2, yScaleFactor2);
traceGroups
.call(Drawing.hideOutsideRangePoints, sp);

sp.plot.selectAll('.barlayer .trace')
.call(Drawing.hideOutsideRangePoints, sp, '.bartext');
// apply an inverse scale to individual points to counteract
// the scale of the trace group.
// apply only when scale changes, as adjusting the scale of
// all the points can be expansive.
if(xScaleFactor2 !== xScaleFactorOld || yScaleFactor2 !== yScaleFactorOld) {
Drawing.setPointGroupScale(sp.zoomScalePts, xScaleFactor2, yScaleFactor2);
Drawing.setTextPointsScale(sp.zoomScaleTxt, xScaleFactor2, yScaleFactor2);
}

Drawing.hideOutsideRangePoints(sp.clipOnAxisFalseTraces, sp);

// update x/y scaleFactor stash
xScaleFactorOld = xScaleFactor2;
yScaleFactorOld = yScaleFactor2;
}
}
}
26 changes: 25 additions & 1 deletion src/plots/cartesian/index.js
Original file line number Diff line number Diff line change
@@ -188,12 +188,14 @@ function plotOne(gd, plotinfo, cdSubplot, transitionOpts, makeOnCompleteCallback
var _module, cdModuleAndOthers, cdModule;

var layerData = [];
var zoomScaleQueryParts = [];

for(var i = 0; i < modules.length; i++) {
_module = modules[i];
var name = _module.name;
var categories = Registry.modules[name].categories;

if(Registry.modules[name].categories.svg) {
if(categories.svg) {
var className = (_module.layerName || name + 'layer');
var plotMethod = _module.plot;

@@ -212,6 +214,10 @@ function plotOne(gd, plotinfo, cdSubplot, transitionOpts, makeOnCompleteCallback
cdModule: cdModule
});
}

if(categories.zoomScale) {
zoomScaleQueryParts.push('.' + className);
}
}
}

@@ -249,6 +255,24 @@ function plotOne(gd, plotinfo, cdSubplot, transitionOpts, makeOnCompleteCallback
cdModule = getModuleCalcData(cdSubplot, _module)[0];
_module.plot(gd, plotinfo, cdModule);
}

// stash "hot" selections for faster interaction on drag and scroll
if(!gd._context.staticPlot) {
if(plotinfo._hasClipOnAxisFalse) {
plotinfo.clipOnAxisFalseTraces = plotinfo.plot
.selectAll('.scatterlayer, .barlayer')
.selectAll('.trace');
}

if(zoomScaleQueryParts.length) {
var traces = plotinfo.plot
.selectAll(zoomScaleQueryParts.join(','))
.selectAll('.trace');

plotinfo.zoomScalePts = traces.selectAll('path.point');
plotinfo.zoomScaleTxt = traces.selectAll('.textpoint');
}
}
}

exports.clean = function(newFullData, newFullLayout, oldFullData, oldFullLayout) {
14 changes: 5 additions & 9 deletions src/plots/cartesian/transition_axes.js
Original file line number Diff line number Diff line change
@@ -239,16 +239,12 @@ module.exports = function transitionAxes(gd, newLayout, transitionOpts, makeOnCo

subplot.plot
.call(Drawing.setTranslate, plotDx, plotDy)
.call(Drawing.setScale, xScaleFactor, yScaleFactor)
.call(Drawing.setScale, xScaleFactor, yScaleFactor);

// This is specifically directed at scatter traces, applying an inverse
// scale to individual points to counteract the scale of the trace
// as a whole:
.selectAll('.points').selectAll('.point')
.call(Drawing.setPointGroupScale, 1 / xScaleFactor, 1 / yScaleFactor);

subplot.plot.selectAll('.points').selectAll('.textpoint')
.call(Drawing.setTextPointsScale, 1 / xScaleFactor, 1 / yScaleFactor);
// apply an inverse scale to individual points to counteract
// the scale of the trace group.
Drawing.setPointGroupScale(subplot.zoomScalePts, 1 / xScaleFactor, 1 / yScaleFactor);
Drawing.setTextPointsScale(subplot.zoomScaleTxt, 1 / xScaleFactor, 1 / yScaleFactor);
}

var onComplete;
2 changes: 1 addition & 1 deletion src/traces/bar/index.js
Original file line number Diff line number Diff line change
@@ -28,7 +28,7 @@ Bar.selectPoints = require('./select');
Bar.moduleType = 'trace';
Bar.name = 'bar';
Bar.basePlotModule = require('../../plots/cartesian');
Bar.categories = ['cartesian', 'svg', 'bar', 'oriented', 'markerColorscale', 'errorBarsOK', 'showLegend', 'draggedPts'];
Bar.categories = ['cartesian', 'svg', 'bar', 'oriented', 'markerColorscale', 'errorBarsOK', 'showLegend', 'zoomScale'];
Bar.meta = {
description: [
'The data visualized by the span of the bars is set in `y`',
2 changes: 1 addition & 1 deletion src/traces/box/index.js
Original file line number Diff line number Diff line change
@@ -25,7 +25,7 @@ Box.selectPoints = require('./select');
Box.moduleType = 'trace';
Box.name = 'box';
Box.basePlotModule = require('../../plots/cartesian');
Box.categories = ['cartesian', 'svg', 'symbols', 'oriented', 'box-violin', 'showLegend', 'draggedPts', 'boxLayout'];
Box.categories = ['cartesian', 'svg', 'symbols', 'oriented', 'box-violin', 'showLegend', 'boxLayout', 'zoomScale'];
Box.meta = {
description: [
'In vertical (horizontal) box plots,',
2 changes: 1 addition & 1 deletion src/traces/scatter/index.js
Original file line number Diff line number Diff line change
@@ -35,7 +35,7 @@ Scatter.animatable = true;
Scatter.moduleType = 'trace';
Scatter.name = 'scatter';
Scatter.basePlotModule = require('../../plots/cartesian');
Scatter.categories = ['cartesian', 'svg', 'symbols', 'markerColorscale', 'errorBarsOK', 'showLegend', 'scatter-like', 'draggedPts'];
Scatter.categories = ['cartesian', 'svg', 'symbols', 'markerColorscale', 'errorBarsOK', 'showLegend', 'scatter-like', 'zoomScale'];
Scatter.meta = {
description: [
'The scatter trace type encompasses line charts, scatter charts, text charts, and bubble charts.',
2 changes: 1 addition & 1 deletion src/traces/scattercarpet/index.js
Original file line number Diff line number Diff line change
@@ -24,7 +24,7 @@ ScatterCarpet.eventData = require('./event_data');
ScatterCarpet.moduleType = 'trace';
ScatterCarpet.name = 'scattercarpet';
ScatterCarpet.basePlotModule = require('../../plots/cartesian');
ScatterCarpet.categories = ['svg', 'carpet', 'symbols', 'markerColorscale', 'showLegend', 'carpetDependent', 'draggedPts'];
ScatterCarpet.categories = ['svg', 'carpet', 'symbols', 'markerColorscale', 'showLegend', 'carpetDependent', 'zoomScale'];
ScatterCarpet.meta = {
hrName: 'scatter_carpet',
description: [
2 changes: 1 addition & 1 deletion src/traces/violin/index.js
Original file line number Diff line number Diff line change
@@ -24,7 +24,7 @@ module.exports = {
moduleType: 'trace',
name: 'violin',
basePlotModule: require('../../plots/cartesian'),
categories: ['cartesian', 'svg', 'symbols', 'oriented', 'box-violin', 'showLegend', 'draggedPts', 'violinLayout'],
categories: ['cartesian', 'svg', 'symbols', 'oriented', 'box-violin', 'showLegend', 'violinLayout', 'zoomScale'],
meta: {
description: [
'In vertical (horizontal) violin plots,',