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

fix: Deprecate pause and resume in GameLoop #1240

Merged
merged 3 commits into from
Jan 16, 2022

Conversation

st-pasha
Copy link
Contributor

Description

GameLoop methods pause() and resume() are marked as deprecated, with a recommendation to use start() and stop() instead. The reason they are deprecated is because their functionality is identical to that of the other pair of methods, so no reason to keep them around... Also, property previous was made final (with a deprecated getter for backward-compatibility), since changing it externally would break GameLoop's functionality.

Here's the summary of behavior changes:

  • start() can now be called multiple times; previously calling start() twice would produce an error
  • stop() now works the same way as pause(); previously calling stop() and then start() would produce negative times
  • property .previous is no longer settable

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
  • I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples.

Breaking Change

  • No, this is not a breaking change.

Related Issues

This is split out of #1234

@st-pasha
Copy link
Contributor Author

Note: we may also make pause/resume the main methods, and redirect/deprecate start+stop.

}

void dispose() {
_ticker.dispose();
}

void pause() {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think they were meant to be the same, IIRC @erickzanardo added those to fix a bug or something

Copy link
Member

@erickzanardo erickzanardo Dec 18, 2021

Choose a reason for hiding this comment

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

Yes, I do remember something like this too, but I can remember exactly what bug and why, we should hold this until we can remember

Copy link
Member

@spydon spydon Dec 18, 2021

Choose a reason for hiding this comment

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

I also thought we added these, but apparently these were added by someone called @erf 20 months ago, maybe they could explain?

Copy link
Member

Choose a reason for hiding this comment

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

Here is the PR: #302

Copy link
Contributor

Choose a reason for hiding this comment

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

@spydon hi, i took a look now at my PR where i simplified the game loop to use a Ticker. It looks like start and stop replaced the scheduleTick and unscheduleTick calls. The only different between those were that previous Duration were set to zero in pause. It think it would be fine to combine these calls. Not sure if there would be a case where e.g. the user would e.g. want to pause the callbacks, but still make the timer move forward or something like that.

Copy link
Member

Choose a reason for hiding this comment

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

Can you file an issue for that @st-pasha since you have the most context?

Copy link
Member

Choose a reason for hiding this comment

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

Btw, in your test, did you declare the game outside of the GameWidget?

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've tested with the standard example from our Dashbook, I'll have to check if it recreates the game instance upon frame rotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I checked again -- after making sure that the game instance is declared only once in the examples app, the "bug" disappears (i.e. the game state keeps being paused after rotating the frame or changing window size, or opening the side menu). This happens both in the main branch and in this branch.

Copy link
Member

Choose a reason for hiding this comment

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

Super, then it works as intended!

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Lgtm, I don't think there ever was a reason to have both and everything seems to work fine without pause and resume.

@spydon spydon merged commit dc37053 into flame-engine:main Jan 16, 2022
@st-pasha st-pasha deleted the ps/gameloop-pause-resume branch January 16, 2022 21:08
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.

5 participants