Fix glTF animations in tileset #8983

ErixenCruz · 2020-06-22T19:06:41Z

Fixes #8962
Fixed issue where tileset was not playing glTF animations.

cesium-concierge · 2020-06-22T19:06:45Z

Thanks for the pull request @ErixenCruz!

✔️ Signed CLA found.
❔ Unit tests were not updated.
- Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.
Works (or fails gracefully) in IE11.

lilleyse · 2020-06-23T19:56:30Z

@ErixenCruz here are some tilesets to test with.

BatchedAnimated.zip - place in Specs/Data/Cesium3DTiles/Batched/
InstancedAnimated.zip - place in Specs/Data/Cesium3DTiles/Instanced/

Local sandcastle for viewing these

Add two tests to Cesium3DTilesetSpec, one for the batched tileset and another for the instanced tileset. Since the tests will be very similar you can factor out most of the work into a local function that both tests will call. The test should

Load the tileset - see the "renders tileset" test for an example
Once the tileset is loaded, render the scene at a specific time - see "renders with imageBaseLightingFactor" for an example.
Grab the first command in frameState's command list (see "adds stencil clear command first when unresolved") and save it's model matrix (you can use Matrix4.clone)
Render the scene again 0.5 seconds later.
Grab the first command again and compare the model matrix with the saved model matrix. You can use expect(matrixA).not.toEqual(matrixB) for this.

Let me know how this goes!

… tileset.

ErixenCruz · 2020-06-24T21:47:34Z

@lilleyse How's that?

lilleyse

Looks great @ErixenCruz, just one small suggestion for the instanced tileset code.

lilleyse · 2020-06-26T00:25:49Z

Source/Scene/ModelInstanceCollection.js

