diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/MovePerformer.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/MovePerformer.java index bf7712432e..2b376d1e65 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/MovePerformer.java +++ b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/MovePerformer.java @@ -103,7 +103,8 @@ public void execute(final ExecutionStack stack, final IDelegateBridge bridge) { final Route routeUnitUsedToMove = moveDelegate.getRouteUsedToMoveInto(unit, route.getStart()); if (battle != null) { - battle.removeAttack(routeUnitUsedToMove, Set.of(unit)); + Change change = battle.removeAttack(routeUnitUsedToMove, Set.of(unit)); + bridge.addChange(change); } } } @@ -145,8 +146,8 @@ public void execute(final ExecutionStack stack, final IDelegateBridge bridge) { @Override public void execute(final ExecutionStack stack, final IDelegateBridge bridge) { - // if any non enemy territories on route or if any enemy units on route the battles - // on (note water could have enemy but its not owned) + // if any non-enemy territories on route or if any enemy units on route the battles + // on (note water could have enemy, but it's not owned) final GameData data = bridge.getData(); final Predicate mustFightThrough = getMustFightThroughMatch(gamePlayer); final Collection arrived = @@ -206,7 +207,7 @@ public void execute(final ExecutionStack stack, final IDelegateBridge bridge) { // if it's all bombers and there's something to bomb if (allCanBomb && targetsOrEscort && GameStepPropertiesHelper.isCombatMove(data)) { final boolean bombing = getRemotePlayer().shouldBomberBomb(route.getEnd()); - // if bombing and there's something to target- ask what to bomb + // if bombing and there's something to target - ask what to bomb if (bombing) { // CompositeMatchOr unitsToBeBombed = new // CompositeMatchOr(Matches.UnitIsFactory, @@ -353,7 +354,7 @@ private Change markMovementChange( unit, moved.add(unit.getAlreadyMoved()), Unit.ALREADY_MOVED)); } // if neutrals were taken over mark land units with 0 movement - // if entered a non blitzed conquered territory, mark with 0 movement + // if entered a non-blitzed conquered territory, mark with 0 movement if (GameStepPropertiesHelper.isCombatMove(data) && (!MoveDelegate.getEmptyNeutral(route).isEmpty() || hasConqueredNonBlitzed(route))) { for (final Unit unit : CollectionUtils.getMatches(units, Matches.unitIsLand())) { 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 index f36d59b780..d7723350ca 100644 --- 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 @@ -199,11 +199,6 @@ public final boolean isOver() { @Override public void cancelBattle(final IDelegateBridge bridge) {} - @Override - public boolean isBombingRun() { - return battleType.isBombingRun(); - } - @Override public BattleType getBattleType() { return battleType; diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/AirBattle.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/AirBattle.java index c645b53975..5ff7c5951f 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/AirBattle.java +++ b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/AirBattle.java @@ -106,8 +106,9 @@ public Change addAttackChange( } @Override - public void removeAttack(final Route route, final Collection units) { + public Change removeAttack(final Route route, final Collection units) { attackingUnits.removeAll(units); + return new CompositeChange(); } @Override @@ -341,14 +342,11 @@ private void makeBattle(final IDelegateBridge bridge) { recordUnitsWereInAirBattle(attackingUnits, bridge); recordUnitsWereInAirBattle(defendingUnits, bridge); } - // so as of right now, Air Battles are created before both normal battles and strategic bombing - // raids - // once completed, the air battle will create a strategic bombing raid, if that is the purpose - // of those aircraft - // however, if the purpose is a normal battle, it will have already been created by the battle - // tracker / combat move - // so we do not have to create normal battles, only bombing raids - // setup new battle here + // As of right now, Air Battles are created before both normal battles and strategic bombing + // raids. Once completed, the air battle will create a strategic bombing raid, if that is the + // purpose of those aircraft. However, if the purpose is a normal battle, it will have already + // been created by the battle tracker / combat move. + // So we do not have to create normal battles, only bombing raids setup new battle here. if (isBombingRun) { final Collection bombers = CollectionUtils.getMatches(attackingUnits, Matches.unitIsStrategicBomber()); diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/DependentBattle.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/DependentBattle.java index 9259128090..9a1d310a8f 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/DependentBattle.java +++ b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/DependentBattle.java @@ -1,11 +1,14 @@ package games.strategy.triplea.delegate.battle; +import games.strategy.engine.data.Change; +import games.strategy.engine.data.CompositeChange; import games.strategy.engine.data.GameData; import games.strategy.engine.data.GamePlayer; import games.strategy.engine.data.Route; import games.strategy.engine.data.Territory; import games.strategy.engine.data.Unit; import games.strategy.triplea.delegate.Matches; +import games.strategy.triplea.delegate.TransportTracker; import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; @@ -31,11 +34,12 @@ public abstract class DependentBattle extends AbstractBattle { } @Override - public void removeAttack(final Route route, final Collection units) { + public Change removeAttack(final Route route, final Collection units) { + // Note: This code is also duplicated in FinishedBattle, which is not a subclass. attackingUnits.removeAll(units); // the route could be null, in the case of a unit in a territory where a sub is submerged. if (route == null) { - return; + return new CompositeChange(); } final Territory attackingFrom = route.getTerritoryBeforeEnd(); final Collection attackingFromMapUnits = @@ -49,11 +53,14 @@ public void removeAttack(final Route route, final Collection units) { // if none of the units is a land unit, the attack from that territory is no longer an // amphibious assault if (attackingFromMapUnits.stream().noneMatch(Matches.unitIsLand())) { - getAmphibiousAttackTerritories().remove(attackingFrom); + amphibiousAttackFrom.remove(attackingFrom); // do we have any amphibious attacks left? - isAmphibious = !getAmphibiousAttackTerritories().isEmpty(); + isAmphibious = !amphibiousAttackFrom.isEmpty(); } } + // Clear transportedBy for allied air on carriers as these are only set during the battle. + return TransportTracker.clearTransportedByForAlliedAirOnCarrier( + units, battleSite, attacker, gameData); } /** Return attacking from Collection. */ diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/FinishedBattle.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/FinishedBattle.java index 0f540590af..99f35d703c 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/FinishedBattle.java +++ b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/FinishedBattle.java @@ -1,6 +1,7 @@ package games.strategy.triplea.delegate.battle; import games.strategy.engine.data.Change; +import games.strategy.engine.data.CompositeChange; import games.strategy.engine.data.GameData; import games.strategy.engine.data.GamePlayer; import games.strategy.engine.data.Route; @@ -10,6 +11,7 @@ import games.strategy.engine.data.changefactory.ChangeFactory; import games.strategy.engine.delegate.IDelegateBridge; import games.strategy.triplea.delegate.Matches; +import games.strategy.triplea.delegate.TransportTracker; import games.strategy.triplea.delegate.data.BattleRecord; import games.strategy.triplea.delegate.data.BattleRecord.BattleResultDescription; import games.strategy.triplea.util.TuvUtils; @@ -90,29 +92,33 @@ public Change addAttackChange( } @Override - public void removeAttack(final Route route, final Collection units) { + public Change removeAttack(final Route route, final Collection units) { + // Note: This is the same code as in DependentBattle, but this is not a subclass. attackingUnits.removeAll(units); // the route could be null, in the case of a unit in a territory where a sub is submerged. if (route == null) { - return; + return new CompositeChange(); } final Territory attackingFrom = route.getTerritoryBeforeEnd(); - Collection attackingFromMapUnits = attackingFromMap.get(attackingFrom); - // handle possible null pointer - if (attackingFromMapUnits == null) { - attackingFromMapUnits = new ArrayList<>(); - } + final Collection attackingFromMapUnits = + attackingFromMap.getOrDefault(attackingFrom, new ArrayList<>()); attackingFromMapUnits.removeAll(units); + if (attackingFromMapUnits.isEmpty()) { + this.attackingFromMap.remove(attackingFrom); + } // deal with amphibious assaults if (attackingFrom.isWater()) { - // if none of the units is a land unit, the attack from - // that territory is no longer an amphibious assault + // if none of the units is a land unit, the attack from that territory is no longer an + // amphibious assault if (attackingFromMapUnits.stream().noneMatch(Matches.unitIsLand())) { amphibiousAttackFrom.remove(attackingFrom); // do we have any amphibious attacks left? isAmphibious = !amphibiousAttackFrom.isEmpty(); } } + // Clear transportedBy for allied air on carriers as these are only set during the battle. + return TransportTracker.clearTransportedByForAlliedAirOnCarrier( + units, battleSite, attacker, gameData); } @Override diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/IBattle.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/IBattle.java index cb12ae417f..8c3401c5af 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/IBattle.java +++ b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/IBattle.java @@ -29,7 +29,7 @@ enum WhoWon { DEFENDER } - /** The type of a battle. */ + /** The type of battle. */ @AllArgsConstructor @ToString(of = "type") enum BattleType { @@ -68,14 +68,6 @@ public String toDisplayText() { */ Change addAttackChange(Route route, Collection units, Map> targets); - /** - * There are two distinct super-types of battles: Bombing battles, and Fighting battles. There may - * be sub-types of each of these. - * - * @return whether this battle is a bombing run - */ - boolean isBombingRun(); - /** The type of battle occurring, example: MustFightBattle, StrategicBombingRaidBattle, etc. */ BattleType getBattleType(); @@ -112,8 +104,9 @@ void unitsLostInPrecedingBattle( * * @param route - attacking route * @param units - attacking units + * @return any changes to be performed as a result. */ - void removeAttack(Route route, Collection units); + Change removeAttack(Route route, Collection units); /** If we need to cancel the battle, we may need to perform some cleanup. */ void cancelBattle(IDelegateBridge bridge); diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/NonFightingBattle.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/NonFightingBattle.java index 11aca7e88d..165fede312 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/NonFightingBattle.java +++ b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/NonFightingBattle.java @@ -56,7 +56,7 @@ public void fight(final IDelegateBridge bridge) { } // create event bridge.getHistoryWriter().startEvent("Battle in " + battleSite, battleSite); - // if any attacking non air units then win + // if any attacking non-air units then win final boolean someAttacking = hasAttackingUnits(); if (someAttacking) { whoWon = WhoWon.ATTACKER; diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/StrategicBombingRaidBattle.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/StrategicBombingRaidBattle.java index dcc4270ea7..e6accea6f8 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/StrategicBombingRaidBattle.java +++ b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/StrategicBombingRaidBattle.java @@ -2,6 +2,7 @@ import com.google.common.annotations.VisibleForTesting; import games.strategy.engine.data.Change; +import games.strategy.engine.data.CompositeChange; import games.strategy.engine.data.GameData; import games.strategy.engine.data.GamePlayer; import games.strategy.engine.data.Resource; @@ -134,8 +135,9 @@ public boolean isEmpty() { } @Override - public void removeAttack(final Route route, final Collection units) { + public Change removeAttack(final Route route, final Collection units) { removeAttackers(units, true); + return new CompositeChange(); } private void removeAttackers(final Collection units, final boolean removeTarget) { diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/steps/retreat/RetreaterGeneral.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/steps/retreat/RetreaterGeneral.java index 5d48162d7d..e3b9e7ebec 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/steps/retreat/RetreaterGeneral.java +++ b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/steps/retreat/RetreaterGeneral.java @@ -132,7 +132,7 @@ private Change retreatCombatTransportedItems( for (final IBattle dependent : dependentBattles) { final Route route = new Route(battleState.getBattleSite(), dependent.getTerritory()); final Collection retreatedUnits = dependent.getDependentUnits(units); - dependent.removeAttack(route, retreatedUnits); + change.add(dependent.removeAttack(route, retreatedUnits)); TransportTracker.reloadTransports(units, change); change.add(ChangeFactory.moveUnits(dependent.getTerritory(), retreatTo, retreatedUnits)); } diff --git a/game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/FinishedBattleTest.java b/game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/FinishedBattleTest.java index 9d31c8cf1e..210481bf08 100644 --- a/game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/FinishedBattleTest.java +++ b/game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/FinishedBattleTest.java @@ -67,7 +67,7 @@ void testTransportedByClearedAfterDependentBattle() { assertEquals(null, tank.getTransportedBy()); } - private Unit getSingleUnit(Territory t, UnitType type) { + public static Unit getSingleUnit(Territory t, UnitType type) { Collection units = CollectionUtils.getMatches(t.getUnits(), Matches.unitIsOfType(type)); assertEquals(1, units.size()); return CollectionUtils.getAny(units); diff --git a/game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/MustFightBattleTest.java b/game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/MustFightBattleTest.java index 363daded16..fd544bc463 100644 --- a/game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/MustFightBattleTest.java +++ b/game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/MustFightBattleTest.java @@ -4,10 +4,14 @@ import static games.strategy.triplea.delegate.GameDataTestUtil.artillery; import static games.strategy.triplea.delegate.GameDataTestUtil.battleDelegate; import static games.strategy.triplea.delegate.GameDataTestUtil.britain; +import static games.strategy.triplea.delegate.GameDataTestUtil.british; import static games.strategy.triplea.delegate.GameDataTestUtil.britishArtillery; import static games.strategy.triplea.delegate.GameDataTestUtil.britishInfantry; +import static games.strategy.triplea.delegate.GameDataTestUtil.carrier; import static games.strategy.triplea.delegate.GameDataTestUtil.chinese; import static games.strategy.triplea.delegate.GameDataTestUtil.fighter; +import static games.strategy.triplea.delegate.GameDataTestUtil.french; +import static games.strategy.triplea.delegate.GameDataTestUtil.germans; import static games.strategy.triplea.delegate.GameDataTestUtil.infantry; import static games.strategy.triplea.delegate.GameDataTestUtil.japan; import static games.strategy.triplea.delegate.GameDataTestUtil.japaneseInfantry; @@ -15,15 +19,18 @@ import static games.strategy.triplea.delegate.GameDataTestUtil.moveDelegate; import static games.strategy.triplea.delegate.GameDataTestUtil.removeFrom; import static games.strategy.triplea.delegate.GameDataTestUtil.territory; +import static games.strategy.triplea.delegate.GameDataTestUtil.transport; import static games.strategy.triplea.delegate.MockDelegateBridge.advanceToStep; import static games.strategy.triplea.delegate.MockDelegateBridge.newDelegateBridge; import static games.strategy.triplea.delegate.MockDelegateBridge.thenGetRandomShouldHaveBeenCalled; import static games.strategy.triplea.delegate.MockDelegateBridge.whenGetRandom; import static games.strategy.triplea.delegate.MockDelegateBridge.withDiceValues; import static games.strategy.triplea.delegate.MockDelegateBridge.withValues; +import static org.hamcrest.CoreMatchers.nullValue; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.in; +import static org.hamcrest.core.Is.is; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.mockito.ArgumentMatchers.any; @@ -44,6 +51,7 @@ import games.strategy.triplea.attachments.UnitSupportAttachment; import games.strategy.triplea.delegate.AbstractMoveDelegate; import games.strategy.triplea.delegate.GameDataTestUtil; +import games.strategy.triplea.delegate.MoveDelegate; import games.strategy.triplea.settings.AbstractClientSettingTestCase; import games.strategy.triplea.xml.TestMapGameData; import java.util.Collection; @@ -213,6 +221,44 @@ void testStepNamesChangeDuringCombat() { assertThat(indoChina.getUnits(), containsInAnyOrder(attackers.toArray())); } + @Test + void testAlliedCarriedPlanesTransportedByIsResetWhenCancelingBattle() throws Exception { + // Note: Test uses germans, british and france since other countries aren't at war on t1. + final GameData gameData = TestMapGameData.GLOBAL1940.getGameData(); + + // SZ 45 has a german transport. + final Territory sz45 = territory("45 Sea Zone", gameData); + removeFrom(sz45, sz45.getUnits()); + addTo(sz45, transport(gameData).create(1, germans(gameData))); + + // SZ 46 has a british carrier with 2 french planes. + final Territory sz46 = territory("46 Sea Zone", gameData); + removeFrom(sz46, sz46.getUnits()); + Unit carrier = carrier(gameData).create(1, british(gameData)).get(0); + addTo(sz46, List.of(carrier)); + List fighters = fighter(gameData).create(2, french(gameData)); + addTo(sz46, fighters); + + final Territory sz42 = territory("42 Sea Zone", gameData); + + final IDelegateBridge bridge = newDelegateBridge(british(gameData)); + advanceToStep(bridge, "CombatMove"); + MoveDelegate moveDelegate = GameDataTestUtil.moveDelegate(gameData); + moveDelegate.setDelegateBridgeAndPlayer(bridge); + moveDelegate.start(); + + Collection units = List.of(carrier, fighters.get(0), fighters.get(1)); + // For the battle, transportedBy will be set to the carrier so that it's shown in the UI and + // the units destroyed if the carrier is sunk. + GameDataTestUtil.move(units, new Route(sz46, sz45)); + assertThat(fighters.get(0).getTransportedBy(), is(carrier)); + assertThat(fighters.get(1).getTransportedBy(), is(carrier)); + // But if the units move out, then transportedBy should be cleared. + GameDataTestUtil.move(units, new Route(sz45, sz42)); + assertThat(fighters.get(0).getTransportedBy(), is(nullValue())); + assertThat(fighters.get(1).getTransportedBy(), is(nullValue())); + } + private static void setPropertyValue(GameData gameData, String propertyName, T value) { IEditableProperty property = (IEditableProperty)