Skip to content

Commit e33ab46

Browse files
authored
update 3D scene when an <extra-model> is removed (#5187)
Fixes #5186. ### Summary & Root Cause When an `<extra-model>` child element was removed from `<model-viewer>` (e.g. via `element.remove()`), the DOM child change triggered a `slotchange` event, which invoked `this[$updateSource]()`. However, because the primary model had already finished loading, `this.loaded` remained `true`. Inside `[$updateSource]()`, an early return check (`if (this.loaded || ...)`) caused the update to bail out before invoking `scene.setSource()`. As a result, the removed `<extra-model>`'s 3D mesh was orphaned and remained rendered inside the Three.js scene graph. ### Changes Made * **`model-viewer-base.ts`**: * Updated `[$onSlotChange]` to reset `this[$loaded] = false;` and `this[$loadedTime] = 0;` whenever the list of `<extra-model>` URLs or element count changes. This allows `[$updateSource]()` to proceed past the load guard and properly synchronize the Three.js scene graph with the DOM. * Added explicit handling for the edge case where all primary and extra models are removed (`this.src == null && extraModels.length === 0`), directly invoking `this[$scene].reset()` and clearing loaded state. * **`ModelScene.ts`**: * Updated `reset()` to ensure `this.extraUrls = [];` is properly cleared during scene resets. * **`extra-model-spec.ts`**:
1 parent 8dfcf8a commit e33ab46

3 files changed

Lines changed: 71 additions & 21 deletions

File tree

packages/model-viewer/src/model-viewer-base.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,15 @@ export default class ModelViewerElementBase extends ReactiveElement {
212212
// modified its source set
213213
if (newExtraUrls.join(',') !== currentExtraUrls.join(',') ||
214214
extraModels.length !== currentExtraUrls.length) {
215-
this[$updateSource]();
215+
if (this.src == null && extraModels.length === 0) {
216+
this[$loaded] = false;
217+
this[$loadedTime] = 0;
218+
this[$scene].reset();
219+
} else {
220+
this[$loaded] = false;
221+
this[$loadedTime] = 0;
222+
this[$updateSource]();
223+
}
216224
}
217225
};
218226

@@ -230,6 +238,7 @@ export default class ModelViewerElementBase extends ReactiveElement {
230238

231239
if (customEv.detail.srcChanged) {
232240
this[$loaded] = false;
241+
this[$loadedTime] = 0;
233242
this[$updateSource]();
234243
} else {
235244
// Apply Transforms

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

Lines changed: 60 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,45 @@ suite('ExtraModel', () => {
5757
expect(scene._models.length).to.equal(2);
5858
});
5959

60+
test('removes extra-model and updates the scene', async () => {
61+
element.loading = 'eager';
62+
element.src = CUBE_GLB_PATH;
63+
64+
const extra = document.createElement('extra-model');
65+
extra.setAttribute('src', CUBE_GLB_PATH);
66+
element.appendChild(extra);
67+
68+
await waitForEvent(element, 'load');
69+
70+
const scene = (element as any)[$scene];
71+
expect(scene._models.length).to.equal(2);
72+
73+
const loadEvent = waitForEvent(element, 'load');
74+
element.removeChild(extra);
75+
await loadEvent;
76+
77+
expect(scene._models.length).to.equal(1);
78+
});
79+
80+
test('removes all extra-models when src is null', async () => {
81+
element.loading = 'eager';
82+
83+
const extra = document.createElement('extra-model');
84+
extra.setAttribute('src', CUBE_GLB_PATH);
85+
element.appendChild(extra);
86+
87+
await waitForEvent(element, 'load');
88+
89+
const scene = (element as any)[$scene];
90+
expect(scene._models.length).to.equal(1);
91+
92+
element.removeChild(extra);
93+
await timePasses();
94+
95+
expect(scene._models.length).to.equal(0);
96+
expect(scene.extraUrls.length).to.equal(0);
97+
});
98+
6099
test('updates position dynamically when offset changes', async () => {
61100
element.loading = 'eager';
62101
element.src = CUBE_GLB_PATH;
@@ -102,31 +141,32 @@ suite('ExtraModel', () => {
102141
expect(scene._models[1].position.x).to.be.closeTo(3, 0.001);
103142
});
104143

105-
test('applies orientation as rotation on the model quaternion', async () => {
106-
element.loading = 'eager';
107-
element.src = CUBE_GLB_PATH;
144+
test(
145+
'applies orientation as rotation on the model quaternion', async () => {
146+
element.loading = 'eager';
147+
element.src = CUBE_GLB_PATH;
108148

109-
const extra = document.createElement('extra-model');
110-
extra.setAttribute('src', CUBE_GLB_PATH);
111-
// yaw = 90deg (third term in roll/pitch/yaw) → Euler(0, π/2, 0, 'YXZ')
112-
// → quaternion.y ≈ 0.707, quaternion.w ≈ 0.707
113-
extra.setAttribute('orientation', '0deg 0deg 90deg');
114-
element.appendChild(extra);
149+
const extra = document.createElement('extra-model');
150+
extra.setAttribute('src', CUBE_GLB_PATH);
151+
// yaw = 90deg (third term in roll/pitch/yaw) → Euler(0, π/2, 0,
152+
// 'YXZ') → quaternion.y ≈ 0.707, quaternion.w ≈ 0.707
153+
extra.setAttribute('orientation', '0deg 0deg 90deg');
154+
element.appendChild(extra);
115155

116-
await waitForEvent(element, 'load');
156+
await waitForEvent(element, 'load');
117157

118-
const scene = (element as any)[$scene];
119-
const q = scene._models[1].quaternion;
120-
expect(q.y).to.be.closeTo(Math.sin(Math.PI / 4), 0.001);
121-
expect(q.w).to.be.closeTo(Math.cos(Math.PI / 4), 0.001);
158+
const scene = (element as any)[$scene];
159+
const q = scene._models[1].quaternion;
160+
expect(q.y).to.be.closeTo(Math.sin(Math.PI / 4), 0.001);
161+
expect(q.w).to.be.closeTo(Math.cos(Math.PI / 4), 0.001);
122162

123-
// Update dynamically
124-
extra.setAttribute('orientation', '0deg 0deg 0deg');
125-
await timePasses();
163+
// Update dynamically
164+
extra.setAttribute('orientation', '0deg 0deg 0deg');
165+
await timePasses();
126166

127-
expect(scene._models[1].quaternion.y).to.be.closeTo(0, 0.001);
128-
expect(scene._models[1].quaternion.w).to.be.closeTo(1, 0.001);
129-
});
167+
expect(scene._models[1].quaternion.y).to.be.closeTo(0, 0.001);
168+
expect(scene._models[1].quaternion.w).to.be.closeTo(1, 0.001);
169+
});
130170

131171
test(
132172
'does not calculate bounding box synchronously when offset changes',

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -396,6 +396,7 @@ export class ModelScene extends Scene {
396396

397397
reset() {
398398
this.url = null;
399+
this.extraUrls = [];
399400
this.renderCount = 0;
400401
this.queueRender();
401402
if (this.shadow != null) {

0 commit comments

Comments
 (0)