Fix target invalidation and reacquisition in AttackFollow.

OpenRA.Mods.Cnc/Traits/Attack/AttackTDGunboatTurreted.cs

@@ -59,10 +59,10 @@ namespace OpenRA.Mods.Cnc.Traits
 				{
 					// Check that AttackTDGunboatTurreted hasn't cancelled the target by modifying attack.Target
 					// Having both this and AttackTDGunboatTurreted modify that field is a horrible hack.
-					if (hasTicked && attack.Target.Type == TargetType.Invalid)
+					if (hasTicked && attack.requestedTarget.Type == TargetType.Invalid)
 						return NextActivity;
 
-					attack.Target = target;
+					attack.requestedTarget = target;
 					hasTicked = true;
 				}
 

OpenRA.Mods.Common/Traits/Attack/AttackFollow.cs

@@ -13,7 +13,6 @@ using System;
 using System.Drawing;
 using System.Linq;
 using OpenRA.Activities;
-using OpenRA.Mods.Common.Activities;
 using OpenRA.Traits;
 
 namespace OpenRA.Mods.Common.Traits
@@ -26,24 +25,53 @@ namespace OpenRA.Mods.Common.Traits
 
 	public class AttackFollow : AttackBase, INotifyOwnerChanged
 	{
-		public Target Target { get; protected set; }
+		protected Target requestedTarget;
+		protected bool requestedForceAttack;
+		Mobile mobile;
 
 		public AttackFollow(Actor self, AttackFollowInfo info)
 			: base(self, info) { }
 
+		protected override void Created(Actor self)
+		{
+			mobile = self.TraitOrDefault<Mobile>();
+			base.Created(self);
+		}
+
+		protected bool CanAimAtTarget(Actor self, Target target, bool forceAttack)
+		{
+			if (target.Type == TargetType.Actor && !target.Actor.CanBeViewedByPlayer(self.Owner))
+				return false;
+
+			if (target.Type == TargetType.FrozenActor && !target.FrozenActor.IsValid)
+				return false;
+
+			var pos = self.CenterPosition;
+			var armaments = ChooseArmamentsForTarget(target, forceAttack);
+			foreach (var a in armaments)
+				if (target.IsInRange(pos, a.MaxRange()) && (a.Weapon.MinRange == WDist.Zero || !target.IsInRange(pos, a.Weapon.MinRange)))
+					return true;
+
+			return false;
+		}
+
 		protected override void Tick(Actor self)
 		{
-			// We can safely ignore target visibility here - the armament will handle this for us.
-			bool targetIsHiddenActor;
-			Target = Target.Recalculate(self.Owner, out targetIsHiddenActor);
 			if (IsTraitDisabled)
-			{
+				requestedTarget = Target.Invalid;
-				Target = Target.Invalid;
-				return;
-			}
 
-			DoAttack(self, Target);
+			// Can't fire on anything
-			IsAiming = Target.IsValidFor(self);
+			if (mobile != null && !mobile.CanInteractWithGroundLayer(self))
+				return;
+
+			if (requestedTarget.Type != TargetType.Invalid)
+			{
+				IsAiming = CanAimAtTarget(self, requestedTarget, requestedForceAttack);
+				if (IsAiming)
+					DoAttack(self, requestedTarget);
+			}
+			else
+				IsAiming = false;
 
 			base.Tick(self);
 		}
@@ -55,13 +83,18 @@ namespace OpenRA.Mods.Common.Traits
 
 		public override void OnStopOrder(Actor self)
 		{
-			Target = Target.Invalid;
+			requestedTarget = Target.Invalid;
 			base.OnStopOrder(self);
 		}
 
-		public void OnOwnerChanged(Actor self, Player oldOwner, Player newOwner)
+		public bool HasReachableTarget(bool allowMove)
 		{
-			Target = Target.Invalid;
+			return IsReachableTarget(requestedTarget, allowMove);
+		}
+
+		void INotifyOwnerChanged.OnOwnerChanged(Actor self, Player oldOwner, Player newOwner)
+		{
+			requestedTarget = Target.Invalid;
 		}
 
 		class AttackActivity : Activity
@@ -70,7 +103,13 @@ namespace OpenRA.Mods.Common.Traits
 			readonly RevealsShroud[] revealsShroud;
 			readonly IMove move;
 			readonly bool forceAttack;
+
 			Target target;
+			Target lastVisibleTarget;
+			bool useLastVisibleTarget;
+			WDist lastVisibleMaximumRange;
+			WDist lastVisibleMinimumRange;
+			bool wasMovingWithinRange;
 			bool hasTicked;
 
 			public AttackActivity(Actor self, Target target, bool allowMove, bool forceAttack)
@@ -81,67 +120,115 @@ namespace OpenRA.Mods.Common.Traits
 
 				this.target = target;
 				this.forceAttack = forceAttack;
+
+				// 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
+				if ((target.Type == TargetType.Actor && target.Actor.CanBeViewedByPlayer(self.Owner))
+				    || target.Type == TargetType.FrozenActor || target.Type == TargetType.Terrain)
+				{
+					lastVisibleTarget = Target.FromPos(target.CenterPosition);
+					lastVisibleMaximumRange = attack.GetMaximumRangeVersusTarget(target);
+					lastVisibleMinimumRange = attack.GetMinimumRangeVersusTarget(target);
+				}
 			}
 
 			public override Activity Tick(Actor self)
 			{
-				// All of the interesting behaviour to move to the last known target position if it becomes hidden
+				if (IsCanceled)
-				// and to reacquire the target if it is revealed enroute is handled inside MoveWithinRange.
+					return NextActivity;
-				// At this point in the activity chain we are either ticking against the target for the first time
+
-				// (and so don't know where it is), or after MoveWithinRange has lost the target and given up.
+				// Check that AttackFollow hasn't cancelled the target by modifying attack.Target
-				// We can therefore treat a hidden targets as invalid and give up if we can't currently see it.
+				// Having both this and AttackFollow modify that field is a horrible hack.
-				target = target.RecalculateInvalidatingHiddenTargets(self.Owner);
+				if (hasTicked && attack.requestedTarget.Type == TargetType.Invalid)
-				if (IsCanceled || !target.IsValidFor(self))
 					return NextActivity;
 
 				if (attack.IsTraitPaused)
 					return this;
 
-				var weapon = attack.ChooseArmamentsForTarget(target, forceAttack).FirstOrDefault();
+				bool targetIsHiddenActor;
-				if (weapon != null)
+				attack.requestedForceAttack = forceAttack;
+				attack.requestedTarget = target = target.Recalculate(self.Owner, out targetIsHiddenActor);
+				hasTicked = true;
+
+				if (!targetIsHiddenActor && target.Type == TargetType.Actor)
 				{
-					// Check that AttackFollow hasn't cancelled the target by modifying attack.Target
+					lastVisibleTarget = Target.FromTargetPositions(target);
-					// Having both this and AttackFollow modify that field is a horrible hack.
+					lastVisibleMaximumRange = attack.GetMaximumRangeVersusTarget(target);
-					if (hasTicked && attack.Target.Type == TargetType.Invalid)
+					lastVisibleMinimumRange = attack.GetMinimumRange();
-						return NextActivity;
 
-					var targetIsMobile = (target.Type == TargetType.Actor && target.Actor.Info.HasTraitInfo<IMoveInfo>())
+					// Try and sit at least one cell away from the min or max ranges to give some leeway if the target starts moving.
-						|| (target.Type == TargetType.FrozenActor && target.FrozenActor.Info.HasTraitInfo<IMoveInfo>());
+					if (target.Actor.Info.HasTraitInfo<IMoveInfo>())
-
-					// Try and sit at least one cell closer than the max range to give some leeway if the target starts moving.
-					var modifiedRange = weapon.MaxRange();
-					var maxRange = targetIsMobile ? new WDist(Math.Max(weapon.Weapon.MinRange.Length, modifiedRange.Length - 1024))
-						: modifiedRange;
-
-					// Most actors want to be able to see their target before shooting
-					if (!attack.Info.TargetFrozenActors && !forceAttack && target.Type == TargetType.FrozenActor)
 					{
-						var rs = revealsShroud
+						var preferMinRange = Math.Min(lastVisibleMinimumRange.Length + 1024, lastVisibleMaximumRange.Length);
-							.Where(Exts.IsTraitEnabled)
+						var preferMaxRange = Math.Max(lastVisibleMaximumRange.Length - 1024, lastVisibleMinimumRange.Length);
-							.MaxByOrDefault(s => s.Range);
+						lastVisibleMaximumRange = new WDist((lastVisibleMaximumRange.Length - 1024).Clamp(preferMinRange, preferMaxRange));
-
-						// Default to 2 cells if there are no active traits
-						var sightRange = rs != null ? rs.Range : WDist.FromCells(2);
-						if (sightRange < maxRange)
-							maxRange = sightRange;
 					}
-
-					attack.Target = target;
-					hasTicked = true;
-
-					if (move != null)
-						return ActivityUtils.SequenceActivities(
-							move.MoveFollow(self, target, weapon.Weapon.MinRange, maxRange, targetLineColor: Color.Red),
-							this);
-
-					if (target.IsInRange(self.CenterPosition, weapon.MaxRange()) &&
-						!target.IsInRange(self.CenterPosition, weapon.Weapon.MinRange))
-						return this;
 				}
 
-				attack.Target = Target.Invalid;
+				var oldUseLastVisibleTarget = useLastVisibleTarget;
+				var maxRange = lastVisibleMaximumRange;
+				var minRange = lastVisibleMinimumRange;
+				useLastVisibleTarget = targetIsHiddenActor || !target.IsValidFor(self);
 
-				return NextActivity;
+				// Most actors want to be able to see their target before shooting
+				if (target.Type == TargetType.FrozenActor && !attack.Info.TargetFrozenActors && !forceAttack)
+				{
+					var rs = revealsShroud
+						.Where(Exts.IsTraitEnabled)
+						.MaxByOrDefault(s => s.Range);
+
+					// Default to 2 cells if there are no active traits
+					var sightRange = rs != null ? rs.Range : WDist.FromCells(2);
+					if (sightRange < maxRange)
+						maxRange = sightRange;
+				}
+
+				// 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)
+				{
+					attack.requestedTarget = Target.Invalid;
+					return NextActivity;
+				}
+
+				// Update target lines if required
+				if (useLastVisibleTarget != oldUseLastVisibleTarget)
+					self.SetTargetLine(useLastVisibleTarget ? lastVisibleTarget : target, Color.Red, false);
+
+				// Target is hidden or dead, and we don't have a fallback position to move towards
+				if (useLastVisibleTarget && !lastVisibleTarget.IsValidFor(self))
+				{
+					attack.requestedTarget = Target.Invalid;
+					return NextActivity;
+				}
+
+				var pos = self.CenterPosition;
+				var checkTarget = useLastVisibleTarget ? lastVisibleTarget : target;
+
+				// We've reached the required range - if the target is visible and valid then we wait
+				// otherwise if it is hidden or dead we give up
+				if (checkTarget.IsInRange(pos, maxRange) && !checkTarget.IsInRange(pos, minRange))
+				{
+					if (useLastVisibleTarget)
+					{
+						attack.requestedTarget = Target.Invalid;
+						return NextActivity;
+					}
+
+					return this;
+				}
+
+				// We can't move into range, so give up
+				if (move == null)
+				{
+					attack.requestedTarget = Target.Invalid;
+					return NextActivity;
+				}
+
+				wasMovingWithinRange = true;
+				return ActivityUtils.SequenceActivities(
+					move.MoveWithinRange(target, minRange, maxRange, checkTarget.CenterPosition, Color.Red),
+					this);
 			}
 		}
 	}

OpenRA.Mods.Common/Traits/AutoTarget.cs

@@ -236,7 +236,7 @@ namespace OpenRA.Mods.Common.Traits
 
 			// PERF: Avoid LINQ.
 			foreach (var attackFollow in attackFollows)
-				if (!attackFollow.IsTraitDisabled && attackFollow.IsReachableTarget(attackFollow.Target, allowMove))
+				if (!attackFollow.IsTraitDisabled && attackFollow.HasReachableTarget(allowMove))
 					return false;
 
 			return true;

OpenRA.Mods.Common/Traits/Turreted.cs

@@ -255,12 +255,6 @@ namespace OpenRA.Mods.Common.Traits
 			if (attack != null && attack.IsAiming)
 				attack.OnStopOrder(self);
 		}
-
-		protected override void TraitResumed(Actor self)
-		{
-			if (attack != null)
-				FaceTarget(self, attack.Target);
-		}
 	}
 
 	public class TurretFacingInit : IActorInit<int>