Resopond to code review

CHANGES.md

@@ -6,13 +6,13 @@
 
 - Models and tilesets rendered with `ModelExperimental` must set `enableDebugWireframe` to true in order for `debugWireframe` to work in WebGL1. [#10344](https://github.com/CesiumGS/cesium/pull/10344)
 
-##### Fixes :wrench:
+##### Additions :tada:
 
-- Fixed the inaccurate computation of bounding spheres for `ModelExperimental`. [#10339](https://github.com/CesiumGS/cesium/pull/10339/)
+- Added `ModelAnimationCollection.animateWhilePaused` and `ModelAnimation.animationTime` to allow explicit control over a model's animations. [#9339](https://github.com/CesiumGS/cesium/pull/9339)
 
-##### Additions :tada:
+##### Fixes :wrench:
 
-- Added `ModelAnimationCollection.animateWhilePaused` and `ModelAnimation.animationTime` to allow explicit control over a model's animations.
+- Fixed the inaccurate computation of bounding spheres for `ModelExperimental`. [#10339](https://github.com/CesiumGS/cesium/pull/10339/)
 
 ### 1.93 - 2022-05-02
 

Source/Scene/ModelAnimation.js

@@ -38,15 +38,7 @@ function ModelAnimation(options, model, runtimeAnimation) {
   this._multiplier = defaultValue(options.multiplier, 1.0);
   this._reverse = defaultValue(options.reverse, false);
   this._loop = defaultValue(options.loop, ModelAnimationLoop.NONE);
-
-  /**
-   * If this is defined, it will be used to compute the local animation time
-   * instead of the scene's time.
-   *
-   * @type {ModelAnimation.AnimationTimeCallback}
-   * @default undefined
-   */
-  this.animationTime = options.animationTime;
+  this._animationTime = options.animationTime;
   this._prevAnimationDelta = undefined;
 
   /**
@@ -239,6 +231,19 @@ Object.defineProperties(ModelAnimation.prototype, {
       return this._loop;
     },
   },
+
+  /**
+   * If this is defined, it will be used to compute the local animation time
+   * instead of the scene's time.
+   *
+   * @type {ModelAnimation.AnimationTimeCallback}
+   * @default undefined
+   */
+  animationTime: {
+    get: function () {
+      return this._animationTime;
+    },
+  },
 });
 /**
  * A function used to compute the local animation time for a ModelAnimation.
@@ -249,8 +254,7 @@ Object.defineProperties(ModelAnimation.prototype, {
  * @returns {Number} Returns the local animation time.
  *
  * @example
- * // Use real time for model animation (also set
- * // ModelAnimationCollection#animateWhilePaused)
+ * // Use real time for model animation (assuming animateWhilePaused was set to true)
  * function animationTime(duration) {
  *     return Date.now() / 1000 / duration;
  * }
@@ -259,7 +263,7 @@ Object.defineProperties(ModelAnimation.prototype, {
  * // Offset the phase of the animation, so it starts halfway
  * // through its cycle.
  * function animationTime(duration, seconds) {
- *     return seconds / duration + .5;
+ *     return seconds / duration + 0.5;
  * }
  */
 export default ModelAnimation;

Source/Scene/ModelAnimationCollection.js

@@ -423,8 +423,8 @@ ModelAnimationCollection.prototype.update = function (frameState) {
         pastStopTime ? stopTime : sceneTime,
         startTime
       );
-      delta = defined(scheduledAnimation.animationTime)
-        ? scheduledAnimation.animationTime(duration, seconds)
+      delta = defined(scheduledAnimation._animationTime)
+        ? scheduledAnimation._animationTime(duration, seconds)
         : seconds / duration;
     }
 
@@ -447,7 +447,9 @@ ModelAnimationCollection.prototype.update = function (frameState) {
         scheduledAnimation._state === ModelAnimationState.STOPPED;
       // no change to delta, and no change to the animation state means we can
       // skip the update this time around.
-      if (play !== animationStopped) continue;
+      if (play !== animationStopped) {
+        continue;
+      }
     }
     scheduledAnimation._prevAnimationDelta = delta;
 

Source/Scene/ModelExperimental/ModelExperimentalAnimation.js

@@ -43,15 +43,7 @@ function ModelExperimentalAnimation(model, animation, options) {
   this._multiplier = defaultValue(options.multiplier, 1.0);
   this._reverse = defaultValue(options.reverse, false);
   this._loop = defaultValue(options.loop, ModelAnimationLoop.NONE);
-
-  /**
-   * If this is defined, it will be used to compute the local animation time
-   * instead of the scene's time.
-   *
-   * @type {ModelAnimation.AnimationTimeCallback}
-   * @default undefined
-   */
-  this.animationTime = options.animationTime;
+  this._animationTime = options.animationTime;
   this._prevAnimationDelta = undefined;
 
   /**
@@ -333,6 +325,19 @@ Object.defineProperties(ModelExperimentalAnimation.prototype, {
       return this._loop;
     },
   },
+
+  /**
+   * If this is defined, it will be used to compute the local animation time
+   * instead of the scene's time.
+   *
+   * @type {ModelExperimentalAnimation.AnimationTimeCallback}
+   * @default undefined
+   */
+  animationTime: {
+    get: function () {
+      return this._animationTime;
+    },
+  },
 });
 
 function initialize(runtimeAnimation) {
@@ -400,8 +405,7 @@ ModelExperimentalAnimation.prototype.animate = function (time) {
  * @returns {Number} Returns the local animation time.
  *
  * @example
- * // Use real time for model animation (also set
- * // ModelExperimentalAnimationCollection#animateWhilePaused)
+ * // Use real time for model animation (assuming animateWhilePaused was set to true)
  * function animationTime(duration) {
  *     return Date.now() / 1000 / duration;
  * }
@@ -410,7 +414,7 @@ ModelExperimentalAnimation.prototype.animate = function (time) {
  * // Offset the phase of the animation, so it starts halfway
  * // through its cycle.
  * function animationTime(duration, seconds) {
- *     return seconds / duration + .5;
+ *     return seconds / duration + 0.5;
  * }
  */
 export default ModelExperimentalAnimation;

Source/Scene/ModelExperimental/ModelExperimentalAnimationCollection.js

@@ -456,8 +456,8 @@ ModelExperimentalAnimationCollection.prototype.update = function (frameState) {
         reachedStopTime ? stopTime : sceneTime,
         startTime
       );
-      delta = defined(runtimeAnimation.animationTime)
-        ? runtimeAnimation.animationTime(duration, seconds)
+      delta = defined(runtimeAnimation._animationTime)
+        ? runtimeAnimation._animationTime(duration, seconds)
         : seconds / duration;
     }
 
@@ -480,7 +480,9 @@ ModelExperimentalAnimationCollection.prototype.update = function (frameState) {
         runtimeAnimation._state === ModelAnimationState.STOPPED;
       // no change to delta, and no change to the animation state means we can
       // skip the update this time around.
-      if (play !== animationStopped) continue;
+      if (play !== animationStopped) {
+        continue;
+      }
     }
     runtimeAnimation._prevAnimationDelta = delta;
 

Specs/Scene/ModelExperimental/ModelExperimentalAnimationCollectionSpec.js

@@ -627,7 +627,6 @@ describe("Scene/ModelExperimental/ModelExperimentalAnimationCollection", functio
         new Date("January 1, 2014 12:00:00 UTC")
       );
       const animationCollection = model.activeAnimations;
-      animationCollection.animateWhilePaused = false;
       let animationTime = 0;
       const animation = animationCollection.add({
         index: 0,
@@ -642,8 +641,10 @@ describe("Scene/ModelExperimental/ModelExperimentalAnimationCollection", functio
       scene.renderForSpecs(time);
       animationTime = 0.1;
       scene.renderForSpecs(JulianDate.addSeconds(time, 1.0, scratchJulianDate));
+      // no update because animationTime didn't change
       scene.renderForSpecs(JulianDate.addSeconds(time, 2.0, scratchJulianDate));
       animationTime = 0.2;
+      // no update because scene time didn't change
       scene.renderForSpecs(JulianDate.addSeconds(time, 2.0, scratchJulianDate));
       animationTime = 0.3;
       scene.renderForSpecs(JulianDate.addSeconds(time, 3.0, new JulianDate()));
@@ -690,6 +691,7 @@ describe("Scene/ModelExperimental/ModelExperimentalAnimationCollection", functio
       scene.renderForSpecs(time);
       animationTime = 0.1;
       scene.renderForSpecs(time);
+      // no update because animationTime didn't change
       scene.renderForSpecs(time);
       animationTime = 0.3;
       scene.renderForSpecs(time);

Specs/Scene/ModelSpec.js

@@ -1961,7 +1961,7 @@ describe(
 
     it("animates while paused with an explicit animation time", function () {
       const time = JulianDate.fromDate(
-        new Date("January 1, 2014 12:00:00 UTC")
+        new Date("January 1, 2014 12:00:01 UTC")
       );
       const animations = animBoxesModel.activeAnimations;
       animations.animateWhilePaused = true;
@@ -2003,6 +2003,7 @@ describe(
       );
       expect(animations.remove(a)).toEqual(true);
       animBoxesModel.show = false;
+      animations.animateWhilePaused = false;
     });
 
     it("animates with a multiplier", function () {