Improvements to MoveValidator API. (#10517)

- Reduces number of required params for move validation. - Moves combat move bool to ctor. - Moves special move validation from delegates to MoveValidator. - Updates call sites. - Some misc clean ups.
triplea-game · May 27, 2022 · 70ae733 · 70ae733
1 parent 5926be4
commit 70ae733
Show file tree

Hide file tree

Showing 17 changed files with 289 additions and 332 deletions.
diff --git a/game-app/game-core/src/main/java/games/strategy/engine/data/RouteFinder.java b/game-app/game-core/src/main/java/games/strategy/engine/data/RouteFinder.java
@@ -31,15 +31,16 @@ class RouteFinder {
   @Nullable private final GamePlayer player;
 
   RouteFinder(final GameMap map, final Predicate<Territory> condition) {
-    this(new MoveValidator(map.getData()), map, condition, Set.of(), null);
+    this(map, condition, Set.of(), null);
   }
 
   RouteFinder(
       final GameMap map,
       final Predicate<Territory> condition,
       final Collection<Unit> units,
       final GamePlayer player) {
-    this(new MoveValidator(map.getData()), map, condition, units, player);
+    // Note: We only use MoveValidator for canal checks, where isNonCombat isn't used.
+    this(new MoveValidator(map.getData(), false), map, condition, units, player);
   }
 
   Optional<Route> findRouteByDistance(final Territory start, final Territory end) {

diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/ai/pro/ProNonCombatMoveAi.java b/game-app/game-core/src/main/java/games/strategy/triplea/ai/pro/ProNonCombatMoveAi.java
@@ -2483,8 +2483,8 @@ private Map<Territory, ProTerritory> moveInfraUnits(
                       u,
                       player);
           final MoveValidationResult mvr =
-              new MoveValidator(data)
-                  .validateMove(new MoveDescription(List.of(u), r), player, true, null);
+              new MoveValidator(data, true)
+                  .validateMove(new MoveDescription(List.of(u), r), player);
           if (!mvr.isMoveValid()) {
             continue;
           }

diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/ai/pro/ProTechAi.java b/game-app/game-core/src/main/java/games/strategy/triplea/ai/pro/ProTechAi.java
@@ -421,6 +421,7 @@ private static List<Unit> findAttackers(
     Territory current;
     distance.put(start, 0);
     visited.put(start, null);
+    MoveValidator moveValidator = new MoveValidator(data, false);
     while (!q.isEmpty()) {
       current = q.remove();
       if (distance.getInt(current) == maxDistance) {
@@ -431,11 +432,9 @@ private static List<Unit> findAttackers(
           if (!neighbor.anyUnitsMatch(unitCondition) && !routeCondition.test(neighbor)) {
             continue;
           }
-          if (sea) {
-            final Route r = new Route(neighbor, current);
-            if (new MoveValidator(data).validateCanal(r, null, player) != null) {
-              continue;
-            }
+          if (sea
+              && moveValidator.validateCanal(new Route(neighbor, current), null, player) != null) {
+            continue;
           }
           distance.put(neighbor, distance.getInt(current) + 1);
           visited.put(neighbor, current);
@@ -444,11 +443,7 @@ private static List<Unit> findAttackers(
           if (ignoreDistance.contains(dist)) {
             continue;
           }
-          for (final Unit u : neighbor.getUnitCollection()) {
-            if (unitCondition.test(u)) {
-              units.add(u);
-            }
-          }
+          units.addAll(neighbor.getUnitCollection().getMatches(unitCondition));
         }
       }
     }

diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/ai/pro/data/ProTerritoryManager.java b/game-app/game-core/src/main/java/games/strategy/triplea/ai/pro/data/ProTerritoryManager.java
@@ -1117,6 +1117,7 @@ private static void findAmphibMoveOptions(
         int movesLeft =
             getUnitRange(data, myTransport, myUnitTerritory, player, isCheckingEnemyAttacks)
                 .intValue();
+        MoveValidator moveValidator = new MoveValidator(data, !isCombatMove);
         while (movesLeft >= 0) {
           final Set<Territory> nextTerritories = new HashSet<>();
           for (final Territory currentTerritory : currentTerritories) {
@@ -1126,8 +1127,7 @@ private static void findAmphibMoveOptions(
                 gameMap.getNeighbors(currentTerritory, canMoveSeaUnitsThrough);
             for (final Territory possibleNeighborTerritory : possibleNeighborTerritories) {
               final Route route = new Route(currentTerritory, possibleNeighborTerritory);
-              if (new MoveValidator(data).validateCanal(route, List.of(myTransport), player)
-                  == null) {
+              if (moveValidator.validateCanal(route, List.of(myTransport), player) == null) {
                 nextTerritories.add(possibleNeighborTerritory);
               }
             }

diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/ai/pro/util/ProMatches.java b/game-app/game-core/src/main/java/games/strategy/triplea/ai/pro/util/ProMatches.java
@@ -31,7 +31,7 @@ public static BiPredicate<Territory, Territory> noCanalsBetweenTerritories(
       final GamePlayer player, final GameData gameData) {
     return (startTerritory, endTerritory) -> {
       final Route r = new Route(startTerritory, endTerritory);
-      return new MoveValidator(gameData).validateCanal(r, null, player) == null;
+      return new MoveValidator(gameData, false).validateCanal(r, null, player) == null;
     };
   }
 

diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/ai/pro/util/ProMoveUtils.java b/game-app/game-core/src/main/java/games/strategy/triplea/ai/pro/util/ProMoveUtils.java
@@ -235,12 +235,12 @@ public static List<MoveDescription> calculateAmphibRoutes(
                     ProMatches.territoryCanMoveSeaUnitsThrough(data, player, isCombatMove));
             Territory territoryToMoveTo = null;
             int minUnitDistance = Integer.MAX_VALUE;
-            int maxDistanceFromEnd =
-                Integer.MIN_VALUE; // Used to move to farthest away loading territory first
+            // Used to move to farthest away loading territory first
+            int maxDistanceFromEnd = Integer.MIN_VALUE;
+            MoveValidator moveValidator = new MoveValidator(data, !isCombatMove);
             for (final Territory neighbor : neighbors) {
               final Route route = new Route(transportTerritory, neighbor);
-              if (new MoveValidator(data).validateCanal(route, List.of(transport), player)
-                  != null) {
+              if (moveValidator.validateCanal(route, List.of(transport), player) != null) {
                 continue;
               }
               int distanceFromUnloadTerritory = 0;

diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/ai/weak/WeakAi.java b/game-app/game-core/src/main/java/games/strategy/triplea/ai/weak/WeakAi.java
@@ -26,6 +26,7 @@
 import games.strategy.triplea.delegate.TransportTracker;
 import games.strategy.triplea.delegate.battle.BattleDelegate;
 import games.strategy.triplea.delegate.data.PlaceableUnits;
+import games.strategy.triplea.delegate.move.validation.MoveValidator;
 import games.strategy.triplea.delegate.remote.IAbstractPlaceDelegate;
 import games.strategy.triplea.delegate.remote.IMoveDelegate;
 import games.strategy.triplea.delegate.remote.IPurchaseDelegate;
@@ -319,7 +320,7 @@ private static List<MoveDescription> calculateNonCombatSea(
     final Predicate<Territory> routeCond =
         Matches.territoryIsWater()
             .and(Matches.territoryHasEnemyUnits(player, data.getRelationshipTracker()).negate())
-            .and(Matches.territoryHasNonAllowedCanal(player, data).negate());
+            .and(territoryHasNonAllowedCanal(player, data).negate());
     Route r = data.getMap().getRoute(start, destination, routeCond);
     if (r == null || r.hasNoSteps() || !routeCond.test(destination)) {
       return null;
@@ -330,6 +331,12 @@ private static List<MoveDescription> calculateNonCombatSea(
     return r;
   }
 
+  private static Predicate<Territory> territoryHasNonAllowedCanal(
+      final GamePlayer player, final GameData gameData) {
+    return t ->
+        new MoveValidator(gameData, false).validateCanal(new Route(t), null, player) != null;
+  }
+
   private static List<MoveDescription> calculateCombatMoveSea(
       final GameState data, final GamePlayer player) {
     final var moves = new ArrayList<MoveDescription>();
@@ -385,21 +392,21 @@ private static Route getAlternativeAmphibRoute(final GamePlayer player, final Ga
     // should select all territories with loaded transports
     final Predicate<Territory> transportOnSea =
         Matches.territoryIsWater().and(Matches.territoryHasLandUnitsOwnedBy(player));
+    final Predicate<Unit> ownedTransports =
+        Matches.unitCanTransport()
+            .and(Matches.unitIsOwnedBy(player))
+            .and(Matches.unitHasNotMoved());
+    final Predicate<Territory> enemyTerritory =
+        Matches.isTerritoryEnemy(player, data.getRelationshipTracker())
+            .and(Matches.territoryIsLand())
+            .and(Matches.territoryIsNeutralButNotWater().negate())
+            .and(Matches.territoryIsEmpty());
     Route altRoute = null;
     final int length = Integer.MAX_VALUE;
     for (final Territory t : data.getMap()) {
       if (!transportOnSea.test(t)) {
         continue;
       }
-      final Predicate<Unit> ownedTransports =
-          Matches.unitCanTransport()
-              .and(Matches.unitIsOwnedBy(player))
-              .and(Matches.unitHasNotMoved());
-      final Predicate<Territory> enemyTerritory =
-          Matches.isTerritoryEnemy(player, data.getRelationshipTracker())
-              .and(Matches.territoryIsLand())
-              .and(Matches.territoryIsNeutralButNotWater().negate())
-              .and(Matches.territoryIsEmpty());
       final int trans = t.getUnitCollection().countMatches(ownedTransports);
       if (trans > 0) {
         final Route newRoute = Utils.findNearest(t, enemyTerritory, routeCondition, data);
@@ -414,6 +421,19 @@ private static Route getAlternativeAmphibRoute(final GamePlayer player, final Ga
   private List<MoveDescription> calculateNonCombat(final GameData data, final GamePlayer player) {
     final Collection<Territory> territories = data.getMap().getTerritories();
     final List<MoveDescription> moves = movePlanesHomeNonCombat(player, data);
+    // these are the units we can move
+    final Predicate<Unit> moveOfType =
+        Matches.unitIsOwnedBy(player)
+            .and(Matches.unitIsNotAa())
+            // we can never move factories
+            .and(Matches.unitCanMove())
+            .and(Matches.unitIsNotInfrastructure())
+            .and(Matches.unitIsLand());
+    final Predicate<Territory> moveThrough =
+        Matches.territoryIsImpassable()
+            .negate()
+            .and(Matches.territoryIsNeutralButNotWater().negate())
+            .and(Matches.territoryIsLand());
     // move our units toward the nearest enemy capitol
     for (final Territory t : territories) {
       if (t.isWater()) {
@@ -428,19 +448,6 @@ private List<MoveDescription> calculateNonCombat(final GameData data, final Game
           continue;
         }
       }
-      // these are the units we can move
-      final Predicate<Unit> moveOfType =
-          Matches.unitIsOwnedBy(player)
-              .and(Matches.unitIsNotAa())
-              // we can never move factories
-              .and(Matches.unitCanMove())
-              .and(Matches.unitIsNotInfrastructure())
-              .and(Matches.unitIsLand());
-      final Predicate<Territory> moveThrough =
-          Matches.territoryIsImpassable()
-              .negate()
-              .and(Matches.territoryIsNeutralButNotWater().negate())
-              .and(Matches.territoryIsLand());
       final List<Unit> units = t.getUnitCollection().getMatches(moveOfType);
       if (units.isEmpty()) {
         continue;
@@ -449,17 +456,17 @@ private List<MoveDescription> calculateNonCombat(final GameData data, final Game
       Territory to = null;
       // find the nearest enemy owned capital
       for (final GamePlayer otherPlayer : data.getPlayerList().getPlayers()) {
-        final Territory capitol =
+        final Territory capital =
             TerritoryAttachment.getFirstOwnedCapitalOrFirstUnownedCapital(
                 otherPlayer, data.getMap());
-        if (capitol != null
-            && !data.getRelationshipTracker().isAllied(player, capitol.getOwner())) {
-          final Route route = data.getMap().getRoute(t, capitol, moveThrough);
-          if (route != null && moveThrough.test(capitol)) {
+        if (capital != null
+            && !data.getRelationshipTracker().isAllied(player, capital.getOwner())) {
+          final Route route = data.getMap().getRoute(t, capital, moveThrough);
+          if (route != null && moveThrough.test(capital)) {
             final int distance = route.numberOfSteps();
             if (distance != 0 && distance < minDistance) {
               minDistance = distance;
-              to = capitol;
+              to = capital;
             }
           }
         }

diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/AbstractMoveDelegate.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/AbstractMoveDelegate.java
@@ -8,8 +8,6 @@
 import games.strategy.engine.data.Unit;
 import games.strategy.engine.posted.game.pbem.PbemMessagePoster;
 import games.strategy.triplea.delegate.battle.BattleTracker;
-import games.strategy.triplea.delegate.data.MoveValidationResult;
-import games.strategy.triplea.delegate.move.validation.MoveValidator;
 import games.strategy.triplea.delegate.remote.IMoveDelegate;
 import java.io.Serializable;
 import java.util.ArrayList;
@@ -62,9 +60,8 @@ public Serializable saveState() {
   public void loadState(final Serializable state) {
     final AbstractMoveExtendedDelegateState s = (AbstractMoveExtendedDelegateState) state;
     super.loadState(s.superState);
-    // if the undo state wasnt saved, then dont load it. prevents overwriting undo state when we
-    // restore from an undo
-    // move
+    // if the undo state wasn't saved, then don't load it. prevents overwriting undo state when we
+    // restore from an undo move
     if (s.movesToUndo != null) {
       movesToUndo = s.movesToUndo;
     }
@@ -120,19 +117,6 @@ public String move(final Collection<Unit> units, final Route route) {
   @Override
   public abstract String performMove(MoveDescription move);
 
-  public static MoveValidationResult validateMove(
-      final GameData gameData,
-      final MoveType moveType,
-      final MoveDescription move,
-      final GamePlayer player,
-      final boolean isNonCombat,
-      final List<UndoableMove> undoableMoves) {
-    if (moveType == MoveType.SPECIAL) {
-      return SpecialMoveDelegate.validateMove(gameData, move.getUnits(), move.getRoute(), player);
-    }
-    return new MoveValidator(gameData).validateMove(move, player, isNonCombat, undoableMoves);
-  }
-
   @Override
   public Collection<Territory> getTerritoriesWhereAirCantLand(final GamePlayer player) {
     return new AirThatCantLandUtil(bridge).getTerritoriesWhereAirCantLand(player);

diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/Matches.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/Matches.java
@@ -1530,11 +1530,6 @@ public static Predicate<Unit> unitOwnerHasImprovedArtillerySupportTech() {
     return u -> TechTracker.hasImprovedArtillerySupport(u.getOwner());
   }
 
-  public static Predicate<Territory> territoryHasNonAllowedCanal(
-      final GamePlayer player, final GameData gameData) {
-    return t -> new MoveValidator(gameData).validateCanal(new Route(t), null, player) != null;
-  }
-
   public static Predicate<Territory> territoryIsBlockedSea(
       final GamePlayer player,
       final GameProperties properties,

diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/MoveDelegate.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/MoveDelegate.java
@@ -619,9 +619,8 @@ public String performMove(final MoveDescription move) {
     // the current player
     final GamePlayer player = getUnitsOwner(move.getUnits());
     final MoveValidationResult result =
-        new MoveValidator(data)
-            .validateMove(
-                move, player, GameStepPropertiesHelper.isNonCombatMove(data, false), movesToUndo);
+        new MoveValidator(data, GameStepPropertiesHelper.isNonCombatMove(data, false))
+            .validateMove(move, player, movesToUndo);
     final StringBuilder errorMsg = new StringBuilder(100);
     final int numProblems = result.getTotalWarningCount() - (result.hasError() ? 0 : 1);
     final String numErrorsMsg =