Skip to content

Commit 6bc23ee

Browse files
vincentfretinclaude
andcommitted
fix: emit finished event for one-time animations
Two separate bugs prevented the 'finished' event from being dispatched when an animation was played a single time (repetitions: 1): 1. Mixer event subscriptions were lost on model load (regression from 4.3.0). The multi-model refactor recreates the animation mixers on every setSource(), but the mixer event subscriptions are registered only once, when the animation feature is constructed — before any model is loaded, while there are no mixers yet. They were never re-applied to the mixers created later, so 'finished' (and 'loop') were never dispatched. ModelScene now remembers its subscriptions and attaches them to every mixer it creates. 2. Setting the animationName property and then calling play() looped forever (present in all versions). Assigning animationName schedules an asynchronous reactive update whose updated() re-triggers playback; it did so with the default options (repetitions: Infinity / LoopRepeat), overwriting a play({repetitions: 1}) issued in the same tick, so the clip looped and never finished. Using the declarative animation-name attribute avoided this only because the property never changed. play() now records its options and the re-trigger reuses them instead of resetting to an infinite loop. Both paths are covered by regression tests in animation-spec.ts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 33e5562 commit 6bc23ee

3 files changed

Lines changed: 78 additions & 7 deletions

File tree

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

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ const $changeAnimation = Symbol('changeAnimation');
2525
const $appendAnimation = Symbol('appendAnimation');
2626
const $detachAnimation = Symbol('detachAnimation');
2727
const $paused = Symbol('paused');
28+
const $currentAnimationOptions = Symbol('currentAnimationOptions');
2829

