Fix discrepency between ids and names in AnimationCollection

CHANGES.md

@@ -1,5 +1,10 @@
 Change Log
 ==========
+### 1.38 - 2017-10-02
+
+* Added ability to add an animation to `ModelAnimationCollection` by its index. [#5815](https://github.com/AnalyticalGraphicsInc/cesium/pull/5815)
+* Fixed a bug in `ModelAnimationCollection` that caused adding an animation by its name to throw an error. [#5815](https://github.com/AnalyticalGraphicsInc/cesium/pull/5815)
+
 ### 1.37 - 2017-09-01
 
 * Breaking changes

Source/Scene/Model.js

@@ -289,20 +289,6 @@ define([
         this.ready = true;
     };
 
-    function getAnimationIds(cachedGltf) {
-        var animationIds = [];
-        if (defined(cachedGltf)) {
-            var animations = cachedGltf.animations;
-            for (var id in animations) {
-                if (animations.hasOwnProperty(id)) {
-                    animationIds.push(id);
-                }
-            }
-        }
-
-        return animationIds;
-    }
-
     var gltfCache = {};
 
     ///////////////////////////////////////////////////////////////////////////
@@ -368,7 +354,6 @@ define([
         this._cacheKey = cacheKey;
         this._cachedGltf = undefined;
         this._releaseGltfJson = defaultValue(options.releaseGltfJson, false);
-        this._animationIds = undefined;
 
         var cachedGltf;
         if (defined(cacheKey) && defined(gltfCache[cacheKey]) && gltfCache[cacheKey].ready) {
@@ -2523,48 +2508,48 @@ define([
         }
         loadResources.createRuntimeAnimations = false;
 
-        model._runtime.animations = {};
+        model._runtime.animations = [];
 
         var runtimeNodes = model._runtime.nodes;
         var animations = model.gltf.animations;
         var accessors = model.gltf.accessors;
 
-        for (var animationId in animations) {
-            if (animations.hasOwnProperty(animationId)) {
-                var animation = animations[animationId];
-                var channels = animation.channels;
-                var samplers = animation.samplers;
-
-                // Find start and stop time for the entire animation
-                var startTime = Number.MAX_VALUE;
-                var stopTime = -Number.MAX_VALUE;
+        var length = animations.length;
+        for (var i = 0; i < length; ++i) {
+            var animation = animations[i];
+            var channels = animation.channels;
+            var samplers = animation.samplers;
 
-                var length = channels.length;
-                var channelEvaluators = new Array(length);
+            // Find start and stop time for the entire animation
+            var startTime = Number.MAX_VALUE;
+            var stopTime = -Number.MAX_VALUE;
 
-                for (var i = 0; i < length; ++i) {
-                    var channel = channels[i];
-                    var target = channel.target;
-                    var path = target.path;
-                    var sampler = samplers[channel.sampler];
-                    var input = ModelAnimationCache.getAnimationParameterValues(model, accessors[sampler.input]);
-                    var output = ModelAnimationCache.getAnimationParameterValues(model, accessors[sampler.output]);
+            var channelsLength = channels.length;
+            var channelEvaluators = new Array(channelsLength);
 
-                    startTime = Math.min(startTime, input[0]);
-                    stopTime = Math.max(stopTime, input[input.length - 1]);
+            for (var j = 0; j < channelsLength; ++j) {
+                var channel = channels[j];
+                var target = channel.target;
+                var path = target.path;
+                var sampler = samplers[channel.sampler];
+                var input = ModelAnimationCache.getAnimationParameterValues(model, accessors[sampler.input]);
+                var output = ModelAnimationCache.getAnimationParameterValues(model, accessors[sampler.output]);
 
-                    var spline = ModelAnimationCache.getAnimationSpline(model, animationId, animation, channel.sampler, sampler, input, path, output);
+                startTime = Math.min(startTime, input[0]);
+                stopTime = Math.max(stopTime, input[input.length - 1]);
 
-                    // GLTF_SPEC: Support more targets like materials. https://github.com/KhronosGroup/glTF/issues/142
-                    channelEvaluators[i] = getChannelEvaluator(model, runtimeNodes[target.node], target.path, spline);
-                }
+                var spline = ModelAnimationCache.getAnimationSpline(model, i, animation, channel.sampler, sampler, input, path, output);
 
-                model._runtime.animations[animationId] = {
-                    startTime : startTime,
-                    stopTime : stopTime,
-                    channelEvaluators : channelEvaluators
-                };
+                // GLTF_SPEC: Support more targets like materials. https://github.com/KhronosGroup/glTF/issues/142
+                channelEvaluators[j] = getChannelEvaluator(model, runtimeNodes[target.node], target.path, spline);
             }
+
+            model._runtime.animations[i] = {
+                name : animation.name,
+                startTime : startTime,
+                stopTime : stopTime,
+                channelEvaluators : channelEvaluators
+            };
         }
     }
 
@@ -4551,7 +4536,6 @@ define([
                     processPbrMetallicRoughness(this.gltf, options);
                     // We do this after to make sure that the ids don't change
                     addBuffersToLoadResources(this);
-                    this._animationIds = getAnimationIds(this.gltf);
 
                     if (!this._loadRendererResourcesFromCache) {
                         parseBufferViews(this);

Source/Scene/ModelAnimation.js

@@ -30,7 +30,7 @@ define([
      * @see ModelAnimationCollection#add
      */
     function ModelAnimation(options, model, runtimeAnimation) {
-        this._name = options.name;
+        this._name = runtimeAnimation.name;
         this._startTime = JulianDate.clone(options.startTime);
         this._delay = defaultValue(options.delay, 0.0); // in seconds
         this._stopTime = options.stopTime;

Source/Scene/ModelAnimationCollection.js

@@ -82,14 +82,25 @@ define([
         }
     });
 
+    function add(collection, index, options) {
+        var model = collection._model;
+        var animations = model._runtime.animations;
+        var animation = animations[index];
+        var scheduledAnimation = new ModelAnimation(options, model, animation);
+        collection._scheduledAnimations.push(scheduledAnimation);
+        collection.animationAdded.raiseEvent(model, scheduledAnimation);
+        return scheduledAnimation;
+    }
+
     /**
      * Creates and adds an animation with the specified initial properties to the collection.
      * <p>
      * This raises the {@link ModelAnimationCollection#animationAdded} event so, for example, a UI can stay in sync.
      * </p>
      *
      * @param {Object} options Object with the following properties:
-     * @param {String} options.name The glTF animation name that identifies the animation.
+     * @param {String} [options.name] The glTF animation name that identifies the animation. Must be defined if <code>options.id</code> is <code>undefined</code>.
+     * @param {Number} [options.index] The glTF animation index that identifies the animation. Must be defined if <code>options.name</code> is <code>undefined</code>.
      * @param {JulianDate} [options.startTime] The scene time to start playing the animation.  When this is <code>undefined</code>, the animation starts at the next frame.
      * @param {Number} [options.delay=0.0] The delay, in seconds, from <code>startTime</code> to start playing.
      * @param {JulianDate} [options.stopTime] The scene time to stop playing the animation.  When this is <code>undefined</code>, the animation is played for its full duration.
@@ -101,16 +112,23 @@ define([
      *
      * @exception {DeveloperError} Animations are not loaded.  Wait for the {@link Model#readyPromise} to resolve.
      * @exception {DeveloperError} options.name must be a valid animation name.
+     * @exception {DeveloperError} options.index must be a valid animation index.
+     * @exception {DeveloperError} Either options.name or options.index must be defined.
      * @exception {DeveloperError} options.speedup must be greater than zero.
      *
      * @example
-     * // Example 1. Add an animation
+     * // Example 1. Add an animation by name
      * model.activeAnimations.add({
      *   name : 'animation name'
      * });
      *
+     * // Example 2. Add an animation by index
+     * model.activeAnimations.add({
+     *   index : 0
+     * });
+     *
      * @example
-     * // Example 2. Add an animation and provide all properties and events
+     * // Example 3. Add an animation and provide all properties and events
      * var startTime = Cesium.JulianDate.now();
      *
      * var animation = model.activeAnimations.add({
@@ -144,24 +162,38 @@ define([
         if (!defined(animations)) {
             throw new DeveloperError('Animations are not loaded.  Wait for Model.readyPromise to resolve.');
         }
+        if (!defined(options.name) && !defined(options.index)) {
+            throw new DeveloperError('Either options.name or options.index must be defined.');
+        }
+        if (defined(options.speedup) && (options.speedup <= 0.0)) {
+            throw new DeveloperError('options.speedup must be greater than zero.');
+        }
+        if (defined(options.index) && (options.index >= animations.length || options.index < 0)) {
+            throw new DeveloperError('options.index must be a valid animation index.');
+        }
         //>>includeEnd('debug');
 
-        var animation = animations[options.name];
+        if (defined(options.index)) {
+            return add(this, options.index, options);
+        }
 
-        //>>includeStart('debug', pragmas.debug);
-        if (!defined(animation)) {
-            throw new DeveloperError('options.name must be a valid animation name.');
+        // Find the index of the animation with the given name
+        var index;
+        var length = animations.length;
+        for (var i = 0; i < length; ++i) {
+            if (animations[i].name === options.name) {
+                index = i;
+                break;
+            }
         }
 
-        if (defined(options.speedup) && (options.speedup <= 0.0)) {
-            throw new DeveloperError('options.speedup must be greater than zero.');
+        //>>includeStart('debug', pragmas.debug);
+        if (!defined(index)) {
+            throw new DeveloperError('options.name must be a valid animation name.');
         }
         //>>includeEnd('debug');
 
-        var scheduledAnimation = new ModelAnimation(options, model, animation);
-        this._scheduledAnimations.push(scheduledAnimation);
-        this.animationAdded.raiseEvent(model, scheduledAnimation);
-        return scheduledAnimation;
+        return add(this, index, options);
     };
 
     /**
@@ -203,14 +235,12 @@ define([
         }
         //>>includeEnd('debug');
 
-        options = clone(options);
-
         var scheduledAnimations = [];
-        var animationIds = this._model._animationIds;
-        var length = animationIds.length;
+        var model = this._model;
+        var animations = model._runtime.animations;
+        var length = animations.length;
         for (var i = 0; i < length; ++i) {
-            options.name = animationIds[i];
-            scheduledAnimations.push(this.add(options));
+            scheduledAnimations.push(add(this, i, options));
         }
         return scheduledAnimations;
     };

Specs/Scene/ModelSpec.js

@@ -1135,10 +1135,10 @@ defineSuite([
         var spyAdd = jasmine.createSpy('listener');
         animations.animationAdded.addEventListener(spyAdd);
         var a = animations.add({
-            name : 1
+            name : 'animation_1'
         });
         expect(a).toBeDefined();
-        expect(a.name).toEqual(1);
+        expect(a.name).toEqual('animation_1');
         expect(a.startTime).not.toBeDefined();
         expect(a.delay).toEqual(0.0);
         expect(a.stopTime).not.toBeDefined();
@@ -1166,6 +1166,41 @@ defineSuite([
         animations.animationRemoved.removeEventListener(spyRemove);
     });
 
+    it('adds an animation by index', function() {
+        var animations = animBoxesModel.activeAnimations;
+        expect(animations.length).toEqual(0);
+
+        var spyAdd = jasmine.createSpy('listener');
+        animations.animationAdded.addEventListener(spyAdd);
+        var a = animations.add({
+            index : 1
+        });
+        expect(a).toBeDefined();
+        expect(a.name).toEqual('animation_1');
+        animations.remove(a);
+    });
+
+    it('add throws when name and index are not defined', function() {
+        var m = new Model();
+        expect(function() {
+            return m.activeAnimations.add();
+        }).toThrowDeveloperError();
+    });
+
+    it('add throws when index is invalid', function() {
+        var m = new Model();
+        expect(function() {
+            return m.activeAnimations.add({
+                index : -1
+            });
+        }).toThrowDeveloperError();
+        expect(function() {
+            return m.activeAnimations.add({
+                index : 2
+            });
+        }).toThrowDeveloperError();
+    });
+
     it('add throws when model is not loaded', function() {
         var m = new Model();
         expect(function() {
@@ -1207,7 +1242,7 @@ defineSuite([
         var time = JulianDate.fromDate(new Date('January 1, 2014 12:00:00 UTC'));
         var animations = animBoxesModel.activeAnimations;
         var a = animations.add({
-            name : 1,
+            name : 'animation_1',
             startTime : time,
             removeOnStop : true
         });
@@ -1253,7 +1288,7 @@ defineSuite([
 
         var animations = animBoxesModel.activeAnimations;
         var a = animations.add({
-            name : 1,
+            name : 'animation_1',
             startTime : time,
             delay : 1.0
         });
@@ -1277,7 +1312,7 @@ defineSuite([
 
         var animations = animBoxesModel.activeAnimations;
         var a = animations.add({
-            name : 1,
+            name : 'animation_1',
             startTime : time,
             stopTime : stopTime
         });
@@ -1301,7 +1336,7 @@ defineSuite([
         var time = JulianDate.fromDate(new Date('January 1, 2014 12:00:00 UTC'));
         var animations = animBoxesModel.activeAnimations;
         var a = animations.add({
-            name : 1,
+            name : 'animation_1',
             startTime : time,
             speedup : 1.5
         });
@@ -1326,7 +1361,7 @@ defineSuite([
         var time = JulianDate.fromDate(new Date('January 1, 2014 12:00:00 UTC'));
         var animations = animBoxesModel.activeAnimations;
         var a = animations.add({
-            name : 1,
+            name : 'animation_1',
             startTime : time,
             reverse : true
         });
@@ -1353,7 +1388,7 @@ defineSuite([
         var time = JulianDate.fromDate(new Date('January 1, 2014 12:00:00 UTC'));
         var animations = animBoxesModel.activeAnimations;
         var a = animations.add({
-            name : 1,
+            name : 'animation_1',
             startTime : time,
             loop : ModelAnimationLoop.REPEAT
         });
@@ -1383,7 +1418,7 @@ defineSuite([
         var time = JulianDate.fromDate(new Date('January 1, 2014 12:00:00 UTC'));
         var animations = animBoxesModel.activeAnimations;
         var a = animations.add({
-            name : 1,
+            name : 'animation_1',
             startTime : time,
             loop : ModelAnimationLoop.MIRRORED_REPEAT
         });
@@ -1417,7 +1452,7 @@ defineSuite([
             var time = JulianDate.fromDate(new Date('January 1, 2014 12:00:00 UTC'));
             var animations = m.activeAnimations;
             var a = animations.add({
-                name : 1,
+                name : 'animation_1',
                 startTime : time
             });