Improved animation merging for layout and traces #1041

rreusser · 2016-10-14T18:09:29Z

This PR adds some nuance to extendDeepNoArrays with Lib.extendTrace and Lib.extendLayout. It manually dives into container arrays to allow more animation compatibility. Specifically, it removes the blocker for transform animations:

Plotly.animate(gd, {'transforms[0].value': 7})

To do:

Make expandObjectPaths handle a couple more array corner cases. This functionality is unused at the moment, but it should be correct.
Test that it also works in Plotly.animate (arises in both Plots.transition and Plotly.animate)
Confirm existing tests pass

rreusser · 2016-10-14T19:21:52Z

@etpinard A brief explanation of how merging frames works and why this exists.

It copies the input frame with extendDeepNoArrays so that the original frame definitions don't get mangled.
It expands paths in the copied input. That is, a frame with {'line.color': 'red'} gets expanded into {line: {color: 'red'}}
It merges those changes into the destination trace using extendDeepNoArrays.

The reason for this PR: To get this process working with container arrays, it was not enough to simply recurse into container objects. expandObjectPaths did not previously know about array notation so that it was unable to expand things like {transforms[0].operation: '='}. Step 2 now expands arrays.

In case you're wondering, yes, it uses objectFromPath so it's not entirely duplicating functionality. I could have used objectFromPath to recurse slightly differently, but I think the result would be the same.

etpinard · 2016-10-17T14:28:14Z

src/lib/index.js

