Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Multiple .start() and .to(prop:[]) #602

Closed
wants to merge 5 commits into from
Closed

Multiple .start() and .to(prop:[]) #602

wants to merge 5 commits into from

Conversation

MasatoMakino
Copy link
Contributor

Related Issues

Purpose of this Pull Request

This PR fixes the Issue of tween acceleration on multiple .start().

If an object with an array is passed to .to(), _valuesEnd will be irreversibly changed when .start() is executed multiple times.
As a result, each time you run .start(), animation will be faster.

Details of the changes

  1. Added test cases.
  2. Added _hasSetupProperty flag to check if _setupProperties() has been executed.
  3. If you reconfigure .to(), startValue will be changed to the current value of the tween target object. Check _valuesStart and keep it if it is already initialized.

tween.js/src/Tween.ts

Lines 169 to 172 in ae24c58

// Save the starting value, but only once.
if (typeof _valuesStart[property] === 'undefined') {
_valuesStart[property] = startValue
}

I used this code as a reference for change point 3.

Related pull requests

I think it might be difficult to merge this PR as-is, because of conflicts with features such as dynamic to and _propertiesAreSetUp. In that case, I hope the added test cases will contribute to everyone.

@trusktr
Copy link
Member

trusktr commented Apr 19, 2021

@MasatoMakino Thanks for this! Would you mind making an example for this?

@trusktr trusktr added this to Opened Pull Requests in Pull Requests via automation Apr 19, 2021
@trusktr trusktr moved this from Opened Pull Requests to Proposal Accepted in Pull Requests Apr 19, 2021
@trusktr
Copy link
Member

trusktr commented Apr 19, 2021

The more I see changes and fixes like these, the more I start to want to start working on a Timeline/Keyframe/Tween abstraction. It seems Tween is trying to do too much, all in one.

@MasatoMakino
Copy link
Contributor Author

MasatoMakino commented Apr 20, 2021 •
edited

@MasatoMakino Thanks for this! Would you mind making an example for this?

@trusktr Thanks for your reply.

I have made examples of multiple start with CodePen.
tween.js 18.6.4 : multiple start and array
tween.js 18.6.4 + PR #602 : multiple start and array

I also considered adding a new page in ./examples.
However, it is hard to tell what is a difference between examples of multiple start and example of repeat, and there is a risk of confusing users. I did not add examples of multiple start to this PR, but made examples with codepen.

@trusktr trusktr changed the base branch from master to main April 23, 2023 19:37
@trusktr trusktr linked an issue Apr 23, 2023 that may be closed by this pull request
Animation on repeat speeds up over time. #378
Open
@trusktr
Copy link
Member

trusktr commented Apr 23, 2023

Closes #378

@trusktr
Copy link
Member

trusktr commented Jan 15, 2024

Looks like this got fixed already!

https://codepen.io/trusktr/pen/XWGpKyX/10faa7d954bf016a3f0261ca626db17e?editors=1010

There's a similar issue with startWithCurrentValues(), but the problem is gone when using start(). See this issue:

@trusktr trusktr closed this Jan 15, 2024
Pull Requests automation moved this from Proposal Accepted to Done Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
Pull Requests
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Animation on repeat speeds up over time.
2 participants
@MasatoMakino @trusktr