Skip to content

Pausing Logic #1

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

Open
KevWills wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Pausing Logic #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Assets/Plugins/Mediabox/Source/GameKit/GameManager/GameManagerBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ public async void UnloadGameContent() {
}

async Task ResetGame() {
pauseHandler.ResetPauseValues();
Copy link
Collaborator

@marczaku marczaku Nov 22, 2022

Choose a reason for hiding this comment

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

Totally makes sense! I'd love to merge that change into main!

Copy link
Author

@KevWills KevWills Nov 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

To the point above, this reset game is called at the end of a game when content is unloaded, and at the start, so setting isPause to false, would mean we can't unpause when the next game is loaded. Trapping us in a paused state.

if (SceneManager.GetActiveScene().name != this.DefaultSceneName)
await SceneManager.LoadSceneAsync(this.DefaultSceneName);
UnloadBundle();
Expand Down
56 changes: 42 additions & 14 deletions Assets/Plugins/Mediabox/Source/GameKit/GameManager/PauseHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,30 +2,58 @@

namespace Mediabox.GameKit.GameManager {
public class PauseHandler {
float timeScaleBeforePause;
float volumeBeforePause;
bool isPaused;
float timeScaleBeforePause = 1;
float volumeBeforePause = 1;
Comment on lines +5 to +6
Copy link
Collaborator

@marczaku marczaku Nov 22, 2022

Choose a reason for hiding this comment

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

These initialized values wouldn't be necessary if it wasn't for the double-pause change below.

Copy link
Author

@KevWills KevWills Nov 23, 2022

Choose a reason for hiding this comment

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

These are actually in case someone were to use a subscribed pauseEvent, and somehow removed the subscription before the unpause. In that case no value for the timescalebeforepause would be recorded, and the default being 0, would mean that the game would "unpause" to timescale 0.

Copy link
Author

@KevWills KevWills Nov 23, 2022

Choose a reason for hiding this comment

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

(reiterating a note from above, if we move the Pause into the Game base. Then I think we need to initialize these values in each game, as the memory for them will be lost as the games unload and then load anew.)

bool isPaused = false;
Copy link
Collaborator

@marczaku marczaku Nov 22, 2022

Choose a reason for hiding this comment

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

false is the default value of type bool

Copy link
Author

@KevWills KevWills Nov 23, 2022

Choose a reason for hiding this comment

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

(am I being marked down for style sheets?) :P


public delegate void OnPause(bool pausing);
public event OnPause onPause;
Comment on lines +9 to +10
Copy link
Collaborator

@marczaku marczaku Nov 22, 2022

Choose a reason for hiding this comment

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

The PauseHandler is currently only being used as a protected Field in GameManagerBase, or?
What's the use of this event exactly?
Instead of hidden dependency injection using this method where assigning a delegate will replace the internal functionality, I suggest moving the Pause Logic over to the Game-base classes and leaving it extensible over there. What do you think?

The flow would be:

GameManager.OnApplicationPause() -> FindGame() -> Game.Pause()
Game could then use the provided PauseHandler in a more extensible variant (virtual methods ) per default, but the behaviour could also be replaced by assigning a new IPauseHandler implementation to specific games if there's a need.

Then again, I don't think that there's any need for this right now, as long as ApplicationPause is expected to have a consistent effect of the Application being silent and updates being paused.

Copy link
Author

@KevWills KevWills Nov 23, 2022

Choose a reason for hiding this comment

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

I actually think this change is much better than having the pause handler at all. As we currently have to do the work of locating the PauseHandler before we can subscribe to its delegate event to do the unique pausing functions we want. Putting the default pause into the Game base, and then letting us override it I think will be much simpler for new users to the plugin to work with.

Copy link
Author

@KevWills KevWills Nov 23, 2022

Choose a reason for hiding this comment

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

The Game base float value for timeScaleBeforePause, will have to be initialized as 1. As the Game will unload when exiting, and a new Game will load when entering, and then that game will not have a stored value to react to the UnPause. This might be a nicer pattern, as we don't have to call Reset values at all, as each new game, when initialized will be providing reset values.



public void Pause() {
if (!this.isPaused) {
this.timeScaleBeforePause = Time.timeScale;
this.volumeBeforePause = AudioListener.volume;
this.isPaused = true;
} else {


if (this.isPaused){
Debug.LogWarning("Pause() has been called multiple times. To avoid unintended behavior, it is recommended to ensure calling Unpause() before Pausing again.");
}
Time.timeScale = 0f;

this.isPaused = true;
this.volumeBeforePause = AudioListener.volume;
Copy link
Collaborator

@marczaku marczaku Nov 22, 2022

Choose a reason for hiding this comment

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

This can skew output for Games where Pause() or Unpause() is called twice. Are you sure that you want to execute this method anyways, even if the PauseManager is currently not in a paused state?
I'd rather tend to throw an Exception (making it even more strict) rather than loosening the impact by allowing Double-Pause and Double-Unpause.

Copy link
Author

@KevWills KevWills Nov 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I figured this would allow mediabox to reset the game back to a playable state if all else failed for the user. For example, something happens when pausing that messes up the game. Instead of being locked in a time.timescale = 0 state. The user can use the wonderz button (blue arrow) to exit the game fully, pick the game again (or a new game), now the values for timeBeforePause have been hard reset. They can enter the game with the timescale corrected, and they can continue on playing without having to force quit the app.

Copy link
Author

@KevWills KevWills Nov 23, 2022

Choose a reason for hiding this comment

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

Basically if they get to this point, we've already messed up, and we get the error flag while testing. But for the end user the product is still "serviceable" even while broken.

AudioListener.volume = 0f;

if (onPause != null){
if (onPause.GetInvocationList().Length > 0){
Copy link
Collaborator

@marczaku marczaku Nov 22, 2022

Choose a reason for hiding this comment

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

This is not necessary as far as I know, since delegates don't pass a null check when the InvocationList is empty as far as I know. Do you know more?

Copy link
Author

@KevWills KevWills Nov 23, 2022

Choose a reason for hiding this comment

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

I was going off of the belief that the delegate if null when you add the first subscriber will make a new delegate, and that the += operator was overridden for that use case. So it was possible to subscribe and then unsubscribe leaving a delegate that has an empty invocation list. However I have not tested this, should be simple enough to create a test tonight.

onPause.Invoke(true);
return;
}
}

this.timeScaleBeforePause = Time.timeScale;
Time.timeScale = 0f;

}

public void Unpause() {
if (this.isPaused) {
Time.timeScale = this.timeScaleBeforePause;
AudioListener.volume = this.volumeBeforePause;
this.isPaused = false;
} else {

if (!this.isPaused){
Debug.LogWarning("Unpause() has been called multiple times. To avoid unintended behavior, it is recommended to ensure calling Pause() before Unpausing again.");
}

this.isPaused = false;
AudioListener.volume = this.volumeBeforePause;
if (onPause != null){
if (onPause.GetInvocationList().Length > 0){
onPause.Invoke(false);
return;
}
}
Time.timeScale = this.timeScaleBeforePause;
}

public void ResetPauseValues(){
onPause = null;
timeScaleBeforePause = 1;
volumeBeforePause = 1;
}
Comment on lines +53 to 57
Copy link
Collaborator

@marczaku marczaku Nov 22, 2022

Choose a reason for hiding this comment

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

I really like this function! :)
It should before anything else reset isPaused to the value false, though.
If that's done and the double-pausind is disabled again, then the other two values won't even need to be reset anymore.

Copy link
Author

@KevWills KevWills Nov 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ah! now I remember why I wrote it like this. Not having Daniel's code, I couldn't tell if UnPause was called before or after the game was loaded. If I set isPause to false before the unpause it would cause a double-unpause error.
In addition I only set the timeScaleBeforePause and volumeBeforePause instead of just resetting the audioListener.volume and Time.time, because if I had reset the audioListener directly, it could spill playing music from the game into the main app.


}
Expand Down