bugfix(logic): Fix broken camera edge scroll after loading a savegame from the ingame menu#2000
Conversation
d92da0d to
513cdf8
Compare
|
|
||
| m_rankPointsToAddAtGameStart = 0; | ||
|
|
||
| TheMouse->onGamePaused(FALSE); |
There was a problem hiding this comment.
The observation makes sense.
The implementation is not optimal. It will be better to replace
m_pauseFrame = 0;
m_gamePaused = FALSE;
m_pauseSound = FALSE;
m_pauseMusic = FALSE;
m_pauseInput = FALSE;
m_inputEnabledMemory = TRUE;
m_mouseVisibleMemory = TRUE;
m_logicTimeScaleEnabledMemory = FALSE;with
setGamePaused(FALSE);
m_pauseFrame = 0;
m_inputEnabledMemory = TRUE;
m_mouseVisibleMemory = TRUE;
m_logicTimeScaleEnabledMemory = FALSE;higher up in this function and probably also in the init function.
This way other events can also properly update on pause state change.
Not sure if
m_inputEnabledMemory = TRUE;
m_mouseVisibleMemory = TRUE;
m_logicTimeScaleEnabledMemory = FALSE;
still need to be reset then. I suspect it does not matter, but not sure.
There was a problem hiding this comment.
I've made the requested changes and moved the memory resets above the setGamePaused(FALSE) so that things like TheMouse->setVisibility are updated as expected
Mouse edge scroll behavior, music and sound effects and mouse appear fine after the change. I do now notice a very short audio music glitch when loading the savegame probably (confirmed) because we're now calling TheAudio->resumeAudio(AudioAffect_Music) in reset()
7797d40 to
474d1da
Compare
|
The description has been updated to link the issues. I've been looking for a clean solution, the problem right now is that setGamePaused does not only affect state but also performs actions like resuming sounds and music, which now leads to an audio glitch when you do that in reset() or init(), which ideally is a function only concerned about state. Also setGamePaused deals with memory variables for restoring variables which does not apply for init() and reset(). So I think either we add some parameter to setGamePaused to indicate we only want state changes and no side effects, or we keep reset() much like it was before and i will do another pass to see that we properly reset everything. It was possible to clean up init() a bit by using reset() there see refactor commit. |
|
Better make the deduplication a separate refactor change to reduce the complexity of this bug fix. As for music bug, check why it happens and try to figure out an elegant way to avoid this. |
474d1da to
6dfcb89
Compare
|
Alright the deduplication commit is gone from this and I've added a parameter to The audio mishap was because reset would resume the music, and then milliseconds later the audiomanager subsystem would also reset which does a hard stop on all sounds |
7f91a78 to
3a4c348
Compare
|
I am confused by this change. The title says that this concerns camera edge scrolling, but the code touches resume audio code. How does this relate? |
|
In your first review you suggested using In hindsight calling |
|
If we can not need For example Note I did not debug this issue so I cannot accurately say what the right course of action now is, but seeing the observations in this pull and the attempted workarounds make it look brittle. |
|
What is the status of this change? |
|
I will be focusing on this again. Probably will need a separate refactor PR for the audio |
|
| Filename | Overview |
|---|---|
| Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp | Replaced direct member variable assignments with proper pauseGameLogic(FALSE) and pauseGameInput(FALSE) calls to ensure mouse capture state is reset correctly |
| GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp | Replaced direct member variable assignments with proper pauseGameLogic(FALSE) and pauseGameInput(FALSE) calls to ensure mouse capture state is reset correctly |
Last reviewed commit: 1f46bfd
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp
Outdated
Show resolved
Hide resolved
|
I think this should be good to go. The edge scrolling after loading is fixed, and I noticed no audio glitches anymore. Played through some skirmish/campaign/loads and sounds are normal |
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp
Outdated
Show resolved
Hide resolved
ec0563f to
1f46bfd
Compare
| m_mouseVisibleMemory = TRUE; | ||
| m_logicTimeScaleEnabledMemory = FALSE; | ||
| pauseGameLogic(FALSE); | ||
| pauseGameInput(FALSE); |
There was a problem hiding this comment.
This means
m_inputEnabledMemory = TRUE;
m_mouseVisibleMemory = TRUE;
is set before pauseGameInput is called. Is this correct? Should not be called before?
There was a problem hiding this comment.
I think it's alright, with paused set to false and m_inputEnabledMemory set to true it will trigger an additional call to TheInGameUI->SetInputEnabled(TRUE) however very shortly after it will call the reset function of TheInGameUI and reset any changes anyway
So it should be fine regardless of order but I’ll check if swapping is more efficient here in case m_inputEnabledMemory is often false
There was a problem hiding this comment.
I had another look still think it's OK: input=enabled and mouse=visible are the right default values to enforce, plus I was only ever able to see both m_inputEnabledMemory = TRUE and m_mouseVisibleMemory = TRUE in the debugger when entering reset.
Closes #1922
The capture state of
TheMouseincludes a paused state, however this paused state is not currently reset when a new game is started. This PR addresses this by resetting the paused state inGameLogic:reset().