+// scoping vs. recompilation tradeoff, but at least it's not just inlining it into
+// the inner loop.
+var dottedPropertyRegex = /^([^\[\.]+)\.(.+)?/;
+var indexedPropertyRegex = /^([^\.]+)\[([0-9]+)\](\.)?(.+)?/;

 lib.expandObjectPaths = function(data) {


thanks for reusing this method 🎉

etpinard · 2016-10-17T14:29:49Z

src/plots/plots.js

+plots.extendLayout = function(destLayout, srcLayout) {
+    return plots.extendObjectWithContainers(destLayout, srcLayout, [
+        'annotations',
+        'shapes'


you're missing a few items here:

images

updatemenus

sliders

Ah, thanks! I didn't have a full list on hand, but yeah, designed it so should be trivial to add as needed. Will add.

etpinard · 2016-10-17T14:32:40Z

src/plots/plots.js

+plots.extendLayout = function(destLayout, srcLayout) {
+    return plots.extendObjectWithContainers(destLayout, srcLayout, [
+        'annotations',
+        'shapes'


More generally, I've been thinking about adding a preprocess step that would crawl through the plot schema and output this list of array container (as determined by _isLinkedToArray) to a json file in build/. That build/ file would then be required in the relevant plotly.js src file(s).

But, that will be for a different PR ...

Sounds good. 👍

etpinard · 2016-10-17T14:34:28Z

test/jasmine/tests/lib_test.js

+
+        // TODO: This test is unimplemented since it's a currently-unused corner case.
+        // Getting the test to pass requires some extension (pun?) to extendDeepNoArrays
+        // that's intelligent enough to only selectively merge *some* arrays, in particular


those arrays are declared as info_array e.g. pie domain.

Does

var input = {'marker[1].range': [4, 5] }; Lib.expandObjectPaths(input); // => {marker: [undefined, {range: [4, 5]}]}

??

Yes, that one works. It's fairly robust when there's just one property. It's the merging of arrays that doesn't currently work.

On that note, found a simpler case that might not be okay:

Lib.expandObjectPaths({'marker.range[0]': 5, 'marker.range[1]': 2}) // => {marker: {range: [null, 2]}}

This happens because there's not a way for it to tell how deeply it should merge, i.e. whether range is an data_array or not.

Note that this case does work because the array merge is not deep:

Lib.expandObjectPaths({'range[0]': 5, 'range[1]': 2} // => {range: [5, 2]}

My best guess at a workaround was to flag arrays that have been expanded by this routine with {_extendDeepArrays: true}, and then modify the extendDeepNoArrays to optionally extend deep if that override is set.

@etpinard I added these examples to test comments so that when we regroup and come up with a more complete solution, the cases that work vs. do not work should be readily apparent. There's no regression in functionality here (the opposite, in fact), so it doesn't bother me to consider this mergable even though it's not as complete a solution as I'd like.

Side note: I've still debated just making this a deep merge with arrays since the cost of iterating through ~500 array contents is still probably an order of magnitude or two less than the cost of SVG rendering ~500 elements, which kinda gets up to the limit of what you can animate nicely with SVG anyway.

etpinard · 2016-10-17T14:38:22Z

src/plots/plots.js

+ * The result is the original object reference with the new contents merged in.
+ */
+plots.extendTrace = function(destTrace, srcTrace) {
+    return plots.extendObjectWithContainers(destTrace, srcTrace, ['transforms']);


For completeness, transforms is currently the only array containers found in data traces until perhaps #1031

👍

etpinard

@rreusser looking good

After review:

1 blocking comment: Improved animation merging for layout and traces #1041 (comment)
1 question: Improved animation merging for layout and traces #1041 (review)

etpinard

Great work 💃

etpinard · 2016-10-17T16:18:43Z

src/plots/plots.js

+        'shapes',
+        'images',
+        'sliders',
+        'updatemenus'


etpinard · 2016-10-17T16:19:46Z

test/jasmine/tests/lib_test.js

+        //   Lib.expandObjectPaths({'marker.range[1]': 2})
+        //
+        //   // => {marker: {range: [null, 2]}}
+        //


very nice 👍

rreusser added 3 commits October 14, 2016 12:25

Add failing test fro transition transform

0e86edd

Mostly factor out trace and layout merge logic

a0da51d

Methods for extendTrace and extendLayout

b612ac3

rreusser added bug something broken feature something new status: in progress labels Oct 14, 2016

rreusser added 2 commits October 14, 2016 14:40

Lock down and document the unimplemented case in expandObjectPaths

8d35b71

Verify function of animated transforms

c93c18f

rreusser added 2 commits October 14, 2016 15:38

Tweak trivial things to get the test to pass

33e23db

Delete best guess at failing test

511de3d

etpinard added this to the v1.19.0 milestone Oct 17, 2016

etpinard reviewed Oct 17, 2016

View reviewed changes

etpinard suggested changes Oct 17, 2016

View reviewed changes

rreusser added 2 commits October 17, 2016 11:42

Add missing layout container arrays

adee9b2

Add additional docs on expandObjectPaths subtleties

e085a53

etpinard removed this from the v1.19.0 milestone Oct 17, 2016

etpinard approved these changes Oct 17, 2016

View reviewed changes

etpinard added status: reviewable and removed status: in progress labels Oct 17, 2016

rreusser merged commit c4efa80 into master Oct 17, 2016

rreusser deleted the merge-container-arrays-2 branch October 17, 2016 16:22

rreusser mentioned this pull request Oct 25, 2016

Layout animations for container array'd objects #1081

Merged

etpinard mentioned this pull request Nov 7, 2016

Make frame with nulls clear items & array containers #1118

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved animation merging for layout and traces #1041

Improved animation merging for layout and traces #1041

rreusser commented Oct 14, 2016 •

edited

Loading

rreusser commented Oct 14, 2016 •

edited

Loading

etpinard Oct 17, 2016

etpinard Oct 17, 2016

rreusser Oct 17, 2016

etpinard Oct 17, 2016 •

edited

Loading

rreusser Oct 17, 2016

etpinard Oct 17, 2016

etpinard Oct 17, 2016

rreusser Oct 17, 2016 •

edited

Loading

rreusser Oct 17, 2016

rreusser Oct 17, 2016 •

edited

Loading

etpinard Oct 17, 2016 •

edited

Loading

etpinard left a comment

etpinard left a comment

etpinard Oct 17, 2016

etpinard Oct 17, 2016

Improved animation merging for layout and traces #1041

Improved animation merging for layout and traces #1041

Conversation

rreusser commented Oct 14, 2016 • edited Loading

rreusser commented Oct 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Oct 17, 2016 • 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

rreusser Oct 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Oct 17, 2016 • edited Loading

Choose a reason for hiding this comment

etpinard Oct 17, 2016 • edited Loading

Choose a reason for hiding this comment

etpinard left a comment

Choose a reason for hiding this comment

etpinard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser commented Oct 14, 2016 •

edited

Loading

rreusser commented Oct 14, 2016 •

edited

Loading

etpinard Oct 17, 2016 •

edited

Loading

rreusser Oct 17, 2016 •

edited

Loading

rreusser Oct 17, 2016 •

edited

Loading

etpinard Oct 17, 2016 •

edited

Loading