refactor(pathfind): Optimize pathfind snippets for higher performance

[Skyaero42]

This PR optimizes three code snippets in the pathfinding algorithm to improve its performance. In end-games like FFA's with high number of units, pathfinding can take up to 80% of all CPU time.

For convenience, each snippet optimization is its own commit. PR can be merged by either squash or rebase.

Commits

refactor(pathfind): Optimize appending node to end of the path.
PathNode::appendToList() walks the entire list from the head to find the tail on every call, making repeated appendNode() calls O(n^2) in path length. Path already tracks m_pathTail, so append directly onto it in O(1) instead. Removed pathNode::appendToList() as it is not used anywhere else.

refactor(pathfind): Take parents cell's position outside of for-loop for optimization.
The parent cell's world position fromPos never changes across the neighbour loop, so compute it once instead.

refactor(pathfind): Remove redundant isCrusher recomputation for optimization.
The parameter isCrusher is calculated in the ExamineCellsStruct and then recalculated in theexamineCellsCallback. By caching the result in the ExamineCellStruct it reduced the number of evaluations needed.

Performance

VS's performance analyser was used.
The appending node commit reduced 3.1% in absolute CPU time for pathfinding.
The parent's cells position and isCrusher optimizations combined reduced 1.2% in absolute CPU time for pathfinding.

Testing

This PR has been tested against 50 normal replays and 1 replay with the pathfind failover activated.

Disclaimer

This PR and its description was fully made by a human.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Optimize path construction and neighbor evaluation

✨ Enhancement 🕐 10-20 Minutes

AI Description

• Append path nodes through the tracked tail, reducing path construction from quadratic to linear.
• Cache invariant parent position and crusher state during neighboring-cell evaluation.
• Remove the obsolete list-scanning append API after redirecting its only caller.

Diagram

graph TD
  PF["Pathfinder"] --> Build["Build Path"] --> Tail["Tail Append"]
  PF --> Expand["Expand Neighbors"] --> Cache["Cached Context"] --> Validate["Movement Validation"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Lazy-cache parent elevation

• ➕ Avoids the terrain-height lookup when no downhill or cliff check needs it.
• ➕ Still computes the invariant value at most once per neighbor expansion.
• ➖ Adds state and branching inside an already complex hot loop.
• ➖ Likely offers little benefit when relevant checks are common.

Recommendation: Keep the direct tail append and cached crusher state as implemented. The parent position hoist is also reasonable and simpler than lazy caching, but profiling should confirm that its unconditional terrain-height lookup remains beneficial across representative maps and locomotor types.

Files changed (2) +16 / -39

Enhancement (1) +16 / -35

AIPathfind.cppEliminate repeated work in pathfinding hot paths +16/-35

Eliminate repeated work in pathfinding hot paths

• Changes 'Path::appendNode()' to link through 'm_pathTail', making repeated appends O(1) each, and deletes the list-scanning helper. It also reuses the caller-computed crusher flag and hoists the parent cell's world position outside neighbor iteration.

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

Refactor (1) +0 / -4

AIPathfind.hRemove the obsolete linear-time append helper +0/-4

Remove the obsolete linear-time append helper

• Removes 'PathNode::appendToList()' from the public path-node API because path appends now use the tracked tail directly.

Core/GameEngine/Include/GameLogic/AIPathfind.h

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. Terrain lookup becomes unconditional 🐞 Bug ➹ Performance

Description

examineNeighboringCells now calls getGroundHeight once for every expanded pathfinding cell, even
when the locomotor is not downhill-only and none of the accepted neighbors is an unpinched cliff;
previously those cases performed no parent-height lookup. Since A* invokes this function for each
cell removed from the open list, this adds avoidable terrain work to the common hot path the PR is
intended to optimize.

Code

Core/GameEngine/Source/GameLogic/AI/AIPathfind.cpp[R6311-6314]

+		Coord3D fromPos;
+		fromPos.x = parentCell->getXIndex() * PATHFIND_CELL_SIZE_F ;
+		fromPos.y = parentCell->getYIndex() * PATHFIND_CELL_SIZE_F ;
+		fromPos.z = TheTerrainLogic->getGroundHeight(fromPos.x , fromPos.y);

Evidence

The new lines perform the lookup before any neighbor filtering. The only consumers of fromPos.z
are guarded by isDownhillOnly() or by an accepted neighbor being an unpinched CELL_CLIFF, while
the A* loop calls examineNeighboringCells once for each expanded open-list cell;
LocomotorSet::clear also defaults downhill-only to false, so the downhill consumer is not
universally active.

Core/GameEngine/Source/GameLogic/AI/AIPathfind.cpp[6311-6316]
Core/GameEngine/Source/GameLogic/AI/AIPathfind.cpp[6351-6360]
Core/GameEngine/Source/GameLogic/AI/AIPathfind.cpp[6418-6425]
Core/GameEngine/Source/GameLogic/AI/AIPathfind.cpp[6693-6745]
Generals/Code/GameEngine/Source/GameLogic/Object/Locomotor.cpp[2753-2778]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The parent terrain-height lookup was moved out of the neighbor loop, but is now executed for every expanded cell even when neither the downhill check nor cliff-cost calculation needs it.

## Issue Context
Retain the benefit of computing the parent height at most once per invocation while initializing it lazily only when a downhill or qualifying cliff branch first consumes it.

## Fix Focus Areas
- Core/GameEngine/Source/GameLogic/AI/AIPathfind.cpp[6311-6314]
- Core/GameEngine/Source/GameLogic/AI/AIPathfind.cpp[6351-6360]
- Core/GameEngine/Source/GameLogic/AI/AIPathfind.cpp[6418-6425]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[Caball009]

Is this check necessary? Could maybe be changed to an assertion given that it's already checked at the call site.

[Mauller]

I wonder if pulling the creation of toPos outside of the loops along with fromPos can help perf a little since we aren't creating and destroying a more complex object constantly.

[Caball009]

toPos is updated each iteration due to its dependency on newCellCoord.

[Mauller]

toPos is updated each iteration due to its dependency on newCellCoord.

i meant just the variable, the assignment to it can be left in the loop.

[Caball009]

Why would that help? It doesn't even have user-defined constructors, so the x y z assignment is the cost of construction.

[Skyaero42]

According to Godbolt, the location of the definition doesn't matter.

[Mauller]

The appendNode code seems wrong, it never does any checking to see if the list is initialised properly and will only update m_pathTail.

it might be worth adding an initial check before this block.

PathNode *node = newInstance(PathNode);

node->setPosition( pos );
node->setLayer(layer);

if (!m_path)
{
 m_path = node;
 m_pathTail = node;

#ifdef CPOP_STARTS_FROM_PREV_SEG
 m_cpopRecentStart = nullptr;
#endif
 return;
}

This block you added can then be simplified to m_pathTail->apend(node);