Fixed bugs with unit movement (#5126)

* Fixed bugs with unit movement

* Optimized function call

* Cleaning up some code

* Removed comments that are no longer applicable

core/src/com/unciv/Constants.kt

@@ -78,4 +78,6 @@ object Constants {
 
     const val rising = "Rising"
     const val lowering = "Lowering"
+    
+    const val minimumMovementEpsilon = 0.05
 }

core/src/com/unciv/logic/automation/SpecificUnitAutomation.kt

@@ -273,7 +273,7 @@ object SpecificUnitAutomation {
 
         val citiesByNearbyAirUnits = pathsToCities.keys
                 .groupBy { key ->
-                    key.getTilesInDistance(unit.getRange() * 2)
+                    key.getTilesInDistance(unit.getMaxMovementForAirUnits())
                             .count {
                                 val firstAirUnit = it.airUnits.firstOrNull()
                                 firstAirUnit != null && firstAirUnit.civInfo.isAtWarWith(unit.civInfo)
@@ -358,7 +358,7 @@ object SpecificUnitAutomation {
 
     private fun tryRelocateToCitiesWithEnemyNearBy(unit: MapUnit): Boolean {
         val immediatelyReachableCitiesAndCarriers = unit.currentTile
-                .getTilesInDistance(unit.getRange() * 2).filter { unit.movement.canMoveTo(it) }
+                .getTilesInDistance(unit.getMaxMovementForAirUnits()).filter { unit.movement.canMoveTo(it) }
 
         for (city in immediatelyReachableCitiesAndCarriers) {
             if (city.getTilesInDistance(unit.getRange())

core/src/com/unciv/logic/map/MapUnit.kt

@@ -360,6 +360,10 @@ class MapUnit {
         return range
     }
     
+    fun getMaxMovementForAirUnits(): Int {
+        return getRange() * 2
+    }
+
     fun isEmbarked(): Boolean {
         if (!baseUnit.isLandUnit()) return false
         return currentTile.isWater
@@ -815,15 +819,20 @@ class MapUnit {
 
     fun disband() {
         // evacuation of transported units before disbanding, if possible. toListed because we're modifying the unit list.
-        for (unit in currentTile.getUnits().filter { it.isTransported && isTransportTypeOf(it) }
+        for (unit in currentTile.getUnits()
-            .toList()) {
+            .filter { it.isTransported && isTransportTypeOf(it) }
+            .toList()
+        ) {
             // if we disbanded a unit carrying other units in a city, the carried units can still stay in the city
-            if (currentTile.isCityCenter() && unit.movement.canMoveTo(currentTile)) continue
+            if (currentTile.isCityCenter() && unit.movement.canMoveTo(currentTile)) {
+                unit.isTransported = false
+                continue
+            }
             // if no "fuel" to escape, should be disbanded as well
-            if (unit.currentMovement < 0.1)
+            if (unit.currentMovement < Constants.minimumMovementEpsilon)
                 unit.disband()
             // let's find closest city or another carrier where it can be evacuated
-            val tileCanMoveTo = unit.currentTile.getTilesInDistance(unit.getRange() * 2)
+            val tileCanMoveTo = unit.currentTile.getTilesInDistance(unit.getMaxMovementForAirUnits())
                 .filterNot { it == currentTile }.firstOrNull { unit.movement.canMoveTo(it) }
 
             if (tileCanMoveTo != null)

core/src/com/unciv/logic/map/UnitMovementAlgorithms.kt

@@ -274,7 +274,7 @@ class UnitMovementAlgorithms(val unit:MapUnit) {
 
     fun canReachInCurrentTurn(destination: TileInfo): Boolean {
         if (unit.baseUnit.movesLikeAirUnits())
-            return unit.currentTile.aerialDistanceTo(destination) <= unit.getRange()*2
+            return unit.currentTile.aerialDistanceTo(destination) <= unit.getMaxMovementForAirUnits()
         if (unit.isPreparingParadrop())
             return getDistance(unit.currentTile.position, destination.position) <= unit.paradropRange && canParadropOn(destination)
         return getDistanceToTiles().containsKey(destination)
@@ -283,7 +283,7 @@ class UnitMovementAlgorithms(val unit:MapUnit) {
     fun getReachableTilesInCurrentTurn(): Sequence<TileInfo> {
         return when {
             unit.baseUnit.movesLikeAirUnits() ->
-                unit.getTile().getTilesInDistanceRange(IntRange(1, unit.getRange() * 2))
+                unit.getTile().getTilesInDistanceRange(IntRange(1, unit.getMaxMovementForAirUnits()))
             unit.isPreparingParadrop() ->
                 unit.getTile().getTilesInDistance(unit.paradropRange)
                     .filter { unit.movement.canParadropOn(it) }
@@ -407,16 +407,18 @@ class UnitMovementAlgorithms(val unit:MapUnit) {
             return
         val pathToLastReachableTile = distanceToTiles.getPathToTile(lastReachableTile)
 
-        if (!unit.civInfo.gameInfo.gameParameters.godMode) {
+        if (unit.isFortified() || unit.isSetUpForSiege() || unit.isSleeping() || unit.isAutomated())
-            unit.currentMovement -= distanceToTiles[lastReachableTile]!!.totalDistance
+            unit.action = null // un-fortify/un-setup/un-sleep/un-automate after moving
-            if (unit.currentMovement < 0.1) unit.currentMovement = 0f // silly floats which are "almost zero"
-        }
-        if (unit.isFortified() || unit.isSetUpForSiege() || unit.isSleeping())
-            unit.action = null // un-fortify/un-setup after moving
 
         // If this unit is a carrier, keep record of its air payload whereabouts.
         val origin = unit.getTile()
         var needToFindNewRoute = false
+        // Cache this in case something goes wrong
+        
+        var lastReachedEnterableTile = unit.getTile()
+        
+        unit.removeFromTile()
+        
         for (tile in pathToLastReachableTile) {
             if (!unit.movement.canPassThrough(tile)) {
                 // AAAH something happened making our previous path invalid
@@ -425,13 +427,29 @@ class UnitMovementAlgorithms(val unit:MapUnit) {
                 // Anyway: PANIC!! We stop this route, and after leaving the game in a valid state,
                 // we try again.
                 needToFindNewRoute = true
-                break
+                break // If you ever remove this break, remove the `assumeCanPassThrough` param below
             }
-            unit.removeFromTile()
+            unit.moveThroughTile(tile)
-            unit.putInTile(tile)
+            
+            // In case something goes wrong, cache the last tile we were able to end on
+            // We can assume we can pass through this tile, as we would have broken earlier
+            if (unit.movement.canMoveTo(tile, assumeCanPassThrough = true)) {
+                lastReachedEnterableTile = tile
+            }
+            
             if (unit.isDestroyed) break
         }
         
+        if (!unit.isDestroyed)
+            unit.putInTile(lastReachedEnterableTile)
+
+        if (!unit.civInfo.gameInfo.gameParameters.godMode) {
+            unit.currentMovement -= distanceToTiles[lastReachedEnterableTile]!!.totalDistance
+            if (unit.currentMovement < Constants.minimumMovementEpsilon) 
+                unit.currentMovement = 0f // silly floats which are "almost zero"
+            // const Epsilon, anyone?
+        }
+
         // The .toList() here is because we have a sequence that's running on the units in the tile,
         // then if we move one of the units we'll get a ConcurrentModificationException, se we save them all to a list
         for (payload in origin.getUnits().filter { it.isTransported && unit.canTransport(it) }.toList()) {  // bring along the payloads
@@ -445,24 +463,6 @@ class UnitMovementAlgorithms(val unit:MapUnit) {
             && (origin.isCityCenter() || lastReachableTile.isCityCenter())
             && unit.civInfo.hasUnique("Units in cities cost no Maintenance")
         ) unit.civInfo.updateStatsForNextTurn()
-
-        // Move through all intermediate tiles to get ancient ruins, barb encampments
-        // and to view tiles along the way
-        // We only activate the moveThroughTile AFTER the putInTile because of a really weird bug -
-        // If you're going to (or past) a ruin, and you activate the ruin bonus, and A UNIT spawns.
-        // That unit could now be blocking your entrance to the destination, so the putInTile would fail! =0
-        // Instead, we move you to the destination directly, and only afterwards activate the various tiles on the way.
-        
-        // Actually, we will now stop doing that becasue of _another_ really weird bug (actually two)
-        // 1. Through some ancient ruins bonuses, we could upgrade our unit, effectively replacing it
-        // with another unit. However, doing so halfway through a movement would make it impossible
-        // to reach the last tile, as the new unit spawns with 0 movement points and not taking
-        // the old route again. Therefore, we might trigger barbarian encampments or ancient ruins
-        // at the destination field we in fact never reach
-        // 2. Which tile we reach might change during the route. Maybe our route used to go through
-        // fog of war, and it turns out there was an enemy city on the route we should take.
-        // As we can't go through cities, we need to find another route, and therefore this
-        // route should be impossible.
         if (needToFindNewRoute) moveToTile(destination, considerZoneOfControl)
     }
 
@@ -494,11 +494,11 @@ class UnitMovementAlgorithms(val unit:MapUnit) {
      * Designates whether we can enter the tile - without attacking
      * DOES NOT designate whether we can reach that tile in the current turn
      */
-    fun canMoveTo(tile: TileInfo): Boolean {
+    fun canMoveTo(tile: TileInfo, assumeCanPassThrough: Boolean = false): Boolean {
         if (unit.baseUnit.movesLikeAirUnits())
             return canAirUnitMoveTo(tile, unit)
 
-        if (!canPassThrough(tile))
+        if (!assumeCanPassThrough && !canPassThrough(tile))
             return false
 
         // even if they'll let us pass through, we can't enter their city - unless we just captured it