unify(pathfinder): Remove legacy debug and merge safe code

[Mauller]

merge by rebase

This PR is a small breakout from the main unify PR to cleanup legacy debug code from generals and to merge blocks of safe to merge code.

This mostly focuses on code that has no possible functional changes that require conditional compilation.
Only PathfindCell::costToHier() has a functional change which actually prevents a crash, so would not mismatch if the altered code was ever hit.

The merging of the CellType enumeration is completely safe as the pathfind cell types are only ever used internally within the pathfinder so they can take whatever value we please. Only instances where their use has altered between generals and zero hour will cause a mismatch.

The only time external data is used to set PathfindCell types is within classifyMap when calls to TheTerrainLogic->isCliffCell or TheTerrainLogic->isUnderwater are called and set CELL_CLIFF or CELL_WATER from CellType.

The removal of the argument for markZonesDirty in zero hours code is a small thing to move the unification along more before hitting the more dangerous stuff within PathfindZoneManger.

[greptile-apps]

Greptile Summary

This PR incrementally unifies the Generals pathfinder with its GeneralsMD counterpart by removing legacy debug/QPF-timing blocks, renumbering and extending the CellType enum (adding CELL_BRIDGE_IMPASSABLE, renumbering CELL_IMPASSABLE from 0x0B to 0x06), promoting setTypeAsObstacle/removeObstacle to return Bool, adding null-safety guards to setCostSoFar, setTotalCost, and getParentCell, replacing the crashing DEBUG_ASSERTCRASH in costToHierGoal with a graceful fallback, and removing the unused Bool insert parameter from PathfindZoneManager::markZonesDirty in the Zero Hour build.

Key observations:

• CellType renumbering — Values are internal to the pathfinder and not serialised externally, so the 0x0B → 0x06 shift for CELL_IMPASSABLE is safe.
• IS_IMPASSABLE helper — Added to Generals/AIPathfind.cpp with no call sites yet; this is a temporary state during incremental unification.
• costToHierGoal fix — Converting the hard crash on a null m_info to a graceful return 100000 is the only intentional behaviour change and is positive.
• markZonesDirty cleanup — The Bool insert parameter was ignored inside the Zero Hour implementation; removing it is safe.

Confidence Score: 4/5

• PR is safe to merge; changes are incremental unification with mechanical refactoring and one positive behavior improvement.
• The vast majority of changes are mechanical: debug log/assert removal, enum renumbering, and signature alignment with GeneralsMD. The intentional behavior changes (null-safety additions and the costToHierGoal graceful fallback) are positive. The PR author and senior developer have reviewed the alignment decisions and accepted the incremental unification strategy.
• No files require special attention

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Include/GameLogic/AIPathfind.h	CellType enum unified with GeneralsMD (adds CELL_BRIDGE_IMPASSABLE=0x05, renumbers CELL_IMPASSABLE to 0x06, removes CELL_unused); setTypeAsObstacle/removeObstacle return Bool; setCostSoFar/setTotalCost add m_info null guards; getParentCell gains m_info null safety via ternary chain; typo fixes.
Generals/Code/GameEngine/Source/GameLogic/AI/AIPathfind.cpp	Adds IS_IMPASSABLE helper (no Generals call sites yet, temporary during unification), changes setTypeAsObstacle/removeObstacle to return Bool, replaces costToHierGoal assert with graceful null-return, adds CELL_BRIDGE_IMPASSABLE handling in switch/debug rendering, removes several debug assertions and QPF timing blocks.
GeneralsMD/Code/GameEngine/Include/GameLogic/AIPathfind.h	Removes unused Bool insert parameter from markZonesDirty() declaration — straightforward signature cleanup.
GeneralsMD/Code/GameEngine/Source/GameLogic/AI/AIPathfind.cpp	Updates markZonesDirty() implementation to remove the now-unused insert parameter, and updates all four call sites accordingly — mechanical, safe change.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[PathfindCell::CellType] --> B[CELL_CLEAR = 0x00]
    A --> C[CELL_WATER = 0x01]
    A --> D[CELL_CLIFF = 0x02]
    A --> E[CELL_RUBBLE = 0x03]
    A --> F[CELL_OBSTACLE = 0x04\nOccupied by a structure]
    A --> G[CELL_BRIDGE_IMPASSABLE = 0x05\nNew in Generals via this PR]
    A --> H[CELL_IMPASSABLE = 0x06\nRenum from 0x0B]

    F --> IS[IS_IMPASSABLE helper\nreturns true]
    G --> IS
    H --> IS

    IS --> LOCO[chooseBestLocomotorForPosition\nreturns LOCOMOTORSURFACE_AIR]

    G -.->|No setter yet in Generals| NOTE[CELL_BRIDGE_IMPASSABLE\nnever set on cells in Generals\nfuture unification work]
    G --> DBG[doDebugIcons\nblue+red color]

