Pause menu sets last time acceleration after closing

[Max5377]

The pause menu now sets the last time acceleration that was before it was opened. Its behavior will be the same as when setting up time acceleration without pressing CTRL.
Added two events onPauseMenuClosed and onPauseMenuOpen that will set input binding and last time acceleration.
Edit:

Pause.menu.sets.last.time.acceleration.mp4

[Web-eWorks]

I don't think restoring forced time acceleration is in scope for the pause menu. On resuming from pause, the game should attempt to restore the previous time acceleration according to the game's limits, and ignore the "forced" time acceleration set by the player.

Consider for example a player who hits the ESC key out of desperation because they are about to crash into a planet during forced time acceleration; they would not be able to resume a "safe" time acceleration out of the pause menu due to the need to close the menu and then click on a time acceleration button.

While your code currently addresses this correctly, the ability to Ctrl+Click the "close" button to restore time acceleration is very non-obvious to the player, and I think is better handled by simply allowing the player to Ctrl+Click the time acceleration buttons after resuming gameplay to select their desired time acceleration override, rather than attempting to "intelligently" determine when to break the game's self-imposed limits.

[Max5377]

@Web-eWorks The original plan was to Ctrl+Esc to force time acceleration but right now if you do that, it will just open Start Menu on Windows and not close pause menu. I will delete bit about forcing time acceleration through Ctrl+Click Close button.

[impaktor]

Not directly related to this PR, but I had a thought when reading the discussion:
what if when you hold down Ctrl-key, the time accel button icons shift to red or something, indicating a new mode / layer?
(and similarly for other buttons that change their function when combined with modifier key).

[Max5377]

Found some oversight. If the game was already paused, onPauseMenuOpen will not be executed. Instead, it will be executed when pause menu gets closed and acceleration will be set to 1x. Then onPauseMenuClosed will be queued and also be executed.

[11:50:57.116714] Info:     onPauseMenuOpen
[11:50:57.116825] Info:     onPauseMenuClosed

Still works fine, but onPauseMenuOpen has Input.EnableBindings(false). Comment about this function:

// Call EnableBindings() to temporarily disable handling input bindings while
// you're recording a new input binding or are in a modal window.

Also, when you press End game onPauseMenuClosed will not be executed and Input.EnableBindings() will not be called.
I still can assign keys and I can't control ship in the pause menu, but can this cause problems in the future?
Edit: If it will, maybe it's better to return to first approach with settingsMenuWasOpened variable?

[Web-eWorks]

EnableBindings() should probably be called at the beginning of the game and menu lifecycle in Pi.cpp. I'd prefer we have a consistent input state entering the menu. You don't need to move back to the old way of handling things if you do the above; I'll look at adding the separate event queue for "game"/"application" events as it will handle both this case and some other workarounds in the codebase.

[impaktor]

I've only skimmed the above post-merge comment, but if there's (potential) bug / "fix-me", that needs to be documented, I'd advice to either open an issue, or push a fix-me comment to the code.

(This issue tracker is filled with "roger that, I'll fix it shortly", then a decade passes. Which is not a criticism, I just prefer to have TO-DOs documented somewhere so they don't get lost, especially a comment in a merged PR).

[Max5377]

Done some rewrite on my computer. Grouped ui.optionsWindow:open() and ui.optionsWindow:close() into one method ui.optionsWindow:changeState() and events onPauseMenuOpen and onPauseMenuClosed will be queued in said methods, so EnableBindings() in Pi.cpp is not needed. Will do update of this PR once event queue will be separated for "game"/"application" events.
P.S.: Now I only wish I had done this before the merge...

[Web-eWorks]

No worries, sometimes subtle issues like this sneak past the first round of PR review and testing. Feel free to open another PR with the fixes.