Clean up automargin pipeline a bit #3323
Conversation
| // Figure out which subplot to draw ticks, labels, & axis lines on | ||
| // do this as a separate loop so we already have all the | ||
| // _mainAxis and _anchorAxis links set | ||
| ax._mainSubplot = findMainSubplot(ax, fullLayout); |
There was a problem hiding this comment.
moved this into plots.linkSubplots, which is both a more obvious place for it and happens earlier (in supplyDefaults) so _mainSubplot is available sooner.
| }; | ||
|
|
||
| function findMainSubplot(ax, fullLayout) { |
There was a problem hiding this comment.
Is moving findMainSubplot from lsInner to supplyDefaults necessary? I suspect this will make supplyDefaults for large splom traces significantly slower.
There was a problem hiding this comment.
Something or other failed with it in lsInner, though I don't recall what. I'll take a look at 🐎 for large sploms, I'm sure there's a shortcut we can take there.
There was a problem hiding this comment.
I'm sure there's a shortcut we can take there.
Yeah, I would be nice to leave findMainSubplot in supplyDefaults and add a fullLayout._hasOnlyLargeSploms condition to bypass the loop-over-subplot-for-all-axes loops.
There was a problem hiding this comment.
Large sploms usually avoid the subplot loop there - the first try based on _anchorAxis works unless the labels are unanchored - upper half with lower labels, or both halves with no diagonal.
Anyway I added an extra collection of counteraxes and subplots per axis in f55e769, which means we don't have to search the full subplot list anymore here (and in fact Axes.getSubplots is completely unused within plotly.js, but we still have it in streambed ATM). So even when splom does go through this loop, it now only takes 0.3ms at 20 dimensions - ~2ms at 50 dimensions if it's O(n^2).
There was a problem hiding this comment.
works unless the labels are unanchored - upper half with lower labels, or both halves with no diagonal.
Ha right, I remember making that happen 3 or 4 splom-perf PRs ago.
So even when splom does go through this loop, it now only takes 0.3ms at 20 dimensions - ~2ms at 50 dimensions if it's O(n^2).
Beautiful 🐎
This is fantastic 🎉
Yeah, I agree. The current baseline appears wrong. 👌 |
this means Axes.getSubplots and Axes.findSubplotsWithAxis are essentially unused internally, but we have some outside callers using getSubplots
Working on this has uncovered some more bugs - will address those in a separate bugfix PR. |
|
💃 let's get this in #3300 |
@etpinard I based this branch off
multicategory(including commits from this morning) because we were both working in axis drawing code.The main goal was to get
plots.doAutoMarginout ofsupplyDefaults- because that can lead to a full redraw of the plot if margins have changed, which is completely inappropriate forsupplyDefaults- somehow it never caused explicit problems before, but with big enough changes inPlotly.react, ie #3255, it does.It turned out the biggest challenge with this was rangesliders. The main thing I did to fix this was to combine rangeslider and axis automargins to happen together - note though that in principle both can contribute independently to the margins, if you have a rangeslider (on the bottom) and a top axis with long labels (oh right, I meant to add a test for that case - I'll add that)
One baseline image,
range_slider_rangemode, changed in this PR b2a4b76 - just a little bit, but I believe it was in fact wrong before, as you can see by interacting with this mock on master: start dragging the range and the box expands vertically a little bit, to match what's in the new baseline. On this branch there's no such jump. Not quite sure what in this PR fixed it 😅 but it's fixed.