Fix race condition in animation resolution #1108

rreusser · 2016-11-02T15:09:21Z

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.

etpinard · 2016-11-02T18:04:57Z

I think maybe the transition duration should be clipped to Math.min(transition.duration, frame.duration).

👍 that sounds about right to me.

etpinard · 2016-11-02T18:05:24Z

test/jasmine/tests/animate_test.js

+        ).then(function() {
+            // Something is delayed a single tick, it seems, so the redraw
+            // isn't triggered until next tick:
+            expect(relayouts).toBe(1);


nice test 🎉

Oops. I removed the requestAnimationFrame around this, so the comment is now incorrect. 😭

@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 · 2016-11-09T23:28:00Z

test/jasmine/tests/animate_test.js

            // Check only one animating was fired:
            expect(cnt).toEqual(1);

            // Check unused frames did not affect the current frame:
            expect(gd._fullLayout._currentFrame).toEqual('frame0');
        }).catch(fail).then(done);
    });
+
+    it('null frames should not break everything', function(done) {


This is from a manually merged PR, I believe.

rreusser · 2016-11-09T23:29:27Z

src/plots/plots.js

@@ -1714,6 +1722,9 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)
    var aborted = false;

    function executeTransitions() {
+
+        gd.emit('plotly_transitioning', []);


Subtle, but I didn't like that this was executed asynchronously with the transition actually starting, hence the move.

rreusser · 2016-11-09T23:31:00Z

src/plot_api/plot_api.js

@@ -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);


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.

rreusser · 2016-11-09T23:33:19Z

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

etpinard · 2016-11-10T00:18:50Z

test/jasmine/tests/animate_test.js

+            {layout: {'xaxis.range': [0, 1]}},
+            {frame: {redraw: true, duration: dur}, transition: {duration: dur}}
+        ).then(function() {
+            expect(redraws).toBe(1);


So, if I get this right, before this PR, redraws === 2 here?

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.

Yep that makes sense. Thanks for the info!

etpinard · 2016-11-10T00:20:42Z

src/plots/plots.js

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


loving the honesty here 👍

Aiming for useful comments. 😄

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.

etpinard · 2016-11-10T16:05:03Z

💃

Add slight modification to animation resolution

0616495

rreusser added bug status: in progress labels Nov 2, 2016

etpinard reviewed Nov 2, 2016

View reviewed changes

Improve robustness for dangling callbacks on Plotly.purge

31a77ac

rreusser added status: reviewable and removed status: in progress labels Nov 8, 2016

Remove invalid comments

445daef

rreusser added status: in progress and removed status: reviewable labels Nov 9, 2016

rreusser added 4 commits November 9, 2016 10:24

Make purging transitionData safer

e0d2e17

Merge branch 'master' into fix-layout-animation

133b9d6

Conflicts: src/plots/plots.js test/jasmine/tests/animate_test.js

Merge branch 'master' into fix-layout-animation

77aaad2

Conflicts: test/jasmine/tests/animate_test.js

Fix a broken animation test

c100d6c

rreusser changed the title ~~Add slight modification to animation resolution~~ Fix race condition in animation resolution Nov 9, 2016

rreusser added status: reviewable and removed status: in progress labels Nov 9, 2016

rreusser commented Nov 9, 2016

View reviewed changes

etpinard added this to the v1.20.0 milestone Nov 10, 2016

etpinard reviewed Nov 10, 2016

View reviewed changes

rreusser merged commit ab193b2 into master Nov 10, 2016

rreusser deleted the fix-layout-animation branch November 10, 2016 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

Fix race condition in animation resolution #1108

Fix race condition in animation resolution #1108

rreusser commented Nov 2, 2016 •

edited

Loading

etpinard commented Nov 2, 2016

etpinard Nov 2, 2016

rreusser Nov 2, 2016

rreusser Nov 8, 2016 •

edited

Loading

rreusser Nov 9, 2016

rreusser Nov 9, 2016

rreusser Nov 9, 2016 •

edited

Loading

rreusser commented Nov 9, 2016 •

edited

Loading

etpinard Nov 10, 2016 •

edited

Loading

rreusser Nov 10, 2016

etpinard Nov 10, 2016

etpinard Nov 10, 2016

rreusser Nov 10, 2016

rreusser Nov 10, 2016

etpinard commented Nov 10, 2016

Fix race condition in animation resolution #1108

Fix race condition in animation resolution #1108

Conversation

rreusser commented Nov 2, 2016 • edited Loading

etpinard commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

rreusser commented Nov 9, 2016 • edited Loading

etpinard Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Nov 10, 2016

rreusser commented Nov 2, 2016 •

edited

Loading

rreusser Nov 8, 2016 •

edited

Loading

rreusser Nov 9, 2016 •

edited

Loading

rreusser commented Nov 9, 2016 •

edited

Loading

etpinard Nov 10, 2016 •

edited

Loading