Better cartesian trace updates and removal #2574
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- so that we no longer have the clear all their <g.trace> on every Cartesian.plot call !!! - Note that other svg traces (e.g. heatmap & contours) use trace.uid to append/update/clear their SVG layers weren't the focus of this commit.
- use fullLayout[axName] instead of indexOf into axis list - for-in loop instead Object.keys + for-loop
- so that we can lookup subplots in newFullLayout._plots instead of indexOf into (something looong) subplots lists.
... when corresponding module is gone from fullLayout._modules To do so, call plot methods of visible modules AND of recently deleted ones, by stashing list of module on plot after Cartesian.plot in each plotinfo object. Make sure to list unique 'plot method' NOT unique trace module as sometimes multiple module use same 'plot method'.
- which is a weird case. It relies on trace.uid (like heatmaps/contour),
but has <g.trace> group (like scatter/bar/...) which previously
got deleted in that big selectAll('g.trace').remove() in
Cartesian.plot. Now make _module.plot take care of trace deletion!
src/plots/get_data.js
Outdated
|
|
||
| var plotMethod; | ||
| if(arg1) { | ||
| var typeOfArg1 = typeof arg1; |
There was a problem hiding this comment.
Does a falsy arg1 happen?
Also, I recall seeing somewhere that js can optimize typeof arg1 === 'string' internally so it never has to generate the string values, whereas with var typeOfArg1 = typeof arg1; typeOfArg1 === 'string' it can't do that so it's actually better to write out typeof arg1 === '...' in each clause. I can't find it now though...
There was a problem hiding this comment.
Thanks for tips!
Simplified in -> e5c860a
|
Beautiful. Only the one very nonblocking comment, lets do it! 💃 |
|
Thanks for the review @alexcjohnson, I think I'll wait a few days before merging this thing in case we need to make another patch release in the |
|
Good call - like my upcoming react/transforms fix... |
- should fix some intermittent hangups when running the image tests locally
- make trace module layers 'data-driven', one layer per unique plot module. Use _module.layerName to denote reused plot methods. - pass layer to trace _module.plot (as 3rd arg) to not mutate node data and allow a given _module.plot method to trace in multiple layers - no need for previous `_plotMethods` stash attempt - rename 'imagelayer' -> 'heatmaplayer', 'maplayer' -> 'contourlayer' for consistency for other trace modules
... instead of relying on slow selectAll() during subroutines.drawData
and Plots.cleanPlot. This can be a major perf improvement on
graph with many DOM nodes (e.g. from many traces and/or many subplots)
... to take into account take now not all trace module layers
are added to each subplot.
- previously, Contour.plot called Heatmap.call for traces with 'contour.coloring': 'heatmap' which plotted a heatmap in <g.imagelayer> always below the contours. Now as <g.heatmaplayer> does not always exist, we plot the heatmap fill inside <g.contourlayer> in a ensureSingle layer.
- I don't see the need to optimize here using e.g a lookup object. traceLayerClasses.length ~ 10, and fullLayout._modules.length < 20, most often 1 or 2.
Even better cartesian trace updates and removal
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #2563
I hope @alexcjohnson will like this solution.