AI uses better rally point placement

- AI places rally points at pathable locations. A pathable location isn't strictly required, but avoids the AI setting rally points at seemingly dumb locations. This is an addtional check on top of the existing buildability check.
- AI now evaluates rally points every AssignRallyPointsInterval (default 100 ticks). Invalid rally points aren't that harmful, so no need to check them every tick. Additionally we do a rolling update so rally points for multiple locations are spread across multiple ticks to reduce any potential lag spikes.

OpenRA.Mods.Common/Traits/BotModules/BaseBuilderBotModule.cs

@@ -9,6 +9,7 @@
  */
 #endregion
 
+using System;
 using System.Collections.Generic;
 using System.Linq;
 using OpenRA.Traits;
@@ -135,6 +136,9 @@ namespace OpenRA.Mods.Common.Traits
 		[Desc("Only queue construction of a new structure when above this requirement.")]
 		public readonly int ProductionMinCashRequirement = 500;
 
+		[Desc("Delay (in ticks) between reassigning rally points.")]
+		public readonly int AssignRallyPointsInterval = 100;
+
 		public override object Create(ActorInitializer init) { return new BaseBuilderBotModule(init.Self, this); }
 	}
 
@@ -159,9 +163,13 @@ namespace OpenRA.Mods.Common.Traits
 		PowerManager playerPower;
 		PlayerResources playerResources;
 		IResourceLayer resourceLayer;
+		IPathFinder pathFinder;
 		IBotPositionsUpdated[] positionsUpdatedModules;
 		CPos initialBaseCenter;
 
+		readonly Stack<TraitPair<RallyPoint>> rallyPoints = new();
+		int assignRallyPointsTicks;
+
 		readonly BaseBuilderQueueManager[] builders;
 		int currentBuilderIndex = 0;
 
@@ -187,6 +195,7 @@ namespace OpenRA.Mods.Common.Traits
 			playerPower = self.Owner.PlayerActor.TraitOrDefault<PowerManager>();
 			playerResources = self.Owner.PlayerActor.Trait<PlayerResources>();
 			resourceLayer = self.World.WorldActor.TraitOrDefault<IResourceLayer>();
+			pathFinder = self.World.WorldActor.TraitOrDefault<IPathFinder>();
 			positionsUpdatedModules = self.Owner.PlayerActor.TraitsImplementing<IBotPositionsUpdated>().ToArray();
 
 			var i = 0;
@@ -198,6 +207,12 @@ namespace OpenRA.Mods.Common.Traits
 				builders[i++] = new BaseBuilderQueueManager(this, defense, player, playerPower, playerResources, resourceLayer);
 		}
 
+		protected override void TraitEnabled(Actor self)
+		{
+			// Avoid all AIs reevaluating assignments on the same tick, randomize their initial evaluation delay.
+			assignRallyPointsTicks = world.LocalRandom.Next(0, Info.AssignRallyPointsInterval);
+		}
+
 		void IBotPositionsUpdated.UpdatedBaseCenter(CPos newLocation)
 		{
 			initialBaseCenter = newLocation;
@@ -212,8 +227,23 @@ namespace OpenRA.Mods.Common.Traits
 
 		void IBotTick.BotTick(IBot bot)
 		{
-			// TODO: this causes pathfinding lag when AI's gets blocked in
+			if (--assignRallyPointsTicks <= 0)
-			SetRallyPointsForNewProductionBuildings(bot);
+			{
+				assignRallyPointsTicks = Math.Max(2, Info.AssignRallyPointsInterval);
+				foreach (var rp in world.ActorsWithTrait<RallyPoint>().Where(rp => rp.Actor.Owner == player))
+					rallyPoints.Push(rp);
+			}
+			else
+			{
+				// PERF: Spread out rally point assignments updates across multiple ticks.
+				var updateCount = Exts.IntegerDivisionRoundingAwayFromZero(rallyPoints.Count, assignRallyPointsTicks);
+				for (var i = 0; i < updateCount; i++)
+				{
+					var rp = rallyPoints.Pop();
+					if (rp.Actor.Owner == player)
+						SetRallyPoint(bot, rp);
+				}
+			}
 
 			BuildingsBeingProduced.Clear();
 
@@ -275,28 +305,31 @@ namespace OpenRA.Mods.Common.Traits
 					n.UpdatedDefenseCenter(e.Attacker.Location);
 		}
 
-		void SetRallyPointsForNewProductionBuildings(IBot bot)
+		void SetRallyPoint(IBot bot, TraitPair<RallyPoint> rp)
 		{
-			foreach (var rp in world.ActorsWithTrait<RallyPoint>())
+			var needsRallyPoint = rp.Trait.Path.Count == 0;
-			{
-				if (rp.Actor.Owner != player)
-					continue;
 
-				if (rp.Trait.Path.Count == 0 || !IsRallyPointValid(rp.Trait.Path[0], rp.Actor.Info.TraitInfoOrDefault<BuildingInfo>()))
+			if (!needsRallyPoint)
+			{
+				var locomotors = LocomotorsForProducibles(rp.Actor);
+				needsRallyPoint = !IsRallyPointValid(rp.Actor.Location, rp.Trait.Path[0], locomotors, rp.Actor.Info.TraitInfoOrDefault<BuildingInfo>());
+			}
+
+			if (needsRallyPoint)
+			{
+				bot.QueueOrder(new Order("SetRallyPoint", rp.Actor, Target.FromCell(world, ChooseRallyLocationNear(rp.Actor)), false)
 				{
-					bot.QueueOrder(new Order("SetRallyPoint", rp.Actor, Target.FromCell(world, ChooseRallyLocationNear(rp.Actor)), false)
+					SuppressVisualFeedback = true
-					{
+				});
-						SuppressVisualFeedback = true
-					});
-				}
 			}
 		}
 
 		// Won't work for shipyards...
 		CPos ChooseRallyLocationNear(Actor producer)
 		{
+			var locomotors = LocomotorsForProducibles(producer);
 			var possibleRallyPoints = world.Map.FindTilesInCircle(producer.Location, Info.RallyPointScanRadius)
-				.Where(c => IsRallyPointValid(c, producer.Info.TraitInfoOrDefault<BuildingInfo>()))
+				.Where(c => IsRallyPointValid(producer.Location, c, locomotors, producer.Info.TraitInfoOrDefault<BuildingInfo>()))
 				.ToList();
 
 			if (possibleRallyPoints.Count == 0)
@@ -308,9 +341,38 @@ namespace OpenRA.Mods.Common.Traits
 			return possibleRallyPoints.Random(world.LocalRandom);
 		}
 
-		bool IsRallyPointValid(CPos x, BuildingInfo info)
+		Locomotor[] LocomotorsForProducibles(Actor producer)
 		{
-			return info != null && world.IsCellBuildable(x, null, info);
+			var buildingInfo = producer.Info.TraitInfoOrDefault<BuildingInfo>();
+			var productionInfo = producer.Info.TraitInfoOrDefault<ProductionInfo>();
+			var locomotors = Array.Empty<Locomotor>();
+
+			if (productionInfo != null && productionInfo.Produces.Length > 0)
+			{
+				var productionQueues = producer.Owner.PlayerActor.TraitsImplementing<ProductionQueue>()
+					.Where(pq => productionInfo.Produces.Contains(pq.Info.Type));
+				var producibles = productionQueues.SelectMany(pq => pq.BuildableItems());
+				var locomotorNames = producibles
+					.Select(p => p.TraitInfoOrDefault<MobileInfo>())
+					.Where(mi => mi != null)
+					.Select(mi => mi.Locomotor)
+					.ToHashSet();
+				locomotors = world.WorldActor.TraitsImplementing<Locomotor>()
+					.Where(l => locomotorNames.Contains(l.Info.Name))
+					.ToArray();
+			}
+
+			return locomotors;
+		}
+
+		bool IsRallyPointValid(CPos producerLocation, CPos rallyPointLocation, Locomotor[] locomotors, BuildingInfo buildingInfo)
+		{
+			return
+				(pathFinder == null ||
+					locomotors.All(l => pathFinder.PathMightExistForLocomotorBlockedByImmovable(l, producerLocation, rallyPointLocation)))
+				&&
+				(buildingInfo == null ||
+					world.IsCellBuildable(rallyPointLocation, null, buildingInfo));
 		}
 
 		// Require at least one refinery, unless we can't build it.

OpenRA.Mods.Common/Traits/World/PathFinder.cs

@@ -258,6 +258,25 @@ namespace OpenRA.Mods.Common.Traits
 			return hierarchicalPathFindersBlockedByNoneByLocomotor[locomotor].PathExists(source, target);
 		}
 
+		/// <summary>
+		/// Determines if a path exists between source and target.
+		/// Terrain and a *subset* of immovable actors are taken into account,
+		/// i.e. as if a subset of <see cref="BlockedByActor.Immovable"/> was given.
+		/// This would apply for any actor using the given <see cref="Locomotor"/>.
+		/// </summary>
+		/// <remarks>
+		/// It is allowed for an actor to occupy an inaccessible space and move out of it if another adjacent cell is
+		/// accessible, but it is not allowed to move into an inaccessible target space. Therefore it is vitally
+		/// important to not mix up the source and target locations. A path can exist from an inaccessible source space
+		/// to an accessible target space, but if those parameters as swapped then no path can exist.
+		/// As only a subset of immovable actors are taken into account,
+		/// this method can return false positives, indicating a path might exist where none is possible.
+		/// </remarks>
+		public bool PathMightExistForLocomotorBlockedByImmovable(Locomotor locomotor, CPos source, CPos target)
+		{
+			return hierarchicalPathFindersBlockedByImmovableByLocomotor[locomotor].PathExists(source, target);
+		}
+
 		static Locomotor GetActorLocomotor(Actor self)
 		{
 			// PERF: This PathFinder trait requires the use of Mobile, so we can be sure that is in use.

OpenRA.Mods.Common/TraitsInterfaces.cs

@@ -943,5 +943,19 @@ namespace OpenRA.Mods.Common.Traits
 		/// <remarks>Path searches are not guaranteed to by symmetric,
 		/// the source and target locations cannot be swapped.</remarks>
 		bool PathExistsForLocomotor(Locomotor locomotor, CPos source, CPos target);
+
+		/// <summary>
+		/// Determines if a path exists between source and target.
+		/// Terrain and immovable actors are taken into account,
+		/// i.e. as if <see cref="BlockedByActor.Immovable"/> was given.
+		/// Implementations are permitted to only account for a subset of actors, for performance.
+		/// This would apply for any actor using the given <see cref="Locomotor"/>.
+		/// </summary>
+		/// <remarks>Path searches are not guaranteed to by symmetric,
+		/// the source and target locations cannot be swapped.
+		/// If this method returns false, there is guaranteed to be no path.
+		/// If it returns true, there *might* be a path.
+		/// </remarks>
+		bool PathMightExistForLocomotorBlockedByImmovable(Locomotor locomotor, CPos source, CPos target);
 	}
 }