From 06164955a7a36da685306ee0abebfe697677201d Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Wed, 2 Nov 2016 11:02:49 -0400 Subject: [PATCH 1/5] Add slight modification to animation resolution --- src/plot_api/plot_api.js | 24 +++++++-- src/plots/plots.js | 9 ++-- test/jasmine/tests/animate_test.js | 85 ++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 6 deletions(-) diff --git a/src/plot_api/plot_api.js b/src/plot_api/plot_api.js index cbb4ad51e10..f081d6f3e7f 100644 --- a/src/plot_api/plot_api.js +++ b/src/plot_api/plot_api.js @@ -2213,6 +2213,20 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) { } } + // Execute a callback after the wrapper function has been called n times. + // This is used to defer the resolution until a transition has resovled *and* + // the frame has completed. If it's not done this way, then we get a race + // condition in which the animation might resolve before a transition is complete + // or vice versa. + function callbackOnNthTime(cb, n) { + var cnt = 0; + return function() { + if(cb && ++cnt === n) { + return cb(); + } + }; + } + return new Promise(function(resolve, reject) { function discardExistingFrames() { if(trans._frameQueue.length === 0) { @@ -2264,7 +2278,7 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) { // loop (which may exist as a result of a *different* .animate call) // still resolves or rejecdts this .animate call's promise. once it's // complete. - nextFrame.onComplete = resolve; + nextFrame.onComplete = callbackOnNthTime(resolve, 2); nextFrame.onInterrupt = reject; } @@ -2302,7 +2316,6 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) { // Execute the callback and unset it to ensure it doesn't // accidentally get called twice trans._currentFrame.onComplete(); - trans._currentFrame.onComplete = null; } var newFrame = trans._currentFrame = trans._frameQueue.shift(); @@ -2322,7 +2335,12 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) { helpers.coerceTraceIndices(gd, newFrame.frame.traces), newFrame.frameOpts, newFrame.transitionOpts - ); + ).then(function() { + if(newFrame.onComplete) { + newFrame.onComplete(); + } + + }); gd.emit('plotly_animatingframe', { name: newFrame.name, diff --git a/src/plots/plots.js b/src/plots/plots.js index 1254552dcec..21181aa2d5b 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -1700,6 +1700,9 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts) var aborted = false; function executeTransitions() { + + gd.emit('plotly_transitioning', []); + return new Promise(function(resolve) { // This flag is used to disabled things like autorange: gd._transitioning = true; @@ -1828,13 +1831,13 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts) var seq = [plots.previousPromises, interruptPreviousTransitions, prepareTransitions, executeTransitions]; - var transitionStarting = Lib.syncOrAsync(seq, gd); - if(!transitionStarting || !transitionStarting.then) transitionStarting = Promise.resolve(); + if(!transitionStarting || !transitionStarting.then) { + transitionStarting = Promise.resolve(); + } return transitionStarting.then(function() { - gd.emit('plotly_transitioning', []); return gd; }); }; diff --git a/test/jasmine/tests/animate_test.js b/test/jasmine/tests/animate_test.js index c5aeb5acb3b..795ff7d2743 100644 --- a/test/jasmine/tests/animate_test.js +++ b/test/jasmine/tests/animate_test.js @@ -579,3 +579,88 @@ describe('Test animate API', function() { }); }); }); + +describe('layout animation', function() { + 'use strict'; + + var gd; + var dur = 30; + + beforeEach(function(done) { + gd = createGraphDiv(); + var mockCopy = Lib.extendDeep({}, mock); + Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(done); + }); + + afterEach(function() { + Plotly.purge(gd); + destroyGraphDiv(); + }); + + it('redraws after a layout animation', function(done) { + var redraws = 0; + gd.on('plotly_redraw', function() {redraws++;}); + + Plotly.animate(gd, + {layout: {'xaxis.range': [0, 1]}}, + {frame: {redraw: true, duration: dur}, transition: {duration: dur}} + ).then(function() { + // Something is delayed a single tick, it seems, so the redraw + // isn't triggered until next tick: + expect(redraws).toBe(1); + }).catch(fail).then(done); + }); + + it('forces a relayout after layout animations', function(done) { + var relayouts = 0; + var restyles = 0; + var redraws = 0; + gd.on('plotly_relayout', function() {relayouts++;}); + gd.on('plotly_restyle', function() {restyles++;}); + gd.on('plotly_redraw', function() {redraws++;}); + + Plotly.animate(gd, + {layout: {'xaxis.range': [0, 1]}}, + {frame: {redraw: false, duration: dur}, transition: {duration: dur}} + ).then(function() { + // Something is delayed a single tick, it seems, so the redraw + // isn't triggered until next tick: + expect(relayouts).toBe(1); + expect(restyles).toBe(0); + expect(redraws).toBe(0); + }).catch(fail).then(done); + }); + + it('triggers plotly_animated after a single layout animation', function(done) { + var animateds = 0; + gd.on('plotly_animated', function() {animateds++;}); + + Plotly.animate(gd, [ + {layout: {'xaxis.range': [0, 1]}}, + ], {frame: {redraw: false, duration: dur}, transition: {duration: dur}} + ).then(function() { + // Wait a bit just to be sure: + setTimeout(function() { + expect(animateds).toBe(1); + done(); + }, dur); + }); + }); + + it('triggers plotly_animated after a multi-step layout animation', function(done) { + var animateds = 0; + gd.on('plotly_animated', function() {animateds++;}); + + Plotly.animate(gd, [ + {layout: {'xaxis.range': [0, 1]}}, + {layout: {'xaxis.range': [2, 4]}}, + ], {frame: {redraw: false, duration: dur}, transition: {duration: dur}} + ).then(function() { + // Wait a bit just to be sure: + setTimeout(function() { + expect(animateds).toBe(1); + done(); + }, dur); + }); + }); +}); From 31a77ac9fedd2d303e6f2e747ffe8bdada5cea39 Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Tue, 8 Nov 2016 16:16:03 -0500 Subject: [PATCH 2/5] Improve robustness for dangling callbacks on Plotly.purge --- src/plots/plots.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/plots/plots.js b/src/plots/plots.js index 21181aa2d5b..2b49005fd13 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -1087,6 +1087,10 @@ plots.purge = function(gd) { if(fullLayout._glcontainer !== undefined) fullLayout._glcontainer.remove(); if(fullLayout._geocontainer !== undefined) fullLayout._geocontainer.remove(); + // Ensure any dangling callbacks are simply dropped if the plot is purged. + // This is more or less only actually important for testing. + gd._transitionData._interruptCallbacks.length = 0; + // remove modebar if(fullLayout._modeBar) fullLayout._modeBar.destroy(); @@ -1787,6 +1791,11 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts) } function completeTransition(callback) { + // This a simple workaround for tests which purge the graph before animations + // have completed. That's not a very common case, so this is the simplest + // fix. + if(!gd._transitionData) return; + flushCallbacks(gd._transitionData._interruptCallbacks); return Promise.resolve().then(function() { From 445daef50f90ef0c6fbffe0464aa79e89a3f4e10 Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Tue, 8 Nov 2016 16:17:37 -0500 Subject: [PATCH 3/5] Remove invalid comments --- test/jasmine/tests/animate_test.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/jasmine/tests/animate_test.js b/test/jasmine/tests/animate_test.js index 795ff7d2743..d7a5a5fe95d 100644 --- a/test/jasmine/tests/animate_test.js +++ b/test/jasmine/tests/animate_test.js @@ -605,8 +605,6 @@ describe('layout animation', function() { {layout: {'xaxis.range': [0, 1]}}, {frame: {redraw: true, duration: dur}, transition: {duration: dur}} ).then(function() { - // Something is delayed a single tick, it seems, so the redraw - // isn't triggered until next tick: expect(redraws).toBe(1); }).catch(fail).then(done); }); @@ -623,8 +621,6 @@ describe('layout animation', function() { {layout: {'xaxis.range': [0, 1]}}, {frame: {redraw: false, duration: dur}, transition: {duration: dur}} ).then(function() { - // Something is delayed a single tick, it seems, so the redraw - // isn't triggered until next tick: expect(relayouts).toBe(1); expect(restyles).toBe(0); expect(redraws).toBe(0); From e0d2e170c5faeb7ec1bd8f1e899e8caa0db1d8d2 Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Wed, 9 Nov 2016 10:24:13 -0500 Subject: [PATCH 4/5] Make purging transitionData safer --- src/plots/plots.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/plots/plots.js b/src/plots/plots.js index 2b49005fd13..d0a3a59b681 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -1087,15 +1087,19 @@ plots.purge = function(gd) { if(fullLayout._glcontainer !== undefined) fullLayout._glcontainer.remove(); if(fullLayout._geocontainer !== undefined) fullLayout._geocontainer.remove(); - // Ensure any dangling callbacks are simply dropped if the plot is purged. - // This is more or less only actually important for testing. - gd._transitionData._interruptCallbacks.length = 0; - // remove modebar if(fullLayout._modeBar) fullLayout._modeBar.destroy(); - if(gd._transitionData && gd._transitionData._animationRaf) { - window.cancelAnimationFrame(gd._transitionData._animationRaf); + if(gd._transitionData) { + // Ensure any dangling callbacks are simply dropped if the plot is purged. + // This is more or less only actually important for testing. + if(gd._transitionData._interruptCallbacks) { + gd._transitionData._interruptCallbacks.length = 0; + } + + if(gd._transitionData._animationRaf) { + window.cancelAnimationFrame(gd._transitionData._animationRaf); + } } // data and layout From c100d6ccbbbce2d904590b5af11c5c5dda4f6514 Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Wed, 9 Nov 2016 18:13:26 -0500 Subject: [PATCH 5/5] Fix a broken animation test --- test/jasmine/tests/animate_test.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/jasmine/tests/animate_test.js b/test/jasmine/tests/animate_test.js index 4c5d9540f7a..f3eebb92d56 100644 --- a/test/jasmine/tests/animate_test.js +++ b/test/jasmine/tests/animate_test.js @@ -593,10 +593,11 @@ describe('Animate API details', function() { var gd; var dur = 30; + var mockCopy; beforeEach(function(done) { gd = createGraphDiv(); - var mockCopy = Lib.extendDeep({}, mock); + mockCopy = Lib.extendDeep({}, mock); Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(done); }); @@ -685,7 +686,9 @@ describe('Animate API details', function() { var cnt = 0; gd.on('plotly_animatingframe', function() {cnt++;}); - Plotly.animate(gd, ['frame0', null, undefined], {transition: {duration: 0}, frame: {duration: 0}}).then(function() { + Plotly.addFrames(gd, mockCopy.frames).then(function() { + return Plotly.animate(gd, ['frame0', null, undefined], {transition: {duration: 0}, frame: {duration: 0}}); + }).then(function() { // Check only one animating was fired: expect(cnt).toEqual(1);