Page MenuHomeWildfire Games

Fix non-visual replay hash mismatch caused by CCmpSound condition in CCmpVisualActor in rP21359
ClosedPublic

Authored by Stan on May 24 2018, 9:47 AM.

Details

Reviewers
elexis
temple
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#5205
Summary

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.

Test Plan

Test that it works now, and that I didn't do anything stupid in the previous revision.

  1. Start a multiplayer match without any patches and task some units
  2. Replay that file in non-visual replay mode (pyrogenesis -replay=...commands.txt -mod=public)
  3. If there are some hash mismatches, you reproduced the bug. Otherwise go back to step 1.
  4. 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 6140
Build 10216: Vulcan BuildJenkins

Event Timeline

Vulcan added a subscriber: Vulcan.May 24 2018, 9:48 AM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/557/display/redirect

Stan updated this revision to Diff 6630.EditedMay 24 2018, 10:45 AM
Stan added a subscriber: wraitii.

Better version as suggested by @wraitii in rP21359.

I was using m_Unit before checking it existed which was dumb.
Using GetEntityHandle() means there is no reason getting cmpSound would fail if m_Unit is null.

It's also done like this for other components in that file.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/558/display/redirect

Stan updated this revision to Diff 6631.May 24 2018, 10:52 AM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/559/display/redirect

Stan updated this revision to Diff 6632.May 24 2018, 10:54 AM
Owners added a subscriber: Restricted Owners Package.May 24 2018, 10:54 AM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/560/display/redirect

Stan updated this revision to Diff 6633.May 24 2018, 10:57 AM

Last try.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/561/display/redirect

elexis edited the test plan for this revision. (Show Details)May 24 2018, 11:03 AM
Stan updated this revision to Diff 6635.EditedMay 24 2018, 12:11 PM
Stan added a subscriber: Itms.

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.

Stan updated this revision to Diff 6637.May 24 2018, 12:21 PM

Better with an updated SVN

elexis added a subscriber: elexis.May 24 2018, 8:34 PM

This doesn't compile

Stan updated this revision to Diff 6638.May 24 2018, 11:42 PM
temple added a subscriber: temple.May 25 2018, 12:21 AM
temple added inline comments.
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.

Stan updated this revision to Diff 6639.May 25 2018, 5:48 PM

Simplify the call.

Stan marked an inline comment as done.May 25 2018, 5:48 PM

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

Stan added a comment.May 26 2018, 11:52 AM

Notice the last diff built (Just in case)

elexis retitled this revision from Fix Full Hash Mismatch caused by rP21359 to Fix non-visual replay hash mismatch caused by CCmpSound condition in CCmpVisualActor in rP21359.May 26 2018, 12:42 PM

Tried to reproduce again about 4 times with different maps and commands but failed. Why doesn't this always bug?

Stan added a comment.May 26 2018, 2:36 PM

Maybe some global stuff has always cmp sound no matter what.

elexis accepted this revision.May 28 2018, 3:04 AM

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.

This revision is now accepted and ready to land.May 28 2018, 3:04 AM

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.

Stan added a comment.EditedMay 28 2018, 10:30 AM

@elexis, thanks for the review. I'll commit it tonight (as when I have access to my svn and I'm back from work) unless @temple wants me to change to change something.

Should I add the comment ?

// Not initialized in non-visual mode

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.

Do I have to change something for this ?

In D1519#62413, @Stan wrote:

Should I add the comment ?

// Not initialized in non-visual mode

Would be useful, above the declaration of the variable

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.

Do I have to change something for this ?

Only if you changed a different place where a serialized value depends on m_Unit

In D1519#62415, @elexis wrote:
In D1519#62413, @Stan wrote:

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.

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

Stan updated this revision to Diff 6670.May 28 2018, 3:59 PM

Add the comment.

That's not the declaration of m_Unit

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/591/display/redirect

Stan added inline comments.May 28 2018, 4:25 PM
source/simulation2/components/CCmpVisualActor.cpp
573

@elexis here ?

source/simulation2/components/CCmpVisualActor.cpp
573

nope

Stan added inline comments.May 28 2018, 4:43 PM
source/simulation2/components/CCmpVisualActor.cpp
573

Didn't want to touch the .h file but okay.

temple accepted this revision.May 28 2018, 8:47 PM

Think it's good.

elexis added inline comments.May 28 2018, 8:51 PM
source/simulation2/components/CCmpVisualActor.cpp
70

\n
// comment\n
between 70 and 71

Stan updated this revision to Diff 6672.May 28 2018, 8:58 PM

Should be good to go.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/592/display/redirect

elexis accepted this revision.May 28 2018, 9:05 PM
Stan closed this revision.May 28 2018, 9:13 PM

Closed by rP21828

elexis updated the Trac tickets for this revision.Jun 4 2018, 2:49 PM