fix(multi-model): resolve hotspot anchoring, test stability, and defer bounding box recalcs

Author: diegoteran

packages/model-viewer-effects/src/test/utilities-spec.ts

@@ -79,6 +79,7 @@ suite('Screenshot Baseline Test', () => {
       expect(renderer).to.not.be.undefined;
       element.jumpCameraToGoal();
       await rafPasses();
+      await new Promise(resolve => setTimeout(resolve, 100)); // Allow shader recompilation frames to catch up on Chromium
       composerScreenshot = screenshot(element);
       await rafPasses();
       const screenshot2 = screenshot(element);
@@ -89,7 +90,8 @@ suite('Screenshot Baseline Test', () => {
       }
     });
 
-    test('Empty EffectComposer and base Renderer are identical', () => {
+    test('Empty EffectComposer and base Renderer are identical', async () => {
+      await new Promise(resolve => setTimeout(resolve, 100)); // Wait for potential rendering stabilization
       const similarity = CompareArrays(baseScreenshot, composerScreenshot);
       if (!Number.isNaN(similarity)) {
         expect(similarity).to.be.greaterThan(0.999);

packages/model-viewer/src/features/annotation.ts

@@ -106,6 +106,7 @@ export const AnnotationMixin = <T extends Constructor<ModelViewerElementBase>>(
 
       const scene = this[$scene];
       scene.forHotspots((hotspot) => {
+        scene.updateHotspotAttachment(hotspot);
         scene.updateSurfaceHotspot(hotspot);
       });
     }

packages/model-viewer/src/features/controls.ts

@@ -193,6 +193,7 @@ const maxCameraOrbitIntrinsics = (element: ModelViewerElementBase) => {
 };
 
 export const cameraTargetIntrinsics = (element: ModelViewerElementBase) => {
+  element[$scene].updateBoundingBoxAndShadowIfDirty();
   const center = element[$scene].boundingBox.getCenter(new Vector3());
 
   return {

packages/model-viewer/src/features/loading.ts

@@ -252,10 +252,12 @@ export const LoadingMixin = <T extends Constructor<ModelViewerElementBase>>(
      * turntable rotation.
      */
     getDimensions(): Vector3D {
+      this[$scene].updateBoundingBoxAndShadowIfDirty();
       return toVector3D(this[$scene].size);
     }
 
     getBoundingBoxCenter(): Vector3D {
+      this[$scene].updateBoundingBoxAndShadowIfDirty();
       return toVector3D(this[$scene].boundingBox.getCenter(new Vector3()));
     }
 

packages/model-viewer/src/test/features/controls-spec.ts

@@ -216,6 +216,7 @@ suite('Controls', () => {
       expect(fov).to.be.closeTo(DEFAULT_FOV_DEG, .001);
       element.setAttribute('style', 'width: 200px; height: 300px');
       await rafPasses();
+      await timePasses(50);
       await rafPasses();
 
       expect(element.getFieldOfView()).to.be.greaterThan(fov);

packages/model-viewer/src/test/features/extra-model-spec.ts

@@ -75,5 +75,68 @@ suite('ExtraModel', () => {
 
         expect(scene._models[1].position.x).to.equal(5);
     });
+
+    test('does not calculate bounding box synchronously when offset changes', async () => {
+        element.loading = 'eager';
+        element.src = CUBE_GLB_PATH;
+        
+        const extra = document.createElement('extra-model');
+        extra.setAttribute('src', CUBE_GLB_PATH);
+        extra.setAttribute('offset', '1 0 0');
+        element.appendChild(extra);
+
+        await waitForEvent(element, 'load');
+        const scene = (element as any)[$scene];
+
+        // Ensure bounds are clean initially
+        scene.updateBoundingBoxAndShadowIfDirty();
+        const oldMaxX = scene.boundingBox.max.x;
+
+        // Change offset
+        extra.setAttribute('offset', '10 0 0');
+        await timePasses(); // allow MutationObserver to trigger updateModelTransforms
+
+        // The bounds should be dirty, but the actual boundingBox value hasn't mathematically updated yet!
+        expect(scene.boundsAndShadowDirty).to.be.true;
+        expect(scene.boundingBox.max.x).to.equal(oldMaxX);
+
+        // However, reading via getDimensions flushes it
+        element.getDimensions();
+        expect(scene.boundsAndShadowDirty).to.be.false;
+        expect(scene.boundingBox.max.x).to.be.greaterThan(oldMaxX);
+    });
+  });
+
+  suite('hotspot attachment', () => {
+    test('data-model-index forces hotspot attachment to extra model', async () => {
+        element.loading = 'eager';
+        element.src = CUBE_GLB_PATH;
+        
+        const extra = document.createElement('extra-model');
+        extra.setAttribute('src', CUBE_GLB_PATH);
+        extra.setAttribute('offset', '5 0 0');
+        element.appendChild(extra);
+
+        const hotspot = document.createElement('button');
+        hotspot.slot = 'hotspot-test';
+        hotspot.setAttribute('data-model-index', '1');
+        // Legacy 8-number string implies index 0 (base model). It should be overridden!
+        hotspot.setAttribute('data-surface', '0 0 10 11 12 0.3 0.3 0.4');
+        element.appendChild(hotspot);
+
+        await waitForEvent(element, 'load');
+        
+        const scene = (element as any)[$scene];
+        
+        // Find the internal Hotspot instance by checking children for the annotation wrapper class
+        const hotspotNode = scene._models[1].children.find((c: any) => c.element && c.element.classList.contains('annotation-wrapper'));
+        
+        // Assert it is reparented to the extra model, NOT the base model
+        expect(hotspotNode).to.be.ok;
+        expect(hotspotNode.parent).to.equal(scene._models[1]);
+        
+        // Assert the internal modelIndex was coerced
+        expect(hotspotNode.modelIndex).to.equal(1);
+    });
   });
 });

packages/model-viewer/src/three-components/ModelScene.ts

@@ -117,6 +117,7 @@ export class ModelScene extends Scene {
 
   private _currentGLTFs: ModelViewerGLTFInstance[] = [];
   private _models: Object3D[] = [];
+  private boundsAndShadowDirty = false;
   private mixers: AnimationMixer[] = [];
   private mixerPausedStates: boolean[] = [];
   private cancelPendingSourceChange: (() => void)|null = null;
@@ -320,9 +321,8 @@ export class ModelScene extends Scene {
     this.setGroundedSkybox();
   }
 
-  updateModelTransforms(index: number, offset?: string|null, orientation?: string|null, scale?: string|null) {
+  updateModelTransforms(index: number, offset?: string|null, _orientation?: string|null, scale?: string|null) {
     const model = this._models[index];
-    console.log(`[ModelScene] updateModelTransforms index: ${index} modelFound: ${!!model} offset: ${offset} orientation: ${orientation}`);
     if (!model) return;
 
     if (offset) {
@@ -342,11 +342,27 @@ export class ModelScene extends Scene {
     }
 
     model.updateMatrixWorld(true);
-    this.updateBoundingBox();
-    this.updateShadow();
+    // Defer bounding box and shadow recalculations.
+    // If developers animate `<extra-model>` offset or scale properties via requestAnimationFrame,
+    // recalculating bounding boxes synchronously every single frame here blocks the main thread and tanks frame rates.
+    // Instead, we mark the bounds as dirty and wait for the render loop or a public dimensions getter to flush the changes.
+    this.boundsAndShadowDirty = true;
     this.queueRender();
   }
 
+  /**
+   * Evaluates bounding box recalculations asynchronously.
+   * Flushed right before a frame is rendered or when dimension properties are formally queried
+   * to ensure that high-frequency layout changes don't stall execution natively.
+   */
+  updateBoundingBoxAndShadowIfDirty() {
+    if (this.boundsAndShadowDirty) {
+      this.boundsAndShadowDirty = false;
+      this.updateBoundingBox();
+      this.updateShadow();
+    }
+  }
+
   reset() {
     this.url = null;
     this.renderCount = 0;
@@ -1241,6 +1257,7 @@ export class ModelScene extends Scene {
   }
 
   renderShadow(renderer: WebGLRenderer) {
+    this.updateBoundingBoxAndShadowIfDirty();
     const shadow = this.shadow;
     if (shadow != null && shadow.needsUpdate == true) {
       shadow.render(renderer, this);
@@ -1397,9 +1414,25 @@ export class ModelScene extends Scene {
    * The following methods are for operating on the set of Hotspot objects
    * attached to the scene. These come from DOM elements, provided to slots
    * by the Annotation Mixin.
+  /**
+   * Evaluates the intended `modelIndex` of the hotspot and safely reparents it
+   * to the corresponding `Object3D` node mapped inside this scene's `_models` array.
+   * This guarantees that declarative offset and layout transforms affect positional anchors.
    */
+  updateHotspotAttachment(hotspot: Hotspot) {
+    const targetNode = (hotspot.modelIndex != null && hotspot.modelIndex > 0 && this._models[hotspot.modelIndex])
+        ? this._models[hotspot.modelIndex]
+        : this.target;
+
+    if (hotspot.parent !== targetNode) {
+      targetNode.add(hotspot);
+      hotspot.updatePosition(hotspot.position.toArray().join(' ') + 'm'); // Force bounds sync to fresh parent
+      hotspot.updateMatrixWorld(true);
+    }
+  }
+
   addHotspot(hotspot: Hotspot) {
-    this.target.add(hotspot);
+    this.updateHotspotAttachment(hotspot);
     // This happens automatically in render(), but we do it early so that
     // the slots appear in the shadow DOM and the elements get attached,
     // allowing us to dispatch events on them.
@@ -1408,20 +1441,35 @@ export class ModelScene extends Scene {
   }
 
   removeHotspot(hotspot: Hotspot) {
-    this.target.remove(hotspot);
+    if (hotspot.parent) {
+      hotspot.parent.remove(hotspot);
+    }
   }
 
   /**
    * Helper method to apply a function to all hotspots.
    */
   forHotspots(func: (hotspot: Hotspot) => void) {
-    const {children} = this.target;
+    const children = [...this.target.children];
     for (let i = 0, l = children.length; i < l; i++) {
       const hotspot = children[i];
       if (hotspot instanceof Hotspot) {
         func(hotspot);
       }
     }
+    
+    // Also traverse extra models to find any hotspots already reparented to them
+    for (const model of this._models) {
+      if (model && model !== this.target) {
+        const extraChildren = [...model.children];
+        for (let i = 0, l = extraChildren.length; i < l; i++) {
+          const hotspot = extraChildren[i];
+          if (hotspot instanceof Hotspot) {
+            func(hotspot);
+          }
+        }
+      }
+    }
   }
 
   /**
@@ -1439,18 +1487,22 @@ export class ModelScene extends Scene {
 
     // Determine format: 8 numbers = legacy (index 0), 9 numbers = indexed
     const isLegacy = nodes.length === 8;
-    const modelIndex = isLegacy ? 0 : nodes[0].number;
+    const parsedModelIndex = isLegacy ? 0 : nodes[0].number;
     const offset = isLegacy ? 0 : 1;
 
-    const model = modelIndex === 0 ? this.element.model : this.element.extraModels?.[modelIndex - 1];
+    // DOM attribute (`data-model-index`) takes precedence over the parsed surface index.
+    const finalModelIndex = hotspot.modelIndex ?? parsedModelIndex;
+    
+    // Assign resolved modelIndex to the hotspot
+    hotspot.modelIndex = finalModelIndex;
+
+    // Ensure physical attachment matches the logical model index
+    this.updateHotspotAttachment(hotspot);
+
+    const model = finalModelIndex === 0 ? this.element.model : this.element.extraModels?.[finalModelIndex - 1];
     if (model == null) {
       return;
     }
-    
-    // Assign parsed modelIndex to the hotspot if not already set manually
-    if (hotspot.modelIndex == null) {
-      hotspot.modelIndex = modelIndex;
-    }
 
     const primitiveNode =
         model[$nodeFromIndex](nodes[0 + offset].number, nodes[1 + offset].number);

packages/modelviewer.dev/examples/scenegraph/index.html

@@ -1308,6 +1308,7 @@ <h2 class="demo-title">Multiple Models in One Scene</h2>
                   shadow-intensity="1" camera-controls touch-action="pan-y" ar alt="A multi-model scene">
                   <extra-model id="duck-model" src="../../shared-assets/models/glTF-Sample-Assets/Models/Duck/glTF-Binary/Duck.glb" offset="-2 0 0"></extra-model>
                   <extra-model src="../../shared-assets/models/glTF-Sample-Assets/Models/BoxAnimated/glTF-Binary/BoxAnimated.glb" offset="2 0 0"></extra-model>
+                  <button slot="hotspot-duck" class="hotspot" data-model-index="1" data-surface="0 0 10 11 12 0.5 0.5 0" style="padding: 4px; border-radius: 4px; border: none;">Duck</button>
                   <div class="controls glass">
                     <button id="toggle-animations">Play/Pause Box Animation</button>
                     <button id="color-duck">Randomize Duck Color</button>