2930
interface PlayAnimationOptions {
3031
repetitions: number;
@@ -97,6 +98,13 @@ export const AnimationMixin = <T extends Constructor<ModelViewerElementBase>>(
9798
animationCrossfadeDuration: number = 300;
9899

99100
protected[$paused]: boolean = true;
101+
// Remembers the options from the most recent play() call so re-applying the
102+
// current animation (e.g. when animationName changes) preserves them rather
103+
// than resetting to an infinite loop. Without this, `animationName = name`
104+
// followed by `play({repetitions: 1})` loops forever and never emits
105+
// 'finished', because the asynchronous animationName update re-triggers
106+
// playback with the default (infinite) options.
107+
private[$currentAnimationOptions]: PlayAnimationOptions = DEFAULT_PLAY_OPTIONS;
100108

101109
constructor(...args: any[]) {
102110
super(args);
@@ -189,7 +197,8 @@ export const AnimationMixin = <T extends Constructor<ModelViewerElementBase>>(
189197
const modelIndex = options?.modelIndex ?? null;
190198
this[$scene].unpauseAnimation(modelIndex);
191199

192-
this[$changeAnimation](options);
200+
this[$currentAnimationOptions] = {...DEFAULT_PLAY_OPTIONS, ...options};
201+
this[$changeAnimation](this[$currentAnimationOptions]);
193202

194203
this.dispatchEvent(new CustomEvent('play'));
195204
}
@@ -256,17 +265,22 @@ export const AnimationMixin = <T extends Constructor<ModelViewerElementBase>>(
256265
}
257266
}
258267

259-
[$changeAnimation](options: PlayAnimationOptions = DEFAULT_PLAY_OPTIONS) {
260-
const repetitions = options.repetitions ?? Infinity;
261-
const mode = options.pingpong ?
268+
[$changeAnimation](options?: PlayAnimationOptions) {
269+
// When re-triggered without explicit options (e.g. from an animationName
270+
// change or onModelLoad), reuse the options from the most recent play()
271+
// so an in-flight `play({repetitions: 1})` is not overwritten with the
272+
// infinite-loop default.
273+
const opts = options ?? this[$currentAnimationOptions];
274+
const repetitions = opts.repetitions ?? Infinity;
275+
const mode = opts.pingpong ?
262276
LoopPingPong :
263277
(repetitions === 1 ? LoopOnce : LoopRepeat);
264278
this[$scene].playAnimation(
265279
this.animationName,
266280
this.animationCrossfadeDuration / MILLISECONDS_PER_SECOND,
267281
mode,
268282
repetitions,
269-
options.modelIndex);
283+
opts.modelIndex);
270284

271285
// If we are currently paused, we need to force a render so that
272286
// the scene updates to the first frame of the new animation

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,43 @@ suite('Animation', () => {
8181
expect(element.paused).to.be.true;
8282
});
8383

84+
test('dispatches "finished" when a repetition-limited animation completes',
85+
async () => {
86+
element.play({repetitions: 1, pingpong: false});
87+
await element.updateComplete;
88+
89+
const finished = waitForEvent(element, 'finished');
90+
// Jump near the end so the LoopOnce action completes quickly (the
91+
// default clip is longer than the test timeout).
92+
element.currentTime = Math.max(element.duration - 0.05, 0);
93+
await timePasses(500);
94+
await finished;
95+
});
96+
97+
// Regression test: setting `animationName` schedules an async reactive
98+
// update whose updated() re-triggers playback. It used to do so with the
99+
// default (infinite-loop) options, clobbering a `play({repetitions: 1})`
100+
// issued in the same tick, so the animation looped forever and never
101+
// emitted 'finished'.
102+
test('dispatches "finished" when animationName is set then played once',
103+
async () => {
104+
const target = element.availableAnimations.find(
105+
(name) => name !== element.animationName) ??
106+
element.availableAnimations[0];
107+
108+
element.animationName = target;
109+
element.play({repetitions: 1, pingpong: false});
110+
111+
// Flush the animationName update, where the clobber used to happen.
112+
await element.updateComplete;
113+
114+
const finished = waitForEvent(element, 'finished');
115+
// Jump near the end so the LoopOnce action completes quickly.
116+
element.currentTime = Math.max(element.duration - 0.05, 0);
117+
await timePasses(500);
118+
await finished;
119+
});
120+
84121
suite('when play is invoked with no options', () => {
85122
setup(async () => {
86123
const animationsPlay = waitForEvent(element, 'play');

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

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,12 @@ export class ModelScene extends Scene {
120120
private boundsAndShadowDirty = false;
121121
private mixers: AnimationMixer[] = [];
122122
private mixerPausedStates: boolean[] = [];
123+
// Mixer event subscriptions are kept so they can be re-applied to the mixers
124+
// that are recreated on every setSource() (one per loaded glTF).
125+
private mixerEventSubscriptions: Array<{
126+
event: keyof AnimationMixerEventMap,
127+
callback: (...args: any[]) => void
128+
}> = [];
123129
private cancelPendingSourceChange: (() => void)|null = null;
124130
private animationsByName: Map<string, AnimationClip> = new Map();
125131
private currentAnimationActions: (AnimationAction|null)[] = [];
@@ -281,11 +287,11 @@ export class ModelScene extends Scene {
281287
if (gltf != null) {
282288
this._models.push(gltf.scene);
283289
this.target.add(gltf.scene);
284-
this.mixers.push(new AnimationMixer(gltf.scene));
290+
this.mixers.push(this.createMixer(gltf.scene));
285291
this.mixerPausedStates.push(false);
286292
this.currentAnimationActions.push(null);
287293
} else {
288-
this.mixers.push(new AnimationMixer(this.target));
294+
this.mixers.push(this.createMixer(this.target));
289295
this.mixerPausedStates.push(false);
290296
this.currentAnimationActions.push(null);
291297
}
@@ -1282,11 +1288,25 @@ export class ModelScene extends Scene {
12821288

12831289
subscribeMixerEvent(
12841290
event: keyof AnimationMixerEventMap, callback: (...args: any[]) => void) {
1291+
// Remember the subscription so it survives the mixers being recreated on
1292+
// the next setSource(), then apply it to the mixers that already exist.
1293+
this.mixerEventSubscriptions.push({event, callback});
12851294
for (const mixer of this.mixers) {
12861295
mixer.addEventListener(event, callback);
12871296
}
12881297
}
12891298

1299+
// Creates an AnimationMixer with the currently subscribed events already
1300+
// attached, so events (e.g. 'finished', 'loop') keep firing after a new
1301+
// model is loaded.
1302+
private createMixer(root: Object3D): AnimationMixer {
1303+
const mixer = new AnimationMixer(root);
1304+
for (const {event, callback} of this.mixerEventSubscriptions) {
1305+
mixer.addEventListener(event, callback);
1306+
}
1307+
return mixer;
1308+
}
1309+
12901310
/**
12911311
* Call if the object has been changed in such a way that the shadow's
12921312
* shape has changed (not a rotation about the Y axis).

0 commit comments

Comments
 (0)