Loading

_{Last reviewed commit: 95cfca3}

[greptile-apps]

_{4 files reviewed, 5 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

Generals/Code/GameEngine/Source/GameLogic/AI/AIPathfind.cpp
Removed RELEASE_CRASH guard exposes out-of-bounds access

The following guard was removed without any replacement:

if (m_maxZone >= m_zonesAllocated) {
    RELEASE_CRASH("Pathfind allocation error - fatal. see jba.");
}

Unlike the DEBUG_* macros removed elsewhere in this PR, RELEASE_CRASH fires in all build configurations, not only debug builds. The guard immediately precedes the loops that index into m_hierarchicalZones, m_groundCliffZones, m_groundWaterZones, m_groundRubbleZones, m_terrainZones, and m_crusherZones up to m_maxZone. All of these arrays are sized to m_zonesAllocated by allocateZones(). If m_maxZone >= m_zonesAllocated were ever true, the subsequent flattening loops would perform out-of-bounds array accesses, causing undefined behaviour or memory corruption.

Could you confirm that allocateZones() (called just before) always guarantees m_zonesAllocated > m_maxZone, making this guard provably unreachable? If so, a DEBUG_ASSERTCRASH at minimum would be preferable to a silent removal.

Prompt To Fix With AI

This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameLogic/AI/AIPathfind.cpp
Line: 2736-2740

Comment:
**Removed `RELEASE_CRASH` guard exposes out-of-bounds access**

The following guard was removed without any replacement:

```cpp
if (m_maxZone >= m_zonesAllocated) {
    RELEASE_CRASH("Pathfind allocation error - fatal. see jba.");
}
```

Unlike the `DEBUG_*` macros removed elsewhere in this PR, `RELEASE_CRASH` fires in **all** build configurations, not only debug builds. The guard immediately precedes the loops that index into `m_hierarchicalZones`, `m_groundCliffZones`, `m_groundWaterZones`, `m_groundRubbleZones`, `m_terrainZones`, and `m_crusherZones` up to `m_maxZone`. All of these arrays are sized to `m_zonesAllocated` by `allocateZones()`. If `m_maxZone >= m_zonesAllocated` were ever true, the subsequent flattening loops would perform out-of-bounds array accesses, causing undefined behaviour or memory corruption.

Could you confirm that `allocateZones()` (called just before) always guarantees `m_zonesAllocated > m_maxZone`, making this guard provably unreachable? If so, a `DEBUG_ASSERTCRASH` at minimum would be preferable to a silent removal.

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

[Mauller]

Mistakenly removed a difference that had a logical change, corrected that and reverted that part of the debug code removal as it also had a continue statement within it.

[Skyaero42]

wondering if the debug_crash is still needed with the patch hack.

[Mauller]

I honestly wouldn't be surprised if they were seeing pathfinding crashes due to all the problems we already cleaned up.

Can't wait to get rid of or at least conditionally remove code like this when i get the non allocating pathfinding going.

[Skyaero42]

This looks good to me.

[xezon]

Very clean commits. Looks very safe.

The commit titles can be improved.

All/most merge commits that I made are titled in the following style:

unify(system): Merge ClassName, FileName code from GameName

[Mauller]

Updated commit titles

[xezon]

Can you also polish the other titles a bit? The first one writes generals game title with lowercase letter.

Better: chore(pathfinder): Remove legacy Pathfind debug code from Generals (#2381)

[Mauller]

Can you also polish the other titles a bit? The first one writes generals game title with lowercase letter.

Better: chore(pathfinder): Remove legacy Pathfind debug code from Generals (#2381)

Tweaked that and the PathfindCell commit