Closed alexcjohnson closed 4 months ago
we can determine all sizes before we draw anything, then each component need only be drawn once
This wasn't possible the first time round because as the margins change, some components change how they draw themselves... for example as the left margin increases, the x-axis tick labels compress and then draw themselves on an angle.
After an investigation, I'm realising that our "pipeline problems" are a little deeper than first expected, specifically for cartesian axes.
In brief, cartesian auto-margin push values depend on the axis range (as the axis range determines which tick labels appear on the graph and thus their size) AND cartesian axis auto-range depends on the margins (via ax._length
which is used as a scale factor for padded auto-range computations). Unfortunately this can lead to infinite loops (cc https://github.com/plotly/plotly.js/issues/4028). Margins and axis ranges get even more intertwined when axis constraints are set.
To illustrate, consider:
Plotly.newPlot('graph', [{
mode: 'markers',
marker: {size: 100},
x: [1, 200, 6000]
}], {
width: 400, height: 400,
yaxis: {
visible: false,
range: [-1, 3]
},
xaxis: {
tickfont: {size: 32},
tickangle: 'auto',
automargin: true,
autorange: true
},
margin: {l: 0, t: 0, r: 0, b: 0}
})
where the auto-range padding and auto-margin push values are exaggerated via marker.size
and xaxis.tickfont.size
respectively. In this simplified example, only the x-axis contributes to the auto-margin computations (e.g. there are no legend, colorbar etc) and only the x-axis is auto-ranged.
With the current pipeline, Plotly.newPlot
:
ax._length
being the graph's width (i.e. 400
in our case here).margin: {l: 0, t: 0, b: 0, r: 0}
, we replotax._length = gs.w
which is now smaller than 400
. As the axis length in pixel space in now smaller: the auto-axis range has now a larger span than during the first iterationax._length
, the x-axis labels overlap and get auto-rotated to tickangle: 90
meaning a much smaller bounding box widthx
margin push values are still bigger than margin: {l: 0, t: 0, b: 0, r: 0}
, we replot againax._length
is larger than during the 2nd. In our case, this leads x-axis labels *not overlapping on the horizontal, big x
margin push values and one more replotx
margin push values.Uncaught RangeError: Maximum call stack size exceeded
:boom: So, there are a few ways to "quickly" fix the problem. Off the top of my head:
Plots.doAutoMargin
, bail out after say the 2nd replot
newPlot
and some relayout
callsPlots.doAutoMargin
cycle
All in all, I can wait to get rid of the replot calls inside Plots.doAutoMargin
.
The approach I used in my original *axis.automargin
PR is to avoid backtracking: basically the margins only ever get bigger, even if it looks like they could shrink.
Writing down a few notes I took on the topic:
automargin: true
)Coming soon:
automargin
flag)_replotting = true
setPositions
became crossTraceCalc
calcAutorange
(async)calcAutorange
(sync)_replotting = false
Drawing.bBox
before doAutoRangeAndConstraints
. We could do this in Scatter.calc
or add a more async friendly Scatter.calcAutorange
Drawing.bBox
resultscartesian/set_convert.js
- https://github.com/plotly/plotly.js/issues/4155#issuecomment-525929245v1.50.0
update:Unfortunately, I won't be able to complete this project in time for v1.50.0
. I'll find a (possibly hacky) way to guard against https://github.com/plotly/plotly.js/issues/4028 - but the "pipeline" will remain the same at least until v1.51.0
.
For those of you interested, branch
https://github.com/plotly/plotly.js/compare/auto-margin-pipeline-DEV
now has a pretty solid proof-of-concept. In brief,
pushMargin
method, where the text elements are drawn in the Drawing.tester
node to compute their sizeAxes.drawOne
no longer depends on px-valued stashed fields computed in lsInner
Axes.drawOne
into Axes.pushMargin
Plotly.plot
internal redraw calls are needed:Some general TODOs:
relayout
work again for margin-pushing components
Plots.autoMargin
can no longer trigger a redrawDrawing.tester
into graph div, instead of drawing them twiceI'll find a (possibly hacky) way to guard against #4028
Dropping from the v1.51.0
. The workarounds introduced in 1.50.0
appear to work ok for now.
Unfortunately, I'm not sure if we'll have the time to resume work on this ticket until the start of 2020.
Quick note saying that I won't remove branch
https://github.com/plotly/plotly.js/tree/auto-margin-pipeline-DEV
which was referenced above in https://github.com/plotly/plotly.js/issues/2704#issuecomment-534302295
This branch is already too far behind master
to consider rebasing it, but by looking through the commits, I hope it will help conceptualized the strategy I thought about implementing.
Hi - this issue has been sitting for a while, so as part of our effort to tidy up our public repositories I'm going to close it. If it's still a concern, we'd be grateful if you could open a new issue (with a short reproducible example if appropriate) so that we can add it to our stack. Cheers - @gvwilson
Our automatic margin expansion system is a mess: components generally determine their sizes - and thus the amount they need to increase the margin - only while being drawn. So in order for the new margin to take effect, the component gets drawn, which updates the margin calculation, then all of these components need to be drawn again if the new margin is different from the old one. The situation is even worse for axis automargins, since axes are drawn later in the process, changes they make to the margins result in an entire redraw of the plot, ie nearly doubling the initial plot time.
Much better would be to separate out margin calculations from component drawing; that way we can determine all sizes before we draw anything, then each component need only be drawn once. The challenge with this is that these sizes depend on text bounding boxes, so in order for this not to add its own overhead we may need to create the text elements as part of the margin calculation step, stash them somewhere hidden, and then pull them back out during the drawing step and attach them to the component. Or perhaps the two steps are 1) component creation and sizing, 2) component positioning and updating?
Some related discussion happened during #2681. See also #1988 and #2434.