battleCalculator #1 (#12816)

issue 12688 (battle calc on own territory shows own player as attacker and defender)
Desired:
1. Attacker: Current player always, except when only allied units present (then next enemy)
2. Defender: As enemy to attacker with most units (fallback: whomever is at war and has the next turn)

AttackerAndDefenderSelectorTest.java
- simplification of method getAttackerAndDefender()
- new methods getBestAttacker() and getBestAttacker()
- removed now unused methods getOpponentWithPriorityList, playersAtWarWith, neutralPlayersTowards, getEnemyWithMostUnits

UnitCollection.java
- rename method getPlayersByUnitCount to getPlayersSortedByUnitCount

BattleCalculatorDialog.java
- extract common logic from method addAttackers and addDefenders to new method adjustBattleCalculatorPanel
- extract static attribute modifications from instance method dispose into new static method disposeInstance (especially pack also for adding attacker)

BattleCalculatorPanel.java
- introduce new methods setAttackerWithUnits and setDefenderWithUnits to limit attacker/defender change to own instance
- make setter of attribute attacker/defender private
- rename methods hasAttackingUnitsAdded and hasDefendingUnitsAdded to hasAttackingUnits and hasDefendingUnits
- rename attribute attackerUnitsTotalHitpoints to attackerUnitsTotalHitPoints

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

@@ -162,7 +162,7 @@ public IntegerMap<GamePlayer> getPlayerUnitCounts() {
     return count;
   }
 
