Introduce MoveCooldownHelper to prevent lag spikes from failed pathfinding

Several activities that queue child Move activities can get into a bad scenario where the actor is pathfinding and then gets stuck because the destination is unreachable. When the Move activity then completes, then parent activity sees it has yet to reach the destination and tries to move again. However, the actor is still blocked in the same spot as before and thus the movment finishes immediately. This causes a performance death spiral where the actor attempts to pathfind every tick. The pathfinding attempt can also be very expensive if it must exhaustively check the whole map to determine no route is possible.

In order to prevent blocked actors from running into this scenario, we introduce MoveCooldownHelper. In its default setup it allows the parent activity to bail out if the actor was blocked during a pathfinding attempt. This means the activity will be dropped rather than trying to move endlessly. It also has an option to allow retrying if pathfinding was blocked, but applies a cooldown to avoid the performance penalty. For activities such as Enter, this means the actors will still try and enter their target if it is unreachable, but will only attempt once a second now rather than every tick.

MoveAdjacentTo will now cancel if it fails to reach the destination. This fixes MoveOntoAndTurn to skip the Turn if the move didn't reach the intended destination. Any other derived classes will similarly benefit from skipping follow-up actions.

OpenRA.Mods.Common/Activities/Move/Follow.cs

@@ -23,10 +23,10 @@ namespace OpenRA.Mods.Common.Activities
 		readonly WDist maxRange;
 		readonly IMove move;
 		readonly Color? targetLineColor;
+		readonly MoveCooldownHelper moveCooldownHelper;
 		Target target;
 		Target lastVisibleTarget;
 		bool useLastVisibleTarget;
-		bool wasMovingWithinRange;
 
 		public Follow(Actor self, in Target target, WDist minRange, WDist maxRange,
 			WPos? initialTargetPosition, Color? targetLineColor = null)
@@ -36,6 +36,7 @@ namespace OpenRA.Mods.Common.Activities
 			this.maxRange = maxRange;
 			this.targetLineColor = targetLineColor;
 			move = self.Trait<IMove>();
+			moveCooldownHelper = new MoveCooldownHelper(self.World, move as Mobile) { RetryIfDestinationBlocked = true };
 
 			// The target may become hidden between the initial order request and the first tick (e.g. if queued)
 			// Moving to any position (even if quite stale) is still better than immediately giving up
@@ -57,12 +58,9 @@ namespace OpenRA.Mods.Common.Activities
 
 			useLastVisibleTarget = targetIsHiddenActor || !target.IsValidFor(self);
 
-			// If we are ticking again after previously sequencing a MoveWithRange then that move must have completed
-			// Either we are in range and can see the target, or we've lost track of it and should give up
-			if (wasMovingWithinRange && targetIsHiddenActor)
-				return true;
-
-			wasMovingWithinRange = false;
+			var result = moveCooldownHelper.Tick(targetIsHiddenActor);
+			if (result != null)
+				return result.Value;
 
 			// Target is hidden or dead, and we don't have a fallback position to move towards
 			if (useLastVisibleTarget && !lastVisibleTarget.IsValidFor(self))
@@ -77,7 +75,7 @@ namespace OpenRA.Mods.Common.Activities
 				return useLastVisibleTarget;
 
 			// Move into range
-			wasMovingWithinRange = true;
+			moveCooldownHelper.NotifyMoveQueued();
 			QueueChild(move.MoveWithinRange(target, minRange, maxRange, checkTarget.CenterPosition, targetLineColor));
 			return false;
 		}

OpenRA.Mods.Common/Activities/Move/Move.cs

@@ -42,6 +42,7 @@ namespace OpenRA.Mods.Common.Activities
 		List<CPos> path;
 		CPos? destination;
 		int startTicks;
+		bool hadNoPath;
 
 		// For dealing with blockers
 		bool hasWaited;
