Fix logic for damage units selection for 3hp units.

game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/casualty/CasualtySelector.java

@@ -156,9 +156,11 @@ public static CasualtyDetails selectCasualties(
       }
     }
 
+    // Prefer units with less movement left to be killed first.
     casualtyDetails.ensureUnitsAreKilledFirst(
         sortedTargetsToPickFrom, Matches.unitIsAir(), Comparator.comparing(Unit::getMovementLeft));
 
+    // Prefer units with most movement left for damage (to have a better chance to get to safety).
     casualtyDetails.ensureUnitsAreDamagedFirst(
         sortedTargetsToPickFrom,
         Matches.unitIsAir(),

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

@@ -113,17 +113,15 @@ public void ensureUnitsAreDamagedFirst(
       final Collection<Unit> targets,
       final Predicate<Unit> matcher,
       final Comparator<Unit> shouldTakeHitsFirst) {
-
     final Map<UnitOwner, List<Unit>> targetsGroupedByOwnerAndType =
         targets.stream().collect(Collectors.groupingBy(UnitOwner::new, Collectors.toList()));
 
-    final List<Unit> targetsHitWithCorrectOrder = new ArrayList<>();
-
     final Map<UnitOwner, List<Unit>> oldTargetsToTakeHits =
         getDamaged().stream()
             .filter(matcher)
             .collect(Collectors.groupingBy(UnitOwner::new, Collectors.toList()));
 
+    final List<Unit> targetsHitWithCorrectOrder = new ArrayList<>();
     for (final Map.Entry<UnitOwner, List<Unit>> oldTargetsOfOneOwnerAndType :
         oldTargetsToTakeHits.entrySet()) {
       final List<Unit> allTargetsOfOwnerAndTypeThatCanTakeHits =
@@ -134,13 +132,12 @@ public void ensureUnitsAreDamagedFirst(
           allTargetsOfOwnerAndTypeThatCanTakeHits,
           shouldTakeHitsFirst,
           targetsHitWithCorrectOrder);
+      // Note: Although removeAll() removes all duplicates entries, it's it's not a problem in this
+      // case given how we're iterating through units above.
+      damaged.removeAll(oldTargetsOfOneOwnerAndType.getValue());
     }
 
-    damaged.addAll(
-        targetsHitWithCorrectOrder.stream()
-            .filter(unit -> !damaged.contains(unit))
-            .collect(Collectors.toList()));
-    damaged.removeIf(matcher.and(not(targetsHitWithCorrectOrder::contains)));
+    damaged.addAll(targetsHitWithCorrectOrder);
   }
 
   /**
@@ -165,13 +162,27 @@ private static void redistributeHits(
     // collect the hits that are currently distributed to oldTargetsOfOneOwnerAndType
     int hitsToRedistributeToUnit;
     final Iterator<Unit> unitIterator = targets.iterator();
-    for (int hitsToRedistribute = targetsWithHitsBeforeRedistribution.size();
-        hitsToRedistribute > 0;
-        hitsToRedistribute -= hitsToRedistributeToUnit) {
-      final Unit unit = unitIterator.next();
+    // Don't change how many units take 1 hit vs. 2 hits by counting how many hits each unit takes
+    // and re-assigning at that level.
+    final Iterator<Long> numberOfHitsPerUnitIterator =
+        targetsWithHitsBeforeRedistribution.stream()
+            .collect(Collectors.groupingBy(e -> e, Collectors.counting()))
+            .values()
+            .stream()
+            .sorted(Comparator.reverseOrder()) // Sort in descending order
+            .collect(Collectors.toList())
+            .iterator();
+    int hitsToRedistribute = 0;
+    while (numberOfHitsPerUnitIterator.hasNext() || hitsToRedistribute > 0) {
+      if (numberOfHitsPerUnitIterator.hasNext()) {
+        hitsToRedistribute += numberOfHitsPerUnitIterator.next();
+      }
 
+      final Unit unit = unitIterator.next();
       hitsToRedistributeToUnit =
           Math.min(unit.hitsUnitCanTakeHitWithoutBeingKilled(), hitsToRedistribute);
+      // Note: Since the above may result in fewer hits assigned, keep track of the remainder.
+      hitsToRedistribute -= hitsToRedistributeToUnit;
 
       for (int i = 0; i < hitsToRedistributeToUnit; ++i) {
         targetsHitWithCorrectOrder.add(unit);

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

@@ -218,7 +218,7 @@ void keepDamageAtUnitsAlreadyDamaged() {
     casualtyDetails.ensureUnitsAreDamagedFirst(
         units, Matches.unitIsAir(), Comparator.comparing(Unit::getMovementLeft).reversed());
 
-    // The first and third air unit have one movement left so they should be damaged.
+    // Unit 1 should be kept as the damaged one, as unit 0 has only 1 hp left.
     assertThat(casualtyDetails.getDamaged()).containsExactly(units.get(1));
     assertThat(units.get(0).getHits()).isEqualTo(1);
     assertThat(units.get(1).getHits()).isEqualTo(0);
@@ -263,4 +263,31 @@ void killNegativeMarineBonusFirstIfAmphibious() {
     // first.
     assertThat(casualtyDetails.getKilled()).containsExactly(units.get(1));
   }
+
+  @Test
+  void damagedUnitsWithThreeHitPoints() {
+    final UnitType fighter = givenUnitType("fighter");
+    fighter.getUnitAttachment().setHitPoints(3);
+    fighter.getUnitAttachment().setMovement(4);
+    fighter.getUnitAttachment().setIsAir(true);
+
+    final List<Unit> units = fighter.createTemp(3, player1);
+
+    units.get(0).setAlreadyMoved(BigDecimal.valueOf(2));
+    units.get(1).setAlreadyMoved(BigDecimal.ONE);
+    units.get(2).setAlreadyMoved(BigDecimal.valueOf(2));
+
+    // User selected all units to go from 3hp to 2hp and unit 0 to additionally go from 2hp to 1hp.
+    final List<Unit> damaged = List.of(units.get(0), units.get(1), units.get(2), units.get(0));
+    for (Unit u : units) System.err.println(u);
+
+    final CasualtyDetails casualtyDetails = new CasualtyDetails(List.of(), damaged, true);
+    casualtyDetails.ensureUnitsAreDamagedFirst(
+        units, Matches.unitIsAir(), Comparator.comparing(Unit::getMovementLeft).reversed());
+
+    // We expect three units to still be hit, but instead of unit 0 getting two hits, unit 1 should
+    // as it has most movement left.
+    assertThat(casualtyDetails.getDamaged())
+        .containsExactlyInAnyOrder(units.get(0), units.get(1), units.get(2), units.get(1));
+  }
 }