Animations, reframeCamera, pick multiple models #16145

alexandremottet · 2025-02-05T10:06:15Z

Add support for multiple models for :

animations
_reframeCamera with option to use a subset of models
_pick

Add support for multiple models for animations, reframing the camera and picking

bjsplat · 2025-02-05T10:07:57Z

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

bjsplat · 2025-02-05T10:18:10Z

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

bjsplat · 2025-02-05T10:29:10Z

Snapshot stored with reference name:
refs/pull/16145/merge

Test environment:
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16145/merge/index.html

To test a playground add it to the URL, for example:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16145/merge/index.html#WGZLGJ#4600

Links to test babylon tools with this snapshot:

https://playground.babylonjs.com/?snapshot=refs/pull/16145/merge
https://sandbox.babylonjs.com/?snapshot=refs/pull/16145/merge
https://gui.babylonjs.com/?snapshot=refs/pull/16145/merge
https://nme.babylonjs.com/?snapshot=refs/pull/16145/merge

To test the snapshot in the playground with a playground ID add it after the snapshot query string:

https://playground.babylonjs.com/?snapshot=refs/pull/16145/merge#BCU1XR#0

bjsplat · 2025-02-05T10:33:27Z

WebGL2 visualization test reporter:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16145/merge/testResults/webgl2playwright/index.html

bjsplat · 2025-02-05T10:33:38Z

Visualization tests for WebGPU

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16145/merge/testResults/webgpuplaywright/index.html

bjsplat · 2025-02-05T10:46:35Z

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

bjsplat · 2025-02-05T10:59:07Z

Snapshot stored with reference name:
refs/pull/16145/merge

Test environment:
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16145/merge/index.html

To test a playground add it to the URL, for example:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16145/merge/index.html#WGZLGJ#4600

Links to test babylon tools with this snapshot:

https://playground.babylonjs.com/?snapshot=refs/pull/16145/merge
https://sandbox.babylonjs.com/?snapshot=refs/pull/16145/merge
https://gui.babylonjs.com/?snapshot=refs/pull/16145/merge
https://nme.babylonjs.com/?snapshot=refs/pull/16145/merge

To test the snapshot in the playground with a playground ID add it after the snapshot query string:

https://playground.babylonjs.com/?snapshot=refs/pull/16145/merge#BCU1XR#0

bjsplat · 2025-02-05T11:10:42Z

Visualization tests for WebGPU

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16145/merge/testResults/webgpuplaywright/index.html

bjsplat · 2025-02-05T11:11:14Z

WebGL2 visualization test reporter:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16145/merge/testResults/webgl2playwright/index.html

ryantrem · 2025-02-05T17:41:40Z

