bugfix(pathfinder): Fix uninitialized variable in Pathfinder::tightenPathCallback to prevent mismatches

[Caball009]

Another uninitialized variable rears its ugly head; this time in the path finding code. Causes mismatches, which can be reproduced by driving on the terrain around certain bridges.

Reproduction in Sand Serpent:

bridge_mm.mp4

Replay for VC6:

00-04-16_1v1_adapte_Anthony1.zip This replay does not mismatch with retail executable, and first started to mismatch with 4bbf0c0

Map and replay for VS22:

bridge_mm.zip This replay was recorded with a release build, and the mismatch shows when replay is played back with a debug build.

Notes:

I could not see any noticeable issues in-game because of this bug during my testing. I checked more than 3500 replays and this change did not introduce new mismatches. Unfortunately, the mismatch in the above replay did not go away.

TODO:

• Replicate in Generals.

Thanks to @Mauller and @DrGoldFish1 for their help.

[greptile-apps]

Greptile Summary

Fixes an uninitialized variable pos in Pathfinder::tightenPathCallback that caused network mismatches when vehicles drive near certain bridges.

• Initializes pos to d->destPos and zeros it if foundDest is false, matching retail's empirical behavior
• Improves clarity of return statement comments (failure vs success but continue)
• Well-documented with bugfix comment explaining the issue and fix date
• Tested with 3500+ replays showing no new mismatches introduced

Confidence Score: 5/5

• This PR is safe to merge with minimal risk
• Targeted bugfix for undefined behavior with clear logic, thorough testing (3500+ replays), proper documentation, and adherence to all coding standards
• No files require special attention

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Source/GameLogic/AI/AIPathfind.cpp	Initializes previously uninitialized pos variable in tightenPathCallback to prevent undefined behavior and match retail determinism; clarifies return statement comments

_{Last reviewed commit: 51919b3}

[helmutbuhler]

Interesting, that commit you mentioned already caused problems earlier in #1061. I predicted there that it might cause further mismatches, and this seems like confirmation of that. Maybe we should revert #1009 and #1013, at least for VC6 builds?

[stephanmeesters]

Did you mean to do the fix in Generals/ not GeneralsMD/?

[Caball009]

Did you mean to do the fix in Generals/ not GeneralsMD/?

Oops, yeah, that's silly of me. Not sure how that happened.

[Caball009]

Interesting, that commit you mentioned already caused problems earlier in #1061. I predicted there that it might cause further mismatches, and this seems like confirmation of that. Maybe we should revert #1009 and #1013, at least for VC6 builds?

I'm not sure if that solves anything. I tried the current main branch with the wwmath.h header from 2856a23 ( the commit before 4bbf0c0 ) and the VC6 replay still mismatches.

Harmless changes anywhere can change stack layouts and introduce mismatches that hadn't been noticeable before because of uninitialized variables.

[Mauller]

Considering how varied the value of Pos was, from what you showed me, maybe we can't set it to anything specific but take the non retail stuff?

[Caball009]

... but take the non retail stuff?

What do you mean?

[Caball009]

I was able to find a way to initialize this variable without introducing new mismatches. I checked more than 3500 replays to verify that. Unfortunately, the mismatch in the original replay did not go away.