-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Handling of repeats with browser tab sleep #564
Handling of repeats with browser tab sleep #564
Conversation
This is a test to confirm that the tween._repeat has been successfully reduced. I'm worried about whether I should test private variables. But I added this test because I would change the subtraction process for tween._repeat.
Tests for a TWEEN.update() interval of more than 1 repeat duration.
calculateElapsed() outputs a continuous elapsed, even if you give it a multi-repeat time interval.
Thanks for doing this! Will review it soon. |
} | ||
|
||
const timesRepeated = Math.trunc(elapsedTime / durationAndDelay) | ||
const timeIntoCurrentRepeat = elapsedTime - timesRepeated * durationAndDelay |
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.
@MasatoMakino Hello! I renamed some vars for easier reading. F.e. I renamed elapsedTime
to timeIntoCurrentRepeat
.
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.
I agree with what @trusktr said.
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.
I feel like the naming isn't quite right yet. 🤔 EDIT: On second thought, I think naming is fine now.
if (isFinite(this._repeat)) { | ||
this._repeat-- | ||
this._repeat -= completeCount |
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.
Down below, regarding the chained tweens, there is this:
this._chainedTweens[i].start(this._startTime + this._duration)
Does this need to be updated too? Should it now be only
this._chainedTweens[i].start(this._startTime)
because we already advanced _startTime to where the next repeat would have been?
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 chained tween logic happens on the next update right? I wonder if it needs to happen immediately in the updated where _repeat > 0, f.e. when we run this._repeat -= completeCount
and _repeat becomes 0.
Looks like we should add a unit test for repeat + chain used together.
EDIT: Aha! This issue #397 shows the problem of chained tweens starting on the next update.
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 is getting complicated. I think that later we can remove some complexity from here, and instead adding something like a Timeline feature would make everything a lot easier to understand:
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.
Another issue with repeat(N) is that we cannot make the time go backwards, because we lose the initial _startTime every time we increment it. Timeline would make it easy to go forwards/backward anywhere on the timeline.
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.
It is complicated to use "repeat" and "chain" at the same time. I agree with the need for additional unit tests.
* main: (107 commits) v22.0.0 examples: add an example showing we can import into Node.js projects in ESM format fix import for Node.js CommonJS projects v21.1.1 eslint started having an error with the latest environment, so delete it for now chore: ignore output .js files in typescript example examples: add two example projects, one plain JS modules, the other TS compiled to plain JS modules examples: simplify the createGraph function, and make it crispier on devices with devicePixelRatio higher than 1 refactor: remove requestAnimationFrame polyfill, it has been out for a long time in all browsers update formatting update prettier v21.1.0 Update package.json, remove duplicate entry so we can merge fix: Use type definition in ESM change maintainers list and remove dead link Add missing types file to package.json exports docs: update tutorial link fix(package.json): fix export map types Fix CJS entry point v21.0.0 ...
Merged! All existing tests pass, plus the new tests pass, so this is an improvement even if we know it is not working with all features. What we should do now is start to prevent more complexity, and next we should focus on simplifying, and then Timeline. |
Ah shoot, I'm doing some manual testing, and the |
Hmmm, actually, the I'm going to change that example so it only uses positive values, "remove" the undocumented "feature", and keep the change from this PR because it is more valuable. The next release will be a major version bump. |
Released in v23.0.0 |
Related Issues
#557
When the browser tab sleeps, the delay of repeated tweens is not keep
Purpose of this Pull Request
This fixes handling of repeats.
Details of the changes
When interval for TWEEN.update() is more than one repeat
tween._repeat
subtractions