From 4a1cb823a086fbde6b299a88b637bd1f187c33f9 Mon Sep 17 00:00:00 2001 From: Sean Lilley Date: Wed, 6 Sep 2017 20:02:04 -0400 Subject: [PATCH 1/3] Fix discrepency between ids and names in AnimationCollection --- Source/Scene/Model.js | 76 ++++++++++-------------- Source/Scene/ModelAnimation.js | 2 +- Source/Scene/ModelAnimationCollection.js | 47 ++++++++++----- Specs/Scene/ModelSpec.js | 20 +++---- 4 files changed, 72 insertions(+), 73 deletions(-) diff --git a/Source/Scene/Model.js b/Source/Scene/Model.js index 5fb81de4deea..5001d1c5c14d 100644 --- a/Source/Scene/Model.js +++ b/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); diff --git a/Source/Scene/ModelAnimation.js b/Source/Scene/ModelAnimation.js index f47ea3af611c..d5bca39de24d 100644 --- a/Source/Scene/ModelAnimation.js +++ b/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; diff --git a/Source/Scene/ModelAnimationCollection.js b/Source/Scene/ModelAnimationCollection.js index 0ce83970ac46..c9d54dfa8a51 100644 --- a/Source/Scene/ModelAnimationCollection.js +++ b/Source/Scene/ModelAnimationCollection.js @@ -82,6 +82,16 @@ 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. *

@@ -144,24 +154,31 @@ define([ if (!defined(animations)) { throw new DeveloperError('Animations are not loaded. Wait for Model.readyPromise to resolve.'); } + if (!defined(options.name)) { + throw new DeveloperError('options.name must be defined.'); + } + if (defined(options.speedup) && (options.speedup <= 0.0)) { + throw new DeveloperError('options.speedup must be greater than zero.'); + } //>>includeEnd('debug'); - var animation = animations[options.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; + } + } //>>includeStart('debug', pragmas.debug); - if (!defined(animation)) { + if (!defined(index)) { throw new DeveloperError('options.name must be a valid animation name.'); } - - if (defined(options.speedup) && (options.speedup <= 0.0)) { - throw new DeveloperError('options.speedup must be greater than zero.'); - } //>>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 +220,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; }; diff --git a/Specs/Scene/ModelSpec.js b/Specs/Scene/ModelSpec.js index 366752e5853a..2f591a60eab8 100644 --- a/Specs/Scene/ModelSpec.js +++ b/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(); @@ -1207,7 +1207,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 +1253,7 @@ defineSuite([ var animations = animBoxesModel.activeAnimations; var a = animations.add({ - name : 1, + name : 'animation_1', startTime : time, delay : 1.0 }); @@ -1277,7 +1277,7 @@ defineSuite([ var animations = animBoxesModel.activeAnimations; var a = animations.add({ - name : 1, + name : 'animation_1', startTime : time, stopTime : stopTime }); @@ -1301,7 +1301,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 +1326,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 +1353,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 +1383,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 +1417,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 }); From 03de718d9377b47fc6ea7d35cc02503f8d2cc7d2 Mon Sep 17 00:00:00 2001 From: Sean Lilley Date: Wed, 6 Sep 2017 20:13:24 -0400 Subject: [PATCH 2/3] Updated CHANGES.md --- CHANGES.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index aa13b9f20e12..b09ad7292c39 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,9 @@ Change Log ========== +### 1.38 - 2017-10-02 + +* 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 From 3a06502a1160ebf5cc5566806688df3f174a0f1f Mon Sep 17 00:00:00 2001 From: Sean Lilley Date: Thu, 7 Sep 2017 19:29:10 -0400 Subject: [PATCH 3/3] Add options.index --- CHANGES.md | 1 + Source/Scene/ModelAnimationCollection.js | 25 +++++++++++++---- Specs/Scene/ModelSpec.js | 35 ++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 5 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index b09ad7292c39..d4b5dfc10bdb 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -2,6 +2,7 @@ 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 diff --git a/Source/Scene/ModelAnimationCollection.js b/Source/Scene/ModelAnimationCollection.js index c9d54dfa8a51..68d2187198f1 100644 --- a/Source/Scene/ModelAnimationCollection.js +++ b/Source/Scene/ModelAnimationCollection.js @@ -99,7 +99,8 @@ define([ *

* * @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 options.id is undefined. + * @param {Number} [options.index] The glTF animation index that identifies the animation. Must be defined if options.name is undefined. * @param {JulianDate} [options.startTime] The scene time to start playing the animation. When this is undefined, the animation starts at the next frame. * @param {Number} [options.delay=0.0] The delay, in seconds, from startTime to start playing. * @param {JulianDate} [options.stopTime] The scene time to stop playing the animation. When this is undefined, the animation is played for its full duration. @@ -111,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({ @@ -154,14 +162,21 @@ define([ if (!defined(animations)) { throw new DeveloperError('Animations are not loaded. Wait for Model.readyPromise to resolve.'); } - if (!defined(options.name)) { - throw new DeveloperError('options.name must be defined.'); + 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'); + if (defined(options.index)) { + return add(this, options.index, options); + } + // Find the index of the animation with the given name var index; var length = animations.length; diff --git a/Specs/Scene/ModelSpec.js b/Specs/Scene/ModelSpec.js index 2f591a60eab8..7ae7ba9e3385 100644 --- a/Specs/Scene/ModelSpec.js +++ b/Specs/Scene/ModelSpec.js @@ -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() {