Skip to content

feat: make options for appendAnimation and detachAnimation optional#5025

Open
Frank3K wants to merge 3 commits intogoogle:masterfrom
Frank3K:feature/make-append-detach-options-optional
Open

feat: make options for appendAnimation and detachAnimation optional#5025
Frank3K wants to merge 3 commits intogoogle:masterfrom
Frank3K:feature/make-append-detach-options-optional

Conversation

@Frank3K
Copy link
Contributor

@Frank3K Frank3K commented Apr 15, 2025

Description

This PR makes all parameters of AppendAnimationOptions and DetachAnimationOptions optional.

Such that the following is possible:

// Example: Only specify repetitions
appendAnimation('some-animation', { repetitions: 5 });

// Example: Only specify fade for detach
detachAnimation('some-animation', { fade: 200 });

Previously, you had to pass all options, or no options at all (in case the default was used). Current code in master:

interface AppendAnimationOptions {
pingpong: boolean, repetitions: number|null, weight: number,
timeScale: number, fade: boolean|number, warp: boolean|number,
relativeWarp: boolean, time: number|null
}
interface DetachAnimationOptions {
fade: boolean|number
}

@mohammadbaghaei Would you be so kind to do the review? I tried to use appendAnimation, but then I was a bit surprised I needed to pass all parameters. I have no real experience with these methods, so it would be nice if you can take a look and functionally test.

Reference Issue

Such that the following is possible:

// Example: Only specify repetitions
appendAnimation('some-animation', { repetitions: 5 });

// Example: Only specify fade for detach
detachAnimation('some-animation', { fade: 200 });
LoopPingPong :
(repetitions === 1 ? LoopOnce : LoopRepeat);

const needsToStop = repetitions !== Infinity || mode !== LoopPingPong;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mohammadbaghaei I am particularly not 100% sure about this line.

@diegoteran
Copy link
Collaborator

Thanks for requesting these changes, I'll need to test them before submitting

@diegoteran
Copy link
Collaborator

I believe these are all optional already.

If you look at the example in https://modelviewer.dev/examples/animation/index.html#appendAnimation

Do you mind checking again @Frank3K ? In case this change is not necessary.

@Frank3K
Copy link
Contributor Author

Frank3K commented Mar 3, 2026

Do you mind checking again @Frank3K ? In case this change is not necessary.

Thanks for reviewing!

You're right that the options parameter is optional - but the properties inside the object are not. In master, AppendAnimationOptions declares all fields as required, so passing { weight: 0.5 } is a TypeScript type error.

More importantly, it's also a runtime bug. Default parameter values only apply when the argument is undefined (omitted entirely). If you pass { weight: 0.5 }, DEFAULT_APPEND_OPTIONS is never used, and options.timeScale, options.fade, options.warp, etc. all become undefined. The docs example silently produces incorrect behavior on master.

This PR fixes both issues by making the properties optional and merging with defaults inside the function {...DEFAULT_APPEND_OPTIONS, ...options}.

I've updated with master and fixed conflicts.

@diegoteran
Copy link
Collaborator

Thanks for the explanation and changes. I'm trying to figure out fi the broken tests are flaky

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants