[Caball009]

• Follow up for bugfix(pathfinder): Fix uninitialized variable in Pathfinder::tightenPathCallback to prevent mismatches #2309
• Similar to bugfix(pathfinder): Improve initialization of uninitialized variable in Pathfinder::classifyFence #2460
• Similar to bugfix(aiplayer): Improve initialization of uninitialized variable in AIPlayer::onUnitProduced #3379

This PR is a follow up for #2309 to improve the uninitialized variable in Pathfinder::tightenPathCallback, and is for Zero Hour only. The new initialization should produce extremely similar values, if not the same, as the uninitialized variable in the retail Zero Hour executable (Steam version). I'm not aware of any retail replay that still mismatches because of this uninitialized variable, with the changes in this PR.

Here's an archive with 100+ Gentool replays that mismatch with the weekly releases, but play ok with this PR: replays.zip. I've also checked 12K+ replays. I didn't see any replay that mismatched because of these changes, except for one replay that was created with a recent weekly release.

All interesting value changes happen with this call stack:

Bridge::isPointOnBridge && Bridge::getBridgeHeight
TerrainLogic::getLayerForDestination
Pathfinder::clientSafeQuickDoesPathExist
Pathfinder::checkForAdjust
Pathfinder::adjustDestination

TODO:

• Add code comments.
• Restructure commits; put the macro rename for Pathfinder::m_classifyFenceZeroInit as the first commit.

[coderabbitai]

Walkthrough

Under Zero Hour retail-compatible pathfinding, Pathfinder tracks special path-tightening state and saved coordinates. Bridge uses a dedicated triangle test when that state is active. Fence-classification guards now use the pathfinding compatibility condition.

Changes

Special Path Tightening

Layer / File(s)	Summary
Pathfinder special initialization state Core/GameEngine/Include/GameLogic/AIPathfind.h, Core/GameEngine/Source/GameLogic/AI/AIPathfind.cpp, GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp	Pathfinder initializes and updates special initialization state and saved coordinates. During path tightening, it uses the saved coordinates and state while determining the layer. Fence-classification initialization and lifecycle guards now use RETAIL_COMPATIBLE_PATHFINDING instead of RETAIL_COMPATIBLE_CRC.
Bridge point test Core/GameEngine/Include/GameLogic/TerrainLogic.h, Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp	When special initialization is active, Bridge::isPointOnBridge calls the internal triangle test. The helper checks both bridge triangles and sets the output coordinate according to the special-initialization flag.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related issues

• TheSuperHackers/GeneralsGameCode#3333 — The issue lists replacing retail_compat_crc with retail_compat_pathfinding as a planned refactor; this change updates the relevant guards.

Suggested labels: Fix

Suggested reviewers: bobtista

Merge Risk: Low · up to fe87f

Path tightening can use an uninitialized destination in one supported configuration. Align the guards before merging, or explicitly accept that bounded risk.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title describes the main change: initializing pathfinding state to address an uninitialized variable in the path-tightening callback.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description check		The description explains the Zero Hour pathfinding initialization change and its intended effect on replay compatibility. It is related to the changeset.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[coderabbitai]

Additional comments: 1

Core/GameEngine/Source/GameLogic/AI/AIPathfind.cpp: 1

4590-4605: Trivial · Maintainability & Code Quality · Low value

Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Zero m_tightenPathSpecialInitCoords before the layer lookup, or document why this is not needed.

In the Zero Hour branch, info.newDestPos takes the saved coordinates. If no bridge writes them during getLayerForDestination, the value is stale. The value comes from the earlier adjustDestination zeroing or from other bridge checks. This is non-deterministic across call paths. Retail may behave the same way. Add a comment that states this is intentional.

Review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 1a328561-f060-4198-81e9-d6ccc71eedf6

Commits

Reviewing files that changed from the base of the PR and between 998f337 and ab316fd.

Files selected for processing: 4

• Core/GameEngine/Include/GameLogic/AIPathfind.h
• Core/GameEngine/Include/GameLogic/TerrainLogic.h
• Core/GameEngine/Source/GameLogic/AI/AIPathfind.cpp
• Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

[Caball009]

Just a note for the magic value of y coordinate: out.set(0.0f, 1000000000.0f, 0.0f):

That's calculated like this:

// TerrainLogic::getLayerForDestination -> Bridge::getBridgeHeight 
// -> PlaneClass::Set -> Vector3::Normalize -> WWMath::Inv_Sqrt_Legacy

Vector3 result;
Vector3::Cross_Product((right1 - left1), (left2 - left1), &result);

const float length = result.Length2();
const float y = std::bit_cast<float>(std::bit_cast<uint32_t>(length) - 0x800000);

out.set(0.0f, y, 0.0f);

AFAIK the resulting value is always a very large number, often larger than one billion, so I skipped the calculation and went with a fixed number instead.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

Minor · Use the pathfinding guard in the callback. · AIPathfind.cpp:4560-4567

Core/GameEngine/Source/GameLogic/AI/AIPathfind.cpp:4560-4567
Minor · Functional Correctness · Quick win

Use the pathfinding guard in the callback.

If RETAIL_COMPATIBLE_PATHFINDING is disabled and RETAIL_COMPATIBLE_CRC is enabled, tightenPathCallback can read an uninitialized newDestPos during a group destination adjustment.

Suggested fix

-#if RETAIL_COMPATIBLE_CRC
+#if RETAIL_COMPATIBLE_PATHFINDING
 	// TheSuperHackers @bugfix Caball009 27/02/2026 This was originally uninitialized.
 	// The uninitialized values that retail uses here are usually close to zero as long as foundNewDest == false, otherwise it uses the new values of newDestPos.
-	// newDestPos is zero initialized by the caller, so there is no need to check foundNewDest here.
+	// newDestPos is initialized by the caller, so there is no need to check foundNewDest here.

Review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 0c77d8f1-da55-4834-83d4-2ea2ecc6b3bc

Commits

Reviewing files that changed from the base of the PR and between 998f337 and fe87ff3.

Files selected for processing: 5

• Core/GameEngine/Include/GameLogic/AIPathfind.h
• Core/GameEngine/Include/GameLogic/TerrainLogic.h
• Core/GameEngine/Source/GameLogic/AI/AIPathfind.cpp
• Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp
• GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.