Rework harvester resource claiming:

* Maintains lists of claims, and only restricts
   reservations for friendly units.
 * Removes OnNotifyResourceClaimLost; it's not
   clear whether that is still useful, and it
   prevents future necessary cleanups.
 * Moves other code without changing behaviour.

This fixed stale claims from dead units and enemy
claims from preventing otherwise valid harvest
activities.

OpenRA.Mods.Common/AI/HackyAI.cs

@@ -263,8 +263,7 @@ namespace OpenRA.Mods.Common.AI
 		public Player Player { get; private set; }
 
 		readonly DomainIndex domainIndex;
-		readonly ResourceLayer resLayer;
+		readonly ResourceClaimLayer claimLayer;
-		readonly ResourceClaimLayer territory;
 		readonly IPathFinder pathfinder;
 
 		readonly Func<Actor, bool> isEnemyUnit;
@@ -311,8 +310,7 @@ namespace OpenRA.Mods.Common.AI
 				return;
 
 			domainIndex = World.WorldActor.Trait<DomainIndex>();
-			resLayer = World.WorldActor.Trait<ResourceLayer>();
+			claimLayer = World.WorldActor.TraitOrDefault<ResourceClaimLayer>();
-			territory = World.WorldActor.TraitOrDefault<ResourceClaimLayer>();
 			pathfinder = World.WorldActor.Trait<IPathFinder>();
 
 			isEnemyUnit = unit =>
@@ -769,19 +767,22 @@ namespace OpenRA.Mods.Common.AI
 			return targets.MinByOrDefault(target => (target.Actor.CenterPosition - capturer.CenterPosition).LengthSquared);
 		}
 
-		CPos FindNextResource(Actor harvester)
+		CPos FindNextResource(Actor actor, Harvester harv)
 		{
-			var harvInfo = harvester.Info.TraitInfo<HarvesterInfo>();
+			var mobileInfo = actor.Info.TraitInfo<MobileInfo>();
-			var mobileInfo = harvester.Info.TraitInfo<MobileInfo>();
 			var passable = (uint)mobileInfo.GetMovementClass(World.Map.Rules.TileSet);
 
+			Func<CPos, bool> isValidResource = cell =>
+				domainIndex.IsPassable(actor.Location, cell, mobileInfo, passable) &&
+				harv.CanHarvestCell(actor, cell) &&
+				claimLayer.CanClaimCell(actor, cell);
+
 			var path = pathfinder.FindPath(
-				PathSearch.Search(World, mobileInfo, harvester, true,
+				PathSearch.Search(World, mobileInfo, actor, true, isValidResource)
-					loc => domainIndex.IsPassable(harvester.Location, loc, mobileInfo, passable) && harvester.CanHarvestAt(loc, resLayer, harvInfo, territory))
 					.WithCustomCost(loc => World.FindActorsInCircle(World.Map.CenterOfCell(loc), Info.HarvesterEnemyAvoidanceRadius)
-						.Where(u => !u.IsDead && harvester.Owner.Stances[u.Owner] == Stance.Enemy)
+						.Where(u => !u.IsDead && actor.Owner.Stances[u.Owner] == Stance.Enemy)
 						.Sum(u => Math.Max(WDist.Zero.Length, Info.HarvesterEnemyAvoidanceRadius.Length - (World.Map.CenterOfCell(loc) - u.CenterPosition).Length)))
-					.FromPoint(harvester.Location));
+					.FromPoint(actor.Location));
 
 			if (path.Count == 0)
 				return CPos.Zero;
@@ -809,7 +810,7 @@ namespace OpenRA.Mods.Common.AI
 					continue;
 
 				// Tell the idle harvester to quit slacking:
-				var newSafeResourcePatch = FindNextResource(harvester);
+				var newSafeResourcePatch = FindNextResource(harvester, harv);
 				BotDebug("AI: Harvester {0} is idle. Ordering to {1} in search for new resources.".F(harvester, newSafeResourcePatch));
 				QueueOrder(new Order("Harvest", harvester, false) { TargetLocation = newSafeResourcePatch });
 			}

OpenRA.Mods.Common/Activities/FindResources.cs

@@ -24,8 +24,7 @@ namespace OpenRA.Mods.Common.Activities
 		readonly HarvesterInfo harvInfo;
 		readonly Mobile mobile;
 		readonly MobileInfo mobileInfo;
-		readonly ResourceLayer resLayer;
+		readonly ResourceClaimLayer claimLayer;
-		readonly ResourceClaimLayer territory;
 		readonly IPathFinder pathFinder;
 		readonly DomainIndex domainIndex;
 
@@ -37,8 +36,7 @@ namespace OpenRA.Mods.Common.Activities
 			harvInfo = self.Info.TraitInfo<HarvesterInfo>();
 			mobile = self.Trait<Mobile>();
 			mobileInfo = self.Info.TraitInfo<MobileInfo>();
-			resLayer = self.World.WorldActor.Trait<ResourceLayer>();
+			claimLayer = self.World.WorldActor.Trait<ResourceClaimLayer>();
-			territory = self.World.WorldActor.TraitOrDefault<ResourceClaimLayer>();
 			pathFinder = self.World.WorldActor.Trait<IPathFinder>();
 			domainIndex = self.World.WorldActor.Trait<DomainIndex>();
 		}
@@ -90,8 +88,8 @@ namespace OpenRA.Mods.Common.Activities
 			}
 			else
 			{
-				// Attempt to claim a resource as ours
+				// Attempt to claim the target cell
-				if (territory != null && !territory.ClaimResource(self, closestHarvestablePosition.Value))
+				if (!claimLayer.TryClaimCell(self, closestHarvestablePosition.Value))
 					return ActivityUtils.SequenceActivities(new Wait(25), this);
 
 				// If not given a direct order, assume ordered to the first resource location we find:
@@ -115,7 +113,7 @@ namespace OpenRA.Mods.Common.Activities
 		/// </summary>
 		CPos? ClosestHarvestablePos(Actor self)
 		{
-			if (self.CanHarvestAt(self.Location, resLayer, harvInfo, territory))
+			if (harv.CanHarvestCell(self, self.Location) && claimLayer.CanClaimCell(self, self.Location))
 				return self.Location;
 
 			// Determine where to search from and how far to search:
@@ -126,8 +124,8 @@ namespace OpenRA.Mods.Common.Activities
 			// Find any harvestable resources:
 			var passable = (uint)mobileInfo.GetMovementClass(self.World.Map.Rules.TileSet);
 			List<CPos> path;
-			using (var search = PathSearch.Search(self.World, mobileInfo, self, true,
+			using (var search = PathSearch.Search(self.World, mobileInfo, self, true, loc =>
-				loc => domainIndex.IsPassable(self.Location, loc, mobileInfo, passable) && self.CanHarvestAt(loc, resLayer, harvInfo, territory))
+					domainIndex.IsPassable(self.Location, loc, mobileInfo, passable) && harv.CanHarvestCell(self, loc) && claimLayer.CanClaimCell(self, loc))
 				.WithCustomCost(loc =>
 				{
 					if ((avoidCell.HasValue && loc == avoidCell.Value) ||

OpenRA.Mods.Common/Activities/HarvestResource.cs

@@ -20,7 +20,7 @@ namespace OpenRA.Mods.Common.Activities
 		readonly Harvester harv;
 		readonly HarvesterInfo harvInfo;
 		readonly IFacing facing;
-		readonly ResourceClaimLayer territory;
+		readonly ResourceClaimLayer claimLayer;
 		readonly ResourceLayer resLayer;
 		readonly BodyOrientation body;
 
@@ -30,7 +30,7 @@ namespace OpenRA.Mods.Common.Activities
 			harvInfo = self.Info.TraitInfo<HarvesterInfo>();
 			facing = self.Trait<IFacing>();
 			body = self.Trait<BodyOrientation>();
-			territory = self.World.WorldActor.TraitOrDefault<ResourceClaimLayer>();
+			claimLayer = self.World.WorldActor.Trait<ResourceClaimLayer>();
 			resLayer = self.World.WorldActor.Trait<ResourceLayer>();
 		}
 
@@ -38,8 +38,7 @@ namespace OpenRA.Mods.Common.Activities
 		{
 			if (IsCanceled)
 			{
-				if (territory != null)
+				claimLayer.RemoveClaim(self);
-					territory.UnclaimByActor(self);
 				return NextActivity;
 			}
 
@@ -47,8 +46,7 @@ namespace OpenRA.Mods.Common.Activities
 
 			if (harv.IsFull)
 			{
-				if (territory != null)
+				claimLayer.RemoveClaim(self);
-					territory.UnclaimByActor(self);
 				return NextActivity;
 			}
 
@@ -64,8 +62,7 @@ namespace OpenRA.Mods.Common.Activities
 			var resource = resLayer.Harvest(self.Location);
 			if (resource == null)
 			{
-				if (territory != null)
+				claimLayer.RemoveClaim(self);
-					territory.UnclaimByActor(self);
 				return NextActivity;
 			}
 

OpenRA.Mods.Common/ActorExts.cs

@@ -122,32 +122,6 @@ namespace OpenRA.Mods.Common
 			NotifyBlocker(self, positions.SelectMany(p => self.World.ActorMap.GetActorsAt(p)));
 		}
 
-		public static bool CanHarvestAt(this Actor self, CPos pos, ResourceLayer resLayer, HarvesterInfo harvInfo,
-			ResourceClaimLayer territory)
-		{
-			// Resources only exist in the ground layer
-			if (pos.Layer != 0)
-				return false;
-
-			var resType = resLayer.GetResource(pos);
-			if (resType == null)
-				return false;
-
-			// Can the harvester collect this kind of resource?
-			if (!harvInfo.Resources.Contains(resType.Info.Type))
-				return false;
-
-			if (territory != null)
-			{
-				// Another harvester has claimed this resource:
-				ResourceClaim claim;
-				if (territory.IsClaimedByAnyoneElse(self as Actor, pos, out claim))
-					return false;
-			}
-
-			return true;
-		}
-
 		public static CPos ClosestCell(this Actor self, IEnumerable<CPos> cells)
 		{
 			return cells.MinByOrDefault(c => (self.Location - c).LengthSquared);

OpenRA.Mods.Common/OpenRA.Mods.Common.csproj

@@ -527,7 +527,6 @@
     <Compile Include="Pathfinder\BasePathSearch.cs" />
     <Compile Include="Traits\World\PlayerPaletteFromCurrentTileset.cs" />
     <Compile Include="Traits\World\RadarPings.cs" />
-    <Compile Include="Traits\World\ResourceClaim.cs" />
     <Compile Include="Traits\World\ResourceClaimLayer.cs" />
     <Compile Include="Traits\World\ResourceLayer.cs" />
     <Compile Include="Traits\World\ShroudPalette.cs" />

OpenRA.Mods.Common/Traits/Harvester.cs

@@ -78,10 +78,12 @@ namespace OpenRA.Mods.Common.Traits
 
 	public class Harvester : IIssueOrder, IResolveOrder, IPips,
 		IExplodeModifier, IOrderVoice, ISpeedModifier, ISync, INotifyCreated,
-		INotifyResourceClaimLost, INotifyIdle, INotifyBlockingMove, INotifyBuildComplete
+		INotifyIdle, INotifyBlockingMove, INotifyBuildComplete
 	{
 		public readonly HarvesterInfo Info;
 		readonly Mobile mobile;
+		readonly ResourceLayer resLayer;
+		readonly ResourceClaimLayer claimLayer;
 		Dictionary<ResourceTypeInfo, int> contents = new Dictionary<ResourceTypeInfo, int>();
 		bool idleSmart = true;
 		int idleDuration;
@@ -108,6 +110,9 @@ namespace OpenRA.Mods.Common.Traits
 		{
 			Info = info;
 			mobile = self.Trait<Mobile>();
+			resLayer = self.World.WorldActor.Trait<ResourceLayer>();
+			claimLayer = self.World.WorldActor.Trait<ResourceClaimLayer>();
+
 			self.QueueActivity(new CallFunc(() => ChooseNewProc(self, null)));
 		}
 
@@ -211,8 +216,10 @@ namespace OpenRA.Mods.Common.Traits
 
 		public void AcceptResource(ResourceType type)
 		{
-			if (!contents.ContainsKey(type.Info)) contents[type.Info] = 1;
+			if (!contents.ContainsKey(type.Info))
-			else contents[type.Info]++;
+				contents[type.Info] = 1;
+			else
+				contents[type.Info]++;
 		}
 
 		public void UnblockRefinery(Actor self)
@@ -309,6 +316,20 @@ namespace OpenRA.Mods.Common.Traits
 			return contents.Count == 0;
 		}
 
+		public bool CanHarvestCell(Actor self, CPos cell)
+		{
+			// Resources only exist in the ground layer
+			if (cell.Layer != 0)
+				return false;
+
+			var resType = resLayer.GetResource(cell);
+			if (resType == null)
+				return false;
+
+			// Can the harvester collect this kind of resource?
+			return Info.Resources.Contains(resType.Info.Type);
+		}
+
 		public IEnumerable<IOrderTargeter> Orders
 		{
 			get
@@ -354,21 +375,9 @@ namespace OpenRA.Mods.Common.Traits
 
 				CPos? loc;
 				if (order.TargetLocation != CPos.Zero)
-				{
-					loc = order.TargetLocation;
-
-					var territory = self.World.WorldActor.TraitOrDefault<ResourceClaimLayer>();
-					if (territory != null)
 				{
 					// Find the nearest claimable cell to the order location (useful for group-select harvest):
-						loc = mobile.NearestCell(loc.Value, p => mobile.CanEnterCell(p) && territory.ClaimResource(self, p), 1, 6);
+					loc = mobile.NearestCell(order.TargetLocation, p => mobile.CanEnterCell(p) && claimLayer.TryClaimCell(self, p), 1, 6);
-					}
-					else
-					{
-						// Find the nearest cell to the order location (useful for group-select harvest):
-						var taken = new HashSet<CPos>();
-						loc = mobile.NearestCell(loc.Value, p => mobile.CanEnterCell(p) && taken.Add(p), 1, 6);
-					}
 				}
 				else
 				{
@@ -423,15 +432,6 @@ namespace OpenRA.Mods.Common.Traits
 			}
 		}
 
-		public void OnNotifyResourceClaimLost(Actor self, ResourceClaim claim, Actor claimer)
-		{
-			if (self == claimer) return;
-
-			// Our claim on a resource was stolen, find more unclaimed resources:
-			self.CancelActivity();
-			self.QueueActivity(new FindResources(self));
-		}
-
 		PipType GetPipAt(int i)
 		{
 			var n = i * Info.Capacity / Info.PipCount;

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

@@ -1,21 +0,0 @@
-#region Copyright & License Information
-/*
- * Copyright 2007-2017 The OpenRA Developers (see AUTHORS)
- * This file is part of OpenRA, which is free software. It is made
- * available to you under the terms of the GNU General Public License
- * as published by the Free Software Foundation, either version 3 of
- * the License, or (at your option) any later version. For more
- * information, see COPYING.
- */
-#endregion
-
-namespace OpenRA.Mods.Common.Traits
-{
-	public sealed class ResourceClaim
-	{
-		public readonly Actor Claimer;
-		public CPos Cell;
-
-		public ResourceClaim(Actor claimer, CPos cell) { Claimer = claimer; Cell = cell; }
-	}
-}

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

@@ -10,6 +10,7 @@
 #endregion
 
 using System.Collections.Generic;
+using System.Linq;
 using OpenRA.Graphics;
 using OpenRA.Traits;
 
@@ -18,112 +19,55 @@ namespace OpenRA.Mods.Common.Traits
 	[Desc("Allows harvesters to coordinate their operations. Attach this to the world actor.")]
 	public sealed class ResourceClaimLayerInfo : TraitInfo<ResourceClaimLayer> { }
 
-	public sealed class ResourceClaimLayer : IWorldLoaded
+	public sealed class ResourceClaimLayer
 	{
-		Dictionary<CPos, ResourceClaim> claimByCell;
+		readonly Dictionary<CPos, List<Actor>> claimByCell = new Dictionary<CPos, List<Actor>>(32);
-		Dictionary<Actor, ResourceClaim> claimByActor;
+		readonly Dictionary<Actor, CPos> claimByActor = new Dictionary<Actor, CPos>(32);
-
-		private void MakeClaim(Actor claimer, CPos cell)
-		{
-			UnclaimByActor(claimer);
-			UnclaimByCell(cell, claimer);
-			claimByActor[claimer] = claimByCell[cell] = new ResourceClaim(claimer, cell);
-		}
-
-		private void Unclaim(ResourceClaim claim, Actor claimer)
-		{
-			if (claimByActor.Remove(claim.Claimer) & claimByCell.Remove(claim.Cell))
-			{
-				if (claim.Claimer.Disposed) return;
-				if (!claim.Claimer.IsInWorld) return;
-				if (claim.Claimer.IsDead) return;
-
-				claim.Claimer.Trait<INotifyResourceClaimLost>().OnNotifyResourceClaimLost(claim.Claimer, claim, claimer);
-			}
-		}
-
-		public void WorldLoaded(World w, WorldRenderer wr)
-		{
-			// 32 seems a sane default initial capacity for the total # of harvesters in a game. Purely a guesstimate.
-			claimByCell = new Dictionary<CPos, ResourceClaim>(32);
-			claimByActor = new Dictionary<Actor, ResourceClaim>(32);
-		}
 
 		/// <summary>
-		/// Attempt to claim the resource at the cell for the given actor.
+		/// Attempt to reserve the resource in a cell for the given actor.
 		/// </summary>
-		/// <param name="claimer"></param>
+		public bool TryClaimCell(Actor claimer, CPos cell)
-		/// <param name="cell"></param>
-		/// <returns></returns>
-		public bool ClaimResource(Actor claimer, CPos cell)
 		{
-			// Has anyone else claimed this point?
+			var claimers = claimByCell.GetOrAdd(cell);
-			ResourceClaim claim;
-			if (claimByCell.TryGetValue(cell, out claim))
-			{
-				// Same claimer:
-				if (claim.Claimer == claimer) return true;
 
-				// This is to prevent in-fighting amongst friendly harvesters:
+			// Clean up any stale claims
-				if (claimer.Owner == claim.Claimer.Owner) return false;
+			claimers.RemoveAll(a => a.IsDead);
-				if (claimer.Owner.Stances[claim.Claimer.Owner] == Stance.Ally) return false;
 
-				// If an enemy/neutral claimed this, don't respect that claim:
+			// Prevent harvesters from the player or their allies fighting over the same cell
-			}
+			if (claimers.Any(c => c != claimer && claimer.Owner.IsAlliedWith(c.Owner)))
+				return false;
 
-			// Either nobody else claims this point or an enemy/neutral claims it:
+			// Remove the actor's last claim, if it has one
-			MakeClaim(claimer, cell);
+			CPos lastClaim;
+			if (claimByActor.TryGetValue(claimer, out lastClaim))
+				claimByCell.GetOrAdd(lastClaim).Remove(claimer);
+
+			claimers.Add(claimer);
+			claimByActor[claimer] = cell;
 			return true;
 		}
 
 		/// <summary>
-		/// Release the last resource claim made on this cell.
+		/// Returns false if the cell is already reserved by an allied actor.
 		/// </summary>
-		/// <param name="cell"></param>
+		public bool CanClaimCell(Actor claimer, CPos cell)
-		public void UnclaimByCell(CPos cell, Actor claimer)
 		{
-			ResourceClaim claim;
+			return !claimByCell.GetOrAdd(cell)
-			if (claimByCell.TryGetValue(cell, out claim))
+				.Any(c => c != claimer && !c.IsDead && claimer.Owner.IsAlliedWith(c.Owner));
-				Unclaim(claim, claimer);
 		}
 
 		/// <summary>
 		/// Release the last resource claim made by this actor.
 		/// </summary>
 		/// <param name="claimer"></param>
-		public void UnclaimByActor(Actor claimer)
+		public void RemoveClaim(Actor claimer)
 		{
-			ResourceClaim claim;
+			CPos lastClaim;
-			if (claimByActor.TryGetValue(claimer, out claim))
+			if (claimByActor.TryGetValue(claimer, out lastClaim))
-				Unclaim(claim, claimer);
+				claimByCell.GetOrAdd(lastClaim).Remove(claimer);
-		}
 
-		/// <summary>
+			claimByActor.Remove(claimer);
-		/// Is the cell location <paramref name="cell"/> claimed for harvesting by any other actor?
-		/// </summary>
-		/// <param name="self"></param>
-		/// <param name="cell"></param>
-		/// <returns>true if already claimed by an ally that isn't <paramref name="self"/>; false otherwise.</returns>
-		public bool IsClaimedByAnyoneElse(Actor self, CPos cell, out ResourceClaim claim)
-		{
-			if (claimByCell.TryGetValue(cell, out claim))
-			{
-				// Same claimer:
-				if (claim.Claimer == self) return false;
-
-				// This is to prevent in-fighting amongst friendly harvesters:
-				if (self.Owner == claim.Claimer.Owner) return true;
-				if (self.Owner.Stances[claim.Claimer.Owner] == Stance.Ally) return true;
-
-				// If an enemy/neutral claimed this, don't respect that claim and fall through:
-			}
-			else
-			{
-				// No claim.
-				claim = null;
-			}
-
-			return false;
 		}
 	}
 }

OpenRA.Mods.Common/TraitsInterfaces.cs

@@ -28,11 +28,6 @@ namespace OpenRA.Mods.Common.Traits
 		int QuantizedBodyFacings(ActorInfo ai, SequenceProvider sequenceProvider, string race);
 	}
 
-	public interface INotifyResourceClaimLost
-	{
-		void OnNotifyResourceClaimLost(Actor self, ResourceClaim claim, Actor claimer);
-	}
-
 	public interface IPlaceBuildingDecorationInfo : ITraitInfo
 	{
 		IEnumerable<IRenderable> Render(WorldRenderer wr, World w, ActorInfo ai, WPos centerPosition);