Conversation
Allows custom pause functions. Fixes Pausing error when exiting game that is self-paused, and then entering a new game (previously caused pause lock out)
marczaku
left a comment
There was a problem hiding this comment.
I have read your reasoning for the event by now. I agree that we need higher extensibility. But I don't understand, yet, why an event on the PauseHandler is used. Maybe, we should move that functionality to Game Baseclass instead.
| public delegate void OnPause(bool pausing); | ||
| public event OnPause onPause; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| Time.timeScale = 0f; | ||
|
|
||
| this.isPaused = true; | ||
| this.volumeBeforePause = AudioListener.volume; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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){ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| float timeScaleBeforePause = 1; | ||
| float volumeBeforePause = 1; |
There was a problem hiding this comment.
These initialized values wouldn't be necessary if it wasn't for the double-pause change below.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(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; | ||
| float timeScaleBeforePause = 1; | ||
| float volumeBeforePause = 1; | ||
| bool isPaused = false; |
There was a problem hiding this comment.
false is the default value of type bool
There was a problem hiding this comment.
(am I being marked down for style sheets?) :P
| public void ResetPauseValues(){ | ||
| onPause = null; | ||
| timeScaleBeforePause = 1; | ||
| volumeBeforePause = 1; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| async Task ResetGame() { | ||
| pauseHandler.ResetPauseValues(); |
There was a problem hiding this comment.
Totally makes sense! I'd love to merge that change into main!
There was a problem hiding this comment.
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.
Allows custom pause functions. Fixes Pausing error when exiting game that is self-paused, and then entering a new game (previously caused pause lock out)