bugfix(pathfinder): Fix uninitialized variable in Pathfinder::tightenPathCallback to prevent mismatches by Caball009 · Pull Request #2309 · TheSuperHackers/GeneralsGameCode

Caball009 · 2026-02-15T02:46:22Z

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.

The retail compatible code uses effectively this initialization logic:

Coord3D pos = (!d->foundNewDest) ? Coord3D(0.0f, 0.0f, 0.0f) : d->newDestPos;

TODO:

Replicate in Generals.

Thanks to @Mauller and @DrGoldFish1 for their help.

greptile-apps · 2026-02-15T02:48:10Z

Greptile Summary

This PR fixes an uninitialized-variable bug in Pathfinder::tightenPathCallback (both Generals/ and GeneralsMD/ copies) that caused determinism mismatches, most visibly when units drove near certain bridges. The local Coord3D pos was previously stack-allocated without initialization; its garbage value was passed to checkForAdjust as a starting point, producing different results between debug and retail builds.

Key changes:

TightenPathStruct gains an orgDestPos field (original destination) alongside the renamed foundNewDest / newDestPos, giving the callback a well-defined starting value.
Under RETAIL_COMPATIBLE_CRC, pos is seeded from the accumulated newDestPos (zero-initialized by the caller), approximating the retail stack layout where the value was near-zero on the first call.
In non-RETAIL_COMPATIBLE_CRC builds, pos is seeded from orgDestPos, providing a fully deterministic, correct initial value.
The fix is applied symmetrically to both Generals/ and GeneralsMD/ code paths, closing the previously noted inconsistency between the two trees.

Confidence Score: 5/5

Safe to merge — the uninitialized variable bug is correctly identified and fixed in both code trees with sound RETAIL_COMPATIBLE_CRC branching logic.
The PR correctly fixes the core uninitialized-variable bug that was causing determinism mismatches. The variable pos in tightenPathCallback is now properly initialized in both retail-compatible and non-retail code paths. The fix is symmetric across both Generals/ and GeneralsMD/ variants, maintaining consistency. All changes are straightforward and safe.
No files require special attention.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["tightenPath(obj, locomotorSet, from, to)"] --> B["Init TightenPathStruct\nfoundNewDest = false\norgDestPos = *to\n[RETAIL] newDestPos.zero()"]
    B --> C["iterateCellsAlongLine()\ncalls tightenPathCallback per cell"]
    C --> D{"from == nullptr\nor to == nullptr?"}
    D -- yes --> E["return 0 // failure"]
    D -- no --> F{"layer matches?"}
    F -- no --> E
    F -- yes --> G{"RETAIL_COMPATIBLE_CRC?"}
    G -- yes --> H["pos = d->newDestPos\n(accumulated or zero)"]
    G -- no --> I["pos = d->orgDestPos\n(original destination)"]
    H --> J["checkForAdjust(..., &pos, nullptr)"]
    I --> J
    J -- fails --> E
    J -- succeeds --> K["foundNewDest = true\nnewDestPos = pos"]
    K --> L["return 0 // success, continue"]
    C --> M{"foundNewDest?"}
    M -- yes --> N["*from = newDestPos"]
    M -- no --> O["*from unchanged"]

_{Last reviewed commit: 036368c}

helmutbuhler · 2026-02-15T08:16:58Z

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?

Generals/Code/GameEngine/Source/GameLogic/AI/AIPathfind.cpp

stephanmeesters · 2026-02-15T10:25:25Z

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

Caball009 · 2026-02-15T18:01:35Z

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

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

Caball009 · 2026-02-15T18:16:36Z

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 · 2026-02-21T20:18:52Z

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 · 2026-02-22T23:43:20Z

... but take the non retail stuff?

What do you mean?

Caball009 · 2026-02-27T23:05:16Z

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.

…PathCallback to prevent mismatches.