Repository navigation
Automargin pipeline #2704
Description
Activity
- added a commit that references this issue
on Jul 4, 2018 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._lengthwhich is used as a scale factor for padded auto-range computations). Unfortunately this can lead to infinite loops (cc #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.sizeandxaxis.tickfont.sizerespectively. 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:- Computes an autorange value with
ax._lengthbeing the graph's width (i.e.400in our case here). - Draws the x-axis ticks, computes their margin push values
- As the margin push lead to a bigger margin than the input
margin: {l: 0, t: 0, b: 0, r: 0}, we replot - Computes a 2nd autorange value with
ax._length = gs.wwhich is now smaller than400. As the axis length in pixel space in now smaller: the auto-axis range has now a larger span than during the first iteration - Draw news x-axis ticks. In general, the new tick labels have a different bounding box than during the first iteration. In our case, with the smaller
ax._length, the x-axis labels overlap and get auto-rotated totickangle: 90meaning a much smaller bounding box width - As the new (smaller)
xmargin push values are still bigger thanmargin: {l: 0, t: 0, b: 0, r: 0}, we replot again - Now on this 3rd iteration,
ax._lengthis larger than during the 2nd. In our case, this leads x-axis labels *not overlapping on the horizontal, bigxmargin push values and one more replot - The 4rd iteration leads to vertical x-axis labels and small
xmargin push values. - .... and so on until
Uncaught RangeError: Maximum call stack size exceeded💥
So, there are a few ways to "quickly" fix the problem. Off the top of my head:
- Keep count of the "replots" triggered by the
Plots.doAutoMargin, bail out after say the 2nd replot- but this may lead to inconsistency between
newPlotand somerelayoutcalls
- but this may lead to inconsistency between
- Do not go back and forth between computed "auto" tick angle during the same
Plots.doAutoMargincycle- This would fix the above example, but I suspect it's possible to trigger an infinite even with a fixed angle
- Do not compute the auto-margins from scratch during a replot
- Here too, I'm not convinced this would make us avoid all potential infinite loops
All in all, I can wait to get rid of the replot calls inside
Plots.doAutoMargin.- Computes an autorange value with
The approach I used in my original
*axis.automarginPR 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:
Components that push margins
- legends
- colorbars
- sliders (sync, no MathJax support yet)
- updatemenus (sync, no MathJax support yet)
- Axes (if
automargin: true) - rangeslider (computed together with Axes)
- rangeselector (sync, no MathJax support yet)
Coming soon:
- Pie traces (the ones with outside labels, we need to add an
automarginflag) - Graph titles
Components that depend on the axis auto-range results
- annotations (can also contribute to autorange)
- shapes (can also contribute to autorange)
- images
- Axes
- rangeslider
- rangeselectors (but just its click handlers)
Current sketch of plot pipeline
- supplyDefaults
- set
_replotting = true - doCalcData
- addFrames
- drawFramework
- marginPushers
- Legend.draw
- Rangeselector.draw
- Sliders.draw
- Updatemenus.draw
- Colorbar.draw
- doAutoMargin
- marginPushersAgain
- marginPushers (if margin changed)
- layoutStyles (if margin changed)
- positionAndAutorange
- ... misnomer, the "position" part is gone, since
setPositionsbecamecrossTraceCalc - annotations
calcAutorange(async) - shapes
calcAutorange(sync) - doAutoRangeAndConstraints
- ... misnomer, the "position" part is gone, since
- layoutStyles
- doAutoMargin
- lsInner
- drawAxes
- drawData
- set
_replotting = false
- set
- finalDraw
- Rangeslider.draw
- Rangeselector.draw
- initInteractions
- doAutoMargin (this call is only needed for automargin axes)
Other "plot pipeline" problems (in near future)
- Autorange scatter text
- We would need to prerender the text elements and
Drawing.bBoxbeforedoAutoRangeAndConstraints. We could do this inScatter.calcor add a more async friendlyScatter.calcAutorange
- We would need to prerender the text elements and
- Fit inside tick (and eventually labels drawn inside the plot area) in autorange
- doAutorange would need to have access to the tick label
Drawing.bBoxresults
- doAutorange would need to have access to the tick label
- Cross-axis axis labels overlaps Splom labels running into each other #3505
- Axis labels <--> legend (or other layout components) overlaps Add legend 'auto' x|y and/or 'container' (x|y)ref #1199
- the "throw new Error" piece in
cartesian/set_convert.js- graph should handle the case where the display area is significantly small #4155 (comment)
Reacted by Mojtaba SamimiPre
v1.50.0update: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 #4028 - but the "pipeline" will remain the same at least untilv1.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,
- margin-pushing components now have a
pushMarginmethod, where the text elements are drawn in theDrawing.testernode to compute their size Axes.drawOneno longer depends on px-valued stashed fields computed inlsInner- the margin pushing logic for cartesian axes is split from
Axes.drawOneintoAxes.pushMargin - commit e606c7b presents a new pipeline candiate where no
Plotly.plotinternal redraw calls are needed:
plotly.js/src/plot_api/plot_api.js
Lines 325 to 345 in e606c7b
Some general TODOs:
- fixup the image tests
- margin pushes for colorbar with titles are off
- constrained axes sometimes don't get the right range
- investigate sub-pixels diff
- add new subroutine to make
relayoutwork again for margin-pushing components- this is necessary now that
Plots.autoMargincan no longer trigger a redraw
- this is necessary now that
- perf testing!!
- investigate transfering in text elements from
Drawing.testerinto graph div, instead of drawing them twice
Reacted by Will Burgess- margin-pushing components now have a
Dropping from the
v1.51.0. The workarounds introduced in1.50.0appear 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 #2704 (comment)
This branch is already too far behind
masterto 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.