Escort movement fix (#11810)

* Wrote some failing unit tests to simulate the crash

* Fix escort movement crash

* getDistanceToTiles now searches once using both escorting units

* Added an extra test

* Moved checking for escort unit movement outside of getMovementCostBetweenAdjacentTiles

core/src/com/unciv/logic/map/mapunit/movement/MovementCost.kt

@@ -11,12 +11,31 @@ import com.unciv.models.ruleset.unique.UniqueType
 
 object MovementCost {
 
+    fun getMovementCostBetweenAdjacentTilesEscort(
+        unit: MapUnit,
+        from: Tile,
+        to: Tile,
+        considerZoneOfControl: Boolean = true,
+        includeEscortUnit: Boolean = true,
+    ): Float {
+        if (includeEscortUnit && unit.isEscorting()) {
+            return maxOf(getMovementCostBetweenAdjacentTiles(unit, from, to, considerZoneOfControl),
+                getMovementCostBetweenAdjacentTiles(unit.getOtherEscortUnit()!!, from, to, considerZoneOfControl))
+        } else {
+            return getMovementCostBetweenAdjacentTiles(unit, from, to, considerZoneOfControl)
+        }
+    }
+
     // This function is called ALL THE TIME and should be as time-optimal as possible!
+    /**
+     * Does not include escort unit
+     * @return The cost of movment for the unit between two tiles
+     */
     fun getMovementCostBetweenAdjacentTiles(
         unit: MapUnit,
         from: Tile,
         to: Tile,
-        considerZoneOfControl: Boolean = true
+        considerZoneOfControl: Boolean = true,
     ): Float {
         val civ = unit.civ
 

core/src/com/unciv/logic/map/mapunit/movement/UnitMovement.kt

@@ -31,7 +31,8 @@ class UnitMovement(val unit: MapUnit) {
         considerZoneOfControl: Boolean = true,
         tilesToIgnore: HashSet<Tile>? = null,
         passThroughCache: HashMap<Tile, Boolean> = HashMap(),
-        movementCostCache: HashMap<Pair<Tile, Tile>, Float> = HashMap()
+        movementCostCache: HashMap<Pair<Tile, Tile>, Float> = HashMap(),
+        includeOtherEscortUnit: Boolean = true
     ): PathsToTilesWithinTurn {
         val distanceToTiles = PathsToTilesWithinTurn()
 
@@ -59,19 +60,22 @@ class UnitMovement(val unit: MapUnit) {
                         // cities and units goes kaput.
                         else -> {
                             val key = Pair(tileToCheck, neighbor)
-                            val movementCost =
+                            val movementCost = movementCostCache.getOrPut(key) {
-                                movementCostCache.getOrPut(key) {
+                                MovementCost.getMovementCostBetweenAdjacentTilesEscort(unit, tileToCheck, neighbor, considerZoneOfControl, includeOtherEscortUnit)
-                                    MovementCost.getMovementCostBetweenAdjacentTiles(unit, tileToCheck, neighbor, considerZoneOfControl)
+                            }
-                                }
                             distanceToTiles[tileToCheck]!!.totalDistance + movementCost
                         }
                     }
 
                     if (!distanceToTiles.containsKey(neighbor) || distanceToTiles[neighbor]!!.totalDistance > totalDistanceToTile) { // this is the new best path
-                        if (totalDistanceToTile < unitMovement - Constants.minimumMovementEpsilon)  // We can still keep moving from here!
+                        val usableMovement = if (includeOtherEscortUnit && unit.isEscorting())
+                            minOf(unitMovement, unit.getOtherEscortUnit()!!.currentMovement)
+                        else unitMovement
+
+                        if (totalDistanceToTile < usableMovement - Constants.minimumMovementEpsilon)  // We can still keep moving from here!
                             updatedTiles += neighbor
                         else
-                            totalDistanceToTile = unitMovement
+                            totalDistanceToTile = usableMovement
                         // In Civ V, you can always travel between adjacent tiles, even if you don't technically
                         // have enough movement points - it simply depletes what you have
 
@@ -707,19 +711,12 @@ class UnitMovement(val unit: MapUnit) {
             considerZoneOfControl,
             null,
             passThroughCache,
-            movementCostCache
+            movementCostCache,
+            includeOtherEscortUnit
         )
 
-        if (includeOtherEscortUnit) {
+        pathfindingCache.setDistanceToTiles(considerZoneOfControl, distanceToTiles)
-            // Only save to cache only if we are the original call and not the subsequent escort unit call
+
-            pathfindingCache.setDistanceToTiles(considerZoneOfControl, distanceToTiles)
-            if (unit.isEscorting()) {
-                // We should only be able to move to tiles that our escort can also move to
-                val escortDistanceToTiles = unit.getOtherEscortUnit()!!.movement
-                    .getDistanceToTiles(considerZoneOfControl, includeOtherEscortUnit = false)
-                distanceToTiles.keys.removeAll { !escortDistanceToTiles.containsKey(it) }
-            }
-        }
         return distanceToTiles
     }
 

tests/src/com/unciv/logic/map/UnitFomationTests.kt

@@ -278,4 +278,58 @@ internal class UnitFormationTests {
         assertTrue(civilianUnit.isEscorting())
         assertTrue(TargetHelper.getAttackableEnemies(scout, scout.movement.getDistanceToTiles()).isEmpty())
     }
+
+    @Test
+    fun `test escort path with hills one turn civilian`() {
+        setUp(3)
+        val centerTile = testGame.getTile(Vector2(0f,0f))
+        val hillTile = testGame.getTile(Vector2(1f,1f))
+        val destinationTile = testGame.getTile(Vector2(1f,2f))
+        val militaryUnit = testGame.addUnit("Mechanized Infantry", civInfo, centerTile)
+        val civilianUnit = testGame.addUnit("Worker", civInfo, centerTile)
+        hillTile.addTerrainFeature("Hill")
+        destinationTile.addTerrainFeature("Hill")
+        civilianUnit.startEscorting()
+        civilianUnit.movement.moveToTile(destinationTile)
+        assertEquals(civilianUnit.getTile(), destinationTile)
+        assertEquals(militaryUnit.getTile(), destinationTile)
+    }
+
+    @Test
+    fun `test escort path with hills one turn military`() {
+        setUp(3)
+        val centerTile = testGame.getTile(Vector2(0f,0f))
+        val hillTile = testGame.getTile(Vector2(1f,1f))
+        val destinationTile = testGame.getTile(Vector2(1f,2f))
+        val militaryUnit = testGame.addUnit("Mechanized Infantry", civInfo, centerTile)
+        val civilianUnit = testGame.addUnit("Worker", civInfo, centerTile)
+        hillTile.addTerrainFeature("Hill")
+        destinationTile.addTerrainFeature("Hill")
+        militaryUnit.startEscorting()
+        militaryUnit.movement.moveToTile(destinationTile)
+        assertEquals(civilianUnit.getTile(), destinationTile)
+        assertEquals(militaryUnit.getTile(), destinationTile)
+    }
+
+    @Test
+    fun `test escort with ignore terrain cost unit`() {
+        setUp(5)
+        val centerTile = testGame.getTile(Vector2(0f,0f))
+        val marsh = testGame.getTile(Vector2(1f,1f))
+        marsh.addTerrainFeature("Marsh")
+        val jungle = testGame.getTile(Vector2(2f,2f))
+        jungle.addTerrainFeature("Jungle")
+        testGame.getTile(Vector2(3f,3f)).addTerrainFeature("Hill")
+        testGame.getTile(Vector2(3f,4f)).addTerrainFeature("Hill")
+        val destinationTile = testGame.getTile(Vector2(4f,5f))
+        val tileReached = testGame.getTile(Vector2(1f,2f));
+        val militaryUnit = testGame.addUnit("Scout", civInfo, centerTile)
+        val civilianUnit = testGame.addUnit("Worker", civInfo, centerTile)
+        militaryUnit.startEscorting()
+        val shortestPath = militaryUnit.movement.getShortestPath(destinationTile)
+        assertEquals(true, shortestPath.count() == 3)
+        assertEquals(false, shortestPath.contains(jungle))
+        assertEquals(false, shortestPath.contains(marsh))
+    }
+
 }