-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix race condition in animation resolution #1108
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.
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
Changes from 1 commit
0616495
31a77ac
445daef
e0d2e17
133b9d6
77aaad2
c100d6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1700,6 +1700,9 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts) | |
var aborted = false; | ||
|
||
function executeTransitions() { | ||
|
||
gd.emit('plotly_transitioning', []); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Subtle, but I didn't like that this was executed asynchronously with the transition actually starting, hence the move. |
||
|
||
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; | ||
}); | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, if I get this right, before this PR, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not quite, but similar. Sometimes it was zero because the promise resolved before both the frame and the transition were registered as complete. Now the promise doesn't resolve until both are done—which shouldn't be more than a couple ms difference, but that's enough to mess up the ordering. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep that makes sense. Thanks for the info! |
||
}).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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice test 🎉 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops. I removed the requestAnimationFrame around this, so the comment is now incorrect. 😭 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etpinard removed this comment and added a couple other very minor animation tweaks that drop callbacks if the plot is purged before they're executed—which turns out to be a great way to break the tests one out of every hundred times 😬 (The extra fix was mainly added here to get it to run circle-ci again without the failing enterprise thing) |
||
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); | ||
}); | ||
}); | ||
}); |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this must now be executed in two places before the transition is registered as having completed. That prevents it from resolving before the transition is actually complete. The transition duration is clipped to no greater than the frame duration, but this PR exists because that still didn't ensure that the promise resolved after both had completed, which made ordering difficult to rely upon.