Skip to content

fix layer ordering of scatter subtypes #2978

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 4 commits into from
Sep 11, 2018
Merged

fix layer ordering of scatter subtypes #2978

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aeef7f7
fix layer ordering of scatter subtypes
alexcjohnson Sep 6, 2018
7c54fc2
put back text styling in scattergeo
alexcjohnson Sep 7, 2018
509ed7e
Merge branch 'master' into scatter-layer-order
alexcjohnson Sep 7, 2018
6988818
better algorithm for covering all state transitions
alexcjohnson Sep 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
4 changes: 3 additions & 1 deletion src/components/legend/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,9 @@ module.exports = function style(s, gd) {

var pts = ptgroup.selectAll('path.scatterpts')
.data(showMarkers ? dMod : []);
pts.enter().append('path').classed('scatterpts', true)
// make sure marker is on the bottom, in case it enters after text
pts.enter().insert('path', ':first-child')
.classed('scatterpts', true)
.attr('transform', 'translate(20,0)');
pts.exit().remove();
pts.call(Drawing.pointStyle, tMod, gd);
Expand Down
125 changes: 57 additions & 68 deletions src/traces/scatter/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ var d3 = require('d3');

var Registry = require('../../registry');
var Lib = require('../../lib');
var ensureSingle = Lib.ensureSingle;
var identity = Lib.identity;
var Drawing = require('../../components/drawing');

var subTypes = require('./subtypes');
Expand Down Expand Up @@ -43,7 +45,7 @@ module.exports = function plot(gd, plotinfo, cdscatter, scatterLayer, transition
// the z-order of fill layers is correct.
linkTraces(gd, plotinfo, cdscatter);

createFills(gd, scatterLayer, plotinfo);
createFills(gd, join, plotinfo);

// Sort the traces, once created, so that the ordering is preserved even when traces
// are shown and hidden. This is needed since we're not just wiping everything out
Expand All @@ -52,10 +54,10 @@ module.exports = function plot(gd, plotinfo, cdscatter, scatterLayer, transition
uids[cdscatter[i][0].trace.uid] = i;
}

scatterLayer.selectAll('g.trace').sort(function(a, b) {
join.sort(function(a, b) {
var idx1 = uids[a[0].trace.uid];
var idx2 = uids[b[0].trace.uid];
return idx1 > idx2 ? 1 : -1;
return idx1 - idx2;
});

if(hasTransition) {
Expand Down Expand Up @@ -97,51 +99,45 @@ module.exports = function plot(gd, plotinfo, cdscatter, scatterLayer, transition
scatterLayer.selectAll('path:not([d])').remove();
};

function createFills(gd, scatterLayer, plotinfo) {
var trace;
function createFills(gd, traceJoin, plotinfo) {
traceJoin.each(function(d) {
var fills = ensureSingle(d3.select(this), 'g', 'fills');
Drawing.setClipUrl(fills, plotinfo.layerClipId);

scatterLayer.selectAll('g.trace').each(function(d) {
var tr = d3.select(this);

// Loop only over the traces being redrawn:
trace = d[0].trace;
var trace = d[0].trace;

// make the fill-to-next path now for the NEXT trace, so it shows
// behind both lines.
var fillData = [];
if(trace.fill && (trace.fill.substr(0, 6) === 'tozero' || trace.fill === 'toself' ||
(trace.fill.substr(0, 2) === 'to' && !trace._prevtrace))
) {
fillData = ['_ownFill'];
}
if(trace._nexttrace) {
trace._nextFill = tr.select('.js-fill.js-tonext');
if(!trace._nextFill.size()) {
// make the fill-to-next path now for the NEXT trace, so it shows
// behind both lines.
fillData.push('_nextFill');
}

// If there is an existing tozero fill, we must insert this *after* that fill:
var loc = ':first-child';
if(tr.select('.js-fill.js-tozero').size()) {
loc += ' + *';
}
var fillJoin = fills.selectAll('g')
.data(fillData, identity);

trace._nextFill = tr.insert('path', loc).attr('class', 'js-fill js-tonext');
}
} else {
tr.selectAll('.js-fill.js-tonext').remove();
trace._nextFill = null;
}
fillJoin.enter().append('g');

if(trace.fill && (trace.fill.substr(0, 6) === 'tozero' || trace.fill === 'toself' ||
(trace.fill.substr(0, 2) === 'to' && !trace._prevtrace))) {
trace._ownFill = tr.select('.js-fill.js-tozero');
if(!trace._ownFill.size()) {
trace._ownFill = tr.insert('path', ':first-child').attr('class', 'js-fill js-tozero');
}
} else {
tr.selectAll('.js-fill.js-tozero').remove();
trace._ownFill = null;
}
fillJoin.exit()
.each(function(d) { trace[d] = null; })
.remove();

tr.selectAll('.js-fill').call(Drawing.setClipUrl, plotinfo.layerClipId);
fillJoin.order().each(function(d) {
// make a path element inside the fill group, just so
// we can give it its own data later on and the group can
// keep its simple '_*Fill' data
trace[d] = ensureSingle(d3.select(this), 'path', 'js-fill');
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

A little hard to tell from the diff, but wow createFills is so much cleaner now. 🥇

}

function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transitionOpts) {
var join, i;
var i;

// Since this has been reorganized and we're executing this on individual traces,
// we need to pass it the full list of cdscatter as well as this trace's index (idx)
Expand All @@ -157,12 +153,17 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
var xa = plotinfo.xaxis,
ya = plotinfo.yaxis;

var trace = cdscatter[0].trace,
line = trace.line,
tr = d3.select(element);
var trace = cdscatter[0].trace;
var line = trace.line;
var tr = d3.select(element);

var errorBarGroup = ensureSingle(tr, 'g', 'errorbars');
var lines = ensureSingle(tr, 'g', 'lines');
var points = ensureSingle(tr, 'g', 'points');
var text = ensureSingle(tr, 'g', 'text');
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. So now all scatter traces (no-matter their mode, fill or error_x/error_y settings) get four sub <g>:

image

What if instead we made a "mode" groups data-bind:

var modeData = []
if(trace.error_y.visible || trace.error_x.visible) modeData.push('errorbars');
if(trace.mode.indexOf('lines') !== -1) modeData.push('lines');
if(trace.mode.indexOf('markers') !== -1) modeData.push('points');
if(trace.mode.indexOf('text') !== -1) modeData.push('text');

 var modeGroups = tr.selectAll('g.mode'))
        .data(modeData, identity);
 modeGroups.exit().remove();
 modeGroups.enter().append('g')
        .classed('mode', true)
        .attr('class', identity);
 modeGroups.order();

That way those empty <g> would be gone 🛩️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(discussed offline but for future reference)

I tried to do this... and it was going well for a while, but I ran into issues with the fact that various disjoint pathways (eg styling, errorbars) expect the trace data to be attached to these groups. And if you use selection.data to control creation of these groups, you can't use it to also pass data down to its children. We could nest another single group inside, just for the purpose of passing down the data, but that seems like it's creating exactly the same kind of problem we're trying to avoid. It's almost like we want a parallel data binding, like extending d3 to allow selecting & operating based on a different attribute from __data__... or perhaps we make a separate Lib function specifically for this purpose: ensure the ordering and conditional existence of a bunch of groups, but bind the same data to each group.

Anyway, for now we agreed to leave it as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, for now we agreed to leave it as is.

👌


// error bars are at the bottom
Registry.getComponentMethod('errorbars', 'plot')(tr, plotinfo, transitionOpts);
Registry.getComponentMethod('errorbars', 'plot')(errorBarGroup, plotinfo, transitionOpts);

if(trace.visible !== true) return;

Expand Down Expand Up @@ -303,7 +304,7 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
};
}

var lineJoin = tr.selectAll('.js-line').data(segments);
var lineJoin = lines.selectAll('.js-line').data(segments);

transition(lineJoin.exit())
.style('opacity', 0)
Expand All @@ -325,6 +326,7 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition

if(segments.length) {
if(ownFillEl3) {
ownFillEl3.datum(cdscatter);
if(pt0 && pt1) {
if(ownFillDir) {
if(ownFillDir === 'y') {
Expand Down Expand Up @@ -404,11 +406,10 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
return false;
}

function makePoints(d) {
function makePoints(points, text, cdscatter) {
var join, selection, hasNode;

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

Expand All @@ -417,16 +418,16 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
var textFilter = hideFilter;

if(showMarkers) {
markerFilter = (trace.marker.maxdisplayed || trace._needsCull) ? visFilter : Lib.identity;
markerFilter = (trace.marker.maxdisplayed || trace._needsCull) ? visFilter : identity;
}

if(showText) {
textFilter = (trace.marker.maxdisplayed || trace._needsCull) ? visFilter : Lib.identity;
textFilter = (trace.marker.maxdisplayed || trace._needsCull) ? visFilter : identity;
}

// marker points

selection = s.selectAll('path.point');
selection = points.selectAll('path.point');

join = selection.data(markerFilter, keyFunc);

Expand Down Expand Up @@ -478,7 +479,7 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
}

// text points
selection = s.selectAll('g');
selection = text.selectAll('g');
join = selection.data(textFilter, keyFunc);

// each text needs to go in its own 'g' in case
Expand Down Expand Up @@ -517,28 +518,16 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
join.exit().remove();
}

// NB: selectAll is evaluated on instantiation:
var pointSelection = tr.selectAll('.points');

// Join with new data
join = pointSelection.data([cdscatter]);

// Transition existing, but don't defer this to an async .transition since
// there's no timing involved:
pointSelection.each(makePoints);

join.enter().append('g')
.classed('points', true)
.each(makePoints);

join.exit().remove();
points.datum(cdscatter);
text.datum(cdscatter);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice use of .datum

makePoints(points, text, cdscatter);

// lastly, clip points groups of `cliponaxis !== false` traces
// on `plotinfo._hasClipOnAxisFalse === true` subplots
join.each(function(d) {
var hasClipOnAxisFalse = d[0].trace.cliponaxis === false;
Drawing.setClipUrl(d3.select(this), hasClipOnAxisFalse ? null : plotinfo.layerClipId);
});
var hasClipOnAxisFalse = trace.cliponaxis === false;
var clipUrl = hasClipOnAxisFalse ? null : plotinfo.layerClipId;
Drawing.setClipUrl(points, clipUrl);
Drawing.setClipUrl(text, clipUrl);
}

function selectMarkers(gd, idx, plotinfo, cdscatter, cdscatterAll) {
Expand Down
11 changes: 11 additions & 0 deletions src/traces/scatter/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ function style(gd, cd) {
stylePoints(sel, trace, gd);
});

s.selectAll('g.text').each(function(d) {
var sel = d3.select(this);
var trace = d.trace || d[0].trace;
styleText(sel, trace, gd);
});

s.selectAll('g.trace path.js-line')
.call(Drawing.lineGroupStyle);

Expand All @@ -37,6 +43,9 @@ function style(gd, cd) {

function stylePoints(sel, trace, gd) {
Drawing.pointStyle(sel.selectAll('path.point'), trace, gd);
}

function styleText(sel, trace, gd) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously text was just shoved into the same group as points, which had the pleasant side-effect of letting them be styled together. Now that they're in separate groups they need independent outer selections. AFAICT none of the other users of stylePoints actually wanted to be styling text anyway (as opposed to the users of style), so this may be a slight 🐎 improvement... but @etpinard please take a look.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh hmm, I guess the image failure is telling me geo needs this...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

alright, put back into scatterGeo -> 7c54fc2

Note that scattergeo still removes everything on update so these ordering bugs should not matter there, but we should be able to use essentially the same structure there as here to switch to a more standard update pattern without introducing similar bugs.

Copy link
Contributor

Choose a reason for hiding this comment

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

alright, put back into scatterGeo -> 7c54fc2

Glad to see our tests caught that.

Now that only scattergeo uses it, maybe we could move styleText to

https://github.com/plotly/plotly.js/blob/master/src/traces/scattergeo/style.js ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah never mind, styleText is used in styleOnSelect.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT none of the other users of stylePoints actually wanted to be styling text anyway (as opposed to the users of style), so this may be a slight improvement... but @etpinard please take a look.

👍 for having text and points in different groups!

Drawing.textPointStyle(sel.selectAll('text'), trace, gd);
}

Expand All @@ -49,11 +58,13 @@ function styleOnSelect(gd, cd) {
Drawing.selectedTextStyle(s.selectAll('text'), trace);
} else {
stylePoints(s, trace, gd);
styleText(s, trace, gd);
}
}

module.exports = {
style: style,
stylePoints: stylePoints,
styleText: styleText,
styleOnSelect: styleOnSelect
};
5 changes: 4 additions & 1 deletion src/traces/scattergeo/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ var d3 = require('d3');
var Drawing = require('../../components/drawing');
var Color = require('../../components/color');

var stylePoints = require('../scatter/style').stylePoints;
var scatterStyle = require('../scatter/style');
var stylePoints = scatterStyle.stylePoints;
var styleText = scatterStyle.styleText;

module.exports = function style(gd, calcTrace) {
if(calcTrace) styleTrace(gd, calcTrace);
Expand All @@ -25,6 +27,7 @@ function styleTrace(gd, calcTrace) {
s.style('opacity', calcTrace[0].trace.opacity);

stylePoints(s, trace, gd);
styleText(s, trace, gd);

// this part is incompatible with Drawing.lineGroupStyle
s.selectAll('path.js-line')
Expand Down
22 changes: 22 additions & 0 deletions test/jasmine/assets/custom_assertions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,22 @@ exports.assertNodeOrder = function(selectorBehind, selectorInFront, msg) {
}
};

/**
* Ordering test for any number of nodes - calls assertNodeOrder n-1 times.
* Note that we only take the first matching node for each selector, and it's
* not necessary that the nodes be siblings or at the same level of nesting.
*
* @param {Array[string]} selectorArray: css selectors in the order they should
* appear in the document, from back to front.
* @param {string} msg: context for debugging
*/
exports.assertMultiNodeOrder = function(selectorArray, msg) {
for(var i = 0; i < selectorArray.length - 1; i++) {
var msgi = (msg ? msg + ' - ' : '') + 'entries ' + i + ' and ' + (i + 1);
exports.assertNodeOrder(selectorArray[i], selectorArray[i + 1], msgi);
}
};

function getParents(node) {
var parent = node.parentNode;
if(parent) return getParents(parent).concat(node);
Expand All @@ -310,3 +326,9 @@ function collectionToArray(collection) {
for(var i = 0; i < len; i++) a[i] = collection[i];
return a;
}

exports.assertD3Data = function(selection, expectedData) {
var data = [];
selection.each(function(d) { data.push(d); });
expect(data).toEqual(expectedData);
};
Loading