unify(pathfinder): Merge AIPathfind and update dependencies#2341
unify(pathfinder): Merge AIPathfind and update dependencies#2341Mauller wants to merge 8 commits intoTheSuperHackers:mainfrom
Conversation
60286bd to
e20892a
Compare
|
| Filename | Overview |
|---|---|
| Generals/Code/GameEngine/Include/GameLogic/AIPathfind.h | unified pathfind interface with Zero Hour: updated function signatures, added null checks, renamed quickDoesPathExist to clientSafeQuickDoesPathExist, added UNINITIALIZED_ZONE enum |
| Generals/Code/GameEngine/Source/GameLogic/AI/AIPathfind.cpp | major unification with Zero Hour: merged PathfindZoneManager implementation, added IS_IMPASSABLE helper, updated function signatures to return Bool, added updateZonesForModify function, enhanced null safety checks; one minor date comment issue |
| GeneralsMD/Code/GameEngine/Include/GameLogic/AIPathfind.h | unified with Generals: fixed typos, updated markZonesDirty signature, improved comment alignment, removed unused getNextZone declaration |
| GeneralsMD/Code/GameEngine/Source/GameLogic/AI/AIPathfind.cpp | unified with Generals: refactored PathfindZoneManager calculateZones to use clearer for loops and conditionals, improved code formatting and structure, added debug bounds checks for RTS_GENERALS |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Start[PathfindZoneManager::calculateZones] --> Init[Initialize zone tracking arrays]
Init --> AllocZones[Allocate zone storage]
AllocZones --> BlockLoop[For each zone block xBlock, yBlock]
BlockLoop --> CellLoop[For each cell i, j in block]
CellLoop --> CheckZone{Cell zone == 0?}
CheckZone -->|Yes| AssignZone[Assign new zone m_maxZone++]
CheckZone -->|No| CheckBridge{Cell connects bridge?}
AssignZone --> CheckBridge
CheckBridge -->|Yes| MarkBridge[Mark block interacts with bridge]
CheckBridge -->|No| NextCell
MarkBridge --> NextCell[Next cell]
NextCell --> MoreCells{More cells?}
MoreCells -->|Yes| CellLoop
MoreCells -->|No| MoreBlocks{More blocks?}
MoreBlocks -->|Yes| BlockLoop
MoreBlocks -->|No| Collapse[Collapse hierarchical zones]
Collapse --> MapZones[Map zones to collapsed zones]
MapZones --> BlockCalc[Calculate zones for each block]
BlockCalc --> CompareNeighbors[Compare neighboring cells]
CompareNeighbors --> CheckLeft{i > lo.x?}
CheckLeft -->|Yes| LeftCell{Same type as left?}
LeftCell -->|Yes| ApplyHier1[Apply hierarchical zone]
LeftCell -->|No| Conditional1{RTS_GENERALS?}
Conditional1 -->|Yes| IndepChecks1[Independent terrain/water/cliff checks]
Conditional1 -->|No| ElseLogic1[Use if-else ladder with notTerrainOrCrusher]
CheckLeft -->|No| CheckTop{j > lo.y?}
ApplyHier1 --> CheckTop
IndepChecks1 --> CheckTop
ElseLogic1 --> CheckTop
CheckTop -->|Yes| TopCell{Same type as top?}
TopCell -->|Yes| ApplyHier2[Apply hierarchical zone]
TopCell -->|No| Conditional2{RTS_GENERALS?}
Conditional2 -->|Yes| IndepChecks2[Independent terrain/water/cliff checks]
Conditional2 -->|No| ElseLogic2[Use if-else ladder]
CheckTop -->|No| NextCompare[Next cell comparison]
ApplyHier2 --> NextCompare
IndepChecks2 --> NextCompare
ElseLogic2 --> NextCompare
NextCompare --> MoreCompare{More cells?}
MoreCompare -->|Yes| CompareNeighbors
MoreCompare -->|No| Flatten[Flatten zone equivalency arrays]
Flatten --> Debug{Debug mode?}
Debug -->|Yes| DrawIcons[Draw zone visualization icons]
Debug -->|No| SetFrame[Set m_nextFrameToCalculateZones]
DrawIcons --> SetFrame
SetFrame --> End[Done]
Last reviewed commit: 7b3588d
e20892a to
180cc40
Compare
|
Had to fix a small dependency that i missed when building debug in generals |
| { | ||
| #if RTS_GENERALS | ||
| DEBUG_ASSERTCRASH(m_info, ("Has to have info.")); | ||
| #else |
There was a problem hiding this comment.
If m_info is not initialized, this crashes in Generals when m_info->m_pos.x is called.
Therefore the 1.01 patch can be ported to Generals. Game would have crashed at this point anyways, so it will not mismatch.
There was a problem hiding this comment.
Perhaps just take Zero Hour code for both games?
| return; | ||
| } | ||
| } | ||
| #endif |
There was a problem hiding this comment.
I think this can be simplified to:
if (!ai->isDoingGroundMovement())
{
#if RTS_GENERALS
Bool isUnmannedHelicopter = false;
#else
// exception:sniped choppers are on ground
Bool isUnmannedHelicopter = ( obj->isKindOf( KINDOF_PRODUCED_AT_HELIPAD ) && obj->isDisabledByType( DISABLED_UNMANNED ) ) ;
#endif
if ( ! isUnmannedHelicopter )
{
updateAircraftGoal(obj, newGoalPos);
return;
}
}| } | ||
| #endif | ||
| #if RTS_GENERALS | ||
| m_needToCalculateZones = false; |
| #else | ||
| m_zoneManager.updateZonesForModify(m_map, m_layers, cellBounds, m_extent); | ||
|
|
||
| Int i, j; |
|
updated based on feedback |
75d3750 to
35a0d0a
Compare
35a0d0a to
dc18e3d
Compare
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Include/GameLogic/AIPathfind.h
Line: 659
Comment:
typo: `onlyk` should be `only`
```suggestion
Bool clientSafeQuickDoesPathExistForUI( const LocomotorSet& locomotorSet, const Coord3D *from, const Coord3D *to ); ///< Can we build any path at all between the locations (terrain only - fast)
```
How can I resolve this? If you propose a fix, please make it concise. |
dc18e3d to
87f4e50
Compare
|
Is this meant to be Merged with Rebase? The title says "Sanitize" and "Merge" which implies 2 distinct steps and commits. |
| cell.setZone(collapsedZones[cell.getZone()]); | ||
| ++i; | ||
| if (cell.getZone()==0) { | ||
| DEBUG_CRASH(("Zone not set cell %d, %d", i, j)); |
There was a problem hiding this comment.
What is the reason for adding this assert, as opposed to removing it?
There was a problem hiding this comment.
seemed like it might have been helpful
There was a problem hiding this comment.
IMO it should be removed. collapsesZones[0] = 0 by definition. So if getZone() was 0, it stays 0 and the actual issue occurs earlier. This is the wrong place to test it.
|
|
||
| r_thisLayer.setZone( zone ); | ||
| if (!r_thisLayer.isUnused() && !r_thisLayer.isDestroyed() && r_thisLayer.getZone()==0) { | ||
| DEBUG_CRASH(("Zone not set Layer %d", i)); |
There was a problem hiding this comment.
What is the reason for adding this assert, as opposed to removing it?
There was a problem hiding this comment.
same as above, considered it might be helpful
There was a problem hiding this comment.
r_thisLayer.getZone() is never zero because if it was, the above logic (where it is set to zone = m_maxZone, with m_maxZone >= 1) this debug will never fire. It was correctly removed in ZH.
| { | ||
| #if RTS_GENERALS | ||
| DEBUG_ASSERTCRASH(m_info, ("Has to have info.")); | ||
| #else |
There was a problem hiding this comment.
Perhaps just take Zero Hour code for both games?
| Int zone1 = r_thisCell.getZone(); | ||
| Int zone2 = r_leftCell.getZone(); | ||
| if (m_terrainZones[zone1] != m_terrainZones[zone2]) { | ||
| //DEBUG_LOG(("Matching terrain zone %d to %d.", zone1, zone2)); |
There was a problem hiding this comment.
This Generals code does nothing and can be removed?
| Int zone1 = r_thisCell.getZone(); | ||
| Int zone2 = r_topCell.getZone(); | ||
| if (m_terrainZones[zone1] != m_terrainZones[zone2]) { | ||
| //DEBUG_LOG(("Matching terrain zone %d to %d.", zone1, zone2)); |
There was a problem hiding this comment.
This Generals code does nothing and can be removed?
There was a problem hiding this comment.
this is already removed - EDIT - missed this one i was thinking of the first one, is removed now.
It can be squash merged, the title can be changed, i just considered it a bit of a sanitising as well due to the fixing of a fair bit of formatting from refactored zero hour code. I split it into commits to make it easier to review changes |
83c85c8 to
da9b94b
Compare
da9b94b to
6ca4994
Compare
|
I do prefer committing small reviewable chunks to main branch. Makes it also easier to test later if something breaks. I think reviewability can be improved by doing merge in steps if some code has changed in more than one way. For example, if variables were renamed, formatting was changed, and code was added and removed, then edit these things in 3 separate commits. This will make the diffs much cleaner. |
well the biggest changes are due to pathfind zone manager, i could make a seperate PR just to cleanup the mess in that first before other unification and changes. |
| #endif | ||
|
|
||
| //FLATTEN HIERARCHICAL ZONES | ||
| Int zone; |
There was a problem hiding this comment.
I think Int zone within the for-loop is fine (so generals style)
There was a problem hiding this comment.
yes this will be tweaked with a breakout PR that's handling cleaning up a lot of the big dif due to the PathfindZoneManager changes in Zero hour compared to generals.
6ca4994 to
55be278
Compare
…assifyObjectFootprint() (#2341)
…n() to examineNeighboringCells() (#2341)
…ndBrokenBridge() (#2341)
…) to changeBridgeState() (#2341)
55be278 to
7b3588d
Compare
xezon
left a comment
There was a problem hiding this comment.
This change looks risky.
Do Generals 1.08 Replays pass without mismatching? I think some Generals replays will already fail before this change because we do not test and fix it much.
I am skeptical of the use of #if RTS_ZEROHOUR instead of #if !RTS_GENERALS.
Some #if block are difficult to read and maintain when constructed like so:
if (...)
{
}
#if
else
{
...
}
#else
else
{
...
}
#endifConsider a cleaner separation of #if blocks with no sneaky branch carry overs.
The commits would be better organized as logical units, as opposed to fixed regions of the code. Do we need so many individual commits? Do all commits compile in succession?
| CELL_RUBBLE = 0x03, ///< Cell is occupied by rubble. | ||
| CELL_OBSTACLE = 0x04, ///< Occupied by a structure | ||
| CELL_BRIDGE_IMPASSABLE = 0x05, ///< Piece of a bridge that is impassable. | ||
| CELL_IMPASSABLE = 0x06 ///< Just plain impassable except for aircraft. |
There was a problem hiding this comment.
Commit title:
Merge functions from IS_PASSABLE() to costToHierGoal()
What does this mean? I assume it means merge functions from point A to point B. But this is confusing, because 99% of readers will not know what is between these 2 code points. Generally it is better to describe like "Merge findPath related functions", "Merge debug icons related functions", ...
| zoneStorageType ZoneBlock::getEffectiveZone( LocomotorSurfaceTypeMask acceptableSurfaces, | ||
| Bool crusher, zoneStorageType zone) const | ||
| { | ||
| DEBUG_ASSERTCRASH(zone, ("Zone not set")); |
| if ( i > globalBounds.lo.x && r_thisCell.getZone() != map[i-1][j].getZone() ) { | ||
| const PathfindCell &r_leftCell = map[i-1][j]; | ||
|
|
||
| //if this is true, skip all the ones below |
There was a problem hiding this comment.
This looks like logic change that could mismatch
| applyZone(r_thisCell, r_topCell, m_crusherZones, m_maxZone); | ||
| } | ||
| #else | ||
| else |
There was a problem hiding this comment.
Is difficult to read (and maintain) this. Perhaps this can be written better?
| { | ||
| Bool notTerrainOrCrusher = TRUE; // if this is false, skip the if-else-ladder below | ||
|
|
||
| cellBounds.hi.x = bottomRightCorner.x; | ||
| cellBounds.hi.y = bottomRightCorner.y; | ||
|
|
||
| #if RTS_GENERALS |
There was a problem hiding this comment.
Are all of the new RTS_GENERALS still necessary without RETAIL_COMPATIBLE_CRC (or RETAIL_COMPATIBLE_PATHFINDING) ? Will we never drop the Generals specific code in Pathfind?
| } | ||
| } | ||
| } | ||
| #if RTS_ZEROHOUR |
There was a problem hiding this comment.
Better use !RTS_GENERALS instead, because RTS_ZEROHOUR is inconsistent with
#ifdef RTS_GENERALS
...
#else
... // Strictly, code in here means !RTS_GENERALS, not RTS_ZEROHOUR
#endifMany times in this change.
| if (obj->getHeightAboveTerrain() > PATHFIND_CELL_SIZE_F && ( ! obj->isKindOf( KINDOF_BLAST_CRATER ) ) ) { | ||
| return; // Don't add bounds that are up in the air.... unless a blast crater wants to do just that | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Better make this part of the #ifdef blocks
| examinedZones[numExZones] = newZone; | ||
| numExZones++; | ||
| } | ||
| if( adjNewCell->hasInfo() ) { |


Merge by rebase
This PR merged Aipathfind.h and Aipathfind.cpp
The majority of the merge has required conditionals being placed throughout the code, this is due to significant changes between zero hour and generals making the function incompatible.
The majority of the merges are safe and distinct, either directly inherited from zero hour and into generals, or cleaned up from generals into zero hour.
The most involved the majority merge and refactor involves the functions within the PathfindZoneManager, as zero hour changes large portions of this code. The code itself was a mess and i reverted the implementation of parts of it from
whileback toforloops as used within generals to make the code easier to understand.Generals inherited some optimisations from zero hour within PathfindZoneManager but had to retain the original code in other places due to mismatches occuring.
Generals gained the boolean returning versions of the terrain object handling functions, although the generals code does not specifically use them. This was done as a cleaner merge.
Generals also gained some zero hour functions that now check for nulls.
Generals also gained some functional name changes from zero hour which required updating call sites.
Zero hour gained some debugging that was lost from generals and some cleaned up implementation and formatting.
EDIT: I also had to extend an enum in generals' AI.h due to debug changes merged back from zero hour.
I split the unification into commits to make reviewing it easier, the most strenuous commit is the third commit of the PathfindZoneManager. This is where a considerable amount of satinising and refactoring occurred between both implementations.
EDIT - This merge is now muh cleaner after performing some cleanup to the Zero Hour implementation of PathfindZoneMAnager and the porting of optimisations from zero hour to generals.