Improved animation merging for layout and traces

[rreusser]

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]

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

1. It copies the input frame with extendDeepNoArrays so that the original frame definitions don't get mangled.
2. It expands paths in the copied input. That is, a frame with {'line.color': 'red'} gets expanded into {line: {color: 'red'}}
3. 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]

thanks for reusing this method 🎉

[etpinard]

you're missing a few items here:

• images
• updatemenus
• sliders

[rreusser]

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]

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 ...

[rreusser]

Sounds good. 👍

[etpinard]

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

[etpinard]

Does

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

??

[rreusser]

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]}

[rreusser]

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.

[rreusser]

@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]

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: #1041 (comment)
• 1 question: #1041 (review)

[etpinard]

Great work 💃

[etpinard]

👍

[etpinard]

very nice 👍

[rreusser]

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]

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

1. It copies the input frame with extendDeepNoArrays so that the original frame definitions don't get mangled.
2. It expands paths in the copied input. That is, a frame with {'line.color': 'red'} gets expanded into {line: {color: 'red'}}
3. 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]

thanks for reusing this method 🎉

[etpinard]

you're missing a few items here:

• images
• updatemenus
• sliders

[rreusser]

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]

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 ...

[rreusser]

Sounds good. 👍

[etpinard]

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

[etpinard]

Does

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

??

[rreusser]

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]}

[rreusser]

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.

[rreusser]

@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]

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: #1041 (comment)
• 1 question: #1041 (review)

[etpinard]

Great work 💃

[etpinard]

👍

[etpinard]

very nice 👍

[rreusser]

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]

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

1. It copies the input frame with extendDeepNoArrays so that the original frame definitions don't get mangled.
2. It expands paths in the copied input. That is, a frame with {'line.color': 'red'} gets expanded into {line: {color: 'red'}}
3. 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]

thanks for reusing this method 🎉

[etpinard]

you're missing a few items here:

• images
• updatemenus
• sliders

[rreusser]

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]

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 ...

[rreusser]

Sounds good. 👍

[etpinard]

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

[etpinard]

Does

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

??

[rreusser]

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]}

[rreusser]

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.

[rreusser]

@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]

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: #1041 (comment)
• 1 question: #1041 (review)

[etpinard]

Great work 💃

[etpinard]

👍

[etpinard]

very nice 👍

[rreusser]

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]

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

1. It copies the input frame with extendDeepNoArrays so that the original frame definitions don't get mangled.
2. It expands paths in the copied input. That is, a frame with {'line.color': 'red'} gets expanded into {line: {color: 'red'}}
3. 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]

thanks for reusing this method 🎉

[etpinard]

you're missing a few items here:

• images
• updatemenus
• sliders

[rreusser]

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]

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 ...

[rreusser]

Sounds good. 👍

[etpinard]

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

[etpinard]

Does

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

??

[rreusser]

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]}

[rreusser]

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.

[rreusser]

@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]

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: #1041 (comment)
• 1 question: #1041 (review)

[etpinard]

Great work 💃

[etpinard]

👍

[etpinard]

very nice 👍

[rreusser]

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]

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

1. It copies the input frame with extendDeepNoArrays so that the original frame definitions don't get mangled.
2. It expands paths in the copied input. That is, a frame with {'line.color': 'red'} gets expanded into {line: {color: 'red'}}
3. 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]

thanks for reusing this method 🎉

[etpinard]

you're missing a few items here:

• images
• updatemenus
• sliders

[rreusser]

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]

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 ...

[rreusser]

Sounds good. 👍

[etpinard]

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

[etpinard]

Does

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

??

[rreusser]

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]}

[rreusser]

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.

[rreusser]

@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]

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: #1041 (comment)
• 1 question: #1041 (review)

[etpinard]

Great work 💃

[etpinard]

👍

[etpinard]

very nice 👍

[rreusser]

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]

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

1. It copies the input frame with extendDeepNoArrays so that the original frame definitions don't get mangled.
2. It expands paths in the copied input. That is, a frame with {'line.color': 'red'} gets expanded into {line: {color: 'red'}}
3. 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]

thanks for reusing this method 🎉

[etpinard]

you're missing a few items here:

• images
• updatemenus
• sliders

[rreusser]

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]

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 ...

[rreusser]

Sounds good. 👍

[etpinard]

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

[etpinard]

Does

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

??

[rreusser]

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]}

[rreusser]

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.

[rreusser]

@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]

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: #1041 (comment)
• 1 question: #1041 (review)

[etpinard]

Great work 💃

[etpinard]

👍

[etpinard]

very nice 👍

[rreusser]

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]

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

1. It copies the input frame with extendDeepNoArrays so that the original frame definitions don't get mangled.
2. It expands paths in the copied input. That is, a frame with {'line.color': 'red'} gets expanded into {line: {color: 'red'}}
3. 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]

thanks for reusing this method 🎉

[etpinard]

you're missing a few items here:

• images
• updatemenus
• sliders

[rreusser]

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]

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 ...

[rreusser]

Sounds good. 👍

[etpinard]

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

[etpinard]

Does

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

??

[rreusser]

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]}

[rreusser]

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.

[rreusser]

@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]

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: #1041 (comment)
• 1 question: #1041 (review)

[etpinard]

Great work 💃

[etpinard]

👍

[etpinard]

very nice 👍

[rreusser]

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]

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

1. It copies the input frame with extendDeepNoArrays so that the original frame definitions don't get mangled.
2. It expands paths in the copied input. That is, a frame with {'line.color': 'red'} gets expanded into {line: {color: 'red'}}
3. 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]

thanks for reusing this method 🎉

[etpinard]

you're missing a few items here:

• images
• updatemenus
• sliders

[rreusser]

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]

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 ...

[rreusser]

Sounds good. 👍

[etpinard]

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

[etpinard]

Does

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

??

[rreusser]

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]}

[rreusser]

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.

[rreusser]

@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]

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: #1041 (comment)
• 1 question: #1041 (review)

[etpinard]

Great work 💃

[etpinard]

👍

[etpinard]

very nice 👍