fix layer ordering of scatter subtypes #2978
Conversation
test/jasmine/tests/scatter_test.js
Outdated
| indices.push(curIndex); | ||
| curIndex = (curIndex + i) % cases.length; | ||
| } | ||
| } |
There was a problem hiding this comment.
Notice here we have 15 cases, so we're covering 15*14 = 210 transitions between cases. Takes about 4 seconds locally but totally worth it, as it uncovered a few other layering problems besides the lines/markers one that prompted this effort.
There was a problem hiding this comment.
Oh hmm, actually this would work perfectly if cases.length were prime, but as it is I duplicated some and missed some others. I wonder if there is a generic efficient way to do this with a composite number of cases? Anyway most of the transitions are covered so I think this is fine as is but I am curious now...
There was a problem hiding this comment.
Found an algorithm that works efficiently for any cases.length -> 6988818
| @@ -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) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
oh hmm, I guess the image failure is telling me geo needs this...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
Ah never mind, styleText is used in styleOnSelect.
There was a problem hiding this comment.
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!
| var errorBarGroup = ensureSingle(tr, 'g', 'errorbars'); | ||
| var lines = ensureSingle(tr, 'g', 'lines'); | ||
| var points = ensureSingle(tr, 'g', 'points'); | ||
| var text = ensureSingle(tr, 'g', 'text'); |
There was a problem hiding this comment.
Hmm. So now all scatter traces (no-matter their mode, fill or error_x/error_y settings) get four sub <g>:
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 🛩️
There was a problem hiding this comment.
(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.
There was a problem hiding this comment.
Anyway, for now we agreed to leave it as is.
👌
|
TODO:
|
| // 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'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
A little hard to tell from the diff, but wow createFills is so much cleaner now. 🥇
|
|
||
| join.exit().remove(); | ||
| points.datum(cdscatter); | ||
| text.datum(cdscatter); |
|
I made sure all the animations on https://plot.ly/javascript/animations/ still work! Great work @alexcjohnson 🎉 |
|
Nicely done 💃 |
Fixes #2972 - and some other related ordering bugs on mode changes. Basically just by using more standard d3 idioms 🙄
cc @etpinard