Skip to content

SVG trace on selection perf #2583

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 12 commits into from
May 1, 2018
350 changes: 202 additions & 148 deletions src/components/drawing/index.js

Large diffs are not rendered by default.

7 changes: 5 additions & 2 deletions src/components/legend/style.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,8 @@ module.exports = function style(s, gd) {

// constrain text, markers, etc so they'll fit on the legend
if(showMarkers || showText || showLines) {
var dEdit = {},
tEdit = {};
var dEdit = {};
var tEdit = {};

if(showMarkers) {
dEdit.mc = boundVal('marker.color', pickFirst);
Expand Down Expand Up @@ -142,6 +142,9 @@ module.exports = function style(s, gd) {

dMod = [Lib.minExtend(d0, dEdit)];
tMod = Lib.minExtend(trace, tEdit);

// always show legend items in base state
tMod.selectedpoints = null;
}

var ptgroup = d3.select(this).select('g.legendpoints');
Expand Down
6 changes: 4 additions & 2 deletions src/plots/cartesian/select.js
Original file line number Diff line number Diff line change
Expand Up @@ -404,18 +404,20 @@ function updateSelectedState(gd, searchTraces, eventData) {
var len = items.length;
var item0 = items[0];
var trace0 = item0.cd[0].trace;
var _module = item0._module;
var fn = _module.styleOnSelect || _module.style;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like styleOnSelect, very nice solution!

But please call it something more descriptive than fn here - styleSelection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure thing 😉

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 0e91c6e


if(Registry.traceIs(trace0, 'regl')) {
// plot regl traces per module
var cds = new Array(len);
for(j = 0; j < len; j++) {
cds[j] = items[j].cd;
}
item0._module.style(gd, cds);
fn(gd, cds);
} else {
// plot svg trace per trace
for(j = 0; j < len; j++) {
item0._module.style(gd, items[j].cd);
fn(gd, items[j].cd);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -834,7 +834,7 @@ plots.linkSubplots = function(newFullData, newFullLayout, oldFullData, oldFullLa
}
// Then look for a subplot with the counteraxis overlaying the anchor
// If that fails just use the first subplot including this axis
if(!mainSubplotID || ids.indexOf(mainSubplotID) === -1) {
if(!mainSubplotID || !newSubplots[mainSubplotID]) {
mainSubplotID = '';
for(j = 0; j < ids.length; j++) {
id = ids[j];
Expand Down
3 changes: 2 additions & 1 deletion src/traces/bar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ Bar.setPositions = require('./set_positions');
Bar.colorbar = require('../scatter/colorbar');
Bar.arraysToCalcdata = require('./arrays_to_calcdata');
Bar.plot = require('./plot');
Bar.style = require('./style');
Bar.style = require('./style').style;
Bar.styleOnSelect = require('./style').styleOnSelect;
Bar.hoverPoints = require('./hover');
Bar.selectPoints = require('./select');

Expand Down
62 changes: 40 additions & 22 deletions src/traces/bar/style.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ var d3 = require('d3');
var Drawing = require('../../components/drawing');
var Registry = require('../../registry');

module.exports = function style(gd, cd) {
function style(gd, cd) {
var s = cd ? cd[0].node3 : d3.select(gd).selectAll('g.trace.bars');
var barcount = s.size();
var fullLayout = gd._fullLayout;
Expand All @@ -35,34 +35,52 @@ module.exports = function style(gd, cd) {

s.selectAll('g.points').each(function(d) {
var sel = d3.select(this);
var pts = sel.selectAll('path');
var txs = sel.selectAll('text');
var trace = d[0].trace;
stylePoints(sel, trace, gd);
});

Registry.getComponentMethod('errorbars', 'style')(s);
}

Drawing.pointStyle(pts, trace, gd);
Drawing.selectedPointStyle(pts, trace);
function stylePoints(sel, trace, gd) {
var pts = sel.selectAll('path');
var txs = sel.selectAll('text');

txs.each(function(d) {
var tx = d3.select(this);
var textFont;
Drawing.pointStyle(pts, trace, gd);

if(tx.classed('bartext-inside')) {
textFont = trace.insidetextfont;
} else if(tx.classed('bartext-outside')) {
textFont = trace.outsidetextfont;
}
if(!textFont) textFont = trace.textfont;
txs.each(function(d) {
var tx = d3.select(this);
var textFont;

function cast(k) {
var cont = textFont[k];
return Array.isArray(cont) ? cont[d.i] : cont;
}
if(tx.classed('bartext-inside')) {
textFont = trace.insidetextfont;
} else if(tx.classed('bartext-outside')) {
textFont = trace.outsidetextfont;
}
if(!textFont) textFont = trace.textfont;

Drawing.font(tx, cast('family'), cast('size'), cast('color'));
});
function cast(k) {
var cont = textFont[k];
return Array.isArray(cont) ? cont[d.i] : cont;
}

Drawing.selectedTextStyle(txs, trace);
Drawing.font(tx, cast('family'), cast('size'), cast('color'));
});
}

Registry.getComponentMethod('errorbars', 'style')(s);
function styleOnSelect(gd, cd) {
var s = cd[0].node3;
var trace = cd[0].trace;

if(trace.selectedpoints) {
Drawing.selectedPointStyle(s.selectAll('path'), trace);
Drawing.selectedTextStyle(s.selectAll('text'), trace);
} else {
stylePoints(s, trace, gd);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this else? I don't see a corresponding case in the previous code...

BTW, I'm hesitant to suggest this after the .select().select() data-mangling mess, but does it work to do Drawing.selectedPointStyle(s.selectAll('path,text'), trace, gd)? Is that any faster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah oops, this should be Drawing.selectedTextStyle(s.selectAll('text'), trace, gd). I'll add a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we need this else? I

To get back to the original state after double-click. Drawing.selectedPointStyle and Drawing.selectedTextStyle only handle selected / unselected styling off a "base" state given by stylePoints.

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'll add a test.

added in 01df2d2

}
}

module.exports = {
style: style,
styleOnSelect: styleOnSelect
};
3 changes: 2 additions & 1 deletion src/traces/box/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ Box.supplyLayoutDefaults = require('./layout_defaults').supplyLayoutDefaults;
Box.calc = require('./calc');
Box.setPositions = require('./set_positions').setPositions;
Box.plot = require('./plot').plot;
Box.style = require('./style');
Box.style = require('./style').style;
Box.styleOnSelect = require('./style').styleOnSelect;
Box.hoverPoints = require('./hover').hoverPoints;
Box.selectPoints = require('./select');

Expand Down
20 changes: 18 additions & 2 deletions src/traces/box/style.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ var d3 = require('d3');
var Color = require('../../components/color');
var Drawing = require('../../components/drawing');

module.exports = function style(gd, cd) {
function style(gd, cd) {
var s = cd ? cd[0].node3 : d3.select(gd).selectAll('g.trace.boxes');

s.style('opacity', function(d) { return d[0].trace.opacity; });
Expand Down Expand Up @@ -50,7 +50,23 @@ module.exports = function style(gd, cd) {

var pts = el.selectAll('path.point');
Drawing.pointStyle(pts, trace, gd);
Drawing.selectedPointStyle(pts, trace);
}
});
}

function styleOnSelect(gd, cd) {
var s = cd[0].node3;
var trace = cd[0].trace;
var pts = s.selectAll('path.point');

if(trace.selectedpoints) {
Drawing.selectedPointStyle(pts, trace);
} else {
Drawing.pointStyle(pts, trace, gd);
}
}

module.exports = {
style: style,
styleOnSelect: styleOnSelect
};
2 changes: 1 addition & 1 deletion src/traces/candlestick/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ module.exports = {
supplyDefaults: require('./defaults'),
calc: require('./calc'),
plot: require('../box/plot').plot,
style: require('../box/style'),
style: require('../box/style').style,
hoverPoints: require('../ohlc/hover'),
selectPoints: require('../ohlc/select')
};
3 changes: 2 additions & 1 deletion src/traces/choropleth/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ Choropleth.supplyDefaults = require('./defaults');
Choropleth.colorbar = require('../heatmap/colorbar');
Choropleth.calc = require('./calc');
Choropleth.plot = require('./plot');
Choropleth.style = require('./style');
Choropleth.style = require('./style').style;
Choropleth.styleOnSelect = require('./style').styleOnSelect;
Choropleth.hoverPoints = require('./hover');
Choropleth.eventData = require('./event_data');
Choropleth.selectPoints = require('./select');
Expand Down
2 changes: 1 addition & 1 deletion src/traces/choropleth/plot.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ var polygon = require('../../lib/polygon');

var getTopojsonFeatures = require('../../lib/topojson_utils').getTopojsonFeatures;
var locationToFeature = require('../../lib/geo_location_utils').locationToFeature;
var style = require('./style');
var style = require('./style').style;

module.exports = function plot(gd, geo, calcData) {
for(var i = 0; i < calcData.length; i++) {
Expand Down
22 changes: 19 additions & 3 deletions src/traces/choropleth/style.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ var Color = require('../../components/color');
var Drawing = require('../../components/drawing');
var Colorscale = require('../../components/colorscale');

module.exports = function style(gd, calcTrace) {
function style(gd, calcTrace) {
if(calcTrace) styleTrace(gd, calcTrace);
};
}

function styleTrace(gd, calcTrace) {
var trace = calcTrace[0].trace;
Expand All @@ -40,5 +40,21 @@ function styleTrace(gd, calcTrace) {
.style('opacity', marker.opacity);
});

Drawing.selectedPointStyle(locs, trace);
Drawing.selectedPointStyle(locs, trace, gd);
}

function styleOnSelect(gd, calcTrace) {
var s = calcTrace[0].node3;
var trace = calcTrace[0].trace;

if(trace.selectedpoints) {
Drawing.selectedPointStyle(s.selectAll('.choroplethlocation'), trace, gd);
} else {
styleTrace(gd, calcTrace);
}
}

module.exports = {
style: style,
styleOnSelect: styleOnSelect
};
3 changes: 2 additions & 1 deletion src/traces/histogram/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ Histogram.supplyLayoutDefaults = require('../bar/layout_defaults');
Histogram.calc = require('./calc');
Histogram.setPositions = require('../bar/set_positions');
Histogram.plot = require('../bar/plot');
Histogram.style = require('../bar/style');
Histogram.style = require('../bar/style').style;
Histogram.styleOnSelect = require('../bar/style').styleOnSelect;
Histogram.colorbar = require('../scatter/colorbar');
Histogram.hoverPoints = require('./hover');
Histogram.selectPoints = require('../bar/select');
Expand Down
1 change: 1 addition & 0 deletions src/traces/scatter/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ Scatter.arraysToCalcdata = require('./arrays_to_calcdata');
Scatter.plot = require('./plot');
Scatter.colorbar = require('./colorbar');
Scatter.style = require('./style').style;
Scatter.styleOnSelect = require('./style').styleOnSelect;
Scatter.hoverPoints = require('./hover');
Scatter.selectPoints = require('./select');
Scatter.animatable = true;
Expand Down
24 changes: 13 additions & 11 deletions src/traces/scatter/plot.js
Original file line number Diff line number Diff line change
Expand Up @@ -409,14 +409,14 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
function makePoints(d) {
var join, selection, hasNode;

var trace = d[0].trace,
s = d3.select(this),
showMarkers = subTypes.hasMarkers(trace),
showText = subTypes.hasText(trace);
var trace = d[0].trace;
var s = d3.select(this);
var showMarkers = subTypes.hasMarkers(trace);
var showText = subTypes.hasText(trace);

var keyFunc = getKeyFunc(trace),
markerFilter = hideFilter,
textFilter = hideFilter;
var keyFunc = getKeyFunc(trace);
var markerFilter = hideFilter;
var textFilter = hideFilter;

if(showMarkers) {
markerFilter = (trace.marker.maxdisplayed || trace._needsCull) ? visFilter : Lib.identity;
Expand Down Expand Up @@ -444,18 +444,20 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
.style('opacity', 1);
}

var markerScale = showMarkers && Drawing.tryColorscale(trace.marker, '');
var lineScale = showMarkers && Drawing.tryColorscale(trace.marker, 'line');

join.order();

var styleFns;
if(showMarkers) {
styleFns = Drawing.makePointStyleFns(trace);
}

join.each(function(d) {
var el = d3.select(this);
var sel = transition(el);
hasNode = Drawing.translatePoint(d, sel, xa, ya);

if(hasNode) {
Drawing.singlePointStyle(d, sel, trace, markerScale, lineScale, gd);
Drawing.singlePointStyle(d, sel, trace, styleFns, gd);

if(plotinfo.layerClipId) {
Drawing.hideOutsideRangePoint(d, sel, xa, ya, trace.xcalendar, trace.ycalendar);
Expand Down
22 changes: 15 additions & 7 deletions src/traces/scatter/style.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,24 @@ function style(gd, cd) {
}

function stylePoints(sel, trace, gd) {
var pts = sel.selectAll('path.point');
var txs = sel.selectAll('text');
Drawing.pointStyle(sel.selectAll('path.point'), trace, gd);
Drawing.textPointStyle(sel.selectAll('text'), trace, gd);
}

function styleOnSelect(gd, cd) {
var s = cd[0].node3;
var trace = cd[0].trace;

Drawing.pointStyle(pts, trace, gd);
Drawing.textPointStyle(txs, trace, gd);
Drawing.selectedPointStyle(pts, trace);
Drawing.selectedTextStyle(txs, trace);
if(trace.selectedpoints) {
Drawing.selectedPointStyle(s.selectAll('path.point'), trace);
Drawing.selectedTextStyle(s.selectAll('text'), trace);
} else {
stylePoints(s, trace, gd);
}
}

module.exports = {
style: style,
stylePoints: stylePoints
stylePoints: stylePoints,
styleOnSelect: styleOnSelect
};
1 change: 1 addition & 0 deletions src/traces/scattercarpet/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ ScatterCarpet.colorbar = require('../scatter/colorbar');
ScatterCarpet.calc = require('./calc');
ScatterCarpet.plot = require('./plot');
ScatterCarpet.style = require('../scatter/style').style;
ScatterCarpet.styleOnSelect = require('../scatter/style').styleOnSelect;
ScatterCarpet.hoverPoints = require('./hover');
ScatterCarpet.selectPoints = require('../scatter/select');
ScatterCarpet.eventData = require('./event_data');
Expand Down
1 change: 1 addition & 0 deletions src/traces/scattergeo/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ ScatterGeo.colorbar = require('../scatter/colorbar');
ScatterGeo.calc = require('./calc');
ScatterGeo.plot = require('./plot');
ScatterGeo.style = require('./style');
ScatterGeo.styleOnSelect = require('../scatter/style').styleOnSelect;
ScatterGeo.hoverPoints = require('./hover');
ScatterGeo.eventData = require('./event_data');
ScatterGeo.selectPoints = require('./select');
Expand Down
Loading