-  public List<GamePlayer> getPlayersByUnitCount() {
+  public List<GamePlayer> getPlayersSortedByUnitCount() {
     final IntegerMap<GamePlayer> map = getPlayerUnitCounts();
     final List<GamePlayer> players = new ArrayList<>(map.keySet());
     players.sort(Comparator.comparingInt(map::getInt).reversed());

game-app/game-core/src/main/java/games/strategy/triplea/odds/calculator/AttackerAndDefenderSelector.java

@@ -4,13 +4,11 @@
 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.ai.pro.util.ProUtils;
 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;
 import lombok.AccessLevel;
@@ -54,44 +52,24 @@ public Optional<GamePlayer> getDefender() {
   @Nonnull private final RelationshipTracker relationshipTracker;
   @Nullable private final Territory territory;
 
-  /**
-   * Set initial attacker and defender.
-   *
-   * <p>Please read the source code for the order of the players and conditions involved.
-   */
+  /** Set initial attacker as current player and defender according to priority. */
   public AttackerAndDefender getAttackerAndDefender() {
-    // If there is no current player, we cannot choose an opponent.
-    if (currentPlayer == null) {
+
+    final GamePlayer attacker = getBestAttacker();
+    if (attacker == null) {
       return AttackerAndDefender.NONE;
     }
 
-    if (territory == null) {
-      // Without territory, we cannot prioritize any players (except current player); no units to
-      // select.
-      return getAttackerAndDefenderWithPriorityList(List.of(currentPlayer));
+    // determine potential defenders (sub set of all players)
+    final GamePlayer defender = getBestDefender(attacker);
+    if (defender == null) {
+      return AttackerAndDefender.NONE;
     }
-    // Select the defender to be an enemy of the current player if possible, preferring enemies
-    // in the given territory. When deciding for an enemy, usually a player with more units is
-    // more important and more likely to be meant, e.g. a territory with 10 units of player A and
-    // 1 unit of player B. Thus, we use lists and ordered streams.
-    final List<GamePlayer> playersWithUnits = territory.getUnitCollection().getPlayersByUnitCount();
-    // Add the territory owner add the end of the priority list.  This way, when attacking an
-    // empty territory, the owner gets preferred even though they have no units in their land. In
-    // case the owner has units in the land, then they are already in the list but adding a second
-    // entry to the list doesn't impact the algorithm.
-    final GamePlayer territoryOwner = territory.getOwner();
-    if (!territoryOwner.isNull()) {
-      playersWithUnits.add(territoryOwner);
+    if (territory == null) {
+      return AttackerAndDefender.builder().attacker(attacker).defender(defender).build();
     }
-
-    final GamePlayer attacker = currentPlayer;
-    // Attacker fights alone; the defender can also use all the allied units.
-    final GamePlayer defender =
-        getOpponentWithPriorityList(territory, attacker, playersWithUnits).orElse(null);
     final List<Unit> attackingUnits = territory.getMatches(Matches.unitIsOwnedBy(attacker));
-    final List<Unit> defendingUnits =
-        defender == null ? List.of() : territory.getMatches(Matches.alliedUnit(defender));
-
+    final List<Unit> defendingUnits = territory.getMatches(Matches.alliedUnit(defender));
     return AttackerAndDefender.builder()
         .attacker(attacker)
         .defender(defender)
@@ -101,131 +79,90 @@ public AttackerAndDefender getAttackerAndDefender() {
   }
 
   /**
-   * First pick an attacker and then a suitable defender while prioritising players in {@code
-   * priorityPlayers}. 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.
+   * Returns the "best" attacker, i.e. the current player or next enemy when there are only allied
+   * units.
    *
-   * <p>The attacker is picked with following priorities
-   *
-   * <ol>
-   *   <li>the players in {@code priorityPlayers} (in that order)
-   *   <li>any player
-   * </ol>
-   *
-   * <p>The defender is chosen with the following priorities
-   *
-   * <ol>
-   *   <li>the first player in {@code priorityPlayers} who is an enemy of the attacker
-   *   <li>any enemy of the attacker
-   *   <li>the first player {@code priorityPlayers} who is neutral towards the attacker
-   *   <li>any neutral player (with respect to the attacker)
-   *   <li>any player
-   * </ol>
-   *
-   * <p>If the game has no players, empty optionals are returned.
-   *
-   * @param priorityPlayers an ordered list of players which should be considered first
-   * @return attacker and defender
+   * @return best attacker
    */
-  private AttackerAndDefender getAttackerAndDefenderWithPriorityList(
-      final List<GamePlayer> priorityPlayers) {
-    // Attacker
-    final GamePlayer attacker =
-        Stream.of(priorityPlayers.stream(), players.stream())
-            .flatMap(s -> s)
+  @Nullable
+  private GamePlayer getBestAttacker() {
+    if (currentPlayer == null) {
+      return null;
+    }
+    if (territory != null) {
+      final Collection<Unit> units = territory.getUnits();
+      if (!units.isEmpty()
+          && units.stream().map(Unit::getOwner).allMatch(Matches.isAllied(currentPlayer))) {
+        return ProUtils.getEnemyPlayersInTurnOrder(currentPlayer).stream()
             .findFirst()
-            .orElse(null);
-    if (attacker == null) {
-      return AttackerAndDefender.NONE;
+            .orElseThrow();
+      }
     }
-    // Defender
-    final GamePlayer defender =
-        getOpponentWithPriorityList(territory, attacker, priorityPlayers).orElse(null);
-    return AttackerAndDefender.builder().attacker(attacker).defender(defender).build();
+    return currentPlayer;
   }
 
   /**
-   * Return a suitable opponent for player {@code p} with players in {@code priorityPlayers} given
-   * 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.
+   * Returns the "best" defender, i.e. an enemy of the current player. If possible, prefers enemies
+   * in the given territory with the most units.
    *
-   * <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}
-   *   <li>any enemy of {@code p}
-   *   <li>the first player {@code priorityPlayers} who is neutral towards {@code p}
-   *   <li>any neutral player (with respect to {@code p})
-   *   <li>any player
-   * </ol>
-   *
-   * @param player the player to find an opponent for
-   * @param priorityPlayers an ordered list of players which should be considered first
-   * @return an opponent. An empty optional is returned if the game has no players
+   * @return best defender
    */
-  private Optional<GamePlayer> getOpponentWithPriorityList(
-      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;
-          }
-        }
-      }
+  @Nullable
+  private GamePlayer getBestDefender(final GamePlayer attacker) {
+    assert currentPlayer
+        != null; // should not occur checked in calling method getAttackerAndDefender
+
+    final List<GamePlayer> potentialDefenders = ProUtils.getEnemyPlayers(attacker);
+    if (potentialDefenders.isEmpty()) {
+      return null;
     }
-    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,
-            neutralPlayersTowards(player),
-            players.stream())
-        .flatMap(s -> s)
-        .findFirst();
-  }
+    // determine next player after current player who could be a defender
+    final GamePlayer nextPlayerAsDefender =
+        ProUtils.getOtherPlayersInTurnOrder(attacker).stream()
+            .filter(potentialDefenders::contains)
+            .findFirst()
+            .orElseThrow();
 
-  private @Nullable GamePlayer getEnemyWithMostUnits(Territory territory) {
-    return UnitUtils.findPlayerWithMostUnits(
-        territory.getUnits().stream()
-            .filter(Matches.unitIsEnemyOf(currentPlayer))
-            .collect(Collectors.toList()));
-  }
+    if (territory == null) {
+      // Without territory, we cannot prioritize defender by number of units
+      return nextPlayerAsDefender;
+    }
 
-  /**
-   * Returns a stream of all players which are at war with player {@code p}.
-   *
-   * <p>The returned stream might be empty.
-   */
-  private Stream<GamePlayer> playersAtWarWith(final GamePlayer p) {
-    return players.stream().filter(Matches.isAtWar(p));
+    return getBestDefenderFromTerritory(potentialDefenders, nextPlayerAsDefender);
   }
 
-  /**
-   * Returns a stream of all players which are neither allied nor at war with player {@code p}.
-   *
-   * <p>The returned stream might be empty.
-   */
-  private Stream<GamePlayer> neutralPlayersTowards(final GamePlayer p) {
-    return players.stream()
-        .filter(Matches.isAtWar(p).negate())
-        .filter(Matches.isAllied(p).negate());
+  private GamePlayer getBestDefenderFromTerritory(
+      final List<GamePlayer> potentialDefenders, final GamePlayer nextPlayerAsDefender) {
+    // Select the defender to be an enemy of the current player, if possible, prefer enemies
+    // in the given territory.
+    assert territory != null; // should not occur as checked in calling method getBestDefender
+
+    final GamePlayer territoryOwner = territory.getOwner();
+    if (!territoryOwner.isNull() && potentialDefenders.contains(territoryOwner)) {
+      // In case the owner is a potential defender and has units in the land it's the defender
+      if (territory.getUnits().stream()
+          .map(Unit::getOwner)
+          .filter(territoryOwner::equals)
+          .findAny()
+          .isPresent()) {
+        return territoryOwner;
+      }
+    }
+
+    // When deciding for an enemy, usually a player having more units is
+    // more important and more likely to be meant, e.g. a territory with 10 units of player A
+    // and 1 unit of player B. Thus, we use lists and ordered streams.
+    final List<GamePlayer> sortedPlayersForDefender =
+        territory.getUnitCollection().getPlayersSortedByUnitCount();
+
+    // Add the territory owner at the end of the priority list. This way, when attacking an
+    // empty territory, the owner gets preferred even though they have no units in their land.
+    sortedPlayersForDefender.add(territoryOwner);
+
+    sortedPlayersForDefender.removeIf(p -> !potentialDefenders.contains(p));
+    sortedPlayersForDefender.add(nextPlayerAsDefender);
+
+    // Attacker fights alone for this selector; the defender can also use all the allied units.
+    return sortedPlayersForDefender.get(0);
   }
 }