fixed surface hotspot updates (#4867)

google · Aug 12, 2024 · 4f7e758 · 4f7e758
1 parent 626e975
commit 4f7e758
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 45 deletions.
diff --git a/packages/model-viewer/src/features/annotation.ts b/packages/model-viewer/src/features/annotation.ts
@@ -16,7 +16,7 @@
 
 import {Matrix4, Vector3} from 'three';
 
-import ModelViewerElementBase, {$needsRender, $scene, $tick, toVector2D, toVector3D, Vector2D, Vector3D} from '../model-viewer-base.js';
+import ModelViewerElementBase, {$needsRender, $onModelLoad, $scene, $tick, toVector2D, toVector3D, Vector2D, Vector3D} from '../model-viewer-base.js';
 import {Hotspot, HotspotConfiguration} from '../three-components/Hotspot.js';
 import {Constructor} from '../utilities.js';
 
@@ -101,14 +101,23 @@ export const AnnotationMixin = <T extends Constructor<ModelViewerElementBase>>(
       }
     }
 
+    [$onModelLoad]() {
+      super[$onModelLoad]();
+
+      const scene = this[$scene];
+      scene.forHotspots((hotspot) => {
+        scene.updateSurfaceHotspot(hotspot);
+      });
+    }
+
     [$tick](time: number, delta: number) {
       super[$tick](time, delta);
       const scene = this[$scene];
       const {annotationRenderer} = scene;
       const camera = scene.getCamera();
 
       if (scene.shouldRender()) {
-        scene.updateSurfaceHotspots();
+        scene.animateSurfaceHotspots();
         scene.updateHotspotsVisibility(camera.position);
         annotationRenderer.domElement.style.display = '';
         annotationRenderer.render(scene, camera);
@@ -131,6 +140,7 @@ export const AnnotationMixin = <T extends Constructor<ModelViewerElementBase>>(
       hotspot.updatePosition(config.position);
       hotspot.updateNormal(config.normal);
       hotspot.surface = config.surface;
+      this[$scene].updateSurfaceHotspot(hotspot);
       this[$needsRender]();
     }
 

diff --git a/packages/model-viewer/src/test/features/annotation-spec.ts b/packages/model-viewer/src/test/features/annotation-spec.ts
@@ -16,8 +16,8 @@
 import {expect} from '@esm-bundle/chai';
 import {Vector3} from 'three';
 
-import {ModelViewerElement} from '../../model-viewer.js';
 import {$needsRender, $scene, toVector3D, Vector2D, Vector3D} from '../../model-viewer-base.js';
+import {ModelViewerElement} from '../../model-viewer.js';
 import {Hotspot} from '../../three-components/Hotspot.js';
 import {ModelScene} from '../../three-components/ModelScene.js';
 import {timePasses, waitForEvent} from '../../utilities.js';
@@ -154,11 +154,11 @@ suite('Annotation', () => {
       });
 
       test('updateHotspot does change the surface', () => {
+        const hotspot = scene.target.children[numSlots - 1] as Hotspot;
+        const {x} = hotspot.position;
         const surface = '0 0 1 2 3 0.217 0.341 0.442';
         element.updateHotspot({name: 'hotspot-1', surface});
-        const {surface: internalSurface} =
-            (scene.target.children[numSlots - 1] as Hotspot);
-        expect(internalSurface).to.be.deep.equal(surface);
+        expect(x).to.not.be.equal(hotspot.position.x);
       });
 
       test('and removing it does not remove the slot', async () => {

diff --git a/packages/model-viewer/src/three-components/Hotspot.ts b/packages/model-viewer/src/three-components/Hotspot.ts
@@ -143,10 +143,7 @@ export class Hotspot extends CSS2DObject {
     }
   }
 
-  updateSurface(forceUpdate: boolean) {
-    if (!forceUpdate && this.initialized) {
-      return;
-    }
+  updateSurface() {
     const {mesh, tri, bary} = this;
     if (mesh == null || tri == null || bary == null) {
       return;

diff --git a/packages/model-viewer/src/three-components/ModelScene.ts b/packages/model-viewer/src/three-components/ModelScene.ts
@@ -925,6 +925,7 @@ export class ModelScene extends Scene {
     // the slots appear in the shadow DOM and the elements get attached,
     // allowing us to dispatch events on them.
     this.annotationRenderer.domElement.appendChild(hotspot.element);
+    this.updateSurfaceHotspot(hotspot);
   }
 
   removeHotspot(hotspot: Hotspot) {
@@ -947,49 +948,50 @@ export class ModelScene extends Scene {
   /**
    * Lazy initializer for surface hotspots - will only run once.
    */
-  initializeSurface(hotspot: Hotspot) {
-    if (hotspot.surface != null && hotspot.mesh == null) {
-      const nodes = parseExpressions(hotspot.surface)[0].terms as NumberNode[];
-      if (nodes.length != 8) {
-        console.warn(hotspot.surface + ' does not have exactly 8 numbers.');
-        return;
-      }
-      const primitiveNode =
-          this.element.model![$nodeFromIndex](nodes[0].number, nodes[1].number);
-      const tri =
-          new Vector3(nodes[2].number, nodes[3].number, nodes[4].number);
-
-      if (primitiveNode == null) {
-        console.warn(
-            hotspot.surface +
-            ' does not match a node/primitive in this glTF! Skipping this hotspot.');
-        return;
-      }
-
-      const numVert = primitiveNode.mesh.geometry.attributes.position.count;
-      if (tri.x >= numVert || tri.y >= numVert || tri.z >= numVert) {
-        console.warn(
-            hotspot.surface +
-            ' vertex indices out of range in this glTF! Skipping this hotspot.');
-        return;
-      }
+  updateSurfaceHotspot(hotspot: Hotspot) {
+    if (hotspot.surface == null || this.element.model == null) {
+      return;
+    }
+    const nodes = parseExpressions(hotspot.surface)[0].terms as NumberNode[];
+    if (nodes.length != 8) {
+      console.warn(hotspot.surface + ' does not have exactly 8 numbers.');
+      return;
+    }
+    const primitiveNode =
+        this.element.model[$nodeFromIndex](nodes[0].number, nodes[1].number);
+    if (primitiveNode == null) {
+      console.warn(
+          hotspot.surface +
+          ' does not match a node/primitive in this glTF! Skipping this hotspot.');
+      return;
+    }
 
-      const bary =
-          new Vector3(nodes[5].number, nodes[6].number, nodes[7].number);
-      hotspot.mesh = primitiveNode.mesh;
-      hotspot.tri = tri;
-      hotspot.bary = bary;
+    const numVert = primitiveNode.mesh.geometry.attributes.position.count;
+    const tri = new Vector3(nodes[2].number, nodes[3].number, nodes[4].number);
+    if (tri.x >= numVert || tri.y >= numVert || tri.z >= numVert) {
+      console.warn(
+          hotspot.surface +
+          ' vertex indices out of range in this glTF! Skipping this hotspot.');
+      return;
     }
+
+    const bary = new Vector3(nodes[5].number, nodes[6].number, nodes[7].number);
+    hotspot.mesh = primitiveNode.mesh;
+    hotspot.tri = tri;
+    hotspot.bary = bary;
+
+    hotspot.updateSurface();
   }
 
   /**
    * Update positions of surface hotspots to follow model animation.
    */
-  updateSurfaceHotspots() {
-    const forceUpdate = !this.element.paused;
+  animateSurfaceHotspots() {
+    if (this.element.paused) {
+      return;
+    }
     this.forHotspots((hotspot) => {
-      this.initializeSurface(hotspot);
-      hotspot.updateSurface(forceUpdate);
+      hotspot.updateSurface();
     });
   }