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.

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

Merged
merged 6 commits into from
Jan 15, 2024

Conversation

MasatoMakino
Copy link
Contributor

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

  • Continuous elapsed
  • Multiple tween._repeat subtractions

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.
@trusktr
Copy link
Member

trusktr commented Oct 19, 2020

Thanks for doing this! Will review it soon.

}

const timesRepeated = Math.trunc(elapsedTime / durationAndDelay)
const timeIntoCurrentRepeat = elapsedTime - timesRepeated * durationAndDelay
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

@trusktr trusktr Apr 23, 2023

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.

src/Tween.ts Show resolved Hide resolved
if (isFinite(this._repeat)) {
this._repeat--
this._repeat -= completeCount
Copy link
Member

@trusktr trusktr Apr 23, 2023

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?

Copy link
Member

@trusktr trusktr Apr 23, 2023

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.

Copy link
Member

@trusktr trusktr Apr 23, 2023

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:

#647

Copy link
Member

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.

Copy link
Contributor Author

@MasatoMakino MasatoMakino Apr 24, 2023

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.

@trusktr trusktr changed the base branch from master to main July 8, 2023 06:47
@trusktr trusktr changed the base branch from main to test-557-tab-sleep January 15, 2024 04:00
* 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
  ...
@trusktr trusktr merged commit bb608dd into tweenjs:test-557-tab-sleep Jan 15, 2024
@trusktr
Copy link
Member

trusktr commented Jan 15, 2024

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.

@trusktr
Copy link
Member

trusktr commented Jan 15, 2024

Ah shoot, I'm doing some manual testing, and the 13_relative_start_time example broke. Will revert. But I think 13_relative_start_time will provides some clues as to what is missing

@trusktr
Copy link
Member

trusktr commented Jan 15, 2024

Hmmm, actually, the 13_relative_start_time example uses .delay(-500). It is not intuitive what "delay negative 500" means, and the improvement of this PR is a lot more significant.

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.

@trusktr
Copy link
Member

trusktr commented Jan 15, 2024

Released in v23.0.0

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.

When the browser tab sleeps, the delay of repeated tweens is not keep
2 participants