Automargin fixes #2681

alexcjohnson · 2018-05-30T17:50:03Z

Fixes #2651 - and a bunch of other things I encountered along the way:

Stops writing trace.uid back to the user input data, instead reusing the values from oldFullData if the user didn't specify uid in the trace. One more unexpected mutation gone 🎉 but more importantly, Plotly.react users were probably trashing this anyway when making a new figure state, so they were triggering a more aggressive redraw than was really necessary.
Significant 🌴 for colorbars, as well as making them work in some cases where they should (splom, scatterpolar[gl]) or worked but broke on editing (parcoords, histogram)
Automargin logic is now a bit more encapsulated: for example clearPushMargins we used to need in rangesliders and updatemenus (and margin clearing by missing colorbars) are no longer necessary: we start a replot by clearing the list of allowed margin pusher ids, then delete disallowed ids when actually calculating the margins. Edits that don't go through the full replot can still adjust the margins by changing, adding, or deleting pushers, but they don't need to worry about deleting their old pushers on a full replot.

The automargin system is still a mess in some fundamental ways that I didn't address here - really we need to break it out from the drawing steps of all of these components, so we can calculate the margins once and for all, then draw everything; as opposed to drawing, noticing we need to change the margins, then drawing all over again... but I think this is a step in the right direction.

cc @etpinard

- Importantly, Plotly.react users probably were trashing these every time anyway resulting in bigger redraws than we needed. Now we're pulling them from the previous fullData if possible. - Also cleanData is now SOLELY for backward compatibility

Also fixes and tests a few traces that should have supported colorbars but didn't and some cases of editing colorbars that didn't previously work

etpinard · 2018-05-30T18:32:48Z

src/components/rangeslider/draw.js

-
-    // remove push margin object(s)
-    if(rangeSliders.exit().size()) clearPushMargins(gd);
+    }).remove();


Ha exit().each() returns the exit selection, good to know.

etpinard · 2018-05-30T18:37:13Z

src/plot_api/plot_api.js

+        doTicks,
+        subroutines.drawData,
+        subroutines.finalDraw,
+        subroutines.drawMarginPushers


Hmm. That's unfortunate. Why do we need to call drawMarginPushers on axis range relayouts?

Good Q, can't remember now why I needed that, I'll take another look at it...

etpinard · 2018-05-30T18:38:38Z

src/plot_api/subroutines.js

+    // because the margins need to be fully determined before we can call
+    // autorange and update axis ranges (which rangeselector needs to know which
+    // button is active). Can we break out its automargin step from its draw step?
+    Registry.getComponentMethod('rangeselector', 'draw')(gd);


Thanks for those 📚

etpinard · 2018-05-30T18:41:17Z

src/traces/cone/colorbar.js

-        Plots.autoMargin(gd, cbId);
-        return;
-    }
+    if(!trace.showscale) return;


Where is the colorbar automargin push applied now?

... in the next commit -> fa86147 🤕

No, it's in this commit - the autoMargin I removed here is just the one to clear the entry for this colorbar, but that's not needed anymore due to plots.clearAutoMarginIds

Thanks for headsup 👌

etpinard · 2018-05-30T18:42:48Z

src/components/updatemenus/draw.js

