fix(mismatch): Fix uninitialized variable in AIPathfind and possible source of Mismatches in Retail-compatible builds

[helmutbuhler]

Follow-up for #632

Some variables in AIPathfind are kept uninitialized to retain compatibility. But they also might be a source of mismatches. This PR sets them to a value found empirically that keeps compatibility with a lot of replays.

Still, a few replays still mismatch with the current value. I'll try to investigate more. Ideally we reverse-engineer the original binary and find out the source of the values that occupy the stack.

[helmutbuhler]

So, I did some more investigation, but it doesn't look good.
The uninitialized stack variables are in Pathfinder::classifyFence. That function is only called by Pathfinder::classifyObjectFootprint. That in turn is called by:

• Pathfinder::newMap
• Pathfinder::addObjectToPathfindMap
• Pathfinder::removeObjectFromPathfindMap

Because the variables in question are so high up in the stack, none of these callers set that stackregion to a defined value. That's only done by callers of addObjectToPathfindMap and removeObjectFromPathfindMap, and that's a lot, around 30. So in order to set these variables to a defined value that remains compatible, we'd have to analyze all those callsites to find out how that stackregion is set in practice. That's too much work for me.
As an alternative, I also tried to set the variables to specific constant values depending on the callsite, but that also didn't work. I did find some values though that work in most replays, but not all. This PR changes the initialization to those values.

If we merge this PR, the mismatches due to these uninitialized variables will completely stop for SH players. But some replays won't play back correctly anymore. I tested on 1800 replays, and 6 will mismatch with this merged. Assuming those replays are representative, this would then possibly increase the mismatch likelihood in games with both retail and SH players by 0.3%.

If we don't merge this, replay playback will remain compatible, but the underlying issue will still result in mismatches in live games. It's hard to say for how many mismatches this issue is responsible, but I suspect that it's responsible for most of them. In all other cases where we fixed uninitialized variables, it was pretty well defined what value got used in the retail version. Here it's all over the place.

If someone is interested, I can provide the replays that mismatch with this PR and I can also provide the code to debug this issue and the code to set the variables depending on the callsite (it's a bit tricky because the tiniest changes trigger incompatiblities due to different stacklayout and different float-calculations)

If noone is interested in further improving the heuristics, I'd be in favor of merging this. For the reasons mentioned above, and also to easily rule this issue out when a mismatch happens in the future.

[xezon]

Is there a way to estimate how many mismatches these uninitialized variable would cause originally?

[helmutbuhler]

I cannot think of any way.

[xezon]

If we have a way to detect all our clients in a Network match, then we could conditionally opt-in the fix on runtime. Same with the Pathfinder fixes.

[Skyaero42]

Is there a way to estimate how many mismatches these uninitialized variable would cause originally?

The only real experience we have so far are the Legi streams (Generals Online uses a non-vc6 build, so it doesn't count I think). He rarely mismatches. Often it is a player that didn't restart their game or used to play with mods. I don't think these uninitialized variables are that much of an issue.

[greptile-apps]

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

remove commented-out debug log or document if it should be kept for future investigation

Suggested change

//CRCDEBUG_LOG(("Pathfinder::classifyFence - (%d,%d)", cellBounds.hi.x, cellBounds.hi.y));

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

Prompt To Fix With AI

This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameLogic/AI/AIPathfind.cpp
Line: 3870:3870

Comment:
remove commented-out debug log or document if it should be kept for future investigation

```suggestion
```

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

[xezon]

Yes can be removed.

[Caball009]

If we merge this PR, the mismatches due to these uninitialized variables will completely stop for SH players. But some replays won't play back correctly anymore. I tested on 1800 replays, and 6 will mismatch with this merged. Assuming those replays are representative, this would then possibly increase the mismatch likelihood in games with both retail and SH players by 0.3%.

Good effort; 6 out of 1800 seems acceptable to me, and making the behavior for SH clients fully deterministic is a good thing.

[xezon]

This is one of currently 3 open PRs with fixes for uninitialized variables, which come with a small risk of introducing new mismatches. These should be merged together without other PRs in between:

• #1748
• #2087
• #2096

[xezon]

If we have a way to detect all our clients in a Network match, then we could conditionally opt-in the fix on runtime. Same with the Pathfinder fixes.

I think this is the best course of action for this fix (and the Pathfinding + AIGroup fixes).

Detect Game Version from all Peers in Network and Online Lobby, and then set the deterministic behavior if they all use Patched Clients. This state would also need to be written in the Replay however, so that the playback is correct as well.

[Caball009]

If we have a way to detect all our clients in a Network match, then we could conditionally opt-in the fix on runtime. Same with the Pathfinder fixes.

I think this is the best course of action for this fix (and the Pathfinding + AIGroup fixes).

Detect Game Version from all Peers in Network and Online Lobby, and then set the deterministic behavior if they all use Patched Clients. This state would also need to be written in the Replay however, so that the playback is correct as well.

We should make the behavior for our clients fully deterministic, and try to use values that are least likely to cause a mismatch with retail. Anything more than that is a waste of time and effort, I think, especially stuff that needs to be sent across the network and logged in the replay file.

[xezon]

Are we ok with 0.3% more mismatches? It is no ideal...