Persistent point selection with transforms compatibility #2163
Conversation
- 🔪 its select method and reuse Scatter.select instead which is much DRYer.
- 🔪 is select method, reuse Scatter.select instead - using same event-data pipeline for hover and select revealed an inconsistency: hover data includes the raw input coords whereas selection data includes the scaled coords (as in calcdata).
- tagSelected make heavy use of the transforms _indexToPoints map object to determine how input index (called pointIndex) are mapped to calcdata index (called pointNumber). - use pointIndex (not pointNumber) while filling up selectedpoints array to match input indices. - Use tagSelected in scatter, box, and histogram calcSelection methods taking care of all trace types that support selections.
- similar to pointNumber and pointNumbers, make the distinction between 1-to-1 input to selection/hovered pt and many-to-1 maps like for histogram traces and aggregation transforms.
|
Referencing #2126 (review) which first put forward something similar to this PR's |
| // Note also that the hover labels show the scaled version. | ||
| // | ||
| // What about the 'raw' input coordinates? | ||
| // Should we include them in parallel here or replace a/b/c with them? |
There was a problem hiding this comment.
I think the scaled (normalized) coords are the most useful here, and folks can get the raw data back out easily enough using the indices, lets not add them here.
src/lib/index.js
Outdated
| } | ||
|
|
||
| function isPtIndexValid(v) { | ||
| return lib.validate(v, {valType: 'integer', min: 0}); |
There was a problem hiding this comment.
ha, that's super 🌴 but might be a bit slow for this 🌶 path - non-negative integers are just
typeof v === 'number' && v >= 0 && v % 1 === 0 right?
There was a problem hiding this comment.
Thanks!
🐎 > 🌴 in 🌶️ code paths
There was a problem hiding this comment.
Oh hmm I guess this is looking at user input so it should be isNumeric(v) instead of typeof v === 'number' ie allow index '1' as well as 1
src/traces/histogram/calc.js
Outdated
| if(Array.isArray(trace.selectedpoints)) { | ||
| var ptNumber2cdIndex = {}; | ||
|
|
||
| // make histogram-specific pt-number-to-cd-index map object |
There was a problem hiding this comment.
don't we want this available for use later, ie without a recalc? I would have imagined creating this in the main binning loop and stashing it in the full trace, also possibly creating it as an array rather than a map, seems like that would be more efficient and would work just as well?
There was a problem hiding this comment.
don't we want this available for use later, ie without a recalc?
I don't think so. Calling restyle with selectedpoints has to trigger a recalc. On user selections, the calcdata items are mutated directly w/o needing to refer back to input data-array indices. Am I missing something?
There was a problem hiding this comment.
I would have imagined creating this in the main binning loop
Good call, done in -> 7543dc6
There was a problem hiding this comment.
Calling
restylewithselectedpointshas to trigger a recalc.
hmm ok, I was hoping we could avoid that but we can discuss optimizing that later.
- fixup ternary hover event data test accordingly.
|
💃 |
These few commits essentially addresses #2135 (comment) while cleaning up a few things in event data land - mainly making hover and selection event data reuse the same logic.
In brief, this PR adds
pointIndexevent data field and uses its value to fill inselectedpointswith input indices. Note thatpointIndexis the same aspointNumberin all non-transformed traces except histograms. With the help of @monfera 'sindexToPoints, this part was relatively easy 🎉I chose to add a new field instead of fixing the existing
pointNumberfield to retain backward compatibility (I suspect a certain support-assets project we did back in May would be break if we tweakedpointNumber😏 ), but yeahpointIndexis what users should use from now on. We could even 🔪pointNumberin v2.pointIndexalso has a multi-value version calledpointIndicesfor histogram traces and aggregation transforms.I'll wait for @alexcjohnson to 👍 the commits below before pushing new test cases and incorporating them in #2135
cc @monfera