-    if(menus.exit().size()) clearPushMargins(gd);
+    menus.exit().each(function() {
+        // Most components don't need to explicitly remove autoMargin, because
+        // marginPushers does this - but updatemenu updates don't go through


Why is this the case? Is it because updatemenus attributes are under the "arraydraw" edit type? But then, in this case layout sliders should also have the same problem.

Oh man, I missed sliders in this PR but sure enough, they still have the older pattern - thanks, I'll take a look at those.

Updated sliders in 3deab4c

etpinard · 2018-05-30T19:13:30Z

test/jasmine/tests/legend_test.js

@@ -524,18 +524,36 @@ describe('legend relayout update', function() {
    afterEach(destroyGraphDiv);

    it('should hide and show the legend', function(done) {
-        var mockCopy = Lib.extendDeep({}, mock);


Is this commit locking down fixes from this PR, or just improving our test case coverage?

Pretty sure at least one of these was broken before this PR, but I don't recall which anymore.

etpinard · 2018-05-30T19:19:47Z

src/components/colorbar/connect.js

+ *   pre-calculated.
+ */
+module.exports = function connectColorbar(gd, cd, moduleOpts) {
+    if(typeof moduleOpts === 'function') return moduleOpts(gd, cd);


So, contour still gets its own version on module._colorbar in

https://github.com/plotly/plotly.js/blob/fa8614750586a2901ccf65a520521bb570eb7026/src/traces/contour/colorbar.js

... and that's the only exception?

Yes, contour and those that inherit from it (contourcarpet, contourgl, histogram2dcontour).

I should note that there's one other place module.colorbar gets used, that's in colorbar.draw for getting the nestedProperty string for title editing. If contour colorbars weren't at the module root we'd need to set a container property of the function which would be a bit hacky... alternatively we could either a) make this function be module.colorbar.func, or b) incorporate contour into this routine directly, triggered by some other property of module.colorbar.

alternatively we could either a) make this function be module.colorbar.func

This sounds cleaner to me. But no need to do this in this PR.

etpinard · 2018-05-30T19:25:29Z

src/plot_api/plot_api.js

-        Registry.getComponentMethod('rangeselector', 'draw')(gd);
-        Registry.getComponentMethod('sliders', 'draw')(gd);
-        Registry.getComponentMethod('updatemenus', 'draw')(gd);
+        // First reset the list of things that are allowed to change the margins


(for documentation purposes only) this patch is in function marginPushers() {}

etpinard · 2018-05-30T19:29:28Z

src/plot_api/plot_api.js

+        // TODO: shouldn't really need doAutoMargin here, but without it some
+        // edits don't manage to tweak the margins the way they should
+        // in the tests, the only one that presently breaks is axis.position
+        Plots.doAutoMargin,


This is a little unfortunate. This could be a big performance hit. Would it be too much to ask to try to remove this in this PR?

If only axis.position is affected, we could add a special (hopefully temporary) relayout flag to call Plots.doAutoMargin after finalDraw

I'll take a look again, didn't really understand why it was needed so I wasn't confident it was only ax.position - that's just the only one that we test directly that triggered it. Anyway it's a very minor performance hit if the margins don't change at that point.

Anyway it's a very minor performance hit if the margins don't change at that point.

Oh right, doAutoMargin is smart^TM like that. This additional doAutoMargin isn't a big deal then.

On further investigation, this doAutoMargin call is definitely necessary for now. The reason is that axis automargin is not part of marginPushers - they reserve the right to push the margins there now with Axes.allowAutoMargin but the actual adjustment comes later during Axes.doTicks.

As discussed above, this doAutoMargin call won't actually redraw anything if the margins didn't change, so there's usually a minimal cost to this, but when the margins do change this triggers a full replot.

Long-term it would be far better to completely separate out margin calculations from drawing, so we never need to draw multiple times for a single change. But this addition doesn't represent a performance hit relative to the status quo.

The reason is that axis automargin is not part of marginPushers - they reserve the right to push the margins there now with Axes.allowAutoMargin but the actual adjustment comes later during Axes.doTicks.

Thanks for writing this down! Could update that comment in the code above this line to reflect these new findings?

Long-term it would be far better to completely separate out margin calculations from drawing, so we never need to draw multiple times for a single change. But this addition doesn't represent a performance hit relative to the status quo.

The dream.

Oh and about

Long-term it would be far better to completely separate out margin calculations from drawing, so we never need to draw multiple times for a single change. But this addition doesn't represent a performance hit relative to the status quo.

Is there an issue about this already? If not, we should open one.

comments improved in 230a782, and new issue #2704 to track it.

etpinard · 2018-05-30T19:35:08Z

src/plots/plots.js

    var colorway = layout.colorway || Color.defaults;
-    var traceOut = {},
-        defaultColor = colorway[colorIndex % colorway.length];
+    var traceOut = {uid: uid};


Or alternatively, I think we could move this line:

to just before this line:

the above comment is non-blocking.

yeah probably - but I thought it would be better in supplyDefaults so we don't add another order-dependence to the plot step - carpet dependents need this too. I think they're already required to plot after the carpet for other reasons but still...

but I thought it would be better in supplyDefaults so we don't add another order-dependence to the plot step - carpet dependents need this too.

Good point. Your way is 👌

etpinard · 2018-05-30T19:44:51Z

@alexcjohnson Thanks for taking this on. Auto-margin and colorbars were very much in need of some ❤️

I'm a little concerned with:

the additional doAutoMargin call in Plotly.plot - Automargin fixes #2681 (comment)
the additional drawMarginPushers call during axrange relayouts - Automargin fixes #2681 (comment)
updatemenus / sliders asymmetries - Automargin fixes #2681 (comment)

My other comments aren't as important.

Also includes a generalization of `Plots.autoMargin` so an object can have extent in plot fraction, as opposed to just position in plot fraction and extent in pixels. We could use this to allow for example colorbars to be sized based on the actual margins, not the pre-margin-pushing plot size as they are now, see note at colorbar/draw:119-124

etpinard · 2018-06-01T13:56:53Z

test/jasmine/assets/custom_assertions.js

+    var heightLessThan = opts.heightLessThan;
+
+    var actualWidth = d3.select('.ygrid').node().getBoundingClientRect().width;
+    var actualHeight = d3.select('.xgrid').node().getBoundingClientRect().height;


(non-blocking) but wouldn't d3.select('.subplot.xy > .plot').node().getBoundingClientRect be more general (e.g. would work even on graphs with showgrid: false)?

Hmm, good point, but .plot shrinks to the data, which may not reach the edges of the plot area (or could even go beyond with cliponaxis: false). Possibly .bglayer .bg though...

or could even go beyond with cliponaxis: false

Yeah, good point here. Tough problem it turns out.

switched to .bglayer .bg in bb6c720 though this has a caveat about margin.pad 5d31324. I still think it's an improvement: margin.pad isn't used very much and can be corrected for if you really want, and the bg rect is always present.

alexcjohnson · 2018-06-01T14:13:45Z

src/plots/plots.js

+ *         you can specify both edges as plot fractions in each dimension
+ *     l, r, t, b: the pixels to pad past the plot fraction x[l|r] and y[t|b]
+ *     pad: extra pixels to add in all directions, default 12 (why?)
+ */


I mentioned this in the commit message:

Also includes a generalization of Plots.autoMargin so an object can
have extent in plot fraction, as opposed to just position in plot fraction
and extent in pixels. We could use this to allow for example colorbars
to be sized based on the actual margins, not the pre-margin-pushing plot size
as they are now, see note at colorbar/draw:119-124

but for more detail: colorbars (and possibly a few others?) when sized as plot fraction they use the initial margins to convert plot fraction to pixels - meaning that if the margins expand beyond that, a colorbar with length "1" would actually be taller than the plot area. This is probably fine if it's the colorbar itself that's expanding the margin - that means it's off-center and thus not aligned with the plot, so who really cares if it's a little bigger than the plot? But if something else (eg a rangeslider, or axis labels) expanded the margin, all of a sudden the colorbar is misaligned. Using yt/yb we can size it correctly without getting into an infinite loop of automargin calls. Actually now that I think of the case ^^ which would be pretty ugly, I may try to do that in this PR as well.

Thanks for the info 📚

and fix up size mode changes, along with a test that *actually* tests this instead of just looking like it tested it...

alexcjohnson · 2018-06-06T12:55:04Z

src/plots/plots.js

+                // if no explicit pad is given, use 12px unless there's a
+                // specified margin that's smaller than that
+                pad = Math.min(12, margin.l, margin.r, margin.t, margin.b);
+            }


@etpinard seem reasonable? I started looking into this due to test failures at d02c76b - where some colorbars that were previously not able to expand the margin because they were internally pixel-sized suddenly became able to expand it when I switched it to plot-fraction-sized as described in #2681 (comment)... but then this 12px padding overrode user-supplied margins that were smaller - and that didn't seem right to me. Some of the baseline images with small explicit margins changed as a result.

The 12px here is an odd hard-coded feature - I suppose an alternative would be to make this an attribute in its own right (layout.margin.autopad?) and put the min(12, margin.l...) logic in supplyDefaults.

seem reasonable?

Very reasonable. 🥇

I suppose an alternative would be to make this an attribute in its own right (layout.margin.autopad?) and put the min(12, margin.l...) logic in supplyDefaults.

Maybe down the road, but no need in this PR.

must have been necessary mid-development then fixed later on?

etpinard · 2018-06-06T15:01:39Z

src/components/sliders/draw.js

+            delete sliderOpts._commandObserver;
+        }
+
+        Plots.autoMargin(gd, autoMarginId(sliderOpts));


Could we add a comment similar to what's above https://github.com/plotly/plotly.js/pull/2681/files#r191879632 here?

sure -> 230a782

etpinard · 2018-06-06T15:13:51Z

src/components/colorbar/draw.js

+                marginOpts.b = outerheight * bFrac;
+            }
+            else {
+                marginOpts.t = marginOpts.b = 0; // TODO - not zero?


Could you expand on that TODO comment?

Ah thanks, that was just for me to check during development, it's correct as zeros -> removed in 230a782

etpinard · 2018-06-06T17:23:01Z

Awesome 💃

alexcjohnson added 6 commits May 30, 2018 12:52

stop mutating user data to add uids

970db80

- Importantly, Plotly.react users probably were trashing these every time anyway resulting in bigger redraws than we needed. Now we're pulling them from the previous fullData if possible. - Also cleanData is now SOLELY for backward compatibility

some test cleanup

26278de

first cut at updating automargin logic

6dd68bd

refactor colorbars for simpler connection to trace attributes

fa86147

Also fixes and tests a few traces that should have supported colorbars but didn't and some cases of editing colorbars that didn't previously work

test legend, rangeselector, rangeslider automargin updates

95f9b33

provide uids to traces a little earlier - to fix carpet clip ids

8f3da82

etpinard added bug something broken status: reviewable labels May 30, 2018

etpinard added this to the v1.39.0 milestone May 30, 2018

etpinard reviewed May 30, 2018

View reviewed changes

alexcjohnson added 4 commits May 30, 2018 18:04

🔪 colorbar.vals - seems like that was all obsolete anyway

f9fbbc6

cleanup in sliders_test

222f396

simplify inner plot size tests with custom_assertions

bd07df7

etpinard reviewed Jun 1, 2018

View reviewed changes

alexcjohnson commented Jun 1, 2018

View reviewed changes

alexcjohnson added 3 commits June 1, 2018 16:03

move sliders to the updated automargin framework

3deab4c

use bg rect instead of grids for assertPlotSize

bb6c720

base fractional colorbar size on post-expanded-margin plot size

d02c76b

and fix up size mode changes, along with a test that *actually* tests this instead of just looking like it tested it...

alexcjohnson mentioned this pull request Jun 5, 2018

Fix scatter3d scalar hovertext handling #2698

Merged

alexcjohnson added 2 commits June 6, 2018 08:42

comment and fix on assertPlotSize with margin.pad

5d31324

reduce automargin padding for small explicit margins

16ea22a

alexcjohnson commented Jun 6, 2018

View reviewed changes

axRange edits don't need drawMarginPushers after all

24aeb7c

must have been necessary mid-development then fixed later on?

etpinard reviewed Jun 6, 2018

View reviewed changes

alexcjohnson mentioned this pull request Jun 6, 2018

Automargin pipeline #2704

Closed

automargin comment updates

230a782

alexcjohnson merged commit 387db82 into master Jun 6, 2018

alexcjohnson deleted the react-automargin branch June 6, 2018 17:23

etpinard mentioned this pull request Jul 23, 2018

Plotly.deleteTraces leads to bad trace uid sequence #2836

Closed

etpinard mentioned this pull request Sep 4, 2018

Parallel Categories (parcats) trace type for multi dimensional categorical data #2963

Merged

4 tasks

dmt0 mentioned this pull request Oct 10, 2018

Trace uid not generated plotly/react-chart-editor#765

Closed

alexcjohnson mentioned this pull request Nov 10, 2018

Preserve ui state across Plotly.react redraws #3236

Merged

Automargin fixes #2681

Automargin fixes #2681

Conversation

alexcjohnson commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard May 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented May 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Jun 6, 2018

etpinard May 30, 2018 •

edited

Loading

etpinard commented May 30, 2018 •

edited

Loading

etpinard Jun 1, 2018 •

edited

Loading