-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ModelAnimation.startOffset #10043
Conversation
…yed and for them to be paused and resumed
Thanks for the pull request @srcejon!
Reviewers, don't forget to make sure that:
|
Thanks again for your contribution @srcejon! No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with |
} | ||
|
||
let localAnimationTime = delta * duration * scheduledAnimation.multiplier; | ||
let localAnimationTime = | ||
delta * duration * scheduledAnimation.multiplier + startDelta; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like the wrong place to be adding startDelta.
What happens when loop
is set to REPEAT
for example? Or worse, MIRRORED_REPEAT
.
It also seems to be in the wrong units. If its supposed to be a "Fractional offset in to the animation's timeline" shouldn't it be multiplied by duration? By doing it this way, the user has to know what the duration is.
I think the best way to fix this is to add startDelta to delta on line 415. Then startDelta really can be a fraction, and REPEAT, MIRRORED_REPEAT will be properly handled, and play will be correctly handled when repeat is false...
Thanks again for your contribution @srcejon! No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with |
1 similar comment
Thanks again for your contribution @srcejon! No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with |
Thanks again for your contribution @srcejon! No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with |
2 similar comments
Thanks again for your contribution @srcejon! No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with |
Thanks again for your contribution @srcejon! No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with |
I'm closing this due to inactivity. If you believe this is still an issue, please feel free to re-open and we'll give this a fresh review. Thanks! |
As per #10042, this PR adds ModelAnimation.startOffset, which allows specifying at what point an animation starts playing from.