packages/tools/viewer/src/viewer.ts

     */
    public get animations(): readonly string[] {
-        return this._activeModelBacking?.assetContainer.animationGroups.map((group) => group.name) ?? [];
+        return this._loadedModelsContainer.animationGroups.map((group) => group.name) ?? [];


I think the base Viewer class should remain single-model-centric in the public interface. The rest of the public interface basically operates on a single (active) model - playing an animation, selecting a material variant, etc. If we change this, then if there are multiple models you'd see animations for all the models in the animation list, but when playing an animation it would target the active model, which wouldn't work.

Can we leave this as is, and if a subclass needs to enumerate all animations across all models, this can still be done through the _loadedModels property?

ryantrem · 2025-02-05T17:48:19Z

packages/tools/viewer/src/viewer.ts

-        if (this._activeModel) {
-            const model = this._activeModel?.assetContainer;
+        if (this._loadedModels.length > 0) {
+            const container = this._loadedModelsContainer;


Having the extra _loadedModelsContainer state seems like it is mostly here for convenience in enumerating all meshes, all animations, etc. But the added (redundant) state seems like it creates more opportunity for bugs and to me makes it a bit harder to understand the code.

Instead of adding this state, whenever we need to enumerate meshes/animations/etc. across multiple models, can we just do something like:

this._loadedModels.map((model) => model.meshes).flat().foreach((mesh) => {

I think this bit of code is simple enough to use as is and not introduce additional state (_loadedModelsContainer).

ryantrem · 2025-02-05T18:32:33Z

packages/tools/viewer/src/viewer.ts

+    protected _reframeCamera(interpolateCamera?: boolean): void;
+    protected _reframeCamera(interpolateCamera: boolean, models?: Model[]): void {
+        if (!models) {
+            const selectedAnimation = this._selectedAnimation === -1 ? 0 : this._selectedAnimation;
+            const worldBounds = this._activeModel?.getWorldBounds(selectedAnimation);
+            this._reframeCameraFromBounds(worldBounds, interpolateCamera);
+            return;
+        }
+
+        const container = models.length === this._loadedModelsBacking.length ? this._loadedModelsContainer : this._reduceModels(models);
+        const worldBounds = computeBoundingInfos(container);
+        this._reframeCameraFromBounds(worldBounds, interpolateCamera);
+    }


I don't think computing bounds without specifying an animation group (when there is at least one animation group) totally makes sense here. Depending on which animation group is active, the bounds of the model (even when paused on the first frame of the animation) can be wildly different (UFO model for example). I think it would make more sense to always include an animation group in the calculation when a model has animation groups.

To make this work, I think it would make sense to add a selectedAnimation to Model, and move ~half of the _selectedAnimation setter logic to this new model specific property (e.g. initializing the animation group). This way, every model with at least one animation group would have the concept of a selected animation and could be properly framed in the camera.

Once this is in place, then the logic for framing the camera could always be the same - just frame a set of models. It would default to _loadedModels, which in the case of the base/default Viewer would be the single loaded model. E.g.
protected _reframeCamera(interpolateCamera?: boolean, models = this._loadedModels)

bjsplat · 2025-02-06T17:22:48Z

WebGL2 visualization test reporter:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16145/merge/testResults/webgl2playwright/index.html

bjsplat · 2025-02-06T17:23:27Z

Visualization tests for WebGPU

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16145/merge/testResults/webgpuplaywright/index.html

ryantrem · 2025-02-06T22:25:50Z

packages/tools/viewer/test/apps/web/index.html

@@ -77,7 +77,7 @@
        <div id="viewerContainer" style="width: 100vw; height: 100vh">
            <babylon-viewer
                engine="WebGPU"
-                source="https://raw.githubusercontent.com/BabylonJS/Assets/master/meshes/ufo.glb"
+                source="http://localhost:3000/glb/fox.glb"


ryantrem · 2025-02-06T22:30:33Z

packages/tools/viewer/src/viewer.ts

+                get selectedAnimation() {
+                    return selectedAnimation;
+                },
+                selectAnimation: (index: number) => {


I think it would be fine for this to be a property setter. The class level selectAnimation is a function because it needed additional params.

Agree that it should be a setter but Model is marked as Readonly so I cannot change it.
This is why I left it as a function.
I could make change so this._activeModel return a Readonly<Model> and this._activeModelBacking return just a Model. Should I do it ?

packages/tools/viewer/src/viewer.ts

ryantrem · 2025-02-06T22:47:10Z

packages/tools/viewer/src/viewer.ts

@@ -1063,7 +1081,7 @@ export class Viewer implements IDisposable {
                getWorldBounds: (animationIndex: number): Nullable<ViewerBoundingInfo> => {
                    let worldBounds: Nullable<ViewerBoundingInfo> = cachedWorldBounds[animationIndex];
                    if (!worldBounds) {
-                        worldBounds = computeBoundingInfos(assetContainer, assetContainer.animationGroups[animationIndex]);
+                        worldBounds = computeBoundingInfos(assetContainer.meshes, assetContainer.animationGroups[animationIndex]);


This could be:
worldBounds = computeModelsBoundingInfos(model);
and then we could remove computeBoundingInfos, right?

bjsplat · 2025-02-10T10:45:29Z

Visualization tests for WebGPU

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16145/merge/testResults/webgpuplaywright/index.html

bjsplat · 2025-02-10T10:46:19Z

WebGL2 visualization test reporter:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16145/merge/testResults/webgl2playwright/index.html

Animations, reframeCamera, pick multiple models

c6e3f00

Add support for multiple models for animations, reframing the camera and picking

use custom models list for reframing camera

b2ef4eb

remove comment

10a3834

make reframing camera protected

f70a63b

RaananW requested a review from ryantrem February 5, 2025 14:21

ryantrem reviewed Feb 5, 2025

View reviewed changes

alexandremottet added 2 commits February 6, 2025 11:01

remove _loadedModelsContainer

47aadb8

Now Model have the information about selectedAnimation

9cb2998

alexandremottet marked this pull request as draft February 6, 2025 17:00

ryantrem reviewed Feb 6, 2025

View reviewed changes

alexandremottet added 3 commits February 10, 2025 10:34

revert test mesh

a84d52a

use the context of model to access property

f688d2b

remove computeBoundingInfos

b5bdab5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Animations, reframeCamera, pick multiple models #16145

Animations, reframeCamera, pick multiple models #16145

alexandremottet commented Feb 5, 2025 •

edited

Loading

bjsplat commented Feb 5, 2025

bjsplat commented Feb 5, 2025

bjsplat commented Feb 5, 2025

bjsplat commented Feb 5, 2025

bjsplat commented Feb 5, 2025

bjsplat commented Feb 5, 2025

bjsplat commented Feb 5, 2025

bjsplat commented Feb 5, 2025

bjsplat commented Feb 5, 2025

ryantrem Feb 5, 2025

ryantrem Feb 5, 2025

ryantrem Feb 5, 2025

bjsplat commented Feb 6, 2025

bjsplat commented Feb 6, 2025

ryantrem Feb 6, 2025

ryantrem Feb 6, 2025

alexandremottet Feb 10, 2025

ryantrem Feb 6, 2025

bjsplat commented Feb 10, 2025

bjsplat commented Feb 10, 2025

Animations, reframeCamera, pick multiple models #16145

Are you sure you want to change the base?

Animations, reframeCamera, pick multiple models #16145

Conversation

alexandremottet commented Feb 5, 2025 • edited Loading

bjsplat commented Feb 5, 2025

bjsplat commented Feb 5, 2025

bjsplat commented Feb 5, 2025

bjsplat commented Feb 5, 2025

bjsplat commented Feb 5, 2025

bjsplat commented Feb 5, 2025

bjsplat commented Feb 5, 2025

bjsplat commented Feb 5, 2025

bjsplat commented Feb 5, 2025

ryantrem Feb 5, 2025

Choose a reason for hiding this comment

ryantrem Feb 5, 2025

Choose a reason for hiding this comment

ryantrem Feb 5, 2025

Choose a reason for hiding this comment

bjsplat commented Feb 6, 2025

bjsplat commented Feb 6, 2025

ryantrem Feb 6, 2025

Choose a reason for hiding this comment

ryantrem Feb 6, 2025

Choose a reason for hiding this comment

alexandremottet Feb 10, 2025

Choose a reason for hiding this comment

ryantrem Feb 6, 2025

Choose a reason for hiding this comment

bjsplat commented Feb 10, 2025

bjsplat commented Feb 10, 2025

alexandremottet commented Feb 5, 2025 •

edited

Loading