Improve logic for choosing default players in battle calc. (#11924)

This attempts to address some regressions from 2.5 where in a number of cases, the chosen sides did not correspond to what the user likely wanted. See #11664 for details. Some additional tests are added, while others are updated to clarify their intentions and in some cases, provide updated expectations.
triplea-game · Aug 31, 2023 · c9610e5 · c9610e5
1 parent b8122ac
commit c9610e5
Show file tree

Hide file tree

Showing 7 changed files with 501 additions and 360 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
@@ -15,6 +15,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.function.Predicate;
+import javax.annotation.Nullable;
 import lombok.experimental.UtilityClass;
 import org.triplea.java.collections.CollectionUtils;
 import org.triplea.java.collections.IntegerMap;
@@ -241,4 +242,21 @@ private static CompositeChange translateHitPointsAndDamageToOtherUnit(
     }
     return unitChange;
   }
+
+  public static @Nullable GamePlayer findPlayerWithMostUnits(final Iterable<Unit> units) {
+    final IntegerMap<GamePlayer> playerUnitCount = new IntegerMap<>();
+    for (final Unit unit : units) {
+      playerUnitCount.add(unit.getOwner(), 1);
+    }
+    int max = -1;
+    GamePlayer player = null;
+    for (final GamePlayer current : playerUnitCount.keySet()) {
+      final int count = playerUnitCount.getInt(current);
+      if (count > max) {
+        max = count;
+        player = current;
+      }
+    }
+    return player;
+  }
 }
diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/AbstractBattle.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/AbstractBattle.java
@@ -315,23 +315,6 @@ static GamePlayer findDefender(
     return defender;
   }
 
-  static GamePlayer findPlayerWithMostUnits(final Collection<Unit> units) {
-    final IntegerMap<GamePlayer> playerUnitCount = new IntegerMap<>();
-    for (final Unit unit : units) {
-      playerUnitCount.add(unit.getOwner(), 1);
-    }
-    int max = -1;
-    GamePlayer player = null;
-    for (final GamePlayer current : playerUnitCount.keySet()) {
-      final int count = playerUnitCount.getInt(current);
-      if (count > max) {
-        max = count;
-        player = current;
-      }
-    }
-    return player;
-  }
-
   void markDamaged(final Collection<Unit> damaged, final IDelegateBridge bridge) {
     BattleDelegate.markDamaged(damaged, bridge, battleSite);
   }

diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/BattleDelegate.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/BattleDelegate.java
@@ -24,6 +24,7 @@
 import games.strategy.engine.player.Player;
 import games.strategy.engine.random.IRandomStats.DiceType;
 import games.strategy.triplea.Properties;
+import games.strategy.triplea.UnitUtils;
 import games.strategy.triplea.attachments.PlayerAttachment;
 import games.strategy.triplea.attachments.TerritoryAttachment;
 import games.strategy.triplea.attachments.UnitAttachment;
