In rP21359 I introduced a new bug while fixing the walking sound animations that makes any serialization test fail in non obvious ways, because the sounds are different from the real game.
Details
Test that it works now, and that I didn't do anything stupid in the previous revision.
- Start a multiplayer match without any patches and task some units
- Replay that file in non-visual replay mode (pyrogenesis -replay=...commands.txt -mod=public)
- If there are some hash mismatches, you reproduced the bug. Otherwise go back to step 1.
- Apply the patch, do the nonvisual replay of the same commands.txt again and check if it the hash mismatch is gone.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 6136 Build 10212: Vulcan Build Jenkins
Event Timeline
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/differential/557/display/redirect
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/differential/558/display/redirect
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/differential/559/display/redirect
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/differential/560/display/redirect
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/differential/561/display/redirect
Trying again...
@Itms It's weird, but if the diff has
+++ ps/trunk/source/simulation2/components/CCmpVisualActor.cpp
it adds subscribers correctly
but if it is
+++ source/simulation2/components/CCmpVisualActor.cpp
It doesn't.
source/simulation2/components/CCmpVisualActor.cpp | ||
---|---|---|
451 | CmpPtr<ICmpSound> cmpSound(GetEntityHandle()); works. Tested with non-visual replays and hashes match, so it fixed the problem I mentioned. |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/565/display/redirect
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/differential/562/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/566/display/redirect
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/differential/563/display/redirect
Tried to reproduce again about 4 times with different maps and commands but failed. Why doesn't this always bug?
Why m_Unit is not initialized in nonvisual mode:
m_Unit is not initialized in non-visual mode because the CGame constructor in Game.cpp has disabledGraphics = true which leaves m_GameView at NULL, thus no GameView creates an ObjectManager and the SetObjectManager function is not called in CGame constructor.
Hence CUnitManager::CreateUnit doesn't call CUnit::Create and returns NULL when called from CCmpVisualActor::InitModel.
Perhaps we can add a // Not initialized in non-visual mode above the variable declaration.
Why nonvisual replaymode doesn't test well:
The visual replay mode tests whatever hash is found in commands.txt whereas the non-visual replaymode compute hashes only every 100th turn (turn % 100 == 0 in Replay.cpp), presumably to save performance.
But sometimes we prefer certainty over performance, so it should become a commandline argument if we want to skip hashing to improve performance for both replaymodes.
I suspect the slower one should be enabled by default as it already is for visual replays and networked games.
Created D1538 to test this diff more properly.
Why it doesn't always mismatch hashes:
Can reproduce (more easily using D1538) by moving some units and it self-corrects once all units are destroyed that ever moved.
I guess that's because the idle animation doesn't have a SoundGroup.
Why it's correct:
The patch only changes the order of the two statements, resulting in the soundgroup being loaded in non-visual mode too, thus always being initialized correctly, thus computing the correct hash.
temple said on irc yesterday that it is correct and must be correct. Maybe he wants to accept too.
I don't know if it's complete, almost never worked with the components and I didn't read all of the concerned diff.
Should be committed now as this bug makes it harder to debug OOS issues and since we don'T want to have to reapply this patch everytime when debugging a23 reports in the future and reduce false positives.
m_Unit is basically a part of graphics, so shouldn't ever be used to stuff that affects the simulation and/or may be serialised.
Would be useful, above the declaration of the variable
Do I have to change something for this ?
Only if you changed a different place where a serialized value depends on m_Unit
Yes that's what the diff is fixing in the first place, I was just clarifying to not do that in the future either :p
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/591/display/redirect
source/simulation2/components/CCmpVisualActor.cpp | ||
---|---|---|
573 | Didn't want to touch the .h file but okay. |
source/simulation2/components/CCmpVisualActor.cpp | ||
---|---|---|
70 | \n |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/592/display/redirect