Fix reversed path searches from inaccessible locations.

The Harvester trait and MoveAdjacentTo activity called the pathfinder but had a single source and multiple targets. The pathfinder interface only allows for the opposite: multiple sources and a single target. To work around this they would swap the inputs. This works in most cases but not all cases. One aspect of asymmetry is that an actor may move out of an inaccessible source cell, but not onto an inaccessible target cell.

Searches that involved an inaccessible source cell and that applied this swapping method would therefore fail to return a path, when a valid path was possible. Although a rare case, once good way to reproduce is to use a production building that spawns actors on inaccessible cells around it, such as the RA naval yard. A move order uses the pathfinder correctly and the unit will move out. Using a force attack causes the unit to use the broken "swapped" mechanism in MoveAdjacentTo and it will be stuck.

This asymmetry has been longstanding but the pathfinding infrastructure only sporadically accounted for it. It is now documented and applied consistently. Create a new overload on the pathfinder trait that allows a single source and multiple targets, so callers have an overload that does what they need and won't be tempted to swap the positions and run into this issue.

Internally, this requires us to teach Locomotor to ignore the self actor when performing movement cost checks for these "in reverse" searches so the unit doesn't consider the cell blocked by itself.

OpenRA.Mods.Common/Pathfinder/DensePathGraph.cs

@@ -104,7 +104,7 @@ namespace OpenRA.Mods.Common.Pathfinder
 			CVec.Directions.Exclude(new CVec(-1, -1)).ToArray(), // BR
 		};
 
-		public List<GraphConnection> GetConnections(CPos position)
+		public List<GraphConnection> GetConnections(CPos position, Func<CPos, bool> targetPredicate)
 		{
 			var layer = position.Layer;
 			var info = this[position];
@@ -128,7 +128,7 @@ namespace OpenRA.Mods.Common.Pathfinder
 				if (!IsValidNeighbor(neighbor))
 					continue;
 
-				var pathCost = GetPathCostToNode(position, neighbor, dir);
+				var pathCost = GetPathCostToNode(position, neighbor, dir, targetPredicate);
 				if (pathCost != PathGraph.PathCostForInvalidPath &&
 					this[neighbor].Status != CellStatus.Closed)
 					validNeighbors.Add(new GraphConnection(neighbor, pathCost));
@@ -147,7 +147,7 @@ namespace OpenRA.Mods.Common.Pathfinder
 
 					var entryCost = cml.EntryMovementCost(locomotor.Info, layerPosition);
 					if (entryCost != PathGraph.MovementCostForUnreachableCell &&
-						CanEnterNode(position, layerPosition) &&
+						CanEnterNode(position, layerPosition, targetPredicate) &&
 						this[layerPosition].Status != CellStatus.Closed)
 						validNeighbors.Add(new GraphConnection(layerPosition, entryCost));
 				}
@@ -159,7 +159,7 @@ namespace OpenRA.Mods.Common.Pathfinder
 				{
 					var exitCost = CustomMovementLayers[layer].ExitMovementCost(locomotor.Info, groundPosition);
 					if (exitCost != PathGraph.MovementCostForUnreachableCell &&
-						CanEnterNode(position, groundPosition) &&
+						CanEnterNode(position, groundPosition, targetPredicate) &&
 						this[groundPosition].Status != CellStatus.Closed)
 						validNeighbors.Add(new GraphConnection(groundPosition, exitCost));
 				}
@@ -168,16 +168,23 @@ namespace OpenRA.Mods.Common.Pathfinder
 			return validNeighbors;
 		}
 
-		bool CanEnterNode(CPos srcNode, CPos destNode)
+		bool CanEnterNode(CPos srcNode, CPos destNode, Func<CPos, bool> targetPredicate)
 		{
 			return
 				locomotor.MovementCostToEnterCell(actor, srcNode, destNode, check, ignoreActor)
-				!= PathGraph.MovementCostForUnreachableCell;
+				!= PathGraph.MovementCostForUnreachableCell ||
+				(inReverse && targetPredicate(destNode));
 		}
 
-		int GetPathCostToNode(CPos srcNode, CPos destNode, CVec direction)
+		int GetPathCostToNode(CPos srcNode, CPos destNode, CVec direction, Func<CPos, bool> targetPredicate)
 		{
 			var movementCost = locomotor.MovementCostToEnterCell(actor, srcNode, destNode, check, ignoreActor);
+
+			// When doing searches in reverse, we must allow movement onto an inaccessible target location.
+			// Because when reversed this is actually the source, and it is allowed to move out from an inaccessible source.
+			if (movementCost == PathGraph.MovementCostForUnreachableCell && inReverse && targetPredicate(destNode))
+				movementCost = 0;
+
 			if (movementCost != PathGraph.MovementCostForUnreachableCell)
 				return CalculateCellPathCost(destNode, direction, movementCost);
 

OpenRA.Mods.Common/Pathfinder/HierarchicalPathFinder.cs

@@ -73,13 +73,13 @@ namespace OpenRA.Mods.Common.Pathfinder
 	/// nodes, but uses a heuristic informed from the previous level to guide the search in the right direction.</para>
 	///
 	/// <para>This implementation is aware of movement costs over terrain given by
-	/// <see cref="Locomotor.MovementCostToEnterCell(Actor, CPos, CPos, BlockedByActor, Actor)"/>. It is aware of
+	/// <see cref="Locomotor.MovementCostToEnterCell(Actor, CPos, CPos, BlockedByActor, Actor, bool)"/>. It is aware of
 	/// changes to the costs in terrain and able to update the abstract graph when this occurs. It is able to search
 	/// the abstract graph as if <see cref="BlockedByActor.None"/> had been specified. If
 	/// <see cref="BlockedByActor.Immovable"/> is given in the constructor, the abstract graph will additionally
 	/// account for a subset of immovable actors using the same rules as
-	/// <see cref="Locomotor.CanMoveFreelyInto(Actor, CPos, SubCell, BlockedByActor, Actor)"/>. It will be aware of
-	/// changes to actors on the map and update the abstract graph when this occurs. Other types of blocking actors
+	/// <see cref="Locomotor.CanMoveFreelyInto(Actor, CPos, SubCell, BlockedByActor, Actor, bool)"/>. It will be aware
+	/// of changes to actors on the map and update the abstract graph when this occurs. Other types of blocking actors
 	/// will not be accounted for in the heuristic.</para>
 	///
 	/// <para>If the obstacle on the map is from terrain (e.g. a cliff or lake) the heuristic will work well. If the
@@ -633,14 +633,14 @@ namespace OpenRA.Mods.Common.Pathfinder
 
 		/// <summary>
 		/// <see cref="BlockedByActor.Immovable"/> defines immovability based on the mobile trait. The blocking rules
-		/// in <see cref="Locomotor.CanMoveFreelyInto(Actor, CPos, SubCell, BlockedByActor, Actor)"/> allow units to
-		/// pass these immovable actors if they are temporary blockers (e.g. gates) or crushable by the locomotor.
+		/// in <see cref="Locomotor.CanMoveFreelyInto(Actor, CPos, SubCell, BlockedByActor, Actor, bool)"/> allow units
+		/// to pass these immovable actors if they are temporary blockers (e.g. gates) or crushable by the locomotor.
 		/// Since our abstract graph must work for any actor, we have to be conservative and can only consider a subset
 		/// of the immovable actors in the graph - ones we know cannot be passed by some actors due to these rules.
 		/// Both this and <see cref="ActorCellIsBlocking"/> must be true for a cell to be blocked.
 		///
 		/// This method is dependant on the logic in
-		/// <see cref="Locomotor.CanMoveFreelyInto(Actor, CPos, SubCell, BlockedByActor, Actor)"/> and
+		/// <see cref="Locomotor.CanMoveFreelyInto(Actor, CPos, SubCell, BlockedByActor, Actor, bool)"/> and
 		/// <see cref="Locomotor.UpdateCellBlocking"/>. This method must be kept in sync with changes in the locomotor
 		/// rules.
 		/// </summary>

OpenRA.Mods.Common/Pathfinder/IPathGraph.cs

@@ -27,7 +27,7 @@ namespace OpenRA.Mods.Common.Pathfinder
 		/// <remarks>PERF: Returns a <see cref="List{T}"/> rather than an <see cref="IEnumerable{T}"/> as enumerating
 		/// this efficiently is important for pathfinding performance. Callers should interact with this as an
 		/// <see cref="IEnumerable{T}"/> and not mutate the result.</remarks>
-		List<GraphConnection> GetConnections(CPos source);
+		List<GraphConnection> GetConnections(CPos source, Func<CPos, bool> targetPredicate);
 
 		/// <summary>
 		/// Gets or sets the pathfinding information for a given node.

OpenRA.Mods.Common/Pathfinder/PathSearch.cs

@@ -46,7 +46,7 @@ namespace OpenRA.Mods.Common.Pathfinder
 			var graph = new MapPathGraph(LayerPoolForWorld(world), locomotor, self, world, check, customCost, ignoreActor, laneBias, false);
 			var search = new PathSearch(graph, loc => 0, 0, targetPredicate, recorder);
 
-			AddInitialCells(world, locomotor, froms, customCost, search);
+			AddInitialCells(world, locomotor, self, froms, check, customCost, ignoreActor, false, search);
 
 			return search;
 		}
@@ -71,11 +71,22 @@ namespace OpenRA.Mods.Common.Pathfinder
 			heuristic ??= DefaultCostEstimator(locomotor, target);
 			var search = new PathSearch(graph, heuristic, heuristicWeightPercentage, loc => loc == target, recorder);
 
-			AddInitialCells(world, locomotor, froms, customCost, search);
+			AddInitialCells(world, locomotor, self, froms, check, customCost, ignoreActor, inReverse, search);
 
 			return search;
 		}
 
+		/// <summary>
+		/// Determines if a cell is a valid pathfinding location.
+		/// <list type="bullet">
+		/// <item>It is in the world.</item>
+		/// <item>It is either on the ground layer (0) or on an *enabled* custom movement layer.</item>
+		/// <item>It has not been excluded by the <paramref name="customCost"/>.</item>
+		/// </list>
+		/// If required, follow this with a call to
+		/// <see cref="Locomotor.MovementCostToEnterCell(Actor, CPos, CPos, BlockedByActor, Actor, bool)"/> to
+		/// determine if the cell is accessible.
+		/// </summary>
 		public static bool CellAllowsMovement(World world, Locomotor locomotor, CPos cell, Func<CPos, int> customCost)
 		{
 			return world.Map.Contains(cell) &&
@@ -83,10 +94,18 @@ namespace OpenRA.Mods.Common.Pathfinder
 				(customCost == null || customCost(cell) != PathGraph.PathCostForInvalidPath);
 		}
 
-		static void AddInitialCells(World world, Locomotor locomotor, IEnumerable<CPos> froms, Func<CPos, int> customCost, PathSearch search)
+		static void AddInitialCells(World world, Locomotor locomotor, Actor self, IEnumerable<CPos> froms,
+			BlockedByActor check, Func<CPos, int> customCost, Actor ignoreActor, bool inReverse, PathSearch search)
 		{
+			// A source cell is allowed to have an unreachable movement cost.
+			// Therefore we don't need to check if the cell is accessible, only that it allows movement.
+			// *Unless* the search is being done in reverse, in this case the source is really a target,
+			// and a target is required to have a reachable cost.
+			// We also need to ignore self, so we don't consider the location blocked by ourselves!
 			foreach (var sl in froms)
-				if (CellAllowsMovement(world, locomotor, sl, customCost))
+				if (CellAllowsMovement(world, locomotor, sl, customCost) &&
+					(!inReverse || locomotor.MovementCostToEnterCell(self, sl, check, ignoreActor, true)
+						!= PathGraph.MovementCostForUnreachableCell))
 					search.AddInitialCell(sl, customCost);
 		}
 
@@ -229,7 +248,7 @@ namespace OpenRA.Mods.Common.Pathfinder
 			var currentInfo = Graph[currentMinNode];
 			Graph[currentMinNode] = new CellInfo(CellStatus.Closed, currentInfo.CostSoFar, currentInfo.EstimatedTotalCost, currentInfo.PreviousNode);
 
-			foreach (var connection in Graph.GetConnections(currentMinNode))
+			foreach (var connection in Graph.GetConnections(currentMinNode, TargetPredicate))
 			{
 				// Calculate the cost up to that point
 				var costSoFarToNeighbor = currentInfo.CostSoFar + connection.Cost;

OpenRA.Mods.Common/Pathfinder/SparsePathGraph.cs

@@ -32,7 +32,7 @@ namespace OpenRA.Mods.Common.Pathfinder
 			info = new Dictionary<CPos, CellInfo>(estimatedSearchSize);
 		}
 
-		public List<GraphConnection> GetConnections(CPos position)
+		public List<GraphConnection> GetConnections(CPos position, Func<CPos, bool> targetPredicate)
 		{
 			return edges(position) ?? new List<GraphConnection>();
 		}