Fix race condition in animation resolution#1108
Conversation
👍 that sounds about right to me. |
| ).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.
Oops. I removed the requestAnimationFrame around this, so the comment is now incorrect. 😭
There was a problem hiding this comment.
@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)
Conflicts: src/plots/plots.js test/jasmine/tests/animate_test.js
Conflicts: test/jasmine/tests/animate_test.js
rreusser
changed the title
| }).catch(fail).then(done); | ||
| }); | ||
|
|
||
| it('null frames should not break everything', function(done) { |
There was a problem hiding this comment.
This is from a manually merged PR, I believe.
|
|
||
| function executeTransitions() { | ||
|
|
||
| gd.emit('plotly_transitioning', []); |
There was a problem hiding this comment.
Subtle, but I didn't like that this was executed asynchronously with the transition actually starting, hence the move.
| // still resolves or rejecdts this .animate call's promise. once it's | ||
| // complete. | ||
| nextFrame.onComplete = resolve; | ||
| nextFrame.onComplete = callbackOnNthTime(resolve, 2); |
There was a problem hiding this comment.
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.
|
@etpinard Looking back over it, I'm content with this. I added some explanatory comments and checked it over now that other animation PRs have been merged. The precise ordering and asynchronous details have been a challenge to lock down, so this makes it a bit more robust with frame duration === transition duration. No rush though. It's nice but not urgent. |
| {layout: {'xaxis.range': [0, 1]}}, | ||
| {frame: {redraw: true, duration: dur}, transition: {duration: dur}} | ||
| ).then(function() { | ||
| expect(redraws).toBe(1); |
There was a problem hiding this comment.
So, if I get this right, before this PR, redraws === 2 here?
There was a problem hiding this comment.
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.
Yep that makes sense. Thanks for the info!
| 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. |
There was a problem hiding this comment.
loving the honesty here 👍
There was a problem hiding this comment.
Aiming for useful comments. 😄
There was a problem hiding this comment.
Testing hammers this use-case in every conceivable way and only fails once in a rare while, as far as I'm aware only when karma interrupts and restarts the tests. When that happens, it goes kinda haywire and you just have to restart karma. In actual code though, it's so rare that I'd be maybe a little surprised if anyone ever encountered it.
|
💃 |
2 participants
This PR solves a minor issue that resulted in race conditions related to animation and relayout. Specifically, if the transition duration and frame duration were identical, it was hit or miss whether the animation would resolve before the transition had completed. The result is that things would happen slightly out of sequence for layout animations in which the relayout that fixes layer transforms would happen after the animation had already resolved. This wasn't a horrible issue, but it made it difficult to expect a specific sequence of events.
One problem this causes is that it will force animations to resolve only after the transition has completed. This is slightly different from the current behavior, but only for the odd (and perhaps invalid) case where the transitions are longer than the frames. I think maybe the transition duration should be clipped to
Math.min(transition.duration, frame.duration).I'd like to sit on this for just a day or two while I work through related issues and make sure there aren't other things going on.