@@ -625,7 +626,7 @@ private static void setupTerritoriesAbandonedToTheEnemy(
     for (final Territory territory : battleTerritories) {
       final List<Unit> abandonedToUnits =
           territory.getUnitCollection().getMatches(Matches.enemyUnit(player));
-      final GamePlayer abandonedToPlayer = AbstractBattle.findPlayerWithMostUnits(abandonedToUnits);
+      final GamePlayer abandonedToPlayer = UnitUtils.findPlayerWithMostUnits(abandonedToUnits);
 
       // now make sure to add any units that must move with these units, so that they get included
       // as dependencies

diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/MustFightBattle.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/MustFightBattle.java
@@ -28,6 +28,7 @@
 import games.strategy.engine.history.change.units.TransformDamagedUnitsHistoryChange;
 import games.strategy.engine.player.Player;
 import games.strategy.triplea.Properties;
+import games.strategy.triplea.UnitUtils;
 import games.strategy.triplea.delegate.ExecutionStack;
 import games.strategy.triplea.delegate.IExecutable;
 import games.strategy.triplea.delegate.Matches;
@@ -1368,7 +1369,7 @@ private void defenderWins(final IDelegateBridge bridge) {
             battleSite.getUnitCollection().getMatches(Matches.unitIsNotInfrastructure());
         if (!allyOfAttackerUnits.isEmpty()) {
           final GamePlayer abandonedToPlayer =
-              AbstractBattle.findPlayerWithMostUnits(allyOfAttackerUnits);
+              UnitUtils.findPlayerWithMostUnits(allyOfAttackerUnits);
           bridge
               .getHistoryWriter()
               .addChildToEvent(

diff --git a/...ore/src/main/java/games/strategy/triplea/odds/calculator/AttackerAndDefenderSelector.java b/...ore/src/main/java/games/strategy/triplea/odds/calculator/AttackerAndDefenderSelector.java
@@ -4,10 +4,12 @@
 import games.strategy.engine.data.RelationshipTracker;
 import games.strategy.engine.data.Territory;
 import games.strategy.engine.data.Unit;
+import games.strategy.triplea.UnitUtils;
 import games.strategy.triplea.delegate.Matches;
 import java.util.Collection;
 import java.util.List;
 import java.util.Optional;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
@@ -33,10 +35,10 @@ public static class AttackerAndDefender {
     /** NONE = No attacker, no defender and no units. */
     public static final AttackerAndDefender NONE = AttackerAndDefender.builder().build();
 
-    @Nullable private final GamePlayer attacker;
-    @Nullable private final GamePlayer defender;
-    @Builder.Default private final List<Unit> attackingUnits = List.of();
-    @Builder.Default private final List<Unit> defendingUnits = List.of();
+    @Nullable GamePlayer attacker;
+    @Nullable GamePlayer defender;
+    @Builder.Default List<Unit> attackingUnits = List.of();
+    @Builder.Default List<Unit> defendingUnits = List.of();
 
     public Optional<GamePlayer> getAttacker() {
       return Optional.ofNullable(attacker);
@@ -82,10 +84,11 @@ public AttackerAndDefender getAttackerAndDefender() {
       if (!territoryOwner.isNull()) {
         playersWithUnits.add(territoryOwner);
       }
+
       final GamePlayer attacker = currentPlayer;
       // Attacker fights alone; the defender can also use all the allied units.
       final GamePlayer defender =
-          getOpponentWithPriorityList(attacker, playersWithUnits).orElse(null);
+          getOpponentWithPriorityList(territory, attacker, playersWithUnits).orElse(null);
       final List<Unit> attackingUnits =
           territory.getUnitCollection().getMatches(Matches.unitIsOwnedBy(attacker));
       final List<Unit> defendingUnits =
@@ -94,7 +97,7 @@ public AttackerAndDefender getAttackerAndDefender() {
               : territory.getUnitCollection().getMatches(Matches.alliedUnit(defender));
 
       return AttackerAndDefender.builder()
-          .attacker(currentPlayer)
+          .attacker(attacker)
           .defender(defender)
           .attackingUnits(attackingUnits)
           .defendingUnits(defendingUnits)
@@ -141,7 +144,8 @@ private AttackerAndDefender getAttackerAndDefenderWithPriorityList(
       return AttackerAndDefender.NONE;
     }
     // Defender
-    final GamePlayer defender = getOpponentWithPriorityList(attacker, priorityPlayers).orElse(null);
+    final GamePlayer defender =
+        getOpponentWithPriorityList(territory, attacker, priorityPlayers).orElse(null);
     return AttackerAndDefender.builder().attacker(attacker).defender(defender).build();
   }
 
@@ -150,7 +154,8 @@ private AttackerAndDefender getAttackerAndDefenderWithPriorityList(
    * priority. The order in {@code priorityPlayers} determines the priority for those players
    * included in that list. Players not in the list are at the bottom without any order.
    *
-   * <p>The opponent is chosen with the following priorities
+   * <p>Some additional prioritisation is given based on the territory owner and players with units.
+   * Otherwise, the opponent is chosen with the following priorities
    *
    * <ol>
    *   <li>the first player in {@code priorityPlayers} who is an enemy of {@code p}
@@ -165,14 +170,34 @@ private AttackerAndDefender getAttackerAndDefenderWithPriorityList(
    * @return an opponent. An empty optional is returned if the game has no players
    */
   private Optional<GamePlayer> getOpponentWithPriorityList(
-      final GamePlayer player, final List<GamePlayer> priorityPlayers) {
+      Territory territory, final GamePlayer player, final List<GamePlayer> priorityPlayers) {
+    GamePlayer bestDefender = null;
+    // Handle some special cases that the priority ordering logic doesn't handle. See tests.
+    if (territory != null) {
+      if (territory.isWater()) {
+        bestDefender = getEnemyWithMostUnits(territory);
+        if (bestDefender == null) {
+          bestDefender = UnitUtils.findPlayerWithMostUnits(territory.getUnits());
+        }
+      } else {
+        bestDefender = territory.getOwner();
+        // If we're not at war with the owner and there are enemies, fight them.
+        if (!bestDefender.isAtWar(currentPlayer)) {
+          GamePlayer enemyWithMostUnits = getEnemyWithMostUnits(territory);
+          if (enemyWithMostUnits != null) {
+            bestDefender = enemyWithMostUnits;
+          }
+        }
+      }
+    }
     final Stream<GamePlayer> enemiesPriority =
         priorityPlayers.stream().filter(Matches.isAtWar(player));
     final Stream<GamePlayer> neutralsPriority =
         priorityPlayers.stream()
             .filter(Matches.isAtWar(player).negate())
             .filter(Matches.isAllied(player).negate());
     return Stream.of(
+            Optional.ofNullable(bestDefender).stream(),
             enemiesPriority,
             playersAtWarWith(player),
             neutralsPriority,
@@ -182,6 +207,13 @@ private Optional<GamePlayer> getOpponentWithPriorityList(
         .findFirst();
   }
 
+  private @Nullable GamePlayer getEnemyWithMostUnits(Territory territory) {
+    return UnitUtils.findPlayerWithMostUnits(
+        territory.getUnits().stream()
+            .filter(Matches.unitIsEnemyOf(currentPlayer))
+            .collect(Collectors.toList()));
+  }
+
   /**
    * Returns a stream of all players which are at war with player {@code p}.
    *

diff --git a/game-app/game-core/src/test/java/games/strategy/triplea/delegate/GameDataTestUtil.java b/game-app/game-core/src/test/java/games/strategy/triplea/delegate/GameDataTestUtil.java
@@ -57,7 +57,7 @@ public static GamePlayer germany(final GameState data) {
    *
    * @return A italian PlayerId.
    */
-  static GamePlayer italians(final GameState data) {
+  public static GamePlayer italians(final GameState data) {
     return data.getPlayerList().getPlayerId(Constants.PLAYER_NAME_ITALIANS);
   }
 
@@ -105,6 +105,10 @@ public static GamePlayer britain(final GameState data) {
     return data.getPlayerList().getPlayerId("Britain");
   }
 
+  public static GamePlayer french(final GameState data) {
+    return data.getPlayerList().getPlayerId(Constants.PLAYER_NAME_FRENCH);
+  }
+
   /**
    * Get the japanese PlayerId for the given GameData object.
    *