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]

Greptile Overview

Greptile Summary

This PR updates the initialization values for cellBounds.hi.x and cellBounds.hi.y in the classifyFence function from 0 to empirically discovered values (253961804 and 4202797) to improve replay compatibility in retail-compatible builds.

Key Changes

• Changed initialization approach from setting to 0 to using specific stack values observed in retail binary
• These values represent typical stack garbage from the original retail game
• Improves CRC match rate for replays but won't fix all mismatches (stack values are inherently unpredictable)

Technical Analysis

The large initialization values mean cellBounds.hi will rarely be updated during the loop (since actual cell coordinates are much smaller). However, this is safe because updateZonesForModify clamps the bounds to the actual map extent. The trade-off is potentially processing more zone blocks than necessary, but this is acceptable for maintaining compatibility.

Minor Issues

• One commented-out debug log could be cleaned up (line 3870)

Confidence Score: 4/5

• This PR is safe to merge with low risk - it's a targeted fix for replay compatibility that doesn't affect non-retail builds
• The change is well-understood and follows an established pattern in the codebase for handling retail compatibility. The large initialization values are safely handled by downstream clamping logic. The main limitation (that it won't fix all replays due to stack unpredictability) is acknowledged by the author. Minor style issue with commented debug log doesn't affect functionality.
• No files require special attention - the single change is straightforward and well-isolated

Important Files Changed

File Analysis

Filename	Score	Overview
GeneralsMD/Code/GameEngine/Source/GameLogic/AI/AIPathfind.cpp	4/5	Changed cellBounds.hi initialization from 0 to empirically discovered values (253961804, 4202797) to better match retail stack state for CRC compatibility; minor style issue with commented debug log

Sequence Diagram

sequenceDiagram
    participant Caller
    participant classifyFence
    participant updateZonesForModify
    
    Caller->>classifyFence: "Object, insert flag"
    classifyFence->>classifyFence: "Initialize cellBounds.lo from pos"
    
    alt RETAIL_COMPATIBLE_CRC
        classifyFence->>classifyFence: "Set cellBounds.hi = {253961804, 4202797}"
        Note over classifyFence: Empirical values to match retail stack
    else Modern Build
        classifyFence->>classifyFence: "Calculate cellBounds.hi from pos"
    end
    
    Note over classifyFence: Iterate fence cells
    classifyFence->>classifyFence: "For each cell: calculate cx, cy"
    classifyFence->>classifyFence: "Update obstacle map"
    classifyFence->>classifyFence: "Update cellBounds.lo (min tracking)"
    classifyFence->>classifyFence: "Update cellBounds.hi (max tracking)"
    Note over classifyFence: With large init values, hi rarely updates
    
    alt didAnything
        classifyFence->>updateZonesForModify: "cellBounds, m_extent"
        updateZonesForModify->>updateZonesForModify: "Clamp cellBounds to globalBounds"
        Note over updateZonesForModify: Large values safely clamped to map extent
        updateZonesForModify->>updateZonesForModify: "Process affected zone blocks"
        updateZonesForModify-->>classifyFence: "Complete"
    end
    
    classifyFence-->>Caller: "Return"

Loading

[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...