Skip to content

Improved animation merging for layout and traces#1041

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rreusser merged 9 commits into from
Oct 17, 2016
Merged

Improved animation merging for layout and traces #1041

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0e86edd
Add failing test fro transition transform
rreusser Oct 14, 2016
a0da51d
Mostly factor out trace and layout merge logic
rreusser Oct 14, 2016
b612ac3
Methods for extendTrace and extendLayout
rreusser Oct 14, 2016
8d35b71
Lock down and document the unimplemented case in expandObjectPaths
rreusser Oct 14, 2016
c93c18f
Verify function of animated transforms
rreusser Oct 14, 2016
33e23db
Tweak trivial things to get the test to pass
rreusser Oct 14, 2016
511de3d
Delete best guess at failing test
rreusser Oct 14, 2016
adee9b2
Add missing layout container arrays
rreusser Oct 17, 2016
e085a53
Add additional docs on expandObjectPaths subtleties
rreusser Oct 17, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 61 additions & 8 deletions src/lib/index.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -589,18 +589,36 @@ lib.objectFromPath = function(path, value) {
/**
* Iterate through an object in-place, converting dotted properties to objects.
*
* @example
* lib.expandObjectPaths({'nested.test.path': 'value'});
* // returns { nested: { test: {path: 'value'}}}
* Examples:
*
* lib.expandObjectPaths({'nested.test.path': 'value'});
* => { nested: { test: {path: 'value'}}}
*
* It also handles array notation, e.g.:
*
* lib.expandObjectPaths({'foo[1].bar': 'value'});
* => { foo: [null, {bar: value}] }
*
* It handles merges the results when two properties are specified in parallel:
*
* lib.expandObjectPaths({'foo[1].bar': 10, 'foo[0].bar': 20});
* => { foo: [{bar: 10}, {bar: 20}] }
*
* It does NOT, however, merge mulitple mutliply-nested arrays::
*
* lib.expandObjectPaths({'marker[1].range[1]': 5, 'marker[1].range[0]': 4})
* => { marker: [null, {range: 4}] }
*/

// Store this to avoid recompiling regex on every prop since this may happen many
// many times for animations.
// TODO: Premature optimization? Remove?
var dottedPropertyRegex = /^([^\.]*)\../;
// Store this to avoid recompiling regex on *every* prop since this may happen many
// many times for animations. Could maybe be inside the function. Not sure about
// 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) {

@etpinardetpinardOct 17, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for reusing this method 🎉

var match, key, prop, datum;
var match, key, prop, datum, idx, dest, trailingPath;
if(typeof data === 'object' && !Array.isArray(data)) {
for(key in data) {
if(data.hasOwnProperty(key)) {
Expand All@@ -611,12 +629,47 @@ lib.expandObjectPaths = function(data) {
delete data[key];

data[prop] = lib.extendDeepNoArrays(data[prop] || {}, lib.objectFromPath(key, lib.expandObjectPaths(datum))[prop]);
} else if((match = key.match(indexedPropertyRegex))) {
datum = data[key];

prop = match[1];
idx = parseInt(match[2]);

delete data[key];

data[prop] = data[prop] || [];

if(match[3] === '.') {
// This is the case where theere are subsequent properties into which
// we must recurse, e.g. transforms[0].value
trailingPath = match[4];
dest = data[prop][idx] = data[prop][idx] || {};

// NB: Extend deep no arrays prevents this from working on multiple
// nested properties in the same object, e.g.
//
// {
// foo[0].bar[1].range
// foo[0].bar[0].range
// }
//
// In this case, the extendDeepNoArrays will overwrite one array with
// the other, so that both properties *will not* be present in the
// result. Fixing this would require a more intelligent tracking
// of changes and merging than extendDeepNoArrays currently accomplishes.
lib.extendDeepNoArrays(dest, lib.objectFromPath(trailingPath, lib.expandObjectPaths(datum)));
} else {
// This is the case where this property is the end of the line,
// e.g. xaxis.range[0]
data[prop][idx] = lib.expandObjectPaths(datum);
}
} else {
data[key] = lib.expandObjectPaths(data[key]);
}
}
}
}

return data;
};

Expand Down
106 changes: 89 additions & 17 deletions src/plots/plots.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1299,7 +1299,7 @@ plots.modifyFrames = function(gd, operations) {
*/
plots.computeFrame = function(gd, frameName) {
var frameLookup = gd._transitionData._frameHash;
var i, traceIndices, traceIndex, expandedObj, destIndex, copy;
var i, traceIndices, traceIndex, destIndex;

var framePtr = frameLookup[frameName];

Expand All@@ -1326,9 +1326,7 @@ plots.computeFrame = function(gd, frameName) {
// Merge, starting with the last and ending with the desired frame:
while((framePtr = frameStack.pop())) {
if(framePtr.layout) {
copy = Lib.extendDeepNoArrays({}, framePtr.layout);
expandedObj = Lib.expandObjectPaths(copy);
result.layout = Lib.extendDeepNoArrays(result.layout || {}, expandedObj);
result.layout = plots.extendLayout(result.layout, framePtr.layout);
}

if(framePtr.data) {
Expand DownExpand Up@@ -1363,16 +1361,99 @@ plots.computeFrame = function(gd, frameName) {
result.traces[destIndex] = traceIndex;
}

copy = Lib.extendDeepNoArrays({}, framePtr.data[i]);
expandedObj = Lib.expandObjectPaths(copy);
result.data[destIndex] = Lib.extendDeepNoArrays(result.data[destIndex] || {}, expandedObj);
result.data[destIndex] = plots.extendTrace(result.data[destIndex], framePtr.data[i]);
}
}
}

return result;
};

/**
* Extend an object, treating container arrays very differently by extracting
* their contents and merging them separately.
*
* This exists so that we can extendDeepNoArrays and avoid stepping into data
* arrays without knowledge of the plot schema, but so that we may also manually
* recurse into known container arrays, such as transforms.
*
* See extendTrace and extendLayout below for usage.
*/
plots.extendObjectWithContainers = function(dest, src, containerPaths) {
var containerProp, containerVal, i, j, srcProp, destProp, srcContainer, destContainer;
var copy = Lib.extendDeepNoArrays({}, src || {});
var expandedObj = Lib.expandObjectPaths(copy);
var containerObj = {};

// Step through and extract any container properties. Otherwise extendDeepNoArrays
// will clobber any existing properties with an empty array and then supplyDefaults
// will reset everything to defaults.
if(containerPaths && containerPaths.length) {
for(i = 0; i < containerPaths.length; i++) {
containerProp = Lib.nestedProperty(expandedObj, containerPaths[i]);
containerVal = containerProp.get();
containerProp.set(null);
Lib.nestedProperty(containerObj, containerPaths[i]).set(containerVal);
}
}

dest = Lib.extendDeepNoArrays(dest || {}, expandedObj);

if(containerPaths && containerPaths.length) {
for(i = 0; i < containerPaths.length; i++) {
srcProp = Lib.nestedProperty(containerObj, containerPaths[i]);
srcContainer = srcProp.get();

if(!srcContainer) continue;

destProp = Lib.nestedProperty(dest, containerPaths[i]);

destContainer = destProp.get();
if(!Array.isArray(destContainer)) {
destContainer = [];
destProp.set(destContainer);
}

for(j = 0; j < srcContainer.length; j++) {
destContainer[j] = plots.extendObjectWithContainers(destContainer[j], srcContainer[j]);
}
}
}

return dest;
};

/*
* Extend a trace definition. This method:
*
* 1. directly transfers any array references
* 2. manually recurses into container arrays like transforms
*
* The result is the original object reference with the new contents merged in.
*/
plots.extendTrace = function(destTrace, srcTrace) {
return plots.extendObjectWithContainers(destTrace, srcTrace, ['transforms']);

@etpinardetpinardOct 17, 2016•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

👍

};

/*
* Extend a layout definition. This method:
*
* 1. directly transfers any array references (not critically important for
* layout since there aren't really data arrays)
* 2. manually recurses into container arrays like annotations
*
* The result is the original object reference with the new contents merged in.
*/
plots.extendLayout = function(destLayout, srcLayout) {
return plots.extendObjectWithContainers(destLayout, srcLayout, [
'annotations',
'shapes',
'images',
'sliders',
'updatemenus'

@etpinardetpinardOct 17, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

]);
};

/**
* Transition to a set of new data and layout properties
*
Expand DownExpand Up@@ -1411,16 +1492,7 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)

transitionedTraces.push(traceIdx);

// This is a multi-step process. First clone w/o arrays so that
// we're not modifying the original:
var update = Lib.extendDeepNoArrays({}, data[i]);

// Then expand object paths since we don't obey object-overwrite
// semantics here:
update = Lib.expandObjectPaths(update);

// Finally apply the update (without copying arrays, of course):
Lib.extendDeepNoArrays(gd.data[traceIndices[i]], update);
gd.data[traceIndices[i]] = plots.extendTrace(gd.data[traceIndices[i]], data[i]);
}

// Follow the same procedure. Clone it so we don't mangle the input, then
Expand Down
2 changes: 1 addition & 1 deletion test/jasmine/tests/animate_test.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -84,7 +84,7 @@ describe('Test animate API', function() {
});

Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(function() {
Plotly.addFrames(gd, mockCopy.frames);
return Plotly.addFrames(gd, mockCopy.frames);
}).then(done);
});

Expand Down
84 changes: 84 additions & 0 deletions test/jasmine/tests/lib_test.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -537,6 +537,90 @@ describe('Test lib.js:', function() {
expect(input).toBe(expanded);
expect(origArray).toBe(newArray);
});

it('expands bracketed array notation', function() {
var input = {'marker[1]': {color: 'red'}};
var expected = {marker: [undefined, {color: 'red'}]};
expect(Lib.expandObjectPaths(input)).toEqual(expected);
});

it('expands nested arrays', function() {
var input = {'marker[1].range[1]': 5};
var expected = {marker: [undefined, {range: [undefined, 5]}]};
var computed = Lib.expandObjectPaths(input);
expect(computed).toEqual(expected);
});

it('expands bracketed array with more nested attributes', function() {
var input = {'marker[1]': {'color.alpha': 2}};
var expected = {marker: [undefined, {color: {alpha: 2}}]};
var computed = Lib.expandObjectPaths(input);
expect(computed).toEqual(expected);
});

it('expands bracketed array notation without further nesting', function() {
var input = {'marker[1]': 8};
var expected = {marker: [undefined, 8]};
var computed = Lib.expandObjectPaths(input);
expect(computed).toEqual(expected);
});

it('expands bracketed array notation with further nesting', function() {
var input = {'marker[1].size': 8};
var expected = {marker: [undefined, {size: 8}]};
var computed = Lib.expandObjectPaths(input);
expect(computed).toEqual(expected);
});

it('expands bracketed array notation with further nesting', function() {
var input = {'marker[1].size.magnitude': 8};
var expected = {marker: [undefined, {size: {magnitude: 8}}]};
var computed = Lib.expandObjectPaths(input);
expect(computed).toEqual(expected);
});

it('combines changes with single array nesting', function() {
var input = {'marker[1].foo': 5, 'marker[0].foo': 4};
var expected = {marker: [{foo: 4}, {foo: 5}]};
var computed = Lib.expandObjectPaths(input);
expect(computed).toEqual(expected);
});

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

@etpinardetpinardOct 17, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@etpinardetpinardOct 17, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does

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

??

@rreusserrreusserOct 17, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rreusserrreusserOct 17, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rreusserrreusserOct 17, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// not data arrays but yes on arrays that were previously expanded. This is a bit
// tricky to get to work just right and currently doesn't have any known use since
// container arrays are not multiply nested.
//
// Additional notes on what works or what doesn't work. This case does *not* work
// because the two nested arrays that would result from the expansion need to be
// deep merged.
//
// Lib.expandObjectPaths({'marker.range[0]': 5, 'marker.range[1]': 2})
//
// // => {marker: {range: [null, 2]}}
//
// This case *does* work becuase the array merging does not require a deep extend:
//
// Lib.expandObjectPaths({'range[0]': 5, 'range[1]': 2}
//
// // => {range: [5, 2]}
//
// Finally note that this case works fine becuase there's no merge necessary:
//
// Lib.expandObjectPaths({'marker.range[1]': 2})
//
// // => {marker: {range: [null, 2]}}
//

@etpinardetpinardOct 17, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice 👍

/*
it('combines changes', function() {
var input = {'marker[1].range[1]': 5, 'marker[1].range[0]': 4};
var expected = {marker: [undefined, {range: [4, 5]}]};
var computed = Lib.expandObjectPaths(input);
expect(computed).toEqual(expected);
});
*/
});

describe('coerce', function() {
Expand Down
35 changes: 35 additions & 0 deletions test/jasmine/tests/transition_test.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -65,6 +65,41 @@ function runTests(transitionDuration) {
}).catch(fail).then(done);
});

it('transitions a transform', function(done) {
Plotly.restyle(gd, {
'transforms[0]': {
enabled: true,
type: 'filter',
operation: '<',
filtersrc: 'x',
value: 10
}
}, [0]).then(function() {
expect(gd._fullData[0].transforms).toEqual([{
enabled: true,
type: 'filter',
operation: '<',
filtersrc: 'x',
value: 10
}]);

return Plots.transition(gd, [{
'transforms[0].operation': '>'
}], null, [0],
{redraw: true, duration: transitionDuration},
{duration: transitionDuration, easing: 'cubic-in-out'}
);
}).then(function() {
expect(gd._fullData[0].transforms).toEqual([{
enabled: true,
type: 'filter',
operation: '>',
filtersrc: 'x',
value: 10
}]);
}).catch(fail).then(done);
});

