Clean up DuplicateUnitCrateAction.

OpenRA.Mods.RA/Crates/DuplicateUnitCrateAction.cs

@@ -8,6 +8,7 @@
  */
 #endregion
 
+using System;
 using System.Collections.Generic;
 using System.Linq;
 using OpenRA.Mods.RA.Move;
@@ -25,11 +26,14 @@ namespace OpenRA.Mods.RA.Crates
 		[Desc("The minimum number of duplicates to make. Overrules MaxDuplicatesWorth.")]
 		public readonly int MinAmount = 1;
 
-		[Desc("The maximum total cost allowed for the duplicates.", "Duplication stops if the total worth will exceed this number.", "-1 = no limit")]
+		[Desc("The maximum total value allowed for the duplicates.", "Duplication stops if the total worth will exceed this number.", "-1 = no limit")]
-		public readonly int MaxDuplicatesWorth = -1;
+		public readonly int MaxDuplicateValue = -1;
 
-		[Desc("The list of unit types we are allowed to duplicate.")]
+		[Desc("The maximum radius (in cells) that duplicates can be spawned.")]
-		public readonly string[] ValidDuplicateTypes = { "Ground", "Water" };
+		public readonly int MaxRadius = 4;
+
+		[Desc("The list of unit target types we are allowed to duplicate.")]
+		public readonly string[] ValidTargets = { "Ground", "Water" };
 
 		[Desc("Which races this crate action can occur for.")]
 		public readonly string[] ValidRaces = { };
@@ -43,13 +47,10 @@ namespace OpenRA.Mods.RA.Crates
 	class DuplicateUnitCrateAction : CrateAction
 	{
 		readonly DuplicateUnitCrateActionInfo info;
-		readonly Actor self;
-		readonly List<CPos> usedCells = new List<CPos>();
 
 		public DuplicateUnitCrateAction(Actor self, DuplicateUnitCrateActionInfo info)
 			: base(self, info)
 		{
-			this.self = self;
 			this.info = info;
 		}
 
@@ -59,13 +60,15 @@ namespace OpenRA.Mods.RA.Crates
 				return false;
 
 			var targetable = collector.Info.Traits.GetOrDefault<ITargetableInfo>();
-			if (targetable == null || !info.ValidDuplicateTypes.Intersect(targetable.GetTargetTypes()).Any())
+			if (targetable == null || !info.ValidTargets.Intersect(targetable.GetTargetTypes()).Any())
 				return false;
 
-			if (!GetSuitableCells(collector.Location, collector.Info.Name).Any())
+			var positionable = collector.TraitOrDefault<IPositionable>();
+			if (positionable == null)
 				return false;
 
-			return true;
+			return collector.World.Map.FindTilesInCircle(collector.Location, info.MaxRadius)
+				.Any(c => positionable.CanEnterCell(c));
 		}
 
 		public override int GetSelectionShares(Actor collector)
@@ -78,52 +81,32 @@ namespace OpenRA.Mods.RA.Crates
 
 		public override void Activate(Actor collector)
 		{
-			var allowedWorthLeft = info.MaxDuplicatesWorth;
+			var positionable = collector.Trait<IPositionable>();
-			var dupesMade = 0;
+			var candidateCells = collector.World.Map.FindTilesInCircle(collector.Location, info.MaxRadius)
+				.Where(c => positionable.CanEnterCell(c)).Shuffle(collector.World.SharedRandom)
+				.ToArray();
 
-			while ((dupesMade < info.MaxAmount && allowedWorthLeft > 0) || dupesMade < info.MinAmount)
+			var duplicates = Math.Min(candidateCells.Length, info.MaxAmount);
+
+			// Restrict duplicate count to a maximum value
+			if (info.MaxDuplicateValue > 0)
 			{
-				// If the collector has a cost, and we have a max duplicate worth, then update how much dupe worth is left
+				var vi = collector.Info.Traits.GetOrDefault<ValuedInfo>();
-				var unitCost = collector.Info.Traits.Get<ValuedInfo>().Cost;
+				if (vi != null && vi.Cost > 0)
-				allowedWorthLeft -= info.MaxDuplicatesWorth > 0 ? unitCost : 0;
+					duplicates = Math.Min(duplicates, info.MaxDuplicateValue / vi.Cost);
-				if (allowedWorthLeft < 0 && dupesMade >= info.MinAmount)
+			}
-					break;
 
-				dupesMade++;
+			for (var i = 0; i < duplicates; i++)
-
+			{
-				var location = ChooseEmptyCellNear(collector, collector.Info.Name);
+				var cell = candidateCells[i]; // Avoid modified closure bug
-				if (location != null)
+				collector.World.AddFrameEndTask(w => w.CreateActor(collector.Info.Name, new TypeDictionary
 				{
-					usedCells.Add(location.Value);
+					new LocationInit(cell),
-					collector.World.AddFrameEndTask(
+					new OwnerInit(info.Owner ?? collector.Owner.InternalName)
-					w => w.CreateActor(collector.Info.Name, new TypeDictionary
+				}));
-					{
-						new LocationInit(location.Value),
-						new OwnerInit(info.Owner ?? collector.Owner.InternalName)
-					}));
-				}
 			}
 
 			base.Activate(collector);
 		}
-
-		IEnumerable<CPos> GetSuitableCells(CPos near, string unitName)
-		{
-			var mi = self.World.Map.Rules.Actors[unitName].Traits.Get<MobileInfo>();
-
-			for (var i = -3; i < 4; i++)
-				for (var j = -3; j < 4; j++)
-					if (mi.CanEnterCell(self.World, self, near + new CVec(i, j)))
-						yield return near + new CVec(i, j);
-		}
-
-		CPos? ChooseEmptyCellNear(Actor a, string unit)
-		{
-			var possibleCells = GetSuitableCells(a.Location, unit).Where(c => !usedCells.Contains(c)).ToArray();
-			if (possibleCells.Length == 0)
-				return null;
-
-			return possibleCells.Random(self.World.SharedRandom);
-		}
 	}
 }

OpenRA.Utility/UpgradeRules.cs

@@ -517,6 +517,16 @@ namespace OpenRA.Utility
 					}
 				}
 
+				// DuplicateUnitCrateAction was tidied up
+				if (engineVersion < 20140912)
+				{
+					if (depth == 2 && node.Key == "MaxDuplicatesWorth" && parentKey == "DuplicateUnitCrateAction")
+						node.Key = "MaxDuplicateValue";
+
+					if (depth == 2 && node.Key == "ValidDuplicateTypes" && parentKey == "DuplicateUnitCrateAction")
+						node.Key = "ValidTargets";
+				}
+
 				UpgradeActorRules(engineVersion, ref node.Value.Nodes, node, depth + 1);
 			}
 		}

mods/cnc/rules/misc.yaml

@@ -22,7 +22,7 @@ CRATE:
 		SelectionShares: 10
 		MaxAmount: 5
 		MinAmount: 1
-		MaxDuplicatesWorth: 1250
+		MaxDuplicateValue: 1250
 	GiveMcvCrateAction:
 		SelectionShares: 0
 		NoBaseSelectionShares: 120

mods/ra/rules/misc.yaml

@@ -93,7 +93,7 @@ CRATE:
 		SelectionShares: 10
 		MaxAmount: 5
 		MinAmount: 1
-		MaxDuplicatesWorth: 1500
+		MaxDuplicateValue: 1500
 	GiveMcvCrateAction:
 		SelectionShares: 2
 		NoBaseSelectionShares: 100