+    this._model.readyPromise.then(function (model) {
+      model.activeAnimations
+        .addAll({
+          loop: ModelAnimationLoop.REPEAT,
+        })
+        .otherwise(function (error) {
+          that._state = LoadState.FAILED;
+          that._readyPromise.reject(error);
+        });


The otherwise should be chained after .then rather than after .addAll. This .then/.otherwise chain allows the promise to be handled correctly whether the promise is resolved or rejected.

Chaining it after addAll wouldn't end up catching an error since addAll does not return a promise. My mistake for missing this earlier, I think I got lost in the indendation.

Suggested change

this._model.readyPromise.then(function (model) {

model.activeAnimations

.addAll({

loop: ModelAnimationLoop.REPEAT,

})

.otherwise(function (error) {

that._state = LoadState.FAILED;

that._readyPromise.reject(error);

});

this._model.readyPromise

.then(function (model) {

model.activeAnimations.addAll({

loop: ModelAnimationLoop.REPEAT,

});

})

.otherwise(function (error) {

that._state = LoadState.FAILED;

that._readyPromise.reject(error);

});

Despite this code suggestion, I've been thinking more about the overall design here and think it makes sense to bring the functionality to Instanced3DModel3DTileContent instead. I didn't noticed until reviewing that ModelInstanceCollection already has an activeAnimations getter and it's the caller's responsibility whether to add animations or not.

So the change will be something like this: in Instanced3DModel3DTileContent, after ModelInstanceCollection is created, add the animations when the collection is ready. It'll look just like the Batched3DModel3DTileContent code in this PR except that model will be a model instance collection.

lilleyse · 2020-06-26T00:37:05Z

Specs/Scene/Cesium3DTilesetSpec.js

@@ -637,6 +641,35 @@ describe(
      });
    });

+    function checkAnimation(url) {


The tests looks great

lilleyse · 2020-06-26T00:40:51Z

Looks like CI has some test failures. #8983 (comment) seems to be the cause. Once that's addressed there shouldn't be any issues.

…ise check to i3dm content.

ErixenCruz · 2020-06-26T15:24:51Z

Even after the fixes, some tests still fail. In particular, Scene/Instanced3DModel3DTileContent rejects readyPromise on error times out. Investigating.

lilleyse · 2020-06-26T17:50:42Z

Source/Scene/Batched3DModel3DTileContent.js

+      .otherwise(function (error) {
+        //content._state = 3;
+        content._readyPromise.reject(error);
+      });


Note that Batched3DModel3DTileContent does not have a _readyPromise property, the readyPromise getter is actually just content._model.readyPromise. This otherwise block is not necessary and can be removed.

lilleyse · 2020-06-26T17:52:33Z

Source/Scene/Instanced3DModel3DTileContent.js

+    .otherwise(function (error) {
+      //content._state = 3;
+      content._readyPromise.reject(error);
+    });


Similar comment to above - this .otherwise block is not needed.

lilleyse · 2020-06-26T17:52:50Z

Source/Scene/Instanced3DModel3DTileContent.js

+    .then(function (model) {
+      model.activeAnimations.addAll({


Perhaps rename model to collection

lilleyse · 2020-06-26T17:56:11Z

Source/Scene/ModelInstanceCollection.js

-    var that = this;
-    this._model.readyPromise.otherwise(function (error) {
-      that._state = LoadState.FAILED;
-      that._readyPromise.reject(error);
-    });


This code is still required, otherwise the collection's own ready promise won't get rejected if there's a problem when loading the model.

ErixenCruz · 2020-06-26T20:18:00Z

@lilleyse Tests pass! Like I mentioned, I have to rely on TravisCI for testing. Unreliable to run on my browser.

lilleyse · 2020-06-28T00:11:17Z

Local tests work for me. There's some test failures on Mac/Chrome but none introduced in this branch.

Thanks @ErixenCruz

ErixenCruz added 3 commits June 22, 2020 14:57

#8962 fixes glTf animations in tileset

e1ac264

Updated CHANGES.md

2cc24a4

Updated CONTRIBUTORS.md

ef6e554

ErixenCruz requested a review from lilleyse June 22, 2020 19:06

lilleyse mentioned this pull request Jun 23, 2020

Add animation samples CesiumGS/3d-tiles-validator#202

Merged

ErixenCruz mentioned this pull request Jun 24, 2020

Add Color.toCssHexString and unit tests #8987

Merged

ErixenCruz added 3 commits June 24, 2020 16:37

Fix glTF animation. Add testing tilesets. Add unit test for instanced…

647f15c

… tileset.

Change test to advance half a second in the scene instead of IRL.

4fe3913

Add batched animation spec.

12d800c

Merge branch 'master' into fix-glTF-animations-in-tileset

8fb42e8

lilleyse reviewed Jun 26, 2020

View reviewed changes

Remove extra add animation in ModelInstanceCollection. Move readyProm…

d0641b5

…ise check to i3dm content.

lilleyse reviewed Jun 26, 2020

View reviewed changes

Reject readyPromise in ModelInstanceCollection.

7239b1f

lilleyse merged commit 97492e7 into master Jun 28, 2020

lilleyse deleted the fix-glTF-animations-in-tileset branch June 28, 2020 00:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix glTF animations in tileset #8983

Fix glTF animations in tileset #8983

ErixenCruz commented Jun 22, 2020

cesium-concierge commented Jun 22, 2020

lilleyse commented Jun 23, 2020

ErixenCruz commented Jun 24, 2020

lilleyse left a comment

lilleyse Jun 26, 2020

lilleyse Jun 26, 2020

lilleyse Jun 26, 2020

lilleyse commented Jun 26, 2020

ErixenCruz commented Jun 26, 2020

lilleyse Jun 26, 2020

lilleyse Jun 26, 2020

lilleyse Jun 26, 2020

lilleyse Jun 26, 2020

ErixenCruz commented Jun 26, 2020

lilleyse commented Jun 28, 2020

Fix glTF animations in tileset #8983

Fix glTF animations in tileset #8983

Conversation

ErixenCruz commented Jun 22, 2020

cesium-concierge commented Jun 22, 2020

lilleyse commented Jun 23, 2020

ErixenCruz commented Jun 24, 2020

lilleyse left a comment

Choose a reason for hiding this comment

lilleyse Jun 26, 2020

Choose a reason for hiding this comment

lilleyse Jun 26, 2020

Choose a reason for hiding this comment

lilleyse Jun 26, 2020

Choose a reason for hiding this comment

lilleyse commented Jun 26, 2020

ErixenCruz commented Jun 26, 2020

lilleyse Jun 26, 2020

Choose a reason for hiding this comment

lilleyse Jun 26, 2020

Choose a reason for hiding this comment

lilleyse Jun 26, 2020

Choose a reason for hiding this comment

lilleyse Jun 26, 2020

Choose a reason for hiding this comment

ErixenCruz commented Jun 26, 2020

lilleyse commented Jun 28, 2020