Persistent point selection #2135
Conversation
|
scatter and some other types have a |
|
@alexcjohnson — @etpinard and I talked a bit about this and decided the best option for a start is to style them directly. I was concerned we'd lose out on a lot of properties, but if we start with just opacity and color, then direct styling (as opposed to a full |
|
Here's what I come up with: https://github.com/plotly/plotly.js/compare/persistent-point-selection-etienne
|
|
@etpinard Feel free to file a separate PR or just seize control of this PR. |
- namely, hoveredpoints, marker.size and marker.lin selected/unselected attributes.
- and coerce if module's `selectPoints` method exists - don't dive into `selectedpoints` array (leave that for calc, later).
- make distinction between selectedpoints: [] and selectedpoint: undefined stats, where the former mean all pts are unselected and the latter means no selection at all.
- use trace module style method to update DOM nodes (or call relevent gl update methods for gl-based trace types) - add *new* call signature for style methods to style per-trace inside of per-graph (as not all traces in one graph are selected in general).
- coerce selected/unselected marker.opacity, marker.color and textfont.color - add Drawing.selectedPointStyle and Drawing.selectedTextStyle to update DOM nodes when `selectedpoints` exists - replace apply_selection_to_calcdata by calc_selection - add support for per-trace Scatter.style call signature - rm DOM styling in Scatter.selectPoints
- ... by simply propagation 'scatter' logic.
- coerce only 'marker.opacity' and 'marker.color', (textfont.color isn't a thing yet for these trace types).
- in which we needed to expose a 'style' method. - note that geo trace cannot rely on the the style method loop in Plots.style because it needs to call style in the fetch-topojson callback. - so ScatterGeo.style only work per-trace!
- in which a new style per-trace-only style() method is added (similar to 'scattergeo') AND a new attribute: 'marker.opacity' to make selection behavior be attribute-describable.
- by doing so, we can use Drawing.pointStyle straight-up!
- note that Drawing.font needs to be called in Bar.plot to find out if the text label fit inside the bars.
- support only 'marker.opacity' until we bump mapbox-gl
- wait until regl rewrite.
|
Ok, tagging this as
In brief, much of the work here was put into replacing the DOM node styling calls in the trace module For most trace types, I've use In #1848 (comment), there was also talks about adding a cc @monfera @dfcreative @alexcjohnson @bpostlethwaite @cpsievert |
src/components/drawing/index.js
Outdated
|
|
||
| // which is slightly different than: | ||
| // ((d.mo + 1 || opacity + 1) - 1) * (d.dim ? DESELECTDIM : 1); | ||
| // in https://github.com/plotly/plotly.js/blob/master/src/traces/scatter/select.js |
There was a problem hiding this comment.
This means that if marker opacity is an array, you won't be able to keep that during selection. I feel like we can do better than that:
- Only give
[un]selected.marker.opacitya default value if you don't set any other[un]selectedattributes - so if you want to keep opacity intact but just change marker color during selection, you can do that. - If only one of the
[un]selectedopacities is set, the other state should revert to thearrayOklogic(d.mo + 1 || marker.opacity + 1) - 1 - The same is true for colors: if only one is used and the base attribute is an array, the other state should revert to the
arrayOklogic - perhaps we want to stash this color indthough, as this logic is a bit more involved? Anyway, that would allow selections like in parcoords (deselected is gray, selected keeps its color) or like our S&P notifiers (deselected keeps its color, selected is bright red) - And for both opacity and color, if neither is set you don't even need to do that loop.
- Another minor 🐎 - this will probably naturally happen with the above, but you can move
(selectedAttrs.marker || {}).opacitycalculation out of the.eachloop - alsosmcandusmc(Semper Fi!) below. That said I'd love it if some day these attributes all becomearrayOkin which case they'd have to move at least partially back into the loop... But that's not for this PR, single-value is great for now. - My suggestions may make a mess with gradients... perhaps for now if you have a gradient and only one selection color we just revert to uniform
marker.colorfor the other state?
There was a problem hiding this comment.
Only give [un]selected.marker.opacity a default value if you don't set any other [un]selected attributes - so if you want to keep opacity intact but just change marker color during selection, you can do that.
But then [un]selected.marker.opacity won't appear in _fullData, and @monfera 😏 won't be able to populate his style panels. Personally, I think keeping the same default selection behavior is more important, so thanks @alexcjohnson for pointing this out!
My suggestions may make a mess with gradients... perhaps for now if you have a gradient and only one selection color we just revert to uniform marker.color for the other state?
Good point here. I haven't tested any of the mocks with marker.gradient turned on.
- to centralize [un]selected.marker.opacity default logic - use it across in all trace types that support selections
- to match current array marker.opacity behavior. - add mucho tests 🔒
- fixup ternary hover event data test accordingly.
…t-transforms Persistent point selection with transforms compatibility
|
Ok, I think I'm now happy with this PR functionality-wise 🚀 as well as test-coverage-wise 🔒 There's a lot of things going on here, so let me recap what happened since #2135 (comment):
|
|
I looked into this grand PR two weeks ago and a lot of improvements were put in place since, and it's the single biggest step toward fully representing plot state, I'm voting for 💃 Caution: we shouldn't immediately use it in
So, putting it in |
|
@etpinard Let's keep this off streambed for a few weeks if possible |
- now works for unsorted sample arrays.
| ptNumber2cdIndex[pts[j].i] = j; | ||
| } | ||
|
|
||
| Lib.tagSelected(pts, trace, ptNumber2cdIndex); |
There was a problem hiding this comment.
Quick fixup. I noticed that selectedpoints -> calcdata was off for box and violins while making
var gen = v => Math.round(Math.random(v) * 10)
var n = 100
var x = new Array(n).fill(0).map(gen)
var y = new Array(n).fill(0).map(gen)
var c = '#984ea3'
var usmo = 0
Plotly.newPlot(gd, [{
mode: 'markers',
x: x,
y: y,
marker: {color: c},
unselected: { marker: {opacity: usmo} }
}, {
type: 'violin',
name: 'x',
x: x,
points: 'all',
pointpos: 1.5,
box: {visible: true},
yaxis: 'y2',
marker: {color: c},
unselected: { marker: {opacity: usmo} }
}, {
type: 'violin',
name: 'y',
y: y,
points: 'all',
pointpos: 1.5,
box: {visible: true},
xaxis: 'x2',
marker: {color: c},
unselected: { marker: {opacity: usmo} }
}], {
xaxis: {
domain: [0, 0.48]
},
yaxis: {
domain: [0, 0.48]
},
xaxis2: {
domain: [0.52, 1]
},
yaxis2: {
domain: [0.52, 1]
},
showlegend: false,
dragmode: 'lasso',
hovermode: 'closest',
width: 500,
height: 500
})
gd.on('plotly_selected', d => {
if(d && d.points && d.points.length) {
var sel = d.points.map(p => p.pointIndex)
Plotly.restyle(gd, 'selectedpoints', [sel.slice(), sel.slice(), sel.slice()])
} else {
Plotly.restyle(gd, 'selectedpoints', null)
}
})
|
One question that came up in the workshop is whether this will work with animations. E.g., will points selected with the persistent selections API stay selected through frame transitions? |
yes |
|
Tons of great stuff here, nice job! 💃 |
This PR addresses #1848. There is useful code, but it's not yet fully functional. In particular the things that need doing:
plotto make the draw updates happen. I think the best thing, at least for scatter, is to piggyback on the transition capability that bypasses a full replot. Many properties can be handled via that pathway for scatter. For non-scatter traces, I'm not 100% certain what the right solution is yet. We might needmodule.drawSelectto mutate the DOM colors/opacities/etc directly instead of calling a full replot and in order to keep, say, bar drawing logic inside the bar trace (as opposed to worrying about drawing bars within the cartesian plot select code)selectedidswork the same asselectepoints. Right now that's partially omitted./cc @etpinard