From dec3a883797cb0c931316a3f44bcee0a974a4fbe Mon Sep 17 00:00:00 2001
From: asvitkine <alexei.svitkine@gmail.com>
Date: Sun, 21 Apr 2024 01:05:46 -0400
Subject: [PATCH] Fix NPE in AI code. (#12528)

I tracked this down to moveMap being populated only with only potential moves this turn, but the logic for moving consumables to destinations considers paths over multiple turns.

Fixes: #12508
---
 .../triplea/ai/pro/ProNonCombatMoveAi.java      | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

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
index 590155646b..bde81bbe4e 100644
--- 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
@@ -676,9 +676,7 @@ private List<ProTerritory> prioritizeDefendOptions(
       double maxValue = 0;
       Territory maxTerritory = null;
       for (final Territory neighbor : neighbors) {
-        if (moveMap.get(neighbor) != null
-            && moveMap.get(neighbor).isCanHold()
-            && territoryValueMap.get(neighbor) > maxValue) {
+        if (canHold(moveMap, neighbor) && territoryValueMap.get(neighbor) > maxValue) {
           maxTerritory = neighbor;
           maxValue = territoryValueMap.get(neighbor);
         }
@@ -1578,7 +1576,7 @@ private void moveUnitsToBestTerritories(final boolean isCombatMove) {
             // Find best unload territory
             Territory unloadToTerritory =
                 possibleUnloadTerritories.stream()
-                    .filter(t -> moveMap.get(t) != null && moveMap.get(t).isCanHold())
+                    .filter(t -> canHold(moveMap, t))
                     .findAny()
                     .orElse(CollectionUtils.getAny(possibleUnloadTerritories));
             proDestination = proData.getProTerritory(moveMap, unloadToTerritory);
@@ -2347,7 +2345,7 @@ private void moveConsumablesToFactories(
     Predicate<Territory> desiredDestination =
         ProMatches.territoryHasInfraFactoryAndIsLand()
             .and(Matches.isTerritoryOwnedBy(player))
-            .and(t -> moveMap.get(t).isCanHold());
+            .and(t -> canHold(moveMap, t));
 
     for (final Iterator<Unit> it = infraUnitMoveMap.keySet().iterator(); it.hasNext(); ) {
       final Unit u = it.next();
@@ -2427,13 +2425,20 @@ private Optional<Territory> findDestinationOrSafeTerritoryOnTheWay(
     // If nothing chosen and we can't hold the current territory, try to move somewhere safe.
     if (destination.getValue() == null && !moveMap.get(from).isCanHold()) {
       possibleMoves.stream()
-          .filter(t -> moveMap.get(t).isCanHold())
+          .filter(t -> canHold(moveMap, t))
           .findAny()
           .ifPresent(destination::setValue);
     }
     return Optional.ofNullable(destination.getValue());
   }
 
+  private boolean canHold(Map<Territory, ProTerritory> moveMap, Territory t) {
+    // Note: moveMap.get(t) may be null if none of our units can get there this turn,
+    // but this function is used in BFS that looks at potential paths over many moves.
+    ProTerritory proTerritory = moveMap.get(t);
+    return proTerritory != null && proTerritory.isCanHold();
+  }
+
   private Stream<Unit> combinedStream(Collection<Unit> units1, Collection<Unit> units2) {
     return Stream.concat(units1.stream(), units2.stream());
   }