Fix stacking limit logic for placement dialog. (#12000)

The logic to filter out which units should be shown in the place dialog was too aggressive and was resulting in filtering out a unit of type B when it shared a limit with a unit of type A. This change relaxes that logic specifically for the placement dialog case and adds test coverage.

Also moves a helper function to UnitUtils.

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

@@ -5,6 +5,7 @@
 import games.strategy.engine.data.GamePlayer;
 import games.strategy.engine.data.Territory;
 import games.strategy.engine.data.Unit;
+import games.strategy.engine.data.UnitType;
 import games.strategy.engine.data.changefactory.ChangeFactory;
 import games.strategy.engine.data.properties.GameProperties;
 import games.strategy.triplea.attachments.TerritoryAttachment;
@@ -14,7 +15,9 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.function.Predicate;
+import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 import lombok.experimental.UtilityClass;
 import org.triplea.java.collections.CollectionUtils;
@@ -29,6 +32,10 @@
 @UtilityClass
 public class UnitUtils {
 
+  public static Set<UnitType> getUnitTypesFromUnitList(final Collection<Unit> units) {
+    return units.stream().map(Unit::getType).collect(Collectors.toSet());
+  }
+
   public static int getProductionPotentialOfTerritory(
       final Collection<Unit> unitsAtStartOfStepInTerritory,
       final Territory producer,
@@ -151,7 +158,7 @@ public static int getHowMuchCanUnitProduce(
             (Properties.getWW2V2(properties) || Properties.getWW2V3(properties)) ? 0 : 1;
       }
     }
-    // Increase production if have industrial technology
+    // Increase production if we have industrial technology
     if (territoryProduction
         >= techTracker.getMinimumTerritoryValueForProductionBonus(unit.getOwner())) {
       productionCapacity += techTracker.getProductionBonus(unit.getOwner(), unit.getType());

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

@@ -18,6 +18,7 @@
 import games.strategy.engine.data.properties.GameProperties;
 import games.strategy.triplea.Constants;
 import games.strategy.triplea.Properties;
+import games.strategy.triplea.UnitUtils;
 import games.strategy.triplea.delegate.Matches;
 import games.strategy.triplea.delegate.TechTracker;
 import games.strategy.triplea.delegate.TerritoryEffectHelper;
@@ -259,10 +260,6 @@ public static UnitAttachment get(final UnitType type, final String nameOfAttachm
     return getAttachment(type, nameOfAttachment, UnitAttachment.class);
   }
 
-  private static Collection<UnitType> getUnitTypesFromUnitList(final Collection<Unit> units) {
-    return units.stream().map(Unit::getType).collect(Collectors.toSet());
-  }
-
   private TechTracker getTechTracker() {
     return getData().getTechTracker();
   }
@@ -1161,7 +1158,7 @@ private static IntegerMap<Tuple<String, String>> getReceivesAbilityWhenWithMap(
       final UnitTypeList unitTypeList) {
     final IntegerMap<Tuple<String, String>> map = new IntegerMap<>();
     final Collection<UnitType> canReceive =
-        getUnitTypesFromUnitList(
+        UnitUtils.getUnitTypesFromUnitList(
             CollectionUtils.getMatches(units, Matches.unitCanReceiveAbilityWhenWith()));
     for (final UnitType ut : canReceive) {
       final Collection<String> receives = ut.getUnitAttachment().getReceivesAbilityWhenWith();

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

@@ -38,7 +38,6 @@
 import java.util.Map.Entry;
 import java.util.Objects;
 import java.util.function.Predicate;
-import java.util.stream.Collectors;
 import java.util.stream.IntStream;
 import org.triplea.java.collections.CollectionUtils;
 import org.triplea.java.collections.IntegerMap;
@@ -801,6 +800,15 @@ public String canUnitsBePlaced(
     if (allowedUnits == null || !allowedUnits.containsAll(units)) {
       return "Cannot place these units in " + to.getName();
     }
+    // Although getUnitsToBePlaced() has checked stacking limits, it did it on a per-unit type
+    // basis, which is not sufficient, since units may be mutually exclusive. So we need to also
+    // check stacking limits over the full collection.
+    Collection<Unit> filteredUnits =
+        UnitStackingLimitFilter.filterUnits(
+            units, PLACEMENT_LIMIT, player, to, produced.getOrDefault(to, List.of()));
+    if (units.size() != filteredUnits.size()) {
+      return "Cannot place these units in " + to.getName();
+    }
     final IntegerMap<String> constructionMap =
         howManyOfEachConstructionCanPlace(to, to, units, player);
     for (final Unit currentUnit : CollectionUtils.getMatches(units, Matches.unitIsConstruction())) {
@@ -976,13 +984,27 @@ protected Collection<Unit> getUnitsToBePlaced(
       placeableUnits2 = placeableUnits;
     }
     // Limit count of each unit type to the max that can be placed based on unit requirements.
-    for (UnitType ut : placeableUnits2.stream().map(Unit::getType).collect(Collectors.toSet())) {
+    for (UnitType ut : UnitUtils.getUnitTypesFromUnitList(placeableUnits)) {
       var unitsOfType = CollectionUtils.getMatches(placeableUnits2, Matches.unitIsOfType(ut));
       placeableUnits2.removeAll(getUnitsThatCantBePlacedThatRequireUnits(unitsOfType, to));
     }
     // now check stacking limits
-    return UnitStackingLimitFilter.filterUnits(
-        placeableUnits2, PLACEMENT_LIMIT, player, to, produced.getOrDefault(to, List.of()));
+    // Filter each type separately, since we don't want a max on one type to filter out all units of
+    // another type, if the two types have a combined limit. UnitStackingLimitFilter doesn't do
+    // that directly since other call sites (e.g. move validation) do need the combined filtering.
+    // But we need to do it this way here, since the result will be shown for choosing which units
+    // to build (where we want to show all the types that can be built).
+    final var result = new ArrayList<Unit>();
+    for (UnitType ut : UnitUtils.getUnitTypesFromUnitList(units)) {
+      result.addAll(
+          UnitStackingLimitFilter.filterUnits(
+              CollectionUtils.getMatches(placeableUnits2, Matches.unitIsOfType(ut)),
+              PLACEMENT_LIMIT,
+              player,
+              to,
+              produced.getOrDefault(to, List.of())));
+    }
+    return result;
   }
 
   private Predicate<Unit> unitIsCarrierOwnedByCombinedPlayers(GamePlayer player) {

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

@@ -16,6 +16,7 @@
 import games.strategy.engine.data.UnitTypeList;
 import games.strategy.engine.data.properties.GameProperties;
 import games.strategy.triplea.Properties;
+import games.strategy.triplea.UnitUtils;
 import games.strategy.triplea.attachments.AbstractUserActionAttachment;
 import games.strategy.triplea.attachments.ICondition;
 import games.strategy.triplea.attachments.PlayerAttachment;
@@ -220,8 +221,7 @@ public static Predicate<Unit> unitCanParticipateInCombat(
       Territory battleSite,
       int battleRound,
       Collection<Unit> enemyUnits) {
-    final Set<UnitType> enemyUnitTypes =
-        enemyUnits.stream().map(Unit::getType).collect(Collectors.toSet());
+    final Collection<UnitType> enemyUnitTypes = UnitUtils.getUnitTypesFromUnitList(enemyUnits);
     return u -> {
       final boolean landBattle = !battleSite.isWater();
       if (!landBattle && Matches.unitIsLand().test(u)) {

game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/steps/fire/general/TargetGroup.java

@@ -3,6 +3,7 @@
 import com.google.common.collect.Sets;
 import games.strategy.engine.data.Unit;
 import games.strategy.engine.data.UnitType;
+import games.strategy.triplea.UnitUtils;
 import games.strategy.triplea.delegate.Matches;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -43,10 +44,9 @@ public Collection<Unit> getTargetUnits(final Collection<Unit> units) {
    */
   public static List<TargetGroup> newTargetGroups(
       final Collection<Unit> units, final Collection<Unit> enemyUnits) {
-    final Set<UnitType> unitTypes = units.stream().map(Unit::getType).collect(Collectors.toSet());
+    final Set<UnitType> unitTypes = UnitUtils.getUnitTypesFromUnitList(units);
     final boolean destroyerPresent = unitTypes.stream().anyMatch(Matches.unitTypeIsDestroyer());
-    final Set<UnitType> enemyUnitTypes =
-        enemyUnits.stream().map(Unit::getType).collect(Collectors.toSet());
+    final Set<UnitType> enemyUnitTypes = UnitUtils.getUnitTypesFromUnitList(enemyUnits);
     final List<TargetGroup> targetGroups = new ArrayList<>();
     for (final UnitType unitType : unitTypes) {
       final Set<UnitType> targets = findTargets(unitType, destroyerPresent, enemyUnitTypes);

game-app/game-core/src/test/java/games/strategy/triplea/delegate/PlaceDelegateTest.java

@@ -8,6 +8,7 @@
 import static games.strategy.triplea.delegate.remote.IAbstractPlaceDelegate.BidMode.NOT_BID;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.hasSize;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
@@ -228,6 +229,17 @@ void testPlayerAttachmentStackingLimit() {
     // but not 2 battleships and 2 carriers
     units.addAll(create(british, carrier, 1));
     assertError(delegate.canUnitsBePlaced(northSea, units, british));
+    // However, getPlaceableUnits() should return 2 of each, since that's what's for filtering the
+    // options given to the user.
+    assertThat(
+        delegate.getPlaceableUnits(units, northSea).getUnits(),
+        containsInAnyOrder(units.toArray()));
+    units.addAll(create(british, carrier, 5));
+    units.addAll(create(british, battleship, 7));
+    var result = delegate.getPlaceableUnits(units, northSea).getUnits();
+    assertThat(result, hasSize(6));
+    assertThat(CollectionUtils.getMatches(result, Matches.unitIsOfType(battleship)), hasSize(3));
+    assertThat(CollectionUtils.getMatches(result, Matches.unitIsOfType(carrier)), hasSize(3));
   }
 
   @Test
@@ -237,7 +249,7 @@ void testStackingLimitFilteringHappensAfterPlacementRestrictions() {
     // Add a carrier to the sea zone.
     westCanadaSeaZone.getUnitCollection().addAll(create(british, carrier, 1));
 
-    // if we filter list of 2 battleships and 2 carriers, the 2 carriers should be selected.
+    // If we filter list of 2 battleships and 2 carriers, the 2 carriers should be selected.
     List<Unit> units = create(british, battleship, 2);
     units.addAll(create(british, carrier, 2));
     // First, we can't place all of them (expected).

game-app/game-core/src/test/java/games/strategy/triplea/delegate/move/validation/UnitStackingLimitFilterTest.java

@@ -55,7 +55,7 @@ void testUnitAttachmentStackingLimit() {
     assertThat(callFilterUnits(fourTanks, british, uk), hasSize(2));
 
     // but we can include other units that don't have stacking limits in the list
-    // note: still with the two tanks already in the uk
+    // note: still with the two tanks already in the UK
     List<Unit> twoInfantryAndFourTanks = infantry.create(2, british);
     twoInfantryAndFourTanks.addAll(fourTanks);
     assertThat(twoInfantryAndFourTanks, hasSize(6));