BURIED : fix explosion timer #5393
Conversation
|
@kartiksharmakk : Thank you for your contribution. However, two points. Firstly, your commit messages do not comply with https://wiki.scummvm.org/index.php?title=Commit_Guidelines .. They should be something like "BURIED: Fix Castle Galliard Wall Explosion Timer Bug". You should rebase and squash this to fix. |
|
Secondly, while your fix will cause the explosion to occur, it is not the correct fix since it effectively just disables the following test. As per my last comment on Bug #13769, this bug was present in the original Win3.1 executable but fixed in later versions i.e. Win95 release. It will need someone to look at what the actual expected fix is, since I don't think just resetting that value will create the correct game logic... |
|
To be clear, someone needs to look at the disassembled code around these variables in the Win 95 / patched binary with Ghidra or similar to work out what the original does. |
Actually, your fix may be "the right thing", though the following test could be removed since it will always be true. However, I do have a bunch of local WIP code looking at this. It may also be possible to determine this based on original savegames since the variable is written to the savefile. |
kartiksharmakk
force-pushed
the
master
branch
from
b3e28a0
to
93a56c6
Compare
I have tested the game using the saved game from Trac #13769. The explosion occurs as expected, and if we rush into the explosion, our player dies, which is the intended behavior. Additionally, I have successfully completed the mission, so I don't believe it affected the game logic. I also tried recalling from the jump menu and completing the level again. Sir, could you please verify if you can replicate the same actions I described in the game? I believe this could serve as a temporary fix. |
|
The proposed change looks correct, regarding the logic itself. That check for a zero value looks very fishy, and would justify the behavior described in the bug. |
|
After reviewing the proposed again, it is correct. That check for zero value was completely wrong in the constructor. Thanks again for the work on this! |
|
@bluegr Did you check with the original sources before merging? |
in reference to Trac #13769 , during loading of global flag cgMWCatapultData with older value , there was a conflict with getMillis() which has been fixed by reinitalizing cgMWCatapultData to zero . This fixed the explosion timer , which was struck due to failing of condition
(timer + CATAPULT_TIMEOUT_VALUE < g_system->getMillis()))