Fix logic for damage units selection for 3hp units. (#12685)

* Fix logic for damage units selection for 3hp units.

Previously logic was not working correctly and was overriding the user's selection in terms of how many units should take 2hp of damage vs. 1hp.

Adds a test. Also improves/adds some comments on the existing logic.
Fixes: #12672

* Improve comments.

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));
+  }
 }