Fix sycnchronous animation queues

rwaldron#1376 only fixed the problem when animation segments were enqueued in seperate passes through the event loop.  When two animation segments were enqueued on the same pass through the even loop we were starting both segments because the isRunning state was not being set until nextTick.

This should fix all that. I've updated some of the tests which I believe were written in a non-intuitive manner because of this bug. Instead of really understanding the problem I had just written the tests in a way that made them pass. There's probably a name for this sort of behavior. If not, we should invent a name.

Author: dtex

lib/animation.js

@@ -91,7 +91,7 @@ function Animation(target) {
 
   Animation.Segment.call(this);
 
-  this.defaultTarget = target;
+  this.defaultTarget = target || {};
 }
 
 util.inherits(Animation, Emitter);
@@ -152,8 +152,7 @@ Animation.prototype.enqueue = function(opts) {
     opts.target = this.defaultTarget;
   }
 
-  this.segments.push(new Animation.Segment(opts));
-
+  this.segments.push(opts);
 
   /* istanbul ignore if */
   if (!this.paused && !this.isRunning) {
@@ -170,10 +169,16 @@ Animation.prototype.enqueue = function(opts) {
  */
 Animation.prototype.next = function() {
 
-  if (this.segments.length > 0) {
-
-    Object.assign(this, this.segments.shift());
+  if (this.isRunning) {
+    return this;
+  } else {
+    this.isRunning = true;
+  }
 
+  if (this.segments.length > 0) {
+    var segment = new Animation.Segment(this.segments.shift());
+    
+    Object.assign(this, segment);
     this.paused = this.currentSpeed === 0 ? true : false;
 
     if (this.onstart) {
@@ -281,7 +286,6 @@ Animation.prototype.loopFunction = function(loop) {
   // Find the current timeline progress
   var progress = this.calculateProgress(loop.calledAt);
 
-
   // Find the left and right cuePoints/keyFrames;
   var indices = this.findIndices(progress);
 
@@ -320,14 +324,16 @@ Animation.prototype.loopFunction = function(loop) {
       this.endTime = this.startTime + this.scaledDuration;
     } else {
 
-      this.stop();
+      this.isRunning = false;
 
       if (this.oncomplete) {
         process.nextTick(this.oncomplete.bind(this));
       }
 
       if (this.segments.length > 0) {
-        this.next();
+        process.nextTick(() => { this.next(); });
+      } else {
+        this.stop();
       }
     }
   }
@@ -413,6 +419,11 @@ Animation.prototype.tweenedValue = function(indices, progress) {
     progress: null
   };
 
+  // This can happen if the clock ticks over to a new frame before a segment is ready run
+  if (typeof this.normalizedKeyFrames !== "object") {
+    this.normalizeKeyframes();
+  }
+
   var result = this.normalizedKeyFrames.map(function(keyFrame) {
     // Note: "this" is bound to the animation object
 

test/animation.js

@@ -360,7 +360,33 @@ exports["Animation -- Servo"] = {
     );
 
     test.done();
-  }
+  },
+  
+    enqueueOnSameTick: function(test) {
+      
+      test.expect(7);
+  
+      this.animation = new Animation(this.a);
+  
+      this.normalizeKeyframes = this.sandbox.stub(this.animation, "normalizeKeyframes");
+      
+      test.equal(this.animation.isRunning, false);
+      test.equal(this.animation.segments.length, 0);
+      
+      // This is the first segment so it should be immediately shifted off the queue
+      test.equal(this.animation.enqueue(this.segment.single), this.animation);
+      test.equal(this.animation.segments.length, 0);
+      test.equal(this.animation.isRunning, true);
+      
+      // This is the second segment so it should stay in the queue
+      test.equal(this.animation.enqueue(this.segment.single), this.animation);
+      test.equal(this.animation.segments.length, 1);
+      
+      process.nextTick(() => {
+        this.normalizeKeyframes.restore();
+        test.done();
+      });
+    }
 
 };
 
@@ -436,6 +462,7 @@ exports["Animation"] = {
     test.equal(this.animation.segments.length, 1);
     test.done();
   },
+
   next: function(test) {
     test.expect(12);
 
@@ -687,78 +714,6 @@ exports["Animation"] = {
     test.done();
   },
 
-  loopFunctiononcomplete: function(test) {
-    test.expect(2);
-
-    this.clock = this.sandbox.useFakeTimers();
-    this.tfb = this.sandbox.stub(Animation, "TemporalFallback", function() {});
-
-    var startTime = Date.now();
-    var loop = {
-      calledAt: startTime + 1000
-    };
-
-    this.animation.startTime = startTime;
-    this.animation.fallBackTime = 500;
-    this.animation.speed(1);
-
-
-    this.animation.normalizedKeyFrames = [];
-    this.animation.target = {};
-    this.animation.target[Animation.render] = function() {};
-    this.animation.playLoop = null;
-    this.animation.onloop = this.sandbox.stub();
-    this.animation.next = this.sandbox.stub();
-
-    var animation = this.animation;
-
-    this.sandbox.stub(Animation.prototype, "stop");
-    this.animation.oncomplete = function() {
-      test.equal(Animation.TemporalFallback.callCount, 1);
-      test.equal(this, animation);
-
-      animation.oncomplete = null;
-      test.done();
-    };
-
-    this.animation.loopFunction(loop);
-  },
-
-  loopFunctionNOoncompleteSegmentsRemaining: function(test) {
-    test.expect(1);
-
-    this.clock = this.sandbox.useFakeTimers();
-    this.tfb = this.sandbox.stub(Animation, "TemporalFallback", function() {});
-    this.stop = this.sandbox.stub(Animation.prototype, "stop");
-
-    var startTime = Date.now();
-    var loop = {
-      calledAt: startTime + 1000
-    };
-
-    this.animation.startTime = startTime;
-    this.animation.fallBackTime = 500;
-    this.animation.speed(1);
-
-
-    this.animation.normalizedKeyFrames = [];
-    this.animation.target = {};
-    this.animation.target[Animation.render] = function() {};
-    this.animation.playLoop = null;
-    this.animation.onloop = this.sandbox.stub();
-    this.animation.next = this.sandbox.stub();
-
-    this.nextTick = this.sandbox.stub(process, "nextTick");
-
-    this.animation.next = function() {
-      test.equal(this.stop.callCount, 1);
-      test.done();
-    }.bind(this);
-
-    this.animation.segments = [1, 2, 3, 4];
-    this.animation.loopFunction(loop);
-  },
-
   loopFunctionfallBackTimeWithPlayLoop: function(test) {
     test.expect(1);
 
@@ -1021,8 +976,9 @@ exports["Animation.Segment"] = {
   },
 
   instanceof: function(test) {
-    test.expect(1);
+    test.expect(2);
     test.equal(Animation({}) instanceof Animation, true);
+    test.equal(Animation() instanceof Animation, true);
     test.done();
   },
 

test/extended/animation.js

@@ -141,39 +141,61 @@ exports["Animation"] = {
     this.animation.enqueue(this.segment.long);
 
     setTimeout(function() {
+      // calledAt is a property on temporal tasks
       test.ok(this.animation.playLoop.calledAt);
     }.bind(this), 3000);
 
     setTimeout(function() {
+      // interval is the timer on our fallback
       test.ok(this.animation.playLoop.interval);
       this.animation.stop();
       test.done();
     }.bind(this), 6000);
 
   },
 
-  synchronousSegments: function(test) {
+  synchronousNextOnTemporal: function(test) {
 
+    var startTime = Date.now();
     this.animation = new Animation(this.servos);
+    test.expect(2);
 
-    test.expect(3);
-
-    this.animation.enqueue(this.segment.long);
-    this.animation.enqueue(this.segment.short);
+    var segment = Object.assign({}, this.segment.short);
+    segment.fps = 200;
+    segment.oncomplete = () => { 
+      test.equal(Math.abs(Date.now() - startTime - 500) <= 10, true);
+    };
+    this.animation.enqueue(segment);  
 
-    setTimeout(function() {
-      test.ok(Math.abs(this.animation.progress - 0.5) < 0.05);
-    }.bind(this), 3500);
+    segment.oncomplete = () => { 
+      test.equal(Math.abs(Date.now() - startTime - 1000) <= 10, true);
+      test.done();
+    };
+    this.animation.enqueue(segment);
 
-    setTimeout(function() {
-      test.ok(Math.abs(this.animation.progress - 0.5) < 0.51);
-    }.bind(this), 7350);
+  },
+      
+  synchronousNextOnFallback: function(test) {
 
-    setTimeout(function() {
-      test.ok(Math.abs(this.animation.progress) === 1);
-      test.done();
-    }.bind(this), 7700);
+    var startTime = Date.now();
+    this.animation = new Animation(this.servos);
+    test.expect(2);
 
+    var segment = Object.assign({}, this.segment.long);
+    segment.fps = 200;
+    segment.oncomplete = () => { 
+      test.equal(Math.abs(Date.now() - startTime - 7000) <= 10, true);
+    };
+    this.animation.enqueue(segment);
+    
+    segment = Object.assign({}, this.segment.short);
+    segment.fps = 200;
+    segment.oncomplete = () => { 
+      test.equal(Math.abs(Date.now() - startTime - 7500) <= 10, true);
+      test.done();
+    };
+    this.animation.enqueue(segment);
+      
   },
 
   /*
@@ -185,69 +207,27 @@ exports["Animation"] = {
    */
   roundedPi: function(test) {
     this.animation = new Animation(this.servos);
-    test.expect(2);
+    test.expect(1);
 
-    var complete = false;
     var tempSegment = this.segment.short;
 
     tempSegment.easing = "inSine";
     tempSegment.progress = 0.5;
 
-    tempSegment.oncomplete = function() {
-      complete = true;
-    };
-
-    this.animation.enqueue(tempSegment);
-
-    setTimeout(function() {
+    tempSegment.oncomplete = () => {
       test.ok(this.animation.progress === 1);
-      test.ok(complete === true);
       test.done();
-    }.bind(this), 300);
-
-  }
-
-};
-
-
-exports["Animation - Looping"] = {
-  setUp: function(done) {
-    this.sandbox = sinon.sandbox.create();
-
-    this.play = this.sandbox.stub(Animation.prototype, "play");
-
-    this.animation = new Animation({});
-    this.chain = {
-      result: [],
-      "@@render": function(args) {
-        this.result = this.result.concat(args);
-      },
-      "@@normalize": function(keyFrames) {
-        var last = [50, 0, -20];
-
-        // If user passes null as the first element in keyFrames use current position
-        if (keyFrames[0] === null) {
-          keyFrames[0] = {
-            position: last
-          };
-        }
-
-        return keyFrames;
-      }
     };
 
-    done();
-  },
+    this.animation.enqueue(tempSegment);
 
-  tearDown: function(done) {
-    this.sandbox.restore();
-    done();
   },
 
   loopFunction: function(test) {
-    test.expect(34);
+    test.expect(33);
 
-    this.animation = new Animation(this.chain);
+    this.animation = new Animation(this.servos);
+    
     this.animation.playLoop = {
       stop: this.sandbox.spy(),
     };
@@ -308,7 +288,6 @@ exports["Animation - Looping"] = {
     test.equal(this.findIndices.callCount, 5);
     test.equal(this.tweenedValue.callCount, 5);
 
-
     this.animation.loop = true;
     this.animation.progress = 1;
     this.animation.loopback = 1;
@@ -362,22 +341,18 @@ exports["Animation - Looping"] = {
     this.animation.loopback = 1;
     this.animation.metronomic = false;
     this.animation.reverse = false;
-    this.animation.segments.push(1, 2, 3);
     this.animation.loopFunction({ calledAt: 1 });
 
     test.equal(this.stop.callCount, 1);
-    test.equal(this.next.callCount, 1);
     test.equal(this.calculateProgress.callCount, 10);
     test.equal(this.findIndices.callCount, 10);
     test.equal(this.tweenedValue.callCount, 10);
 
-    this.animation.oncomplete = function() {
-      test.done();
-    };
-
     this.animation.loopFunction({ calledAt: 1 });
 
     test.equal(this.animation.target["@@render"].callCount, 11);
-  },
+
+    test.done();
+  }
 
 };