// This doesn't really test anything that the above tests don't cover, but it combines
// the behavior and attempts to ensure chaining and events happen in the correct order.
it('transitions may be chained', function(done) {
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Improved animation merging for layout and traces by rreusser · Pull Request #1041 · plotly/plotly.js · GitHub
Skip to content
Improved animation merging for layout and traces#1041

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

rreusser
merged 9 commits into

from

Oct 17, 2016
 Merged


Improved animation merging for layout and traces
#1041
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


9 commits


Select commit
Hold shift + click to select a range
0e86edd

Add failing test fro transition transform


rreusser Oct 14, 2016
a0da51d

Mostly factor out trace and layout merge logic


rreusser Oct 14, 2016
b612ac3

Methods for extendTrace and extendLayout


rreusser Oct 14, 2016
8d35b71

Lock down and document the unimplemented case in expandObjectPaths


rreusser Oct 14, 2016
c93c18f

Verify function of animated transforms


rreusser Oct 14, 2016
33e23db

Tweak trivial things to get the test to pass


rreusser Oct 14, 2016
511de3d

Delete best guess at failing test


rreusser Oct 14, 2016
adee9b2

Add missing layout container arrays


rreusser Oct 17, 2016
e085a53

Add additional docs on expandObjectPaths subtleties


rreusser Oct 17, 2016

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


69 changes: 61 additions & 8 deletions
src/lib/index.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -589,18 +589,36 @@ lib.objectFromPath = function(path, value) {
/**
 * Iterate through an object in-place, converting dotted properties to objects.
 *
 * @example
 * lib.expandObjectPaths({'nested.test.path': 'value'});
 * // returns { nested: { test: {path: 'value'}}}
 * Examples:
 *
 * lib.expandObjectPaths({'nested.test.path': 'value'});
 * => { nested: { test: {path: 'value'}}}
 *
 * It also handles array notation, e.g.:
 *
 * lib.expandObjectPaths({'foo[1].bar': 'value'});
 * => { foo: [null, {bar: value}] }
 *
 * It handles merges the results when two properties are specified in parallel:
 *
 * lib.expandObjectPaths({'foo[1].bar': 10, 'foo[0].bar': 20});
 * => { foo: [{bar: 10}, {bar: 20}] }
 *
 * It does NOT, however, merge mulitple mutliply-nested arrays::
 *
 * lib.expandObjectPaths({'marker[1].range[1]': 5, 'marker[1].range[0]': 4})
 * => { marker: [null, {range: 4}] }
 */

// Store this to avoid recompiling regex on every prop since this may happen many
// many times for animations.
// TODO: Premature optimization? Remove?
var dottedPropertyRegex = /^([^\.]*)\../;
// Store this to avoid recompiling regex on *every* prop since this may happen many
// many times for animations. Could maybe be inside the function. Not sure about
// 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) {
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for reusing this method 🎉
 var match, key, prop, datum;
 var match, key, prop, datum, idx, dest, trailingPath;
 if(typeof data === 'object' && !Array.isArray(data)) {
 for(key in data) {
 if(data.hasOwnProperty(key)) {
Expand All@@ -611,12 +629,47 @@ lib.expandObjectPaths = function(data) {
 delete data[key];

 data[prop] = lib.extendDeepNoArrays(data[prop] || {}, lib.objectFromPath(key, lib.expandObjectPaths(datum))[prop]);
 } else if((match = key.match(indexedPropertyRegex))) {
 datum = data[key];

 prop = match[1];
 idx = parseInt(match[2]);

 delete data[key];

 data[prop] = data[prop] || [];

 if(match[3] === '.') {
 // This is the case where theere are subsequent properties into which
 // we must recurse, e.g. transforms[0].value
 trailingPath = match[4];
 dest = data[prop][idx] = data[prop][idx] || {};

 // NB: Extend deep no arrays prevents this from working on multiple
 // nested properties in the same object, e.g.
 //
 // {
 // foo[0].bar[1].range
 // foo[0].bar[0].range
 // }
 //
 // In this case, the extendDeepNoArrays will overwrite one array with
 // the other, so that both properties *will not* be present in the
 // result. Fixing this would require a more intelligent tracking
 // of changes and merging than extendDeepNoArrays currently accomplishes.
 lib.extendDeepNoArrays(dest, lib.objectFromPath(trailingPath, lib.expandObjectPaths(datum)));
 } else {
 // This is the case where this property is the end of the line,
 // e.g. xaxis.range[0]
 data[prop][idx] = lib.expandObjectPaths(datum);
 }
 } else {
 data[key] = lib.expandObjectPaths(data[key]);
 }
 }
 }
 }

 return data;
};

Expand Down

106 changes: 89 additions & 17 deletions
src/plots/plots.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1299,7 +1299,7 @@ plots.modifyFrames = function(gd, operations) {
 */
plots.computeFrame = function(gd, frameName) {
 var frameLookup = gd._transitionData._frameHash;
 var i, traceIndices, traceIndex, expandedObj, destIndex, copy;
 var i, traceIndices, traceIndex, destIndex;

 var framePtr = frameLookup[frameName];

Expand All@@ -1326,9 +1326,7 @@ plots.computeFrame = function(gd, frameName) {
 // Merge, starting with the last and ending with the desired frame:
 while((framePtr = frameStack.pop())) {
 if(framePtr.layout) {
 copy = Lib.extendDeepNoArrays({}, framePtr.layout);
 expandedObj = Lib.expandObjectPaths(copy);
 result.layout = Lib.extendDeepNoArrays(result.layout || {}, expandedObj);
 result.layout = plots.extendLayout(result.layout, framePtr.layout);
 }

 if(framePtr.data) {
Expand DownExpand Up@@ -1363,16 +1361,99 @@ plots.computeFrame = function(gd, frameName) {
 result.traces[destIndex] = traceIndex;
 }

 copy = Lib.extendDeepNoArrays({}, framePtr.data[i]);
 expandedObj = Lib.expandObjectPaths(copy);
 result.data[destIndex] = Lib.extendDeepNoArrays(result.data[destIndex] || {}, expandedObj);
 result.data[destIndex] = plots.extendTrace(result.data[destIndex], framePtr.data[i]);
 }
 }
 }

 return result;
};

/**
 * Extend an object, treating container arrays very differently by extracting
 * their contents and merging them separately.
 *
 * This exists so that we can extendDeepNoArrays and avoid stepping into data
 * arrays without knowledge of the plot schema, but so that we may also manually
 * recurse into known container arrays, such as transforms.
 *
 * See extendTrace and extendLayout below for usage.
 */
plots.extendObjectWithContainers = function(dest, src, containerPaths) {
 var containerProp, containerVal, i, j, srcProp, destProp, srcContainer, destContainer;
 var copy = Lib.extendDeepNoArrays({}, src || {});
 var expandedObj = Lib.expandObjectPaths(copy);
 var containerObj = {};

 // Step through and extract any container properties. Otherwise extendDeepNoArrays
 // will clobber any existing properties with an empty array and then supplyDefaults
 // will reset everything to defaults.
 if(containerPaths && containerPaths.length) {
 for(i = 0; i < containerPaths.length; i++) {
 containerProp = Lib.nestedProperty(expandedObj, containerPaths[i]);
 containerVal = containerProp.get();
 containerProp.set(null);
 Lib.nestedProperty(containerObj, containerPaths[i]).set(containerVal);
 }
 }

 dest = Lib.extendDeepNoArrays(dest || {}, expandedObj);

 if(containerPaths && containerPaths.length) {
 for(i = 0; i < containerPaths.length; i++) {
 srcProp = Lib.nestedProperty(containerObj, containerPaths[i]);
 srcContainer = srcProp.get();

 if(!srcContainer) continue;

 destProp = Lib.nestedProperty(dest, containerPaths[i]);

 destContainer = destProp.get();
 if(!Array.isArray(destContainer)) {
 destContainer = [];
 destProp.set(destContainer);
 }

 for(j = 0; j < srcContainer.length; j++) {
 destContainer[j] = plots.extendObjectWithContainers(destContainer[j], srcContainer[j]);
 }
 }
 }

 return dest;
};

/*
 * Extend a trace definition. This method:
 *
 * 1. directly transfers any array references
 * 2. manually recurses into container arrays like transforms
 *
 * The result is the original object reference with the new contents merged in.
 */
plots.extendTrace = function(destTrace, srcTrace) {
 return plots.extendObjectWithContainers(destTrace, srcTrace, ['transforms']);
@etpinardetpinardOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/*
 * Extend a layout definition. This method:
 *
 * 1. directly transfers any array references (not critically important for
 * layout since there aren't really data arrays)
 * 2. manually recurses into container arrays like annotations
 *
 * The result is the original object reference with the new contents merged in.
 */
plots.extendLayout = function(destLayout, srcLayout) {
 return plots.extendObjectWithContainers(destLayout, srcLayout, [
 'annotations',
 'shapes',
 'images',
 'sliders',
 'updatemenus'
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
 ]);
};

/**
 * Transition to a set of new data and layout properties
 *
Expand DownExpand Up@@ -1411,16 +1492,7 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)

 transitionedTraces.push(traceIdx);

 // This is a multi-step process. First clone w/o arrays so that
 // we're not modifying the original:
 var update = Lib.extendDeepNoArrays({}, data[i]);

 // Then expand object paths since we don't obey object-overwrite
 // semantics here:
 update = Lib.expandObjectPaths(update);

 // Finally apply the update (without copying arrays, of course):
 Lib.extendDeepNoArrays(gd.data[traceIndices[i]], update);
 gd.data[traceIndices[i]] = plots.extendTrace(gd.data[traceIndices[i]], data[i]);
 }

 // Follow the same procedure. Clone it so we don't mangle the input, then
Expand Down

2 changes: 1 addition & 1 deletion
test/jasmine/tests/animate_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -84,7 +84,7 @@ describe('Test animate API', function() {
 });

 Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(function() {
 Plotly.addFrames(gd, mockCopy.frames);
return Plotly.addFrames(gd, mockCopy.frames);
 }).then(done);
 });

Expand Down

84 changes: 84 additions & 0 deletions
test/jasmine/tests/lib_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -537,6 +537,90 @@ describe('Test lib.js:', function() {
 expect(input).toBe(expanded);
 expect(origArray).toBe(newArray);
 });

 it('expands bracketed array notation', function() {
 var input = {'marker[1]': {color: 'red'}};
 var expected = {marker: [undefined, {color: 'red'}]};
 expect(Lib.expandObjectPaths(input)).toEqual(expected);
 });

 it('expands nested arrays', function() {
 var input = {'marker[1].range[1]': 5};
 var expected = {marker: [undefined, {range: [undefined, 5]}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array with more nested attributes', function() {
 var input = {'marker[1]': {'color.alpha': 2}};
 var expected = {marker: [undefined, {color: {alpha: 2}}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation without further nesting', function() {
 var input = {'marker[1]': 8};
 var expected = {marker: [undefined, 8]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation with further nesting', function() {
 var input = {'marker[1].size': 8};
 var expected = {marker: [undefined, {size: 8}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation with further nesting', function() {
 var input = {'marker[1].size.magnitude': 8};
 var expected = {marker: [undefined, {size: {magnitude: 8}}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('combines changes with single array nesting', function() {
 var input = {'marker[1].foo': 5, 'marker[0].foo': 4};
 var expected = {marker: [{foo: 4}, {foo: 5}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 // 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
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those arrays are declared as info_array e.g. pie domain.
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does
varinput={'marker[1].range': [4,5]};Lib.expandObjectPaths(input);// => {marker: [undefined, {range: [4, 5]}]}
??
@rreusserrreusserOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]}
@rreusserrreusserOct 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@rreusserrreusserOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.
 // not data arrays but yes on arrays that were previously expanded. This is a bit
 // tricky to get to work just right and currently doesn't have any known use since
 // container arrays are not multiply nested.
 //
 // Additional notes on what works or what doesn't work. This case does *not* work
 // because the two nested arrays that would result from the expansion need to be
 // deep merged.
 //
 // Lib.expandObjectPaths({'marker.range[0]': 5, 'marker.range[1]': 2})
 //
 // // => {marker: {range: [null, 2]}}
 //
 // This case *does* work becuase the array merging does not require a deep extend:
 //
 // Lib.expandObjectPaths({'range[0]': 5, 'range[1]': 2}
 //
 // // => {range: [5, 2]}
 //
 // Finally note that this case works fine becuase there's no merge necessary:
 //
 // Lib.expandObjectPaths({'marker.range[1]': 2})
 //
 // // => {marker: {range: [null, 2]}}
 //
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice 👍
 /*
 it('combines changes', function() {
 var input = {'marker[1].range[1]': 5, 'marker[1].range[0]': 4};
 var expected = {marker: [undefined, {range: [4, 5]}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });
 */
 });

 describe('coerce', function() {
Expand Down

35 changes: 35 additions & 0 deletions
test/jasmine/tests/transition_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -65,6 +65,41 @@ function runTests(transitionDuration) {
 }).catch(fail).then(done);
 });

 it('transitions a transform', function(done) {
 Plotly.restyle(gd, {
 'transforms[0]': {
 enabled: true,
 type: 'filter',
 operation: '<',
 filtersrc: 'x',
 value: 10
 }
 }, [0]).then(function() {
 expect(gd._fullData[0].transforms).toEqual([{
 enabled: true,
 type: 'filter',
 operation: '<',
 filtersrc: 'x',
 value: 10
 }]);

 return Plots.transition(gd, [{
 'transforms[0].operation': '>'
 }], null, [0],
 {redraw: true, duration: transitionDuration},
 {duration: transitionDuration, easing: 'cubic-in-out'}
 );
 }).then(function() {
 expect(gd._fullData[0].transforms).toEqual([{
 enabled: true,
 type: 'filter',
 operation: '>',
 filtersrc: 'x',
 value: 10
 }]);
 }).catch(fail).then(done);
 });

 // This doesn't really test anything that the above tests don't cover, but it combines
 // the behavior and attempts to ensure chaining and events happen in the correct order.
 it('transitions may be chained', function(done) {
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Improved animation merging for layout and traces by rreusser · Pull Request #1041 · plotly/plotly.js · GitHub
Skip to content
Improved animation merging for layout and traces#1041

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

rreusser
merged 9 commits into

from

Oct 17, 2016
 Merged


Improved animation merging for layout and traces
#1041
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


9 commits


Select commit
Hold shift + click to select a range
0e86edd

Add failing test fro transition transform


rreusser Oct 14, 2016
a0da51d

Mostly factor out trace and layout merge logic


rreusser Oct 14, 2016
b612ac3

Methods for extendTrace and extendLayout


rreusser Oct 14, 2016
8d35b71

Lock down and document the unimplemented case in expandObjectPaths


rreusser Oct 14, 2016
c93c18f

Verify function of animated transforms


rreusser Oct 14, 2016
33e23db

Tweak trivial things to get the test to pass


rreusser Oct 14, 2016
511de3d

Delete best guess at failing test


rreusser Oct 14, 2016
adee9b2

Add missing layout container arrays


rreusser Oct 17, 2016
e085a53

Add additional docs on expandObjectPaths subtleties


rreusser Oct 17, 2016

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


69 changes: 61 additions & 8 deletions
src/lib/index.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -589,18 +589,36 @@ lib.objectFromPath = function(path, value) {
/**
 * Iterate through an object in-place, converting dotted properties to objects.
 *
 * @example
 * lib.expandObjectPaths({'nested.test.path': 'value'});
 * // returns { nested: { test: {path: 'value'}}}
 * Examples:
 *
 * lib.expandObjectPaths({'nested.test.path': 'value'});
 * => { nested: { test: {path: 'value'}}}
 *
 * It also handles array notation, e.g.:
 *
 * lib.expandObjectPaths({'foo[1].bar': 'value'});
 * => { foo: [null, {bar: value}] }
 *
 * It handles merges the results when two properties are specified in parallel:
 *
 * lib.expandObjectPaths({'foo[1].bar': 10, 'foo[0].bar': 20});
 * => { foo: [{bar: 10}, {bar: 20}] }
 *
 * It does NOT, however, merge mulitple mutliply-nested arrays::
 *
 * lib.expandObjectPaths({'marker[1].range[1]': 5, 'marker[1].range[0]': 4})
 * => { marker: [null, {range: 4}] }
 */

// Store this to avoid recompiling regex on every prop since this may happen many
// many times for animations.
// TODO: Premature optimization? Remove?
var dottedPropertyRegex = /^([^\.]*)\../;
// Store this to avoid recompiling regex on *every* prop since this may happen many
// many times for animations. Could maybe be inside the function. Not sure about
// 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) {
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for reusing this method 🎉
 var match, key, prop, datum;
 var match, key, prop, datum, idx, dest, trailingPath;
 if(typeof data === 'object' && !Array.isArray(data)) {
 for(key in data) {
 if(data.hasOwnProperty(key)) {
Expand All@@ -611,12 +629,47 @@ lib.expandObjectPaths = function(data) {
 delete data[key];

 data[prop] = lib.extendDeepNoArrays(data[prop] || {}, lib.objectFromPath(key, lib.expandObjectPaths(datum))[prop]);
 } else if((match = key.match(indexedPropertyRegex))) {
 datum = data[key];

 prop = match[1];
 idx = parseInt(match[2]);

 delete data[key];

 data[prop] = data[prop] || [];

 if(match[3] === '.') {
 // This is the case where theere are subsequent properties into which
 // we must recurse, e.g. transforms[0].value
 trailingPath = match[4];
 dest = data[prop][idx] = data[prop][idx] || {};

 // NB: Extend deep no arrays prevents this from working on multiple
 // nested properties in the same object, e.g.
 //
 // {
 // foo[0].bar[1].range
 // foo[0].bar[0].range
 // }
 //
 // In this case, the extendDeepNoArrays will overwrite one array with
 // the other, so that both properties *will not* be present in the
 // result. Fixing this would require a more intelligent tracking
 // of changes and merging than extendDeepNoArrays currently accomplishes.
 lib.extendDeepNoArrays(dest, lib.objectFromPath(trailingPath, lib.expandObjectPaths(datum)));
 } else {
 // This is the case where this property is the end of the line,
 // e.g. xaxis.range[0]
 data[prop][idx] = lib.expandObjectPaths(datum);
 }
 } else {
 data[key] = lib.expandObjectPaths(data[key]);
 }
 }
 }
 }

 return data;
};

Expand Down

106 changes: 89 additions & 17 deletions
src/plots/plots.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1299,7 +1299,7 @@ plots.modifyFrames = function(gd, operations) {
 */
plots.computeFrame = function(gd, frameName) {
 var frameLookup = gd._transitionData._frameHash;
 var i, traceIndices, traceIndex, expandedObj, destIndex, copy;
 var i, traceIndices, traceIndex, destIndex;

 var framePtr = frameLookup[frameName];

Expand All@@ -1326,9 +1326,7 @@ plots.computeFrame = function(gd, frameName) {
 // Merge, starting with the last and ending with the desired frame:
 while((framePtr = frameStack.pop())) {
 if(framePtr.layout) {
 copy = Lib.extendDeepNoArrays({}, framePtr.layout);
 expandedObj = Lib.expandObjectPaths(copy);
 result.layout = Lib.extendDeepNoArrays(result.layout || {}, expandedObj);
 result.layout = plots.extendLayout(result.layout, framePtr.layout);
 }

 if(framePtr.data) {
Expand DownExpand Up@@ -1363,16 +1361,99 @@ plots.computeFrame = function(gd, frameName) {
 result.traces[destIndex] = traceIndex;
 }

 copy = Lib.extendDeepNoArrays({}, framePtr.data[i]);
 expandedObj = Lib.expandObjectPaths(copy);
 result.data[destIndex] = Lib.extendDeepNoArrays(result.data[destIndex] || {}, expandedObj);
 result.data[destIndex] = plots.extendTrace(result.data[destIndex], framePtr.data[i]);
 }
 }
 }

 return result;
};

/**
 * Extend an object, treating container arrays very differently by extracting
 * their contents and merging them separately.
 *
 * This exists so that we can extendDeepNoArrays and avoid stepping into data
 * arrays without knowledge of the plot schema, but so that we may also manually
 * recurse into known container arrays, such as transforms.
 *
 * See extendTrace and extendLayout below for usage.
 */
plots.extendObjectWithContainers = function(dest, src, containerPaths) {
 var containerProp, containerVal, i, j, srcProp, destProp, srcContainer, destContainer;
 var copy = Lib.extendDeepNoArrays({}, src || {});
 var expandedObj = Lib.expandObjectPaths(copy);
 var containerObj = {};

 // Step through and extract any container properties. Otherwise extendDeepNoArrays
 // will clobber any existing properties with an empty array and then supplyDefaults
 // will reset everything to defaults.
 if(containerPaths && containerPaths.length) {
 for(i = 0; i < containerPaths.length; i++) {
 containerProp = Lib.nestedProperty(expandedObj, containerPaths[i]);
 containerVal = containerProp.get();
 containerProp.set(null);
 Lib.nestedProperty(containerObj, containerPaths[i]).set(containerVal);
 }
 }

 dest = Lib.extendDeepNoArrays(dest || {}, expandedObj);

 if(containerPaths && containerPaths.length) {
 for(i = 0; i < containerPaths.length; i++) {
 srcProp = Lib.nestedProperty(containerObj, containerPaths[i]);
 srcContainer = srcProp.get();

 if(!srcContainer) continue;

 destProp = Lib.nestedProperty(dest, containerPaths[i]);

 destContainer = destProp.get();
 if(!Array.isArray(destContainer)) {
 destContainer = [];
 destProp.set(destContainer);
 }

 for(j = 0; j < srcContainer.length; j++) {
 destContainer[j] = plots.extendObjectWithContainers(destContainer[j], srcContainer[j]);
 }
 }
 }

 return dest;
};

/*
 * Extend a trace definition. This method:
 *
 * 1. directly transfers any array references
 * 2. manually recurses into container arrays like transforms
 *
 * The result is the original object reference with the new contents merged in.
 */
plots.extendTrace = function(destTrace, srcTrace) {
 return plots.extendObjectWithContainers(destTrace, srcTrace, ['transforms']);
@etpinardetpinardOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/*
 * Extend a layout definition. This method:
 *
 * 1. directly transfers any array references (not critically important for
 * layout since there aren't really data arrays)
 * 2. manually recurses into container arrays like annotations
 *
 * The result is the original object reference with the new contents merged in.
 */
plots.extendLayout = function(destLayout, srcLayout) {
 return plots.extendObjectWithContainers(destLayout, srcLayout, [
 'annotations',
 'shapes',
 'images',
 'sliders',
 'updatemenus'
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
 ]);
};

/**
 * Transition to a set of new data and layout properties
 *
Expand DownExpand Up@@ -1411,16 +1492,7 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)

 transitionedTraces.push(traceIdx);

 // This is a multi-step process. First clone w/o arrays so that
 // we're not modifying the original:
 var update = Lib.extendDeepNoArrays({}, data[i]);

 // Then expand object paths since we don't obey object-overwrite
 // semantics here:
 update = Lib.expandObjectPaths(update);

 // Finally apply the update (without copying arrays, of course):
 Lib.extendDeepNoArrays(gd.data[traceIndices[i]], update);
 gd.data[traceIndices[i]] = plots.extendTrace(gd.data[traceIndices[i]], data[i]);
 }

 // Follow the same procedure. Clone it so we don't mangle the input, then
Expand Down

2 changes: 1 addition & 1 deletion
test/jasmine/tests/animate_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -84,7 +84,7 @@ describe('Test animate API', function() {
 });

 Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(function() {
 Plotly.addFrames(gd, mockCopy.frames);
return Plotly.addFrames(gd, mockCopy.frames);
 }).then(done);
 });

Expand Down

84 changes: 84 additions & 0 deletions
test/jasmine/tests/lib_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -537,6 +537,90 @@ describe('Test lib.js:', function() {
 expect(input).toBe(expanded);
 expect(origArray).toBe(newArray);
 });

 it('expands bracketed array notation', function() {
 var input = {'marker[1]': {color: 'red'}};
 var expected = {marker: [undefined, {color: 'red'}]};
 expect(Lib.expandObjectPaths(input)).toEqual(expected);
 });

 it('expands nested arrays', function() {
 var input = {'marker[1].range[1]': 5};
 var expected = {marker: [undefined, {range: [undefined, 5]}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array with more nested attributes', function() {
 var input = {'marker[1]': {'color.alpha': 2}};
 var expected = {marker: [undefined, {color: {alpha: 2}}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation without further nesting', function() {
 var input = {'marker[1]': 8};
 var expected = {marker: [undefined, 8]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation with further nesting', function() {
 var input = {'marker[1].size': 8};
 var expected = {marker: [undefined, {size: 8}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation with further nesting', function() {
 var input = {'marker[1].size.magnitude': 8};
 var expected = {marker: [undefined, {size: {magnitude: 8}}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('combines changes with single array nesting', function() {
 var input = {'marker[1].foo': 5, 'marker[0].foo': 4};
 var expected = {marker: [{foo: 4}, {foo: 5}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 // 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
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those arrays are declared as info_array e.g. pie domain.
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does
varinput={'marker[1].range': [4,5]};Lib.expandObjectPaths(input);// => {marker: [undefined, {range: [4, 5]}]}
??
@rreusserrreusserOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]}
@rreusserrreusserOct 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@rreusserrreusserOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.
 // not data arrays but yes on arrays that were previously expanded. This is a bit
 // tricky to get to work just right and currently doesn't have any known use since
 // container arrays are not multiply nested.
 //
 // Additional notes on what works or what doesn't work. This case does *not* work
 // because the two nested arrays that would result from the expansion need to be
 // deep merged.
 //
 // Lib.expandObjectPaths({'marker.range[0]': 5, 'marker.range[1]': 2})
 //
 // // => {marker: {range: [null, 2]}}
 //
 // This case *does* work becuase the array merging does not require a deep extend:
 //
 // Lib.expandObjectPaths({'range[0]': 5, 'range[1]': 2}
 //
 // // => {range: [5, 2]}
 //
 // Finally note that this case works fine becuase there's no merge necessary:
 //
 // Lib.expandObjectPaths({'marker.range[1]': 2})
 //
 // // => {marker: {range: [null, 2]}}
 //
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice 👍
 /*
 it('combines changes', function() {
 var input = {'marker[1].range[1]': 5, 'marker[1].range[0]': 4};
 var expected = {marker: [undefined, {range: [4, 5]}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });
 */
 });

 describe('coerce', function() {
Expand Down

35 changes: 35 additions & 0 deletions
test/jasmine/tests/transition_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -65,6 +65,41 @@ function runTests(transitionDuration) {
 }).catch(fail).then(done);
 });

 it('transitions a transform', function(done) {
 Plotly.restyle(gd, {
 'transforms[0]': {
 enabled: true,
 type: 'filter',
 operation: '<',
 filtersrc: 'x',
 value: 10
 }
 }, [0]).then(function() {
 expect(gd._fullData[0].transforms).toEqual([{
 enabled: true,
 type: 'filter',
 operation: '<',
 filtersrc: 'x',
 value: 10
 }]);

 return Plots.transition(gd, [{
 'transforms[0].operation': '>'
 }], null, [0],
 {redraw: true, duration: transitionDuration},
 {duration: transitionDuration, easing: 'cubic-in-out'}
 );
 }).then(function() {
 expect(gd._fullData[0].transforms).toEqual([{
 enabled: true,
 type: 'filter',
 operation: '>',
 filtersrc: 'x',
 value: 10
 }]);
 }).catch(fail).then(done);
 });

 // This doesn't really test anything that the above tests don't cover, but it combines
 // the behavior and attempts to ensure chaining and events happen in the correct order.
 it('transitions may be chained', function(done) {
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Improved animation merging for layout and traces by rreusser · Pull Request #1041 · plotly/plotly.js · GitHub
Skip to content
Improved animation merging for layout and traces#1041

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

rreusser
merged 9 commits into

from

Oct 17, 2016
 Merged


Improved animation merging for layout and traces
#1041
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


9 commits


Select commit
Hold shift + click to select a range
0e86edd

Add failing test fro transition transform


rreusser Oct 14, 2016
a0da51d

Mostly factor out trace and layout merge logic


rreusser Oct 14, 2016
b612ac3

Methods for extendTrace and extendLayout


rreusser Oct 14, 2016
8d35b71

Lock down and document the unimplemented case in expandObjectPaths


rreusser Oct 14, 2016
c93c18f

Verify function of animated transforms


rreusser Oct 14, 2016
33e23db

Tweak trivial things to get the test to pass


rreusser Oct 14, 2016
511de3d

Delete best guess at failing test


rreusser Oct 14, 2016
adee9b2

Add missing layout container arrays


rreusser Oct 17, 2016
e085a53

Add additional docs on expandObjectPaths subtleties


rreusser Oct 17, 2016

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


69 changes: 61 additions & 8 deletions
src/lib/index.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -589,18 +589,36 @@ lib.objectFromPath = function(path, value) {
/**
 * Iterate through an object in-place, converting dotted properties to objects.
 *
 * @example
 * lib.expandObjectPaths({'nested.test.path': 'value'});
 * // returns { nested: { test: {path: 'value'}}}
 * Examples:
 *
 * lib.expandObjectPaths({'nested.test.path': 'value'});
 * => { nested: { test: {path: 'value'}}}
 *
 * It also handles array notation, e.g.:
 *
 * lib.expandObjectPaths({'foo[1].bar': 'value'});
 * => { foo: [null, {bar: value}] }
 *
 * It handles merges the results when two properties are specified in parallel:
 *
 * lib.expandObjectPaths({'foo[1].bar': 10, 'foo[0].bar': 20});
 * => { foo: [{bar: 10}, {bar: 20}] }
 *
 * It does NOT, however, merge mulitple mutliply-nested arrays::
 *
 * lib.expandObjectPaths({'marker[1].range[1]': 5, 'marker[1].range[0]': 4})
 * => { marker: [null, {range: 4}] }
 */

// Store this to avoid recompiling regex on every prop since this may happen many
// many times for animations.
// TODO: Premature optimization? Remove?
var dottedPropertyRegex = /^([^\.]*)\../;
// Store this to avoid recompiling regex on *every* prop since this may happen many
// many times for animations. Could maybe be inside the function. Not sure about
// 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) {
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for reusing this method 🎉
 var match, key, prop, datum;
 var match, key, prop, datum, idx, dest, trailingPath;
 if(typeof data === 'object' && !Array.isArray(data)) {
 for(key in data) {
 if(data.hasOwnProperty(key)) {
Expand All@@ -611,12 +629,47 @@ lib.expandObjectPaths = function(data) {
 delete data[key];

 data[prop] = lib.extendDeepNoArrays(data[prop] || {}, lib.objectFromPath(key, lib.expandObjectPaths(datum))[prop]);
 } else if((match = key.match(indexedPropertyRegex))) {
 datum = data[key];

 prop = match[1];
 idx = parseInt(match[2]);

 delete data[key];

 data[prop] = data[prop] || [];

 if(match[3] === '.') {
 // This is the case where theere are subsequent properties into which
 // we must recurse, e.g. transforms[0].value
 trailingPath = match[4];
 dest = data[prop][idx] = data[prop][idx] || {};

 // NB: Extend deep no arrays prevents this from working on multiple
 // nested properties in the same object, e.g.
 //
 // {
 // foo[0].bar[1].range
 // foo[0].bar[0].range
 // }
 //
 // In this case, the extendDeepNoArrays will overwrite one array with
 // the other, so that both properties *will not* be present in the
 // result. Fixing this would require a more intelligent tracking
 // of changes and merging than extendDeepNoArrays currently accomplishes.
 lib.extendDeepNoArrays(dest, lib.objectFromPath(trailingPath, lib.expandObjectPaths(datum)));
 } else {
 // This is the case where this property is the end of the line,
 // e.g. xaxis.range[0]
 data[prop][idx] = lib.expandObjectPaths(datum);
 }
 } else {
 data[key] = lib.expandObjectPaths(data[key]);
 }
 }
 }
 }

 return data;
};

Expand Down

106 changes: 89 additions & 17 deletions
src/plots/plots.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1299,7 +1299,7 @@ plots.modifyFrames = function(gd, operations) {
 */
plots.computeFrame = function(gd, frameName) {
 var frameLookup = gd._transitionData._frameHash;
 var i, traceIndices, traceIndex, expandedObj, destIndex, copy;
 var i, traceIndices, traceIndex, destIndex;

 var framePtr = frameLookup[frameName];

Expand All@@ -1326,9 +1326,7 @@ plots.computeFrame = function(gd, frameName) {
 // Merge, starting with the last and ending with the desired frame:
 while((framePtr = frameStack.pop())) {
 if(framePtr.layout) {
 copy = Lib.extendDeepNoArrays({}, framePtr.layout);
 expandedObj = Lib.expandObjectPaths(copy);
 result.layout = Lib.extendDeepNoArrays(result.layout || {}, expandedObj);
 result.layout = plots.extendLayout(result.layout, framePtr.layout);
 }

 if(framePtr.data) {
Expand DownExpand Up@@ -1363,16 +1361,99 @@ plots.computeFrame = function(gd, frameName) {
 result.traces[destIndex] = traceIndex;
 }

 copy = Lib.extendDeepNoArrays({}, framePtr.data[i]);
 expandedObj = Lib.expandObjectPaths(copy);
 result.data[destIndex] = Lib.extendDeepNoArrays(result.data[destIndex] || {}, expandedObj);
 result.data[destIndex] = plots.extendTrace(result.data[destIndex], framePtr.data[i]);
 }
 }
 }

 return result;
};

/**
 * Extend an object, treating container arrays very differently by extracting
 * their contents and merging them separately.
 *
 * This exists so that we can extendDeepNoArrays and avoid stepping into data
 * arrays without knowledge of the plot schema, but so that we may also manually
 * recurse into known container arrays, such as transforms.
 *
 * See extendTrace and extendLayout below for usage.
 */
plots.extendObjectWithContainers = function(dest, src, containerPaths) {
 var containerProp, containerVal, i, j, srcProp, destProp, srcContainer, destContainer;
 var copy = Lib.extendDeepNoArrays({}, src || {});
 var expandedObj = Lib.expandObjectPaths(copy);
 var containerObj = {};

 // Step through and extract any container properties. Otherwise extendDeepNoArrays
 // will clobber any existing properties with an empty array and then supplyDefaults
 // will reset everything to defaults.
 if(containerPaths && containerPaths.length) {
 for(i = 0; i < containerPaths.length; i++) {
 containerProp = Lib.nestedProperty(expandedObj, containerPaths[i]);
 containerVal = containerProp.get();
 containerProp.set(null);
 Lib.nestedProperty(containerObj, containerPaths[i]).set(containerVal);
 }
 }

 dest = Lib.extendDeepNoArrays(dest || {}, expandedObj);

 if(containerPaths && containerPaths.length) {
 for(i = 0; i < containerPaths.length; i++) {
 srcProp = Lib.nestedProperty(containerObj, containerPaths[i]);
 srcContainer = srcProp.get();

 if(!srcContainer) continue;

 destProp = Lib.nestedProperty(dest, containerPaths[i]);

 destContainer = destProp.get();
 if(!Array.isArray(destContainer)) {
 destContainer = [];
 destProp.set(destContainer);
 }

 for(j = 0; j < srcContainer.length; j++) {
 destContainer[j] = plots.extendObjectWithContainers(destContainer[j], srcContainer[j]);
 }
 }
 }

 return dest;
};

/*
 * Extend a trace definition. This method:
 *
 * 1. directly transfers any array references
 * 2. manually recurses into container arrays like transforms
 *
 * The result is the original object reference with the new contents merged in.
 */
plots.extendTrace = function(destTrace, srcTrace) {
 return plots.extendObjectWithContainers(destTrace, srcTrace, ['transforms']);
@etpinardetpinardOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/*
 * Extend a layout definition. This method:
 *
 * 1. directly transfers any array references (not critically important for
 * layout since there aren't really data arrays)
 * 2. manually recurses into container arrays like annotations
 *
 * The result is the original object reference with the new contents merged in.
 */
plots.extendLayout = function(destLayout, srcLayout) {
 return plots.extendObjectWithContainers(destLayout, srcLayout, [
 'annotations',
 'shapes',
 'images',
 'sliders',
 'updatemenus'
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
 ]);
};

/**
 * Transition to a set of new data and layout properties
 *
Expand DownExpand Up@@ -1411,16 +1492,7 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)

 transitionedTraces.push(traceIdx);

 // This is a multi-step process. First clone w/o arrays so that
 // we're not modifying the original:
 var update = Lib.extendDeepNoArrays({}, data[i]);

 // Then expand object paths since we don't obey object-overwrite
 // semantics here:
 update = Lib.expandObjectPaths(update);

 // Finally apply the update (without copying arrays, of course):
 Lib.extendDeepNoArrays(gd.data[traceIndices[i]], update);
 gd.data[traceIndices[i]] = plots.extendTrace(gd.data[traceIndices[i]], data[i]);
 }

 // Follow the same procedure. Clone it so we don't mangle the input, then
Expand Down

2 changes: 1 addition & 1 deletion
test/jasmine/tests/animate_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -84,7 +84,7 @@ describe('Test animate API', function() {
 });

 Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(function() {
 Plotly.addFrames(gd, mockCopy.frames);
return Plotly.addFrames(gd, mockCopy.frames);
 }).then(done);
 });

Expand Down

84 changes: 84 additions & 0 deletions
test/jasmine/tests/lib_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -537,6 +537,90 @@ describe('Test lib.js:', function() {
 expect(input).toBe(expanded);
 expect(origArray).toBe(newArray);
 });

 it('expands bracketed array notation', function() {
 var input = {'marker[1]': {color: 'red'}};
 var expected = {marker: [undefined, {color: 'red'}]};
 expect(Lib.expandObjectPaths(input)).toEqual(expected);
 });

 it('expands nested arrays', function() {
 var input = {'marker[1].range[1]': 5};
 var expected = {marker: [undefined, {range: [undefined, 5]}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array with more nested attributes', function() {
 var input = {'marker[1]': {'color.alpha': 2}};
 var expected = {marker: [undefined, {color: {alpha: 2}}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation without further nesting', function() {
 var input = {'marker[1]': 8};
 var expected = {marker: [undefined, 8]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation with further nesting', function() {
 var input = {'marker[1].size': 8};
 var expected = {marker: [undefined, {size: 8}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation with further nesting', function() {
 var input = {'marker[1].size.magnitude': 8};
 var expected = {marker: [undefined, {size: {magnitude: 8}}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('combines changes with single array nesting', function() {
 var input = {'marker[1].foo': 5, 'marker[0].foo': 4};
 var expected = {marker: [{foo: 4}, {foo: 5}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 // 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
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those arrays are declared as info_array e.g. pie domain.
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does
varinput={'marker[1].range': [4,5]};Lib.expandObjectPaths(input);// => {marker: [undefined, {range: [4, 5]}]}
??
@rreusserrreusserOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]}
@rreusserrreusserOct 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@rreusserrreusserOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.
 // not data arrays but yes on arrays that were previously expanded. This is a bit
 // tricky to get to work just right and currently doesn't have any known use since
 // container arrays are not multiply nested.
 //
 // Additional notes on what works or what doesn't work. This case does *not* work
 // because the two nested arrays that would result from the expansion need to be
 // deep merged.
 //
 // Lib.expandObjectPaths({'marker.range[0]': 5, 'marker.range[1]': 2})
 //
 // // => {marker: {range: [null, 2]}}
 //
 // This case *does* work becuase the array merging does not require a deep extend:
 //
 // Lib.expandObjectPaths({'range[0]': 5, 'range[1]': 2}
 //
 // // => {range: [5, 2]}
 //
 // Finally note that this case works fine becuase there's no merge necessary:
 //
 // Lib.expandObjectPaths({'marker.range[1]': 2})
 //
 // // => {marker: {range: [null, 2]}}
 //
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice 👍
 /*
 it('combines changes', function() {
 var input = {'marker[1].range[1]': 5, 'marker[1].range[0]': 4};
 var expected = {marker: [undefined, {range: [4, 5]}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });
 */
 });

 describe('coerce', function() {
Expand Down

35 changes: 35 additions & 0 deletions
test/jasmine/tests/transition_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -65,6 +65,41 @@ function runTests(transitionDuration) {
 }).catch(fail).then(done);
 });

 it('transitions a transform', function(done) {
 Plotly.restyle(gd, {
 'transforms[0]': {
 enabled: true,
 type: 'filter',
 operation: '<',
 filtersrc: 'x',
 value: 10
 }
 }, [0]).then(function() {
 expect(gd._fullData[0].transforms).toEqual([{
 enabled: true,
 type: 'filter',
 operation: '<',
 filtersrc: 'x',
 value: 10
 }]);

 return Plots.transition(gd, [{
 'transforms[0].operation': '>'
 }], null, [0],
 {redraw: true, duration: transitionDuration},
 {duration: transitionDuration, easing: 'cubic-in-out'}
 );
 }).then(function() {
 expect(gd._fullData[0].transforms).toEqual([{
 enabled: true,
 type: 'filter',
 operation: '>',
 filtersrc: 'x',
 value: 10
 }]);
 }).catch(fail).then(done);
 });

 // This doesn't really test anything that the above tests don't cover, but it combines
 // the behavior and attempts to ensure chaining and events happen in the correct order.
 it('transitions may be chained', function(done) {
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Improved animation merging for layout and traces by rreusser · Pull Request #1041 · plotly/plotly.js · GitHub
Skip to content
Improved animation merging for layout and traces#1041

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

rreusser
merged 9 commits into

from

Oct 17, 2016
 Merged


Improved animation merging for layout and traces
#1041
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


9 commits


Select commit
Hold shift + click to select a range
0e86edd

Add failing test fro transition transform


rreusser Oct 14, 2016
a0da51d

Mostly factor out trace and layout merge logic


rreusser Oct 14, 2016
b612ac3

Methods for extendTrace and extendLayout


rreusser Oct 14, 2016
8d35b71

Lock down and document the unimplemented case in expandObjectPaths


rreusser Oct 14, 2016
c93c18f

Verify function of animated transforms


rreusser Oct 14, 2016
33e23db

Tweak trivial things to get the test to pass


rreusser Oct 14, 2016
511de3d

Delete best guess at failing test


rreusser Oct 14, 2016
adee9b2

Add missing layout container arrays


rreusser Oct 17, 2016
e085a53

Add additional docs on expandObjectPaths subtleties


rreusser Oct 17, 2016

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


69 changes: 61 additions & 8 deletions
src/lib/index.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -589,18 +589,36 @@ lib.objectFromPath = function(path, value) {
/**
 * Iterate through an object in-place, converting dotted properties to objects.
 *
 * @example
 * lib.expandObjectPaths({'nested.test.path': 'value'});
 * // returns { nested: { test: {path: 'value'}}}
 * Examples:
 *
 * lib.expandObjectPaths({'nested.test.path': 'value'});
 * => { nested: { test: {path: 'value'}}}
 *
 * It also handles array notation, e.g.:
 *
 * lib.expandObjectPaths({'foo[1].bar': 'value'});
 * => { foo: [null, {bar: value}] }
 *
 * It handles merges the results when two properties are specified in parallel:
 *
 * lib.expandObjectPaths({'foo[1].bar': 10, 'foo[0].bar': 20});
 * => { foo: [{bar: 10}, {bar: 20}] }
 *
 * It does NOT, however, merge mulitple mutliply-nested arrays::
 *
 * lib.expandObjectPaths({'marker[1].range[1]': 5, 'marker[1].range[0]': 4})
 * => { marker: [null, {range: 4}] }
 */

// Store this to avoid recompiling regex on every prop since this may happen many
// many times for animations.
// TODO: Premature optimization? Remove?
var dottedPropertyRegex = /^([^\.]*)\../;
// Store this to avoid recompiling regex on *every* prop since this may happen many
// many times for animations. Could maybe be inside the function. Not sure about
// 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) {
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for reusing this method 🎉
 var match, key, prop, datum;
 var match, key, prop, datum, idx, dest, trailingPath;
 if(typeof data === 'object' && !Array.isArray(data)) {
 for(key in data) {
 if(data.hasOwnProperty(key)) {
Expand All@@ -611,12 +629,47 @@ lib.expandObjectPaths = function(data) {
 delete data[key];

 data[prop] = lib.extendDeepNoArrays(data[prop] || {}, lib.objectFromPath(key, lib.expandObjectPaths(datum))[prop]);
 } else if((match = key.match(indexedPropertyRegex))) {
 datum = data[key];

 prop = match[1];
 idx = parseInt(match[2]);

 delete data[key];

 data[prop] = data[prop] || [];

 if(match[3] === '.') {
 // This is the case where theere are subsequent properties into which
 // we must recurse, e.g. transforms[0].value
 trailingPath = match[4];
 dest = data[prop][idx] = data[prop][idx] || {};

 // NB: Extend deep no arrays prevents this from working on multiple
 // nested properties in the same object, e.g.
 //
 // {
 // foo[0].bar[1].range
 // foo[0].bar[0].range
 // }
 //
 // In this case, the extendDeepNoArrays will overwrite one array with
 // the other, so that both properties *will not* be present in the
 // result. Fixing this would require a more intelligent tracking
 // of changes and merging than extendDeepNoArrays currently accomplishes.
 lib.extendDeepNoArrays(dest, lib.objectFromPath(trailingPath, lib.expandObjectPaths(datum)));
 } else {
 // This is the case where this property is the end of the line,
 // e.g. xaxis.range[0]
 data[prop][idx] = lib.expandObjectPaths(datum);
 }
 } else {
 data[key] = lib.expandObjectPaths(data[key]);
 }
 }
 }
 }

 return data;
};

Expand Down

106 changes: 89 additions & 17 deletions
src/plots/plots.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1299,7 +1299,7 @@ plots.modifyFrames = function(gd, operations) {
 */
plots.computeFrame = function(gd, frameName) {
 var frameLookup = gd._transitionData._frameHash;
 var i, traceIndices, traceIndex, expandedObj, destIndex, copy;
 var i, traceIndices, traceIndex, destIndex;

 var framePtr = frameLookup[frameName];

Expand All@@ -1326,9 +1326,7 @@ plots.computeFrame = function(gd, frameName) {
 // Merge, starting with the last and ending with the desired frame:
 while((framePtr = frameStack.pop())) {
 if(framePtr.layout) {
 copy = Lib.extendDeepNoArrays({}, framePtr.layout);
 expandedObj = Lib.expandObjectPaths(copy);
 result.layout = Lib.extendDeepNoArrays(result.layout || {}, expandedObj);
 result.layout = plots.extendLayout(result.layout, framePtr.layout);
 }

 if(framePtr.data) {
Expand DownExpand Up@@ -1363,16 +1361,99 @@ plots.computeFrame = function(gd, frameName) {
 result.traces[destIndex] = traceIndex;
 }

 copy = Lib.extendDeepNoArrays({}, framePtr.data[i]);
 expandedObj = Lib.expandObjectPaths(copy);
 result.data[destIndex] = Lib.extendDeepNoArrays(result.data[destIndex] || {}, expandedObj);
 result.data[destIndex] = plots.extendTrace(result.data[destIndex], framePtr.data[i]);
 }
 }
 }

 return result;
};

/**
 * Extend an object, treating container arrays very differently by extracting
 * their contents and merging them separately.
 *
 * This exists so that we can extendDeepNoArrays and avoid stepping into data
 * arrays without knowledge of the plot schema, but so that we may also manually
 * recurse into known container arrays, such as transforms.
 *
 * See extendTrace and extendLayout below for usage.
 */
plots.extendObjectWithContainers = function(dest, src, containerPaths) {
 var containerProp, containerVal, i, j, srcProp, destProp, srcContainer, destContainer;
 var copy = Lib.extendDeepNoArrays({}, src || {});
 var expandedObj = Lib.expandObjectPaths(copy);
 var containerObj = {};

 // Step through and extract any container properties. Otherwise extendDeepNoArrays
 // will clobber any existing properties with an empty array and then supplyDefaults
 // will reset everything to defaults.
 if(containerPaths && containerPaths.length) {
 for(i = 0; i < containerPaths.length; i++) {
 containerProp = Lib.nestedProperty(expandedObj, containerPaths[i]);
 containerVal = containerProp.get();
 containerProp.set(null);
 Lib.nestedProperty(containerObj, containerPaths[i]).set(containerVal);
 }
 }

 dest = Lib.extendDeepNoArrays(dest || {}, expandedObj);

 if(containerPaths && containerPaths.length) {
 for(i = 0; i < containerPaths.length; i++) {
 srcProp = Lib.nestedProperty(containerObj, containerPaths[i]);
 srcContainer = srcProp.get();

 if(!srcContainer) continue;

 destProp = Lib.nestedProperty(dest, containerPaths[i]);

 destContainer = destProp.get();
 if(!Array.isArray(destContainer)) {
 destContainer = [];
 destProp.set(destContainer);
 }

 for(j = 0; j < srcContainer.length; j++) {
 destContainer[j] = plots.extendObjectWithContainers(destContainer[j], srcContainer[j]);
 }
 }
 }

 return dest;
};

/*
 * Extend a trace definition. This method:
 *
 * 1. directly transfers any array references
 * 2. manually recurses into container arrays like transforms
 *
 * The result is the original object reference with the new contents merged in.
 */
plots.extendTrace = function(destTrace, srcTrace) {
 return plots.extendObjectWithContainers(destTrace, srcTrace, ['transforms']);
@etpinardetpinardOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/*
 * Extend a layout definition. This method:
 *
 * 1. directly transfers any array references (not critically important for
 * layout since there aren't really data arrays)
 * 2. manually recurses into container arrays like annotations
 *
 * The result is the original object reference with the new contents merged in.
 */
plots.extendLayout = function(destLayout, srcLayout) {
 return plots.extendObjectWithContainers(destLayout, srcLayout, [
 'annotations',
 'shapes',
 'images',
 'sliders',
 'updatemenus'
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
 ]);
};

/**
 * Transition to a set of new data and layout properties
 *
Expand DownExpand Up@@ -1411,16 +1492,7 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)

 transitionedTraces.push(traceIdx);

 // This is a multi-step process. First clone w/o arrays so that
 // we're not modifying the original:
 var update = Lib.extendDeepNoArrays({}, data[i]);

 // Then expand object paths since we don't obey object-overwrite
 // semantics here:
 update = Lib.expandObjectPaths(update);

 // Finally apply the update (without copying arrays, of course):
 Lib.extendDeepNoArrays(gd.data[traceIndices[i]], update);
 gd.data[traceIndices[i]] = plots.extendTrace(gd.data[traceIndices[i]], data[i]);
 }

 // Follow the same procedure. Clone it so we don't mangle the input, then
Expand Down

2 changes: 1 addition & 1 deletion
test/jasmine/tests/animate_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -84,7 +84,7 @@ describe('Test animate API', function() {
 });

 Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(function() {
 Plotly.addFrames(gd, mockCopy.frames);
return Plotly.addFrames(gd, mockCopy.frames);
 }).then(done);
 });

Expand Down

84 changes: 84 additions & 0 deletions
test/jasmine/tests/lib_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -537,6 +537,90 @@ describe('Test lib.js:', function() {
 expect(input).toBe(expanded);
 expect(origArray).toBe(newArray);
 });

 it('expands bracketed array notation', function() {
 var input = {'marker[1]': {color: 'red'}};
 var expected = {marker: [undefined, {color: 'red'}]};
 expect(Lib.expandObjectPaths(input)).toEqual(expected);
 });

 it('expands nested arrays', function() {
 var input = {'marker[1].range[1]': 5};
 var expected = {marker: [undefined, {range: [undefined, 5]}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array with more nested attributes', function() {
 var input = {'marker[1]': {'color.alpha': 2}};
 var expected = {marker: [undefined, {color: {alpha: 2}}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation without further nesting', function() {
 var input = {'marker[1]': 8};
 var expected = {marker: [undefined, 8]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation with further nesting', function() {
 var input = {'marker[1].size': 8};
 var expected = {marker: [undefined, {size: 8}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation with further nesting', function() {
 var input = {'marker[1].size.magnitude': 8};
 var expected = {marker: [undefined, {size: {magnitude: 8}}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('combines changes with single array nesting', function() {
 var input = {'marker[1].foo': 5, 'marker[0].foo': 4};
 var expected = {marker: [{foo: 4}, {foo: 5}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 // 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
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those arrays are declared as info_array e.g. pie domain.
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does
varinput={'marker[1].range': [4,5]};Lib.expandObjectPaths(input);// => {marker: [undefined, {range: [4, 5]}]}
??
@rreusserrreusserOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]}
@rreusserrreusserOct 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@rreusserrreusserOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.
 // not data arrays but yes on arrays that were previously expanded. This is a bit
 // tricky to get to work just right and currently doesn't have any known use since
 // container arrays are not multiply nested.
 //
 // Additional notes on what works or what doesn't work. This case does *not* work
 // because the two nested arrays that would result from the expansion need to be
 // deep merged.
 //
 // Lib.expandObjectPaths({'marker.range[0]': 5, 'marker.range[1]': 2})
 //
 // // => {marker: {range: [null, 2]}}
 //
 // This case *does* work becuase the array merging does not require a deep extend:
 //
 // Lib.expandObjectPaths({'range[0]': 5, 'range[1]': 2}
 //
 // // => {range: [5, 2]}
 //
 // Finally note that this case works fine becuase there's no merge necessary:
 //
 // Lib.expandObjectPaths({'marker.range[1]': 2})
 //
 // // => {marker: {range: [null, 2]}}
 //
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice 👍
 /*
 it('combines changes', function() {
 var input = {'marker[1].range[1]': 5, 'marker[1].range[0]': 4};
 var expected = {marker: [undefined, {range: [4, 5]}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });
 */
 });

 describe('coerce', function() {
Expand Down

35 changes: 35 additions & 0 deletions
test/jasmine/tests/transition_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -65,6 +65,41 @@ function runTests(transitionDuration) {
 }).catch(fail).then(done);
 });

 it('transitions a transform', function(done) {
 Plotly.restyle(gd, {
 'transforms[0]': {
 enabled: true,
 type: 'filter',
 operation: '<',
 filtersrc: 'x',
 value: 10
 }
 }, [0]).then(function() {
 expect(gd._fullData[0].transforms).toEqual([{
 enabled: true,
 type: 'filter',
 operation: '<',
 filtersrc: 'x',
 value: 10
 }]);

 return Plots.transition(gd, [{
 'transforms[0].operation': '>'
 }], null, [0],
 {redraw: true, duration: transitionDuration},
 {duration: transitionDuration, easing: 'cubic-in-out'}
 );
 }).then(function() {
 expect(gd._fullData[0].transforms).toEqual([{
 enabled: true,
 type: 'filter',
 operation: '>',
 filtersrc: 'x',
 value: 10
 }]);
 }).catch(fail).then(done);
 });

 // This doesn't really test anything that the above tests don't cover, but it combines
 // the behavior and attempts to ensure chaining and events happen in the correct order.
 it('transitions may be chained', function(done) {
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Improved animation merging for layout and traces by rreusser · Pull Request #1041 · plotly/plotly.js · GitHub
Skip to content
Improved animation merging for layout and traces#1041

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

rreusser
merged 9 commits into

from

Oct 17, 2016
 Merged


Improved animation merging for layout and traces
#1041
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


9 commits


Select commit
Hold shift + click to select a range
0e86edd

Add failing test fro transition transform


rreusser Oct 14, 2016
a0da51d

Mostly factor out trace and layout merge logic


rreusser Oct 14, 2016
b612ac3

Methods for extendTrace and extendLayout


rreusser Oct 14, 2016
8d35b71

Lock down and document the unimplemented case in expandObjectPaths


rreusser Oct 14, 2016
c93c18f

Verify function of animated transforms


rreusser Oct 14, 2016
33e23db

Tweak trivial things to get the test to pass


rreusser Oct 14, 2016
511de3d

Delete best guess at failing test


rreusser Oct 14, 2016
adee9b2

Add missing layout container arrays


rreusser Oct 17, 2016
e085a53

Add additional docs on expandObjectPaths subtleties


rreusser Oct 17, 2016

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


69 changes: 61 additions & 8 deletions
src/lib/index.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -589,18 +589,36 @@ lib.objectFromPath = function(path, value) {
/**
 * Iterate through an object in-place, converting dotted properties to objects.
 *
 * @example
 * lib.expandObjectPaths({'nested.test.path': 'value'});
 * // returns { nested: { test: {path: 'value'}}}
 * Examples:
 *
 * lib.expandObjectPaths({'nested.test.path': 'value'});
 * => { nested: { test: {path: 'value'}}}
 *
 * It also handles array notation, e.g.:
 *
 * lib.expandObjectPaths({'foo[1].bar': 'value'});
 * => { foo: [null, {bar: value}] }
 *
 * It handles merges the results when two properties are specified in parallel:
 *
 * lib.expandObjectPaths({'foo[1].bar': 10, 'foo[0].bar': 20});
 * => { foo: [{bar: 10}, {bar: 20}] }
 *
 * It does NOT, however, merge mulitple mutliply-nested arrays::
 *
 * lib.expandObjectPaths({'marker[1].range[1]': 5, 'marker[1].range[0]': 4})
 * => { marker: [null, {range: 4}] }
 */

// Store this to avoid recompiling regex on every prop since this may happen many
// many times for animations.
// TODO: Premature optimization? Remove?
var dottedPropertyRegex = /^([^\.]*)\../;
// Store this to avoid recompiling regex on *every* prop since this may happen many
// many times for animations. Could maybe be inside the function. Not sure about
// 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) {
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for reusing this method 🎉
 var match, key, prop, datum;
 var match, key, prop, datum, idx, dest, trailingPath;
 if(typeof data === 'object' && !Array.isArray(data)) {
 for(key in data) {
 if(data.hasOwnProperty(key)) {
Expand All@@ -611,12 +629,47 @@ lib.expandObjectPaths = function(data) {
 delete data[key];

 data[prop] = lib.extendDeepNoArrays(data[prop] || {}, lib.objectFromPath(key, lib.expandObjectPaths(datum))[prop]);
 } else if((match = key.match(indexedPropertyRegex))) {
 datum = data[key];

 prop = match[1];
 idx = parseInt(match[2]);

 delete data[key];

 data[prop] = data[prop] || [];

 if(match[3] === '.') {
 // This is the case where theere are subsequent properties into which
 // we must recurse, e.g. transforms[0].value
 trailingPath = match[4];
 dest = data[prop][idx] = data[prop][idx] || {};

 // NB: Extend deep no arrays prevents this from working on multiple
 // nested properties in the same object, e.g.
 //
 // {
 // foo[0].bar[1].range
 // foo[0].bar[0].range
 // }
 //
 // In this case, the extendDeepNoArrays will overwrite one array with
 // the other, so that both properties *will not* be present in the
 // result. Fixing this would require a more intelligent tracking
 // of changes and merging than extendDeepNoArrays currently accomplishes.
 lib.extendDeepNoArrays(dest, lib.objectFromPath(trailingPath, lib.expandObjectPaths(datum)));
 } else {
 // This is the case where this property is the end of the line,
 // e.g. xaxis.range[0]
 data[prop][idx] = lib.expandObjectPaths(datum);
 }
 } else {
 data[key] = lib.expandObjectPaths(data[key]);
 }
 }
 }
 }

 return data;
};

Expand Down

106 changes: 89 additions & 17 deletions
src/plots/plots.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1299,7 +1299,7 @@ plots.modifyFrames = function(gd, operations) {
 */
plots.computeFrame = function(gd, frameName) {
 var frameLookup = gd._transitionData._frameHash;
 var i, traceIndices, traceIndex, expandedObj, destIndex, copy;
 var i, traceIndices, traceIndex, destIndex;

 var framePtr = frameLookup[frameName];

Expand All@@ -1326,9 +1326,7 @@ plots.computeFrame = function(gd, frameName) {
 // Merge, starting with the last and ending with the desired frame:
 while((framePtr = frameStack.pop())) {
 if(framePtr.layout) {
 copy = Lib.extendDeepNoArrays({}, framePtr.layout);
 expandedObj = Lib.expandObjectPaths(copy);
 result.layout = Lib.extendDeepNoArrays(result.layout || {}, expandedObj);
 result.layout = plots.extendLayout(result.layout, framePtr.layout);
 }

 if(framePtr.data) {
Expand DownExpand Up@@ -1363,16 +1361,99 @@ plots.computeFrame = function(gd, frameName) {
 result.traces[destIndex] = traceIndex;
 }

 copy = Lib.extendDeepNoArrays({}, framePtr.data[i]);
 expandedObj = Lib.expandObjectPaths(copy);
 result.data[destIndex] = Lib.extendDeepNoArrays(result.data[destIndex] || {}, expandedObj);
 result.data[destIndex] = plots.extendTrace(result.data[destIndex], framePtr.data[i]);
 }
 }
 }

 return result;
};

/**
 * Extend an object, treating container arrays very differently by extracting
 * their contents and merging them separately.
 *
 * This exists so that we can extendDeepNoArrays and avoid stepping into data
 * arrays without knowledge of the plot schema, but so that we may also manually
 * recurse into known container arrays, such as transforms.
 *
 * See extendTrace and extendLayout below for usage.
 */
plots.extendObjectWithContainers = function(dest, src, containerPaths) {
 var containerProp, containerVal, i, j, srcProp, destProp, srcContainer, destContainer;
 var copy = Lib.extendDeepNoArrays({}, src || {});
 var expandedObj = Lib.expandObjectPaths(copy);
 var containerObj = {};

 // Step through and extract any container properties. Otherwise extendDeepNoArrays
 // will clobber any existing properties with an empty array and then supplyDefaults
 // will reset everything to defaults.
 if(containerPaths && containerPaths.length) {
 for(i = 0; i < containerPaths.length; i++) {
 containerProp = Lib.nestedProperty(expandedObj, containerPaths[i]);
 containerVal = containerProp.get();
 containerProp.set(null);
 Lib.nestedProperty(containerObj, containerPaths[i]).set(containerVal);
 }
 }

 dest = Lib.extendDeepNoArrays(dest || {}, expandedObj);

 if(containerPaths && containerPaths.length) {
 for(i = 0; i < containerPaths.length; i++) {
 srcProp = Lib.nestedProperty(containerObj, containerPaths[i]);
 srcContainer = srcProp.get();

 if(!srcContainer) continue;

 destProp = Lib.nestedProperty(dest, containerPaths[i]);

 destContainer = destProp.get();
 if(!Array.isArray(destContainer)) {
 destContainer = [];
 destProp.set(destContainer);
 }

 for(j = 0; j < srcContainer.length; j++) {
 destContainer[j] = plots.extendObjectWithContainers(destContainer[j], srcContainer[j]);
 }
 }
 }

 return dest;
};

/*
 * Extend a trace definition. This method:
 *
 * 1. directly transfers any array references
 * 2. manually recurses into container arrays like transforms
 *
 * The result is the original object reference with the new contents merged in.
 */
plots.extendTrace = function(destTrace, srcTrace) {
 return plots.extendObjectWithContainers(destTrace, srcTrace, ['transforms']);
@etpinardetpinardOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/*
 * Extend a layout definition. This method:
 *
 * 1. directly transfers any array references (not critically important for
 * layout since there aren't really data arrays)
 * 2. manually recurses into container arrays like annotations
 *
 * The result is the original object reference with the new contents merged in.
 */
plots.extendLayout = function(destLayout, srcLayout) {
 return plots.extendObjectWithContainers(destLayout, srcLayout, [
 'annotations',
 'shapes',
 'images',
 'sliders',
 'updatemenus'
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
 ]);
};

/**
 * Transition to a set of new data and layout properties
 *
Expand DownExpand Up@@ -1411,16 +1492,7 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)

 transitionedTraces.push(traceIdx);

 // This is a multi-step process. First clone w/o arrays so that
 // we're not modifying the original:
 var update = Lib.extendDeepNoArrays({}, data[i]);

 // Then expand object paths since we don't obey object-overwrite
 // semantics here:
 update = Lib.expandObjectPaths(update);

 // Finally apply the update (without copying arrays, of course):
 Lib.extendDeepNoArrays(gd.data[traceIndices[i]], update);
 gd.data[traceIndices[i]] = plots.extendTrace(gd.data[traceIndices[i]], data[i]);
 }

 // Follow the same procedure. Clone it so we don't mangle the input, then
Expand Down

2 changes: 1 addition & 1 deletion
test/jasmine/tests/animate_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -84,7 +84,7 @@ describe('Test animate API', function() {
 });

 Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(function() {
 Plotly.addFrames(gd, mockCopy.frames);
return Plotly.addFrames(gd, mockCopy.frames);
 }).then(done);
 });

Expand Down

84 changes: 84 additions & 0 deletions
test/jasmine/tests/lib_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -537,6 +537,90 @@ describe('Test lib.js:', function() {
 expect(input).toBe(expanded);
 expect(origArray).toBe(newArray);
 });

 it('expands bracketed array notation', function() {
 var input = {'marker[1]': {color: 'red'}};
 var expected = {marker: [undefined, {color: 'red'}]};
 expect(Lib.expandObjectPaths(input)).toEqual(expected);
 });

 it('expands nested arrays', function() {
 var input = {'marker[1].range[1]': 5};
 var expected = {marker: [undefined, {range: [undefined, 5]}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array with more nested attributes', function() {
 var input = {'marker[1]': {'color.alpha': 2}};
 var expected = {marker: [undefined, {color: {alpha: 2}}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation without further nesting', function() {
 var input = {'marker[1]': 8};
 var expected = {marker: [undefined, 8]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation with further nesting', function() {
 var input = {'marker[1].size': 8};
 var expected = {marker: [undefined, {size: 8}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation with further nesting', function() {
 var input = {'marker[1].size.magnitude': 8};
 var expected = {marker: [undefined, {size: {magnitude: 8}}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('combines changes with single array nesting', function() {
 var input = {'marker[1].foo': 5, 'marker[0].foo': 4};
 var expected = {marker: [{foo: 4}, {foo: 5}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 // 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
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those arrays are declared as info_array e.g. pie domain.
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does
varinput={'marker[1].range': [4,5]};Lib.expandObjectPaths(input);// => {marker: [undefined, {range: [4, 5]}]}
??
@rreusserrreusserOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]}
@rreusserrreusserOct 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@rreusserrreusserOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.
 // not data arrays but yes on arrays that were previously expanded. This is a bit
 // tricky to get to work just right and currently doesn't have any known use since
 // container arrays are not multiply nested.
 //
 // Additional notes on what works or what doesn't work. This case does *not* work
 // because the two nested arrays that would result from the expansion need to be
 // deep merged.
 //
 // Lib.expandObjectPaths({'marker.range[0]': 5, 'marker.range[1]': 2})
 //
 // // => {marker: {range: [null, 2]}}
 //
 // This case *does* work becuase the array merging does not require a deep extend:
 //
 // Lib.expandObjectPaths({'range[0]': 5, 'range[1]': 2}
 //
 // // => {range: [5, 2]}
 //
 // Finally note that this case works fine becuase there's no merge necessary:
 //
 // Lib.expandObjectPaths({'marker.range[1]': 2})
 //
 // // => {marker: {range: [null, 2]}}
 //
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice 👍
 /*
 it('combines changes', function() {
 var input = {'marker[1].range[1]': 5, 'marker[1].range[0]': 4};
 var expected = {marker: [undefined, {range: [4, 5]}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });
 */
 });

 describe('coerce', function() {
Expand Down

35 changes: 35 additions & 0 deletions
test/jasmine/tests/transition_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -65,6 +65,41 @@ function runTests(transitionDuration) {
 }).catch(fail).then(done);
 });

 it('transitions a transform', function(done) {
 Plotly.restyle(gd, {
 'transforms[0]': {
 enabled: true,
 type: 'filter',
 operation: '<',
 filtersrc: 'x',
 value: 10
 }
 }, [0]).then(function() {
 expect(gd._fullData[0].transforms).toEqual([{
 enabled: true,
 type: 'filter',
 operation: '<',
 filtersrc: 'x',
 value: 10
 }]);

 return Plots.transition(gd, [{
 'transforms[0].operation': '>'
 }], null, [0],
 {redraw: true, duration: transitionDuration},
 {duration: transitionDuration, easing: 'cubic-in-out'}
 );
 }).then(function() {
 expect(gd._fullData[0].transforms).toEqual([{
 enabled: true,
 type: 'filter',
 operation: '>',
 filtersrc: 'x',
 value: 10
 }]);
 }).catch(fail).then(done);
 });

 // This doesn't really test anything that the above tests don't cover, but it combines
 // the behavior and attempts to ensure chaining and events happen in the correct order.
 it('transitions may be chained', function(done) {
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Improved animation merging for layout and traces by rreusser · Pull Request #1041 · plotly/plotly.js · GitHub
Skip to content
Improved animation merging for layout and traces#1041

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

rreusser
merged 9 commits into

from

Oct 17, 2016
 Merged


Improved animation merging for layout and traces
#1041
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


9 commits


Select commit
Hold shift + click to select a range
0e86edd

Add failing test fro transition transform


rreusser Oct 14, 2016
a0da51d

Mostly factor out trace and layout merge logic


rreusser Oct 14, 2016
b612ac3

Methods for extendTrace and extendLayout


rreusser Oct 14, 2016
8d35b71

Lock down and document the unimplemented case in expandObjectPaths


rreusser Oct 14, 2016
c93c18f

Verify function of animated transforms


rreusser Oct 14, 2016
33e23db

Tweak trivial things to get the test to pass


rreusser Oct 14, 2016
511de3d

Delete best guess at failing test


rreusser Oct 14, 2016
adee9b2

Add missing layout container arrays


rreusser Oct 17, 2016
e085a53

Add additional docs on expandObjectPaths subtleties


rreusser Oct 17, 2016

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


69 changes: 61 additions & 8 deletions
src/lib/index.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -589,18 +589,36 @@ lib.objectFromPath = function(path, value) {
/**
 * Iterate through an object in-place, converting dotted properties to objects.
 *
 * @example
 * lib.expandObjectPaths({'nested.test.path': 'value'});
 * // returns { nested: { test: {path: 'value'}}}
 * Examples:
 *
 * lib.expandObjectPaths({'nested.test.path': 'value'});
 * => { nested: { test: {path: 'value'}}}
 *
 * It also handles array notation, e.g.:
 *
 * lib.expandObjectPaths({'foo[1].bar': 'value'});
 * => { foo: [null, {bar: value}] }
 *
 * It handles merges the results when two properties are specified in parallel:
 *
 * lib.expandObjectPaths({'foo[1].bar': 10, 'foo[0].bar': 20});
 * => { foo: [{bar: 10}, {bar: 20}] }
 *
 * It does NOT, however, merge mulitple mutliply-nested arrays::
 *
 * lib.expandObjectPaths({'marker[1].range[1]': 5, 'marker[1].range[0]': 4})
 * => { marker: [null, {range: 4}] }
 */

// Store this to avoid recompiling regex on every prop since this may happen many
// many times for animations.
// TODO: Premature optimization? Remove?
var dottedPropertyRegex = /^([^\.]*)\../;
// Store this to avoid recompiling regex on *every* prop since this may happen many
// many times for animations. Could maybe be inside the function. Not sure about
// 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) {
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for reusing this method 🎉
 var match, key, prop, datum;
 var match, key, prop, datum, idx, dest, trailingPath;
 if(typeof data === 'object' && !Array.isArray(data)) {
 for(key in data) {
 if(data.hasOwnProperty(key)) {
Expand All@@ -611,12 +629,47 @@ lib.expandObjectPaths = function(data) {
 delete data[key];

 data[prop] = lib.extendDeepNoArrays(data[prop] || {}, lib.objectFromPath(key, lib.expandObjectPaths(datum))[prop]);
 } else if((match = key.match(indexedPropertyRegex))) {
 datum = data[key];

 prop = match[1];
 idx = parseInt(match[2]);

 delete data[key];

 data[prop] = data[prop] || [];

 if(match[3] === '.') {
 // This is the case where theere are subsequent properties into which
 // we must recurse, e.g. transforms[0].value
 trailingPath = match[4];
 dest = data[prop][idx] = data[prop][idx] || {};

 // NB: Extend deep no arrays prevents this from working on multiple
 // nested properties in the same object, e.g.
 //
 // {
 // foo[0].bar[1].range
 // foo[0].bar[0].range
 // }
 //
 // In this case, the extendDeepNoArrays will overwrite one array with
 // the other, so that both properties *will not* be present in the
 // result. Fixing this would require a more intelligent tracking
 // of changes and merging than extendDeepNoArrays currently accomplishes.
 lib.extendDeepNoArrays(dest, lib.objectFromPath(trailingPath, lib.expandObjectPaths(datum)));
 } else {
 // This is the case where this property is the end of the line,
 // e.g. xaxis.range[0]
 data[prop][idx] = lib.expandObjectPaths(datum);
 }
 } else {
 data[key] = lib.expandObjectPaths(data[key]);
 }
 }
 }
 }

 return data;
};

Expand Down

106 changes: 89 additions & 17 deletions
src/plots/plots.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1299,7 +1299,7 @@ plots.modifyFrames = function(gd, operations) {
 */
plots.computeFrame = function(gd, frameName) {
 var frameLookup = gd._transitionData._frameHash;
 var i, traceIndices, traceIndex, expandedObj, destIndex, copy;
 var i, traceIndices, traceIndex, destIndex;

 var framePtr = frameLookup[frameName];

Expand All@@ -1326,9 +1326,7 @@ plots.computeFrame = function(gd, frameName) {
 // Merge, starting with the last and ending with the desired frame:
 while((framePtr = frameStack.pop())) {
 if(framePtr.layout) {
 copy = Lib.extendDeepNoArrays({}, framePtr.layout);
 expandedObj = Lib.expandObjectPaths(copy);
 result.layout = Lib.extendDeepNoArrays(result.layout || {}, expandedObj);
 result.layout = plots.extendLayout(result.layout, framePtr.layout);
 }

 if(framePtr.data) {
Expand DownExpand Up@@ -1363,16 +1361,99 @@ plots.computeFrame = function(gd, frameName) {
 result.traces[destIndex] = traceIndex;
 }

 copy = Lib.extendDeepNoArrays({}, framePtr.data[i]);
 expandedObj = Lib.expandObjectPaths(copy);
 result.data[destIndex] = Lib.extendDeepNoArrays(result.data[destIndex] || {}, expandedObj);
 result.data[destIndex] = plots.extendTrace(result.data[destIndex], framePtr.data[i]);
 }
 }
 }

 return result;
};

/**
 * Extend an object, treating container arrays very differently by extracting
 * their contents and merging them separately.
 *
 * This exists so that we can extendDeepNoArrays and avoid stepping into data
 * arrays without knowledge of the plot schema, but so that we may also manually
 * recurse into known container arrays, such as transforms.
 *
 * See extendTrace and extendLayout below for usage.
 */
plots.extendObjectWithContainers = function(dest, src, containerPaths) {
 var containerProp, containerVal, i, j, srcProp, destProp, srcContainer, destContainer;
 var copy = Lib.extendDeepNoArrays({}, src || {});
 var expandedObj = Lib.expandObjectPaths(copy);
 var containerObj = {};

 // Step through and extract any container properties. Otherwise extendDeepNoArrays
 // will clobber any existing properties with an empty array and then supplyDefaults
 // will reset everything to defaults.
 if(containerPaths && containerPaths.length) {
 for(i = 0; i < containerPaths.length; i++) {
 containerProp = Lib.nestedProperty(expandedObj, containerPaths[i]);
 containerVal = containerProp.get();
 containerProp.set(null);
 Lib.nestedProperty(containerObj, containerPaths[i]).set(containerVal);
 }
 }

 dest = Lib.extendDeepNoArrays(dest || {}, expandedObj);

 if(containerPaths && containerPaths.length) {
 for(i = 0; i < containerPaths.length; i++) {
 srcProp = Lib.nestedProperty(containerObj, containerPaths[i]);
 srcContainer = srcProp.get();

 if(!srcContainer) continue;

 destProp = Lib.nestedProperty(dest, containerPaths[i]);

 destContainer = destProp.get();
 if(!Array.isArray(destContainer)) {
 destContainer = [];
 destProp.set(destContainer);
 }

 for(j = 0; j < srcContainer.length; j++) {
 destContainer[j] = plots.extendObjectWithContainers(destContainer[j], srcContainer[j]);
 }
 }
 }

 return dest;
};

/*
 * Extend a trace definition. This method:
 *
 * 1. directly transfers any array references
 * 2. manually recurses into container arrays like transforms
 *
 * The result is the original object reference with the new contents merged in.
 */
plots.extendTrace = function(destTrace, srcTrace) {
 return plots.extendObjectWithContainers(destTrace, srcTrace, ['transforms']);
@etpinardetpinardOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/*
 * Extend a layout definition. This method:
 *
 * 1. directly transfers any array references (not critically important for
 * layout since there aren't really data arrays)
 * 2. manually recurses into container arrays like annotations
 *
 * The result is the original object reference with the new contents merged in.
 */
plots.extendLayout = function(destLayout, srcLayout) {
 return plots.extendObjectWithContainers(destLayout, srcLayout, [
 'annotations',
 'shapes',
 'images',
 'sliders',
 'updatemenus'
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
 ]);
};

/**
 * Transition to a set of new data and layout properties
 *
Expand DownExpand Up@@ -1411,16 +1492,7 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)

 transitionedTraces.push(traceIdx);

 // This is a multi-step process. First clone w/o arrays so that
 // we're not modifying the original:
 var update = Lib.extendDeepNoArrays({}, data[i]);

 // Then expand object paths since we don't obey object-overwrite
 // semantics here:
 update = Lib.expandObjectPaths(update);

 // Finally apply the update (without copying arrays, of course):
 Lib.extendDeepNoArrays(gd.data[traceIndices[i]], update);
 gd.data[traceIndices[i]] = plots.extendTrace(gd.data[traceIndices[i]], data[i]);
 }

 // Follow the same procedure. Clone it so we don't mangle the input, then
Expand Down

2 changes: 1 addition & 1 deletion
test/jasmine/tests/animate_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -84,7 +84,7 @@ describe('Test animate API', function() {
 });

 Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(function() {
 Plotly.addFrames(gd, mockCopy.frames);
return Plotly.addFrames(gd, mockCopy.frames);
 }).then(done);
 });

Expand Down

84 changes: 84 additions & 0 deletions
test/jasmine/tests/lib_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -537,6 +537,90 @@ describe('Test lib.js:', function() {
 expect(input).toBe(expanded);
 expect(origArray).toBe(newArray);
 });

 it('expands bracketed array notation', function() {
 var input = {'marker[1]': {color: 'red'}};
 var expected = {marker: [undefined, {color: 'red'}]};
 expect(Lib.expandObjectPaths(input)).toEqual(expected);
 });

 it('expands nested arrays', function() {
 var input = {'marker[1].range[1]': 5};
 var expected = {marker: [undefined, {range: [undefined, 5]}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array with more nested attributes', function() {
 var input = {'marker[1]': {'color.alpha': 2}};
 var expected = {marker: [undefined, {color: {alpha: 2}}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation without further nesting', function() {
 var input = {'marker[1]': 8};
 var expected = {marker: [undefined, 8]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation with further nesting', function() {
 var input = {'marker[1].size': 8};
 var expected = {marker: [undefined, {size: 8}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation with further nesting', function() {
 var input = {'marker[1].size.magnitude': 8};
 var expected = {marker: [undefined, {size: {magnitude: 8}}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('combines changes with single array nesting', function() {
 var input = {'marker[1].foo': 5, 'marker[0].foo': 4};
 var expected = {marker: [{foo: 4}, {foo: 5}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 // 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
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those arrays are declared as info_array e.g. pie domain.
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does
varinput={'marker[1].range': [4,5]};Lib.expandObjectPaths(input);// => {marker: [undefined, {range: [4, 5]}]}
??
@rreusserrreusserOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]}
@rreusserrreusserOct 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@rreusserrreusserOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.
 // not data arrays but yes on arrays that were previously expanded. This is a bit
 // tricky to get to work just right and currently doesn't have any known use since
 // container arrays are not multiply nested.
 //
 // Additional notes on what works or what doesn't work. This case does *not* work
 // because the two nested arrays that would result from the expansion need to be
 // deep merged.
 //
 // Lib.expandObjectPaths({'marker.range[0]': 5, 'marker.range[1]': 2})
 //
 // // => {marker: {range: [null, 2]}}
 //
 // This case *does* work becuase the array merging does not require a deep extend:
 //
 // Lib.expandObjectPaths({'range[0]': 5, 'range[1]': 2}
 //
 // // => {range: [5, 2]}
 //
 // Finally note that this case works fine becuase there's no merge necessary:
 //
 // Lib.expandObjectPaths({'marker.range[1]': 2})
 //
 // // => {marker: {range: [null, 2]}}
 //
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice 👍
 /*
 it('combines changes', function() {
 var input = {'marker[1].range[1]': 5, 'marker[1].range[0]': 4};
 var expected = {marker: [undefined, {range: [4, 5]}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });
 */
 });

 describe('coerce', function() {
Expand Down

35 changes: 35 additions & 0 deletions
test/jasmine/tests/transition_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -65,6 +65,41 @@ function runTests(transitionDuration) {
 }).catch(fail).then(done);
 });

 it('transitions a transform', function(done) {
 Plotly.restyle(gd, {
 'transforms[0]': {
 enabled: true,
 type: 'filter',
 operation: '<',
 filtersrc: 'x',
 value: 10
 }
 }, [0]).then(function() {
 expect(gd._fullData[0].transforms).toEqual([{
 enabled: true,
 type: 'filter',
 operation: '<',
 filtersrc: 'x',
 value: 10
 }]);

 return Plots.transition(gd, [{
 'transforms[0].operation': '>'
 }], null, [0],
 {redraw: true, duration: transitionDuration},
 {duration: transitionDuration, easing: 'cubic-in-out'}
 );
 }).then(function() {
 expect(gd._fullData[0].transforms).toEqual([{
 enabled: true,
 type: 'filter',
 operation: '>',
 filtersrc: 'x',
 value: 10
 }]);
 }).catch(fail).then(done);
 });

 // This doesn't really test anything that the above tests don't cover, but it combines
 // the behavior and attempts to ensure chaining and events happen in the correct order.
 it('transitions may be chained', function(done) {
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
Improved animation merging for layout and traces by rreusser · Pull Request #1041 · plotly/plotly.js · GitHub
Skip to content
Improved animation merging for layout and traces#1041

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

rreusser
merged 9 commits into

from

Oct 17, 2016
 Merged


Improved animation merging for layout and traces
#1041
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


9 commits


Select commit
Hold shift + click to select a range
0e86edd

Add failing test fro transition transform


rreusser Oct 14, 2016
a0da51d

Mostly factor out trace and layout merge logic


rreusser Oct 14, 2016
b612ac3

Methods for extendTrace and extendLayout


rreusser Oct 14, 2016
8d35b71

Lock down and document the unimplemented case in expandObjectPaths


rreusser Oct 14, 2016
c93c18f

Verify function of animated transforms


rreusser Oct 14, 2016
33e23db

Tweak trivial things to get the test to pass


rreusser Oct 14, 2016
511de3d

Delete best guess at failing test


rreusser Oct 14, 2016
adee9b2

Add missing layout container arrays


rreusser Oct 17, 2016
e085a53

Add additional docs on expandObjectPaths subtleties


rreusser Oct 17, 2016

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


69 changes: 61 additions & 8 deletions
src/lib/index.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -589,18 +589,36 @@ lib.objectFromPath = function(path, value) {
/**
 * Iterate through an object in-place, converting dotted properties to objects.
 *
 * @example
 * lib.expandObjectPaths({'nested.test.path': 'value'});
 * // returns { nested: { test: {path: 'value'}}}
 * Examples:
 *
 * lib.expandObjectPaths({'nested.test.path': 'value'});
 * => { nested: { test: {path: 'value'}}}
 *
 * It also handles array notation, e.g.:
 *
 * lib.expandObjectPaths({'foo[1].bar': 'value'});
 * => { foo: [null, {bar: value}] }
 *
 * It handles merges the results when two properties are specified in parallel:
 *
 * lib.expandObjectPaths({'foo[1].bar': 10, 'foo[0].bar': 20});
 * => { foo: [{bar: 10}, {bar: 20}] }
 *
 * It does NOT, however, merge mulitple mutliply-nested arrays::
 *
 * lib.expandObjectPaths({'marker[1].range[1]': 5, 'marker[1].range[0]': 4})
 * => { marker: [null, {range: 4}] }
 */

// Store this to avoid recompiling regex on every prop since this may happen many
// many times for animations.
// TODO: Premature optimization? Remove?
var dottedPropertyRegex = /^([^\.]*)\../;
// Store this to avoid recompiling regex on *every* prop since this may happen many
// many times for animations. Could maybe be inside the function. Not sure about
// 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) {
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for reusing this method 🎉
 var match, key, prop, datum;
 var match, key, prop, datum, idx, dest, trailingPath;
 if(typeof data === 'object' && !Array.isArray(data)) {
 for(key in data) {
 if(data.hasOwnProperty(key)) {
Expand All@@ -611,12 +629,47 @@ lib.expandObjectPaths = function(data) {
 delete data[key];

 data[prop] = lib.extendDeepNoArrays(data[prop] || {}, lib.objectFromPath(key, lib.expandObjectPaths(datum))[prop]);
 } else if((match = key.match(indexedPropertyRegex))) {
 datum = data[key];

 prop = match[1];
 idx = parseInt(match[2]);

 delete data[key];

 data[prop] = data[prop] || [];

 if(match[3] === '.') {
 // This is the case where theere are subsequent properties into which
 // we must recurse, e.g. transforms[0].value
 trailingPath = match[4];
 dest = data[prop][idx] = data[prop][idx] || {};

 // NB: Extend deep no arrays prevents this from working on multiple
 // nested properties in the same object, e.g.
 //
 // {
 // foo[0].bar[1].range
 // foo[0].bar[0].range
 // }
 //
 // In this case, the extendDeepNoArrays will overwrite one array with
 // the other, so that both properties *will not* be present in the
 // result. Fixing this would require a more intelligent tracking
 // of changes and merging than extendDeepNoArrays currently accomplishes.
 lib.extendDeepNoArrays(dest, lib.objectFromPath(trailingPath, lib.expandObjectPaths(datum)));
 } else {
 // This is the case where this property is the end of the line,
 // e.g. xaxis.range[0]
 data[prop][idx] = lib.expandObjectPaths(datum);
 }
 } else {
 data[key] = lib.expandObjectPaths(data[key]);
 }
 }
 }
 }

 return data;
};

Expand Down

106 changes: 89 additions & 17 deletions
src/plots/plots.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1299,7 +1299,7 @@ plots.modifyFrames = function(gd, operations) {
 */
plots.computeFrame = function(gd, frameName) {
 var frameLookup = gd._transitionData._frameHash;
 var i, traceIndices, traceIndex, expandedObj, destIndex, copy;
 var i, traceIndices, traceIndex, destIndex;

 var framePtr = frameLookup[frameName];

Expand All@@ -1326,9 +1326,7 @@ plots.computeFrame = function(gd, frameName) {
 // Merge, starting with the last and ending with the desired frame:
 while((framePtr = frameStack.pop())) {
 if(framePtr.layout) {
 copy = Lib.extendDeepNoArrays({}, framePtr.layout);
 expandedObj = Lib.expandObjectPaths(copy);
 result.layout = Lib.extendDeepNoArrays(result.layout || {}, expandedObj);
 result.layout = plots.extendLayout(result.layout, framePtr.layout);
 }

 if(framePtr.data) {
Expand DownExpand Up@@ -1363,16 +1361,99 @@ plots.computeFrame = function(gd, frameName) {
 result.traces[destIndex] = traceIndex;
 }

 copy = Lib.extendDeepNoArrays({}, framePtr.data[i]);
 expandedObj = Lib.expandObjectPaths(copy);
 result.data[destIndex] = Lib.extendDeepNoArrays(result.data[destIndex] || {}, expandedObj);
 result.data[destIndex] = plots.extendTrace(result.data[destIndex], framePtr.data[i]);
 }
 }
 }

 return result;
};

/**
 * Extend an object, treating container arrays very differently by extracting
 * their contents and merging them separately.
 *
 * This exists so that we can extendDeepNoArrays and avoid stepping into data
 * arrays without knowledge of the plot schema, but so that we may also manually
 * recurse into known container arrays, such as transforms.
 *
 * See extendTrace and extendLayout below for usage.
 */
plots.extendObjectWithContainers = function(dest, src, containerPaths) {
 var containerProp, containerVal, i, j, srcProp, destProp, srcContainer, destContainer;
 var copy = Lib.extendDeepNoArrays({}, src || {});
 var expandedObj = Lib.expandObjectPaths(copy);
 var containerObj = {};

 // Step through and extract any container properties. Otherwise extendDeepNoArrays
 // will clobber any existing properties with an empty array and then supplyDefaults
 // will reset everything to defaults.
 if(containerPaths && containerPaths.length) {
 for(i = 0; i < containerPaths.length; i++) {
 containerProp = Lib.nestedProperty(expandedObj, containerPaths[i]);
 containerVal = containerProp.get();
 containerProp.set(null);
 Lib.nestedProperty(containerObj, containerPaths[i]).set(containerVal);
 }
 }

 dest = Lib.extendDeepNoArrays(dest || {}, expandedObj);

 if(containerPaths && containerPaths.length) {
 for(i = 0; i < containerPaths.length; i++) {
 srcProp = Lib.nestedProperty(containerObj, containerPaths[i]);
 srcContainer = srcProp.get();

 if(!srcContainer) continue;

 destProp = Lib.nestedProperty(dest, containerPaths[i]);

 destContainer = destProp.get();
 if(!Array.isArray(destContainer)) {
 destContainer = [];
 destProp.set(destContainer);
 }

 for(j = 0; j < srcContainer.length; j++) {
 destContainer[j] = plots.extendObjectWithContainers(destContainer[j], srcContainer[j]);
 }
 }
 }

 return dest;
};

/*
 * Extend a trace definition. This method:
 *
 * 1. directly transfers any array references
 * 2. manually recurses into container arrays like transforms
 *
 * The result is the original object reference with the new contents merged in.
 */
plots.extendTrace = function(destTrace, srcTrace) {
 return plots.extendObjectWithContainers(destTrace, srcTrace, ['transforms']);
@etpinardetpinardOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/*
 * Extend a layout definition. This method:
 *
 * 1. directly transfers any array references (not critically important for
 * layout since there aren't really data arrays)
 * 2. manually recurses into container arrays like annotations
 *
 * The result is the original object reference with the new contents merged in.
 */
plots.extendLayout = function(destLayout, srcLayout) {
 return plots.extendObjectWithContainers(destLayout, srcLayout, [
 'annotations',
 'shapes',
 'images',
 'sliders',
 'updatemenus'
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
 ]);
};

/**
 * Transition to a set of new data and layout properties
 *
Expand DownExpand Up@@ -1411,16 +1492,7 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)

 transitionedTraces.push(traceIdx);

 // This is a multi-step process. First clone w/o arrays so that
 // we're not modifying the original:
 var update = Lib.extendDeepNoArrays({}, data[i]);

 // Then expand object paths since we don't obey object-overwrite
 // semantics here:
 update = Lib.expandObjectPaths(update);

 // Finally apply the update (without copying arrays, of course):
 Lib.extendDeepNoArrays(gd.data[traceIndices[i]], update);
 gd.data[traceIndices[i]] = plots.extendTrace(gd.data[traceIndices[i]], data[i]);
 }

 // Follow the same procedure. Clone it so we don't mangle the input, then
Expand Down

2 changes: 1 addition & 1 deletion
test/jasmine/tests/animate_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -84,7 +84,7 @@ describe('Test animate API', function() {
 });

 Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(function() {
 Plotly.addFrames(gd, mockCopy.frames);
return Plotly.addFrames(gd, mockCopy.frames);
 }).then(done);
 });

Expand Down

84 changes: 84 additions & 0 deletions
test/jasmine/tests/lib_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -537,6 +537,90 @@ describe('Test lib.js:', function() {
 expect(input).toBe(expanded);
 expect(origArray).toBe(newArray);
 });

 it('expands bracketed array notation', function() {
 var input = {'marker[1]': {color: 'red'}};
 var expected = {marker: [undefined, {color: 'red'}]};
 expect(Lib.expandObjectPaths(input)).toEqual(expected);
 });

 it('expands nested arrays', function() {
 var input = {'marker[1].range[1]': 5};
 var expected = {marker: [undefined, {range: [undefined, 5]}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array with more nested attributes', function() {
 var input = {'marker[1]': {'color.alpha': 2}};
 var expected = {marker: [undefined, {color: {alpha: 2}}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation without further nesting', function() {
 var input = {'marker[1]': 8};
 var expected = {marker: [undefined, 8]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation with further nesting', function() {
 var input = {'marker[1].size': 8};
 var expected = {marker: [undefined, {size: 8}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('expands bracketed array notation with further nesting', function() {
 var input = {'marker[1].size.magnitude': 8};
 var expected = {marker: [undefined, {size: {magnitude: 8}}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 it('combines changes with single array nesting', function() {
 var input = {'marker[1].foo': 5, 'marker[0].foo': 4};
 var expected = {marker: [{foo: 4}, {foo: 5}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });

 // 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
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those arrays are declared as info_array e.g. pie domain.
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does
varinput={'marker[1].range': [4,5]};Lib.expandObjectPaths(input);// => {marker: [undefined, {range: [4, 5]}]}
??
@rreusserrreusserOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]}
@rreusserrreusserOct 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@rreusserrreusserOct 17, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.
 // not data arrays but yes on arrays that were previously expanded. This is a bit
 // tricky to get to work just right and currently doesn't have any known use since
 // container arrays are not multiply nested.
 //
 // Additional notes on what works or what doesn't work. This case does *not* work
 // because the two nested arrays that would result from the expansion need to be
 // deep merged.
 //
 // Lib.expandObjectPaths({'marker.range[0]': 5, 'marker.range[1]': 2})
 //
 // // => {marker: {range: [null, 2]}}
 //
 // This case *does* work becuase the array merging does not require a deep extend:
 //
 // Lib.expandObjectPaths({'range[0]': 5, 'range[1]': 2}
 //
 // // => {range: [5, 2]}
 //
 // Finally note that this case works fine becuase there's no merge necessary:
 //
 // Lib.expandObjectPaths({'marker.range[1]': 2})
 //
 // // => {marker: {range: [null, 2]}}
 //
@etpinardetpinardOct 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice 👍
 /*
 it('combines changes', function() {
 var input = {'marker[1].range[1]': 5, 'marker[1].range[0]': 4};
 var expected = {marker: [undefined, {range: [4, 5]}]};
 var computed = Lib.expandObjectPaths(input);
 expect(computed).toEqual(expected);
 });
 */
 });

 describe('coerce', function() {
Expand Down

35 changes: 35 additions & 0 deletions
test/jasmine/tests/transition_test.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -65,6 +65,41 @@ function runTests(transitionDuration) {
 }).catch(fail).then(done);
 });

 it('transitions a transform', function(done) {
 Plotly.restyle(gd, {
 'transforms[0]': {
 enabled: true,
 type: 'filter',
 operation: '<',
 filtersrc: 'x',
 value: 10
 }
 }, [0]).then(function() {
 expect(gd._fullData[0].transforms).toEqual([{
 enabled: true,
 type: 'filter',
 operation: '<',
 filtersrc: 'x',
 value: 10
 }]);

 return Plots.transition(gd, [{
 'transforms[0].operation': '>'
 }], null, [0],
 {redraw: true, duration: transitionDuration},
 {duration: transitionDuration, easing: 'cubic-in-out'}
 );
 }).then(function() {
 expect(gd._fullData[0].transforms).toEqual([{
 enabled: true,
 type: 'filter',
 operation: '>',
 filtersrc: 'x',
 value: 10
 }]);
 }).catch(fail).then(done);
 });

 // This doesn't really test anything that the above tests don't cover, but it combines
 // the behavior and attempts to ensure chaining and events happen in the correct order.
 it('transitions may be chained', function(done) {
Expand Down