@@ -113,6 +114,7 @@ namespace OpenRA.Mods.Common.Activities
 		protected override void OnFirstRun(Actor self)
 		{
 			startTicks = self.World.WorldTick;
+			mobile.MoveResult = MoveResult.InProgress;
 
 			if (evaluateNearestMovableCell && destination.HasValue)
 			{
@@ -137,6 +139,7 @@ namespace OpenRA.Mods.Common.Activities
 			{
 				path?.Clear();
 
+				mobile.MoveResult = MoveResult.CompleteCanceled;
 				return true;
 			}
 
@@ -144,19 +147,35 @@ namespace OpenRA.Mods.Common.Activities
 				return false;
 
 			if (destination == mobile.ToCell)
+			{
+				if (hadNoPath)
+					mobile.MoveResult = MoveResult.CompleteDestinationBlocked;
+				else
+					mobile.MoveResult = MoveResult.CompleteDestinationReached;
+
 				return true;
+			}
 
 			if (path.Count == 0)
 			{
+				hadNoPath = true;
 				destination = mobile.ToCell;
 				return false;
 			}
 
 			destination = path[0];
 
-			var nextCell = PopPath(self);
+			var (nextCell, shouldTryAgain) = PopPath(self);
 			if (nextCell == null)
+			{
+				if (!shouldTryAgain)
+				{
+					mobile.MoveResult = MoveResult.CompleteDestinationBlocked;
+					return true;
+				}
+
 				return false;
+			}
 
 			var firstFacing = self.World.Map.FacingBetween(mobile.FromCell, nextCell.Value.Cell, mobile.Facing);
 
@@ -200,10 +219,10 @@ namespace OpenRA.Mods.Common.Activities
 			return false;
 		}
 
-		(CPos Cell, SubCell SubCell)? PopPath(Actor self)
+		((CPos Cell, SubCell SubCell)? Next, bool ShouldTryAgain) PopPath(Actor self)
 		{
 			if (path.Count == 0)
-				return null;
+				return (null, false);
 
 			var nextCell = path[^1];
 
@@ -211,7 +230,7 @@ namespace OpenRA.Mods.Common.Activities
 			if (!Util.AreAdjacentCells(mobile.ToCell, nextCell))
 			{
 				path = EvalPath(BlockedByActor.Immovable);
-				return null;
+				return (null, false);
 			}
 
 			var containsTemporaryBlocker = self.World.ContainsTemporaryBlocker(nextCell, self);
@@ -230,7 +249,7 @@ namespace OpenRA.Mods.Common.Activities
 					if (path.Count < 2)
 					{
 						path.Clear();
-						return null;
+						return (null, false);
 					}
 
 					// We can reasonably assume that the blocker is friendly and has a similar locomotor type.
@@ -244,7 +263,7 @@ namespace OpenRA.Mods.Common.Activities
 					if (!nudgeOrRepath)
 					{
 						path.Clear();
-						return null;
+						return (null, false);
 					}
 				}
 
@@ -252,7 +271,7 @@ namespace OpenRA.Mods.Common.Activities
 				if (!mobile.CanEnterCell(nextCell, ignoreActor, BlockedByActor.Immovable))
 				{
 					path = EvalPath(BlockedByActor.Immovable);
-					return null;
+					return (null, false);
 				}
 
 				// See if they will move
@@ -263,17 +282,17 @@ namespace OpenRA.Mods.Common.Activities
 				{
 					waitTicksRemaining = mobile.Info.LocomotorInfo.WaitAverage;
 					hasWaited = true;
-					return null;
+					return (null, true);
 				}
 
 				if (--waitTicksRemaining >= 0)
-					return null;
+					return (null, true);
 
 				hasWaited = false;
 
 				// If the blocking actors are already leaving, wait a little longer instead of repathing
 				if (CellIsEvacuating(self, nextCell))
-					return null;
+					return (null, true);
 
 				// Calculate a new path
 				mobile.RemoveInfluence();
@@ -286,7 +305,7 @@ namespace OpenRA.Mods.Common.Activities
 					var newCell = path[^1];
 					path.RemoveAt(path.Count - 1);
 
-					return (newCell, mobile.GetAvailableSubCell(nextCell, mobile.FromSubCell, ignoreActor));
+					return ((newCell, mobile.GetAvailableSubCell(nextCell, mobile.FromSubCell, ignoreActor)), true);
 				}
 				else if (mobile.IsBlocking)
 				{
@@ -297,17 +316,17 @@ namespace OpenRA.Mods.Common.Activities
 						if ((nextCell - newCell).Value.LengthSquared > 2)
 							path.Add(mobile.ToCell);
 
-						return (newCell.Value, mobile.GetAvailableSubCell(newCell.Value, mobile.FromSubCell, ignoreActor));
+						return ((newCell.Value, mobile.GetAvailableSubCell(newCell.Value, mobile.FromSubCell, ignoreActor)), true);
 					}
 				}
 
-				return null;
+				return (null, false);
 			}
 
 			hasWaited = false;
 			path.RemoveAt(path.Count - 1);
 
-			return (nextCell, mobile.GetAvailableSubCell(nextCell, mobile.FromSubCell, ignoreActor));
+			return ((nextCell, mobile.GetAvailableSubCell(nextCell, mobile.FromSubCell, ignoreActor)), true);
 		}
 
 		protected override void OnLastRun(Actor self)
@@ -518,7 +537,7 @@ namespace OpenRA.Mods.Common.Activities
 				var fromSubcellOffset = map.Grid.OffsetOfSubCell(mobile.FromSubCell);
 				var toSubcellOffset = map.Grid.OffsetOfSubCell(mobile.ToSubCell);
 
-				var nextCell = parent.PopPath(self);
+				var (nextCell, _) = parent.PopPath(self);
 				if (nextCell != null)
 				{
 					if (!mobile.IsTraitPaused && !mobile.IsTraitDisabled && IsTurn(self, mobile, nextCell.Value.Cell, map))

OpenRA.Mods.Common/Activities/Move/MoveAdjacentTo.cs

@@ -99,9 +99,17 @@ namespace OpenRA.Mods.Common.Activities
 				QueueChild(Mobile.MoveTo(check => CalculatePathToTarget(self, check)));
 			}
 
-			// The last queued childactivity is guaranteed to be the inner move, so if the childactivity
-			// queue is empty it means we have reached our destination.
-			return TickChild(self);
+			// The last queued child activity is guaranteed to be the inner move,
+			// so if the child activity queue is empty it means the move completed.
+			if (!TickChild(self))
+				return false;
+
+			if (Mobile.MoveResult == MoveResult.CompleteDestinationReached)
+				return true;
+
+			// The move completed but we didn't reach the destination, so Cancel.
+			Cancel(self);
+			return true;
 		}
 
 		protected readonly List<CPos> SearchCells = new();

OpenRA.Mods.Common/Activities/Move/MoveCooldownHelper.cs

@@ -0,0 +1,100 @@
+using OpenRA.Activities;
+using OpenRA.Mods.Common.Traits;
+
+namespace OpenRA.Mods.Common.Activities
+{
+	/// <summary>
+	/// Activities that queue move activities via <see cref="IMove"/> can use this helper to decide
+	/// when moves with blocked destinations should be retried and to apply a cooldown between repeated moves.
+	/// </summary>
+	public sealed class MoveCooldownHelper
+	{
+		/// <summary>
+		/// If a move failed because the destination was blocked, indicates if we should try again.
+		/// When true, <see cref="Tick"/> will return null when the destination is blocked, after the cooldown has been applied.
+		/// When false, <see cref="Tick"/> will return true to indicate the activity should give up and complete.
+		/// Defaults to false.
+		/// </summary>
+		public bool RetryIfDestinationBlocked { get; set; }
+
+		/// <summary>
+		/// The cooldown delay in ticks. After a move with a blocked destination, the cooldown will be started.
+		/// Whilst the cooldown is in effect, <see cref="Tick"/> will return false.
+		/// After the cooldown finishes, <see cref="Tick"/> will return null to allow activity logic to resume.
+		/// This cooldown is important to avoid lag spikes caused by pathfinding every tick because the destination is unreachable.
+		/// Defaults to (20, 31).
+		/// </summary>
+		public (int MinTicksInclusive, int MaxTicksExclusive) Cooldown { get; set; } = (20, 31);
+
+		readonly World world;
+		readonly Mobile mobile;
+		bool wasMoving;
+		bool hasRunCooldown;
+		int cooldownTicks;
+
+		public MoveCooldownHelper(World world, Mobile mobile)
+		{
+			this.world = world;
+			this.mobile = mobile;
+		}
+
+		/// <summary>
+		/// Call this when queuing a move activity.
+		/// </summary>
+		public void NotifyMoveQueued()
+		{
+			wasMoving = true;
+		}
+
+		/// <summary>
+		/// Call this method within the <see cref="Activity.Tick(Actor)"/> method. It will return a tick result.
+		/// </summary>
+		/// <param name="targetIsHiddenActor">If the target is a hidden actor, forces the result to be true, once the move has completed.</param>
+		/// <returns>A result that should be returned from the calling Tick method.
+		/// A non-null result should be returned immediately.
+		/// On a null result, the method should continue with it's usual logic and perform any desired moves.</returns>
+		public bool? Tick(bool targetIsHiddenActor)
+		{
+			// We haven't moved yet, or we did move and we've finished the cooldown, allow the caller to resume with their logic.
+			if (!wasMoving)
+				return null;
+
+			if (!hasRunCooldown)
+			{
+				// The target is hidden, don't continue tracking it.
+				if (targetIsHiddenActor)
+					return true;
+
+				// Movement was cancelled, or we reached our destination, return immediately to allow the caller to perform their next steps.
+				if (mobile == null || mobile.MoveResult == MoveResult.CompleteCanceled || mobile.MoveResult == MoveResult.CompleteDestinationReached)
+				{
+					wasMoving = false;
+					return null;
+				}
+
+				// We couldn't reach the destination, don't try and keep going after the actor.
+				if (!RetryIfDestinationBlocked && mobile.MoveResult == MoveResult.CompleteDestinationBlocked)
+					return true;
+
+				// To avoid excessive pathfinding when the destination is blocked, wait for the cooldown before trying to move again.
+				// Applying some jitter to the wait time helps avoid multiple units repathing on the same tick and creating a lag spike.
+				hasRunCooldown = true;
+				cooldownTicks = world.SharedRandom.Next(Cooldown.MinTicksInclusive, Cooldown.MaxTicksExclusive);
+				return false;
+			}
+			else
+			{
+				if (cooldownTicks > 0)
+					cooldownTicks--;
+
+				if (cooldownTicks <= 0)
+				{
+					hasRunCooldown = false;
+					wasMoving = false;
+				}
+
+				return false;
+			}
+		}
+	}
+}