Automargin pipeline

[alexcjohnson]

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.

[nicolaskruchten]

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.

[etpinard]

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 #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:

• Computes an autorange value with ax._length being the graph's width (i.e. 400 in 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.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 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 to tickangle: 90 meaning a much smaller bounding box width
• As the new (smaller) x margin push values are still bigger than margin: {l: 0, t: 0, b: 0, r: 0}, we replot again
• Now on this 3rd iteration, ax._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 replot
• The 4rd iteration leads to vertical x-axis labels and small x margin 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 newPlot and some relayout calls
• Do not go back and forth between computed "auto" tick angle during the same Plots.doAutoMargin cycle
  • 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.

[nicolaskruchten]

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.

[etpinard]

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 automargin flag)
• 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 setPositions became crossTraceCalc
  • annotations calcAutorange (async)
  • shapes calcAutorange (sync)
  • doAutoRangeAndConstraints
• layoutStyles
  • doAutoMargin
  • lsInner
• drawAxes
• drawData
  • set _replotting = false
• 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.bBox before doAutoRangeAndConstraints. We could do this in Scatter.calc or add a more async friendly Scatter.calcAutorange
• Fit inside tick (and eventually labels drawn inside the plot area) in autorange
  • doAutorange would need to have access to the tick label Drawing.bBox results
• 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)

[etpinard]

Pre v1.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 #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,

• margin-pushing components now have a pushMargin method, where the text elements are drawn in the Drawing.tester node to compute their size
• Axes.drawOne no longer depends on px-valued stashed fields computed in lsInner
• the margin pushing logic for cartesian axes is split from Axes.drawOne into Axes.pushMargin
• commit e606c7b presents a new pipeline candiate where no Plotly.plot internal redraw calls are needed:

plotly.js/src/plot_api/plot_api.js

Lines 325 to 345 in e606c7b

	var seq = [
	Plots.previousPromises,
	addFrames,
	drawFramework,
	positionAndAutorange,
	pushMargin,
	pushMarginAgain,
	pushMarginAgain,
	positionAndAutorange,
	saveInitial,
	subroutines.layoutStyles,
	subroutines.drawData,
	subroutines.drawMarginPushers,
	drawAxes,
	subroutines.finalDraw,
	initInteractions,
	Plots.addLinks,
	Plots.rehover,
	Plots.redrag,
	Plots.previousPromises
	];

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 relayout work again for margin-pushing components
  • this is necessary now that Plots.autoMargin can no longer trigger a redraw
• perf testing!!
• investigate transfering in text elements from Drawing.tester into graph div, instead of drawing them twice

[etpinard]

I'll find a (possibly hacky) way to guard against #4028

Done in #4216

[etpinard]

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.

[etpinard]

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 master to consider rebasing it, but by looking through the commits, I hope it will help conceptualized the strategy I thought about implementing.

[gvwilson]

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