From 2568c5f982764524d5a91ba3b783bc0c8f82d803 Mon Sep 17 00:00:00 2001 From: asvitkine Date: Fri, 29 Sep 2023 13:02:04 -0400 Subject: [PATCH] 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. --- .../games/strategy/triplea/UnitUtils.java | 9 +++++- .../triplea/attachments/UnitAttachment.java | 7 ++--- .../delegate/AbstractPlaceDelegate.java | 30 ++++++++++++++++--- .../strategy/triplea/delegate/Matches.java | 4 +-- .../steps/fire/general/TargetGroup.java | 6 ++-- .../triplea/delegate/PlaceDelegateTest.java | 14 ++++++++- .../UnitStackingLimitFilterTest.java | 2 +- 7 files changed, 55 insertions(+), 17 deletions(-) diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/UnitUtils.java b/game-app/game-core/src/main/java/games/strategy/triplea/UnitUtils.java index a52a8eb29e8..92a49d8af89 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/UnitUtils.java +++ b/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 getUnitTypesFromUnitList(final Collection units) { + return units.stream().map(Unit::getType).collect(Collectors.toSet()); + } + public static int getProductionPotentialOfTerritory( final Collection 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()); diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/attachments/UnitAttachment.java b/game-app/game-core/src/main/java/games/strategy/triplea/attachments/UnitAttachment.java index 1d95388c04f..428648d9e47 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/attachments/UnitAttachment.java +++ b/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 getUnitTypesFromUnitList(final Collection units) { - return units.stream().map(Unit::getType).collect(Collectors.toSet()); - } - private TechTracker getTechTracker() { return getData().getTechTracker(); } @@ -1161,7 +1158,7 @@ private static IntegerMap> getReceivesAbilityWhenWithMap( final UnitTypeList unitTypeList) { final IntegerMap> map = new IntegerMap<>(); final Collection canReceive = - getUnitTypesFromUnitList( + UnitUtils.getUnitTypesFromUnitList( CollectionUtils.getMatches(units, Matches.unitCanReceiveAbilityWhenWith())); for (final UnitType ut : canReceive) { final Collection receives = ut.getUnitAttachment().getReceivesAbilityWhenWith(); diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/AbstractPlaceDelegate.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/AbstractPlaceDelegate.java index 1b6f8703bcd..47ec1c9ffb8 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/AbstractPlaceDelegate.java +++ b/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 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 constructionMap = howManyOfEachConstructionCanPlace(to, to, units, player); for (final Unit currentUnit : CollectionUtils.getMatches(units, Matches.unitIsConstruction())) { @@ -976,13 +984,27 @@ protected Collection 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(); + 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 unitIsCarrierOwnedByCombinedPlayers(GamePlayer player) { diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/Matches.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/Matches.java index 19012f8dabc..cde85482a3a 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/Matches.java +++ b/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 unitCanParticipateInCombat( Territory battleSite, int battleRound, Collection enemyUnits) { - final Set enemyUnitTypes = - enemyUnits.stream().map(Unit::getType).collect(Collectors.toSet()); + final Collection enemyUnitTypes = UnitUtils.getUnitTypesFromUnitList(enemyUnits); return u -> { final boolean landBattle = !battleSite.isWater(); if (!landBattle && Matches.unitIsLand().test(u)) { diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/steps/fire/general/TargetGroup.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/steps/fire/general/TargetGroup.java index 9bca73303ef..d62cac79150 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/steps/fire/general/TargetGroup.java +++ b/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 getTargetUnits(final Collection units) { */ public static List newTargetGroups( final Collection units, final Collection enemyUnits) { - final Set unitTypes = units.stream().map(Unit::getType).collect(Collectors.toSet()); + final Set unitTypes = UnitUtils.getUnitTypesFromUnitList(units); final boolean destroyerPresent = unitTypes.stream().anyMatch(Matches.unitTypeIsDestroyer()); - final Set enemyUnitTypes = - enemyUnits.stream().map(Unit::getType).collect(Collectors.toSet()); + final Set enemyUnitTypes = UnitUtils.getUnitTypesFromUnitList(enemyUnits); final List targetGroups = new ArrayList<>(); for (final UnitType unitType : unitTypes) { final Set targets = findTargets(unitType, destroyerPresent, enemyUnitTypes); diff --git a/game-app/game-core/src/test/java/games/strategy/triplea/delegate/PlaceDelegateTest.java b/game-app/game-core/src/test/java/games/strategy/triplea/delegate/PlaceDelegateTest.java index 215f00fbab4..724754e63df 100644 --- a/game-app/game-core/src/test/java/games/strategy/triplea/delegate/PlaceDelegateTest.java +++ b/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 units = create(british, battleship, 2); units.addAll(create(british, carrier, 2)); // First, we can't place all of them (expected). diff --git a/game-app/game-core/src/test/java/games/strategy/triplea/delegate/move/validation/UnitStackingLimitFilterTest.java b/game-app/game-core/src/test/java/games/strategy/triplea/delegate/move/validation/UnitStackingLimitFilterTest.java index 0469090d7b4..a378536a32a 100644 --- a/game-app/game-core/src/test/java/games/strategy/triplea/delegate/move/validation/UnitStackingLimitFilterTest.java +++ b/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 twoInfantryAndFourTanks = infantry.create(2, british); twoInfantryAndFourTanks.addAll(fourTanks); assertThat(twoInfantryAndFourTanks, hasSize(6));