Skip to content

Commit

Permalink
Fix HierarchicalPathFinder considering some unreachable cells as reac…
Browse files Browse the repository at this point in the history 
…hable.

When using the internal AbstractCellForLocalCell method to check if a local cell is reachable, this should return null when the cell is unreachable. If multiple abstract cells were required for that grid, this worked as intended. Only reachable cells are stored in the localCellToAbstractCell mapping. For a grid that required only a single abstract cell, which is the common case, we optimize this to store only the single abstract cell rather than the whole mapping for potentially 100 cells in that grid. However this makes no distinction between the reachable and unreachable cells, so when we check later we get incorrect results. If a cell is unreachable but belongs to the same grid as a single group of reachable cells then we incorrectly report it as reachable. The easiest way to see this incorrect behaviour is when the PathExists is called and can sometimes indicate a path exists when it does not.

To fix this, we now ensure we perform a check to see if the cell is reachable in this single layer case, this allows us to retain the optimization where we don't need to store the whole mapping, but allows us to correctly indicate when cells are unreachable.
  • Loading branch information
@RoosterDragon
RoosterDragon committed Aug 31, 2022
1 parent 16babc1 commit b791564
Showing 1 changed file with 34 additions and 28 deletions.
62 changes: 34 additions & 28 deletions OpenRA.Mods.Common/Pathfinder/HierarchicalPathFinder.cs
View file
Expand Up @@ -137,12 +137,21 @@ public GridInfo(CPos?[] singleAbstractCellForLayer, Dictionary<CPos, CPos> local
/// <summary>
/// Maps a local cell to a abstract node in the graph.
/// Returns null when the local cell is unreachable.
/// Pass a null locomotor to skip cost checks if the caller already checked for unreachable cells.
/// </summary>
public CPos? AbstractCellForLocalCell(CPos localCell)
public CPos? AbstractCellForLocalCell(CPos localCell, Locomotor locomotor)
{
var abstractCell = singleAbstractCellForLayer[localCell.Layer];
if (abstractCell != null)
{
// All reachable cells in the grid are joined together so only a single abstract cell was needed,
// but there may be unreachable cells in the grid which we must exclude.
if (locomotor != null && locomotor.MovementCostForCell(localCell) == PathGraph.MovementCostForUnreachableCell)
return null;
return abstractCell;
}

// Only reachable cells would be populated in the lookup, so no need to check their cost.
if (localCellToAbstractCell.TryGetValue(localCell, out var abstractCellFromMap))
return abstractCellFromMap;
return null;
Expand Down Expand Up @@ -432,7 +441,8 @@ void AddAbstractEdges(int xIncrement, int yIncrement, CVec adjacentVec, int2 off
for (var x = startX; x < startX + GridSize; x += xIncrement)
{
var cell = new CPos(x, y, gridLayer);
if (locomotor.MovementCostForCell(cell) == PathGraph.MovementCostForUnreachableCell)
var abstractCell = AbstractCellForLocalCell(cell);
if (abstractCell == null)
continue;

var adjacentCell = cell + adjacentVec;
Expand All @@ -442,13 +452,7 @@ void AddAbstractEdges(int xIncrement, int yIncrement, CVec adjacentVec, int2 off
if (locomotor.MovementCostToEnterCell(null, cell, candidateCell, BlockedByActor.None, null) !=
PathGraph.MovementCostForUnreachableCell)
{
var gridInfo = gridInfos[GridIndex(cell)];
var abstractCell = gridInfo.AbstractCellForLocalCell(cell);
if (abstractCell == null)
continue;

var gridInfoAdjacent = gridInfos[GridIndex(candidateCell)];
var abstractCellAdjacent = gridInfoAdjacent.AbstractCellForLocalCell(candidateCell);
var abstractCellAdjacent = AbstractCellForLocalCellNoCostCheck(candidateCell);
if (abstractCellAdjacent == null)
continue;

Expand Down Expand Up @@ -479,7 +483,8 @@ void AddAbstractCustomLayerEdges()
for (var x = gridX; x < gridX + GridSize; x++)
{
var cell = new CPos(x, y, gridLayer);
if (locomotor.MovementCostForCell(cell) == PathGraph.MovementCostForUnreachableCell)
var abstractCell = AbstractCellForLocalCell(cell);
if (abstractCell == null)
continue;

CPos candidateCell;
Expand All @@ -499,14 +504,7 @@ void AddAbstractCustomLayerEdges()
if (locomotor.MovementCostToEnterCell(null, cell, candidateCell, BlockedByActor.None, null) ==
PathGraph.MovementCostForUnreachableCell)
continue;

var gridInfo = gridInfos[GridIndex(cell)];
var abstractCell = gridInfo.AbstractCellForLocalCell(cell);
if (abstractCell == null)
continue;

var gridInfoAdjacent = gridInfos[GridIndex(candidateCell)];
var abstractCellAdjacent = gridInfoAdjacent.AbstractCellForLocalCell(candidateCell);
var abstractCellAdjacent = AbstractCellForLocalCellNoCostCheck(candidateCell);
if (abstractCellAdjacent == null)
continue;

Expand Down Expand Up @@ -754,12 +752,10 @@ public bool PathExists(CPos source, CPos target)

RebuildDomains();

var sourceGridInfo = gridInfos[GridIndex(source)];
var targetGridInfo = gridInfos[GridIndex(target)];
var abstractSource = sourceGridInfo.AbstractCellForLocalCell(source);
var abstractSource = AbstractCellForLocalCell(source);
if (abstractSource == null)
return false;
var abstractTarget = targetGridInfo.AbstractCellForLocalCell(target);
var abstractTarget = AbstractCellForLocalCell(target);
if (abstractTarget == null)
return false;
var sourceDomain = abstractDomains[abstractSource.Value];
Expand Down Expand Up @@ -876,11 +872,22 @@ List<GraphConnection> AbstractEdge(CPos abstractCell)
}

/// <summary>
/// Maps a local cell to a abstract node in the graph.
/// Returns null when the local cell is unreachable.
/// Maps a local cell to a abstract node in the graph. Returns null when the local cell is unreachable.
/// </summary>
CPos? AbstractCellForLocalCell(CPos localCell)
{
return gridInfos[GridIndex(localCell)].AbstractCellForLocalCell(localCell, locomotor);
}

/// <summary>
/// Maps a local cell to a abstract node in the graph. Returns null when the local cell is unreachable.
/// Skips the <see cref="Locomotor.MovementCostForCell"/> check, if it has already been performed.
/// If a movement cost check has not been performed, call <see cref="AbstractCellForLocalCell"/> instead.
/// </summary>
CPos? AbstractCellForLocalCell(CPos localCell) =>
gridInfos[GridIndex(localCell)].AbstractCellForLocalCell(localCell);
CPos? AbstractCellForLocalCellNoCostCheck(CPos localCell)
{
return gridInfos[GridIndex(localCell)].AbstractCellForLocalCell(localCell, null);
}

/// <summary>
/// Creates a <see cref="GraphEdge"/> from the <paramref name="localCell"/> to the <paramref name="abstractCell"/>.
Expand Down Expand Up @@ -909,8 +916,7 @@ List<GraphConnection> AbstractEdge(CPos abstractCell)
// So we don't need to handle an abstract cell lookup failing, or the search failing to expand.
// Cells added as initial starting points for the search are filtered out if they aren't reachable.
// The search only explores accessible cells from then on.
var gridInfo = gridInfos[GridIndex(cell)];
var abstractCell = gridInfo.AbstractCellForLocalCell(cell).Value;
var abstractCell = AbstractCellForLocalCellNoCostCheck(cell).Value;
var info = graph[abstractCell];
// Expand the abstract search only if we have yet to get a route to the abstract cell.
Expand Down

0 comments on commit b791564

Please sign in to comment.