Fix: Apply PlayerAttachment stack limits when placing units. (#11826)

With this change, these restrictions are checked before showing the dialog of which units to place in a territory.
This way, invalid placements will be omitted from the dialog already, rather than showing up and causing an error when selected.

Tested on the "1941 Global Command Decision" map, where all the factory types have a stacking limit of 1 per territory. Before the change, if you build them and try to place units in a territory with a factory already, the factory would show up in the dialog. With the change, it will be omitted. (On this map, the presence of the factory in the list of units to place would also show an increased max for how many units can be placed for non-factory units - e.g. being able to select 3 infantry to place in a territory that only allows 2.)

This change refactors the code to move the stacking limits logic to a new UnitStackingLimitFilter class, combining the logic that was previously split between two places behind a better API that allows simplification of all the call sites.
Additional test coverage is added, both for the new class and for the PlaceDelegate, the latter also covering the sequence of the different calls (i.e. that placement restrictions are checked before stacking limit filtering).

Additionally, one call site no longer needed to do the checks as it was already doing the validation indirectly through another function it was calling.

The cleanup removes several hundred lines of code, although many lines of test code are also added,

game-app/game-core/src/main/java/games/strategy/engine/data/UnitCollection.java

@@ -226,19 +226,36 @@ public <T> T[] toArray(final T[] array) {
 
   @Override
   public boolean remove(final Object object) {
-    final boolean result = units.remove(object);
-    holder.notifyChanged();
-    return result;
+    final boolean changed = units.remove(object);
+    if (changed) {
+      holder.notifyChanged();
+    }
+    return changed;
+  }
+
+  @Override
+  public boolean removeIf(final Predicate<? super Unit> predicate) {
+    final boolean changed = units.removeIf(predicate);
+    if (changed) {
+      holder.notifyChanged();
+    }
+    return changed;
   }
 
   @Override
   public boolean retainAll(final Collection<?> collection) {
-    return units.retainAll(collection);
+    final boolean changed = units.retainAll(collection);
+    if (changed) {
+      holder.notifyChanged();
+    }
+    return changed;
   }
 
   @Override
   public void clear() {
-    units.clear();
-    holder.notifyChanged();
+    if (!units.isEmpty()) {
+      units.clear();
+      holder.notifyChanged();
+    }
   }
 }

game-app/game-core/src/main/java/games/strategy/triplea/attachments/PlayerAttachment.java

@@ -7,19 +7,12 @@
 import games.strategy.engine.data.GameState;
 import games.strategy.engine.data.MutableProperty;
 import games.strategy.engine.data.Resource;
-import games.strategy.engine.data.Territory;
-import games.strategy.engine.data.Unit;
 import games.strategy.engine.data.UnitType;
 import games.strategy.engine.data.gameparser.GameParseException;
-import games.strategy.triplea.delegate.Matches;
-import java.util.ArrayList;
-import java.util.Collection;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
-import java.util.function.Predicate;
 import javax.annotation.Nullable;
-import org.triplea.java.collections.CollectionUtils;
 import org.triplea.java.collections.IntegerMap;
 import org.triplea.util.Triple;
 
@@ -93,7 +86,7 @@ private void setPlacementLimit(final Set<Triple<Integer, String, Set<UnitType>>>
     placementLimit = value;
   }
 
-  private Set<Triple<Integer, String, Set<UnitType>>> getPlacementLimit() {
+  public Set<Triple<Integer, String, Set<UnitType>>> getPlacementLimit() {
     return getSetProperty(placementLimit);
   }
 
@@ -112,7 +105,7 @@ private void setMovementLimit(final Set<Triple<Integer, String, Set<UnitType>>>
     movementLimit = value;
   }
 
-  private Set<Triple<Integer, String, Set<UnitType>>> getMovementLimit() {
+  public Set<Triple<Integer, String, Set<UnitType>>> getMovementLimit() {
     return getSetProperty(movementLimit);
   }
 
@@ -131,7 +124,7 @@ private void setAttackingLimit(final Set<Triple<Integer, String, Set<UnitType>>>
     attackingLimit = value;
   }
 
-  private Set<Triple<Integer, String, Set<UnitType>>> getAttackingLimit() {
+  public Set<Triple<Integer, String, Set<UnitType>>> getAttackingLimit() {
     return getSetProperty(attackingLimit);
   }
 
@@ -165,64 +158,6 @@ private Triple<Integer, String, Set<UnitType>> parseUnitLimit(
     return Triple.of(max, s[1].intern(), types);
   }
 
-  /**
-   * Returns {@code true} if the specified units can move into the specified territory without
-   * violating the specified stacking limit (movement, attack, or placement).
-   */
-  public static boolean getCanTheseUnitsMoveWithoutViolatingStackingLimit(
-      final String limitType,
-      final Collection<Unit> unitsMoving,
-      final Territory toMoveInto,
-      final GamePlayer owner) {
-    final PlayerAttachment pa = PlayerAttachment.get(owner);
-    if (pa == null) {
-      return true;
-    }
-    final Set<Triple<Integer, String, Set<UnitType>>> stackingLimits;
-    switch (limitType) {
-      case "movementLimit":
-        stackingLimits = pa.getMovementLimit();
-        break;
-      case "attackingLimit":
-        stackingLimits = pa.getAttackingLimit();
-        break;
-      case "placementLimit":
-        stackingLimits = pa.getPlacementLimit();
-        break;
-      default:
-        throw new IllegalStateException("Invalid limitType: " + limitType);
-    }
-    if (stackingLimits.isEmpty()) {
-      return true;
-    }
-    final Predicate<Unit> notOwned = Matches.unitIsOwnedBy(owner).negate();
-    final Predicate<Unit> notAllied = Matches.alliedUnit(owner).negate();
-    for (final Triple<Integer, String, Set<UnitType>> currentLimit : stackingLimits) {
-      // first make a copy of unitsMoving
-      final Collection<Unit> copyUnitsMoving = new ArrayList<>(unitsMoving);
-      final Collection<Unit> currentInTerritory = new ArrayList<>(toMoveInto.getUnits());
-      final String type = currentLimit.getSecond();
-      // first remove units that do not apply to our current type
-      if (type.equals("owned")) {
-        currentInTerritory.removeAll(CollectionUtils.getMatches(currentInTerritory, notOwned));
-        copyUnitsMoving.removeAll(CollectionUtils.getMatches(copyUnitsMoving, notOwned));
-      } else if (type.equals("allied")) {
-        currentInTerritory.removeAll(CollectionUtils.getMatches(currentInTerritory, notAllied));
-        copyUnitsMoving.removeAll(CollectionUtils.getMatches(copyUnitsMoving, notAllied));
-      }
-      // now remove units that are not part of our list
-      final Predicate<Unit> matchesUnits = Matches.unitIsOfTypes(currentLimit.getThird());
-      currentInTerritory.retainAll(CollectionUtils.getMatches(currentInTerritory, matchesUnits));
-      copyUnitsMoving.retainAll(CollectionUtils.getMatches(copyUnitsMoving, matchesUnits));
-      // now test
-      final Integer max = currentLimit.getFirst();
-      if (max < (currentInTerritory.size() + copyUnitsMoving.size())) {
-        return false;
-      }
-    }
-    return true;
-  }
-
   private void setSuicideAttackTargets(final String value) throws GameParseException {
     suicideAttackTargets = parseUnitTypes("suicideAttackTargets", value, suicideAttackTargets);
   }

game-app/game-core/src/main/java/games/strategy/triplea/attachments/UnitAttachment.java

@@ -2488,7 +2488,7 @@ private void setPlacementLimit(final Tuple<Integer, String> value) {
     placementLimit = value;
   }
 
-  private @Nullable Tuple<Integer, String> getPlacementLimit() {
+  public @Nullable Tuple<Integer, String> getPlacementLimit() {
     return placementLimit;
   }
 
@@ -2548,61 +2548,6 @@ public void resetCanRetreatOnStalemate() {
     canRetreatOnStalemate = null;
   }
 
-  /**
-   * Returns the maximum number of units of the specified type that can be placed in the specified
-   * territory according to the specified stacking limit (movement, attack, or placement).
-   *
-   * @return {@link Integer#MAX_VALUE} if there is no stacking limit for the specified conditions.
-   */
-  public static int getMaximumNumberOfThisUnitTypeToReachStackingLimit(
-      final String limitType, final UnitType ut, final Territory t, final GamePlayer owner) {
-    final UnitAttachment ua = ut.getUnitAttachment();
-    final GameProperties properties = t.getData().getProperties();
-    final Tuple<Integer, String> stackingLimit;
-    switch (limitType) {
-      case "movementLimit":
-        stackingLimit = ua.getMovementLimit();
-        break;
-      case "attackingLimit":
-        stackingLimit = ua.getAttackingLimit();
-        break;
-      case "placementLimit":
-        stackingLimit = ua.getPlacementLimit();
-        break;
-      default:
-        throw new IllegalStateException(
-            "getMaximumNumberOfThisUnitTypeToReachStackingLimit does not allow limitType: "
-                + limitType);
-    }
-    if (stackingLimit == null) {
-      return Integer.MAX_VALUE;
-    }
-    int max = stackingLimit.getFirst();
-    // under certain rules (classic rules) there can only be 1 aa gun in a territory.
-    if (max == Integer.MAX_VALUE
-        && (ua.getIsAaForBombingThisUnitOnly() || ua.getIsAaForCombatOnly())
-        && !(Properties.getWW2V2(properties)
-            || Properties.getWW2V3(properties)
-            || Properties.getMultipleAaPerTerritory(properties))) {
-      max = 1;
-    }
-    final Predicate<Unit> stackingMatch;
-    final String stackingType = stackingLimit.getSecond();
-    switch (stackingType) {
-      case "owned":
-        stackingMatch = Matches.unitIsOfType(ut).and(Matches.unitIsOwnedBy(owner));
-        break;
-      case "allied":
-        stackingMatch = Matches.unitIsOfType(ut).and(Matches.isUnitAllied(owner));
-        break;
-      default:
-        stackingMatch = Matches.unitIsOfType(ut);
-        break;
-    }
-    final int totalInTerritory = CollectionUtils.countMatches(t.getUnits(), stackingMatch);
-    return Math.max(0, max - totalInTerritory);
-  }
-
   @Override
   public void validate(final GameState data) throws GameParseException {
     if (isAir) {
@@ -3498,22 +3443,32 @@ private void addAaDescription(
     }
   }
 
+  public int getStackingLimitMax(final Tuple<Integer, String> stackingLimit) {
+    int max = stackingLimit.getFirst();
+    if (max != Integer.MAX_VALUE) {
+      return max;
+    }
+    // under certain rules (classic rules) there can only be 1 aa gun in a territory.
+    final GameProperties properties = getData().getProperties();
+    if ((getIsAaForBombingThisUnitOnly() || getIsAaForCombatOnly())
+        && !(Properties.getWW2V2(properties)
+            || Properties.getWW2V3(properties)
+            || Properties.getMultipleAaPerTerritory(properties))) {
+      max = 1;
+    }
+    return max;
+  }
+
   private void addStackingLimitDescription(
       final Tuple<Integer, String> stackingLimit,
       final String description,
       final Formatter formatter) {
     if (stackingLimit != null) {
-      if (stackingLimit.getFirst() == Integer.MAX_VALUE
-          && (getIsAaForBombingThisUnitOnly() || getIsAaForCombatOnly())
-          && !(Properties.getWW2V2(getData().getProperties())
-              || Properties.getWW2V3(getData().getProperties())
-              || Properties.getMultipleAaPerTerritory(getData().getProperties()))) {
-        formatter.append(
-            "Max " + stackingLimit.getSecond() + " Units " + description + " per Territory", "1");
-      } else if (stackingLimit.getFirst() < 10000) {
+      int max = getStackingLimitMax(stackingLimit);
+      if (max < 10000) {
         formatter.append(
             "Max " + stackingLimit.getSecond() + " Units " + description + " per Territory",
-            String.valueOf(stackingLimit.getFirst()));
+            String.valueOf(max));
       }
     }
   }

game-app/game-core/src/main/java/games/strategy/triplea/delegate/AbstractPlaceDelegate.java

@@ -1,5 +1,7 @@
 package games.strategy.triplea.delegate;
 
+import static games.strategy.triplea.delegate.move.validation.UnitStackingLimitFilter.PLACEMENT_LIMIT;
+
 import games.strategy.engine.data.Change;
 import games.strategy.engine.data.CompositeChange;
 import games.strategy.engine.data.GameData;
@@ -21,6 +23,7 @@
 import games.strategy.triplea.delegate.battle.BattleTracker;
 import games.strategy.triplea.delegate.data.PlaceableUnits;
 import games.strategy.triplea.delegate.move.validation.AirMovementValidator;
+import games.strategy.triplea.delegate.move.validation.UnitStackingLimitFilter;
 import games.strategy.triplea.delegate.remote.IAbstractPlaceDelegate;
 import games.strategy.triplea.formatter.MyFormatter;
 import java.io.Serializable;
@@ -835,32 +838,13 @@ public String canUnitsBePlaced(
       }
       // make sure all units are land
       if (units.isEmpty() || !units.stream().allMatch(Matches.unitIsNotSea())) {
-        return "Cant place sea units on land";
+        return "Can't place sea units on land";
       }
     }
     // make sure we can place consuming units
     if (!canWeConsumeUnits(units, to, false, null)) {
       return "Not Enough Units To Upgrade or Be Consumed";
     }
-    // now check for stacking limits
-    final Collection<UnitType> typesAlreadyChecked = new ArrayList<>();
-    for (final Unit currentUnit : units) {
-      final UnitType ut = currentUnit.getType();
-      if (typesAlreadyChecked.contains(ut)) {
-        continue;
-      }
-      typesAlreadyChecked.add(ut);
-      final int maxForThisType =
-          UnitAttachment.getMaximumNumberOfThisUnitTypeToReachStackingLimit(
-              "placementLimit", ut, to, player);
-      if (CollectionUtils.countMatches(units, Matches.unitIsOfType(ut)) > maxForThisType) {
-        return "UnitType " + ut.getName() + " is over stacking limit of " + maxForThisType;
-      }
-    }
-    if (!PlayerAttachment.getCanTheseUnitsMoveWithoutViolatingStackingLimit(
-        "placementLimit", units, to, player)) {
-      return "Units Cannot Go Over Stacking Limit";
-    }
     // now return null (valid placement) if we have placement restrictions disabled in game options
     if (!Properties.getUnitPlacementRestrictions(getData().getProperties())) {
       return null;
@@ -971,46 +955,34 @@ protected Collection<Unit> getUnitsToBePlaced(
         }
       }
     }
-    // now check stacking limits
-    final Collection<Unit> placeableUnits2 = new ArrayList<>();
-    final var typesAlreadyChecked = new HashSet<UnitType>();
-    for (final Unit currentUnit : placeableUnits) {
-      final UnitType ut = currentUnit.getType();
-      if (typesAlreadyChecked.contains(ut)) {
-        continue;
-      }
-      typesAlreadyChecked.add(ut);
-      int max =
-          UnitAttachment.getMaximumNumberOfThisUnitTypeToReachStackingLimit(
-              "placementLimit", ut, to, player);
-      placeableUnits2.addAll(
-          CollectionUtils.getNMatches(placeableUnits, max, Matches.unitIsOfType(ut)));
-    }
-    if (!Properties.getUnitPlacementRestrictions(properties)) {
-      return placeableUnits2;
-    }
-    final Collection<Unit> placeableUnits3 = new ArrayList<>();
-    for (final Unit currentUnit : placeableUnits2) {
-      final UnitAttachment ua = currentUnit.getUnitAttachment();
-      // Can be null!
-      final TerritoryAttachment ta = TerritoryAttachment.get(to);
-      if (ua.getCanOnlyBePlacedInTerritoryValuedAtX() != -1
-          && ua.getCanOnlyBePlacedInTerritoryValuedAtX() > (ta == null ? 0 : ta.getProduction())) {
-        continue;
-      }
-      if (unitWhichRequiresUnitsHasRequiredUnits(to, false).negate().test(currentUnit)) {
-        continue;
-      }
-      if (Matches.unitCanOnlyPlaceInOriginalTerritories().test(currentUnit)
-          && !Matches.territoryIsOriginallyOwnedBy(player).test(to)) {
-        continue;
-      }
-      // account for any unit placement restrictions by territory
-      if (!ua.unitPlacementRestrictionsContain(to)) {
-        placeableUnits3.add(currentUnit);
+    final Collection<Unit> placeableUnits2;
+    if (Properties.getUnitPlacementRestrictions(properties)) {
+      final int territoryProduction = TerritoryAttachment.getProduction(to);
+      placeableUnits2 = new ArrayList<>();
+      for (final Unit currentUnit : placeableUnits) {
+        final UnitAttachment ua = currentUnit.getUnitAttachment();
+        if (ua.getCanOnlyBePlacedInTerritoryValuedAtX() != -1
+            && ua.getCanOnlyBePlacedInTerritoryValuedAtX() > territoryProduction) {
+          continue;
+        }
+        if (unitWhichRequiresUnitsHasRequiredUnits(to, false).negate().test(currentUnit)) {
+          continue;
+        }
+        if (Matches.unitCanOnlyPlaceInOriginalTerritories().test(currentUnit)
+            && !Matches.territoryIsOriginallyOwnedBy(player).test(to)) {
+          continue;
+        }
+        // account for any unit placement restrictions by territory
+        if (!ua.unitPlacementRestrictionsContain(to)) {
+          placeableUnits2.add(currentUnit);
+        }
       }
+    } else {
+      placeableUnits2 = placeableUnits;
     }
-    return placeableUnits3;
+    // now check stacking limits
+    return UnitStackingLimitFilter.filterUnits(
+        placeableUnits2, PLACEMENT_LIMIT, player, to, produced.getOrDefault(to, List.of()));
   }
 
   private Predicate<Unit> unitIsCarrierOwnedByCombinedPlayers(GamePlayer player) {