Fix units not entering cities upon capture (#4839)

* Renamed and added doc comments for some some can-pass-through functions

* Moved a city-state pass-through exception to a lower-level function

Moved the exception that players (but not AI) can always pass through
city-states from TileInfo.canCivPassThrough to
CivilizationInfo.canPassThroughTiles (which is called by the former).
This makes the behavior of the latter function accurate to its name, and
makes the code more understandable overall.

A side-effect of this change is that functions that used the lower-level
CivilizationInfo.canPassThroughTiles function directly now consider the
aforementioned exception. For example, if a city-state expands its
border, it should now no longer push out units of a player civilization.

It may make even more sense to move the exception to the "canMoveTo"
logic, since AI should probably be able to pass through city-state tiles
even when they don't want to end on them. That might however affect the
AI more significantly.

* Fixed bug where units would not enter a city upon capture

As listed in #4697.

* Fixed a bug where captured units were pushed out of captured cities

Before, capturing a city with a civilian unit in it would cause the
capturing military unit to enter the city but also cause the civilian
unit to leave the city. Now, the civilian unit stays in the city as
well.

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

@@ -2,7 +2,6 @@ package com.unciv.logic.automation
 
 import com.unciv.Constants
 import com.unciv.logic.city.CityInfo
-import com.unciv.logic.city.IConstruction
 import com.unciv.logic.city.INonPerpetualConstruction
 import com.unciv.logic.city.PerpetualConstruction
 import com.unciv.logic.civilization.*
@@ -399,7 +398,7 @@ object NextTurnAutomation {
         fun isTileCanMoveThrough(tileInfo: TileInfo): Boolean {
             val owner = tileInfo.getOwner()
             return !tileInfo.isImpassible()
-                    && (owner == otherCiv || owner == null || civInfo.canEnterTiles(owner))
+                    && (owner == otherCiv || owner == null || civInfo.canPassThroughTiles(owner))
         }
 
         val reachableEnemyCitiesBfs = BFS(civInfo.getCapital().getCenterTile()) { isTileCanMoveThrough(it) }

core/src/com/unciv/logic/battle/Battle.kt

@@ -69,11 +69,11 @@ object Battle {
 
         // check if unit is captured by the attacker (prize ships unique)
         // As ravignir clarified in issue #4374, this only works for aggressor
-        val captureSuccess = defender is MapUnitCombatant && attacker is MapUnitCombatant
+        val captureMilitaryUnitSuccess = defender is MapUnitCombatant && attacker is MapUnitCombatant
                 && defender.isDefeated() && !defender.unit.isCivilian()
                 && tryCaptureUnit(attacker, defender)
 
-        if (!captureSuccess)  // capture creates a new unit, but `defender` still is the original, so this function would still show a kill message
+        if (!captureMilitaryUnitSuccess) // capture creates a new unit, but `defender` still is the original, so this function would still show a kill message
             postBattleNotifications(attacker, defender, attackedTile, attacker.getTile())
 
         postBattleNationUniques(defender, attackedTile, attacker)
@@ -104,9 +104,8 @@ object Battle {
                 attacker.unit.action = null
         }
 
-        // we're a melee unit and we destroyed\captured an enemy unit
         // Should be called after tryCaptureUnit(), as that might spawn a unit on the tile we go to
-        if (!captureSuccess)
+        if (!captureMilitaryUnitSuccess)
             postBattleMoveToAttackedTile(attacker, defender, attackedTile)
 
         reduceAttackerMovementPointsAndAttacks(attacker, defender)
@@ -401,12 +400,12 @@ object Battle {
 
         attackerCiv.addNotification("We have conquered the city of [${city.name}]!", city.location, NotificationIcon.War)
 
+        city.hasJustBeenConquered = true
         city.getCenterTile().apply {
             if (militaryUnit != null) militaryUnit!!.destroy()
             if (civilianUnit != null) captureCivilianUnit(attacker, MapUnitCombatant(civilianUnit!!))
             for (airUnit in airUnits.toList()) airUnit.destroy()
         }
-        city.hasJustBeenConquered = true
 
         for (unique in attackerCiv.getMatchingUniques("Upon capturing a city, receive [] times its [] production as [] immediately")) {
             attackerCiv.addStat(

core/src/com/unciv/logic/city/CityExpansionManager.kt

@@ -8,7 +8,6 @@ import com.unciv.logic.map.TileInfo
 import com.unciv.ui.utils.withItem
 import com.unciv.ui.utils.withoutItem
 import kotlin.math.max
-import kotlin.math.min
 import kotlin.math.pow
 import kotlin.math.roundToInt
 
@@ -158,7 +157,7 @@ class CityExpansionManager {
         cityInfo.cityStats.update()
 
         for (unit in tileInfo.getUnits().toList()) // toListed because we're modifying
-            if (!unit.civInfo.canEnterTiles(cityInfo.civInfo))
+            if (!unit.civInfo.canPassThroughTiles(cityInfo.civInfo))
                 unit.movement.teleportToClosestMoveableTile()
 
         cityInfo.civInfo.updateViewableTiles()

core/src/com/unciv/logic/civilization/CivilizationInfo.kt

@@ -12,6 +12,7 @@ import com.unciv.logic.civilization.diplomacy.DiplomacyManager
 import com.unciv.logic.civilization.diplomacy.DiplomaticStatus
 import com.unciv.logic.map.MapUnit
 import com.unciv.logic.map.TileInfo
+import com.unciv.logic.map.UnitMovementAlgorithms
 import com.unciv.logic.trade.TradeEvaluation
 import com.unciv.logic.trade.TradeRequest
 import com.unciv.models.Counter
@@ -730,14 +731,25 @@ class CivilizationInfo {
         return greatPersonPoints
     }
 
-    fun canEnterTiles(otherCiv: CivilizationInfo): Boolean {
+    /**
+     * @returns whether units of this civilization can pass through the tiles owned by [otherCiv],
+     * considering only civ-wide filters.
+     * Use [TileInfo.canCivPassThrough] to check whether units of a civilization can pass through
+     * a specific tile, considering only civ-wide filters.
+     * Use [UnitMovementAlgorithms.canPassThrough] to check whether a specific unit can pass through
+     * a specific tile.
+     */
+    fun canPassThroughTiles(otherCiv: CivilizationInfo): Boolean {
         if (otherCiv == this) return true
         if (otherCiv.isBarbarian()) return true
         if (nation.isBarbarian() && gameInfo.turns >= gameInfo.difficultyObject.turnBarbariansCanEnterPlayerTiles)
             return true
         val diplomacyManager = diplomacy[otherCiv.civName]
-                ?: return false // not encountered yet
-        return (diplomacyManager.hasOpenBorders || diplomacyManager.diplomaticStatus == DiplomaticStatus.War)
+        if (diplomacyManager != null && (diplomacyManager.hasOpenBorders || diplomacyManager.diplomaticStatus == DiplomaticStatus.War))
+            return true
+        // Players can always pass through city-state tiles
+        if (isPlayerCivilization() && otherCiv.isCityState()) return true
+        return false
     }
 
 

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

@@ -653,7 +653,7 @@ class MapUnit {
             action = null
 
         val tileOwner = getTile().getOwner()
-        if (tileOwner != null && !civInfo.canEnterTiles(tileOwner) && !tileOwner.isCityState()) // if an enemy city expanded onto this tile while I was in it
+        if (tileOwner != null && !civInfo.canPassThroughTiles(tileOwner) && !tileOwner.isCityState()) // if an enemy city expanded onto this tile while I was in it
             movement.teleportToClosestMoveableTile()
     }
 

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

@@ -565,15 +565,17 @@ open class TileInfo {
 
     fun isAdjacentToRiver() = neighbors.any { isConnectedByRiver(it) }
 
-    fun canCivEnter(civInfo: CivilizationInfo): Boolean {
+    /**
+     * @returns whether units of [civInfo] can pass through this tile, considering only civ-wide filters.
+     * Use [UnitMovementAlgorithms.canPassThrough] to check whether a specific unit can pass through a tile.
+     */
+    fun canCivPassThrough(civInfo: CivilizationInfo): Boolean {
         val tileOwner = getOwner()
-        if (tileOwner == null || tileOwner == civInfo) return true
         // comparing the CivInfo objects is cheaper than comparing strings
+        if (tileOwner == null || tileOwner == civInfo) return true
         if (isCityCenter() && civInfo.isAtWarWith(tileOwner)
                 && !getCity()!!.hasJustBeenConquered) return false
-        if (!civInfo.canEnterTiles(tileOwner)
-                && !(civInfo.isPlayerCivilization() && tileOwner.isCityState())) return false
-        // AIs won't enter city-state's border.
+        if (!civInfo.canPassThroughTiles(tileOwner)) return false
         return true
     }
 

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

@@ -431,11 +431,14 @@ class UnitMovementAlgorithms(val unit:MapUnit) {
         if (!canPassThrough(tile))
             return false
 
-        if (tile.isCityCenter() && tile.getOwner() != unit.civInfo) return false // even if they'll let us pass through, we can't enter their city
+        // even if they'll let us pass through, we can't enter their city - unless we just captured it
+        if (tile.isCityCenter() && tile.getOwner() != unit.civInfo && !tile.getCity()!!.hasJustBeenConquered)
+            return false
 
-        if (unit.isCivilian())
-            return tile.civilianUnit == null && (tile.militaryUnit == null || tile.militaryUnit!!.owner == unit.owner)
-        else return tile.militaryUnit == null && (tile.civilianUnit == null || tile.civilianUnit!!.owner == unit.owner)
+        return if (unit.isCivilian())
+            tile.civilianUnit == null && (tile.militaryUnit == null || tile.militaryUnit!!.owner == unit.owner)
+        else
+            tile.militaryUnit == null && (tile.civilianUnit == null || tile.civilianUnit!!.owner == unit.owner)
     }
 
     private fun canAirUnitMoveTo(tile: TileInfo, unit: MapUnit): Boolean {
@@ -460,9 +463,15 @@ class UnitMovementAlgorithms(val unit:MapUnit) {
         return true
     }
 
-    // This is the most called function in the entire game,
-    // so multiple callees of this function have been optimized,
-    // because optimization on this function results in massive benefits!
+    /**
+     * @returns whether this unit can pass through [tile].
+     * Note that sometimes, a tile can be passed through but not entered. Use [canMoveTo] to
+     * determine whether a unit can enter a tile.
+     *
+     * This is the most called function in the entire game,
+     * so multiple callees of this function have been optimized,
+     * because optimization on this function results in massive benefits!
+     */
     fun canPassThrough(tile: TileInfo): Boolean {
         if (tile.isImpassible()) {
             // special exception - ice tiles are technically impassible, but some units can move through them anyway
@@ -490,7 +499,7 @@ class UnitMovementAlgorithms(val unit:MapUnit) {
         }
         if (tile.naturalWonder != null) return false
 
-        if (!tile.canCivEnter(unit.civInfo)) return false
+        if (!tile.canCivPassThrough(unit.civInfo)) return false
 
         val firstUnit = tile.getFirstUnit()
         if (firstUnit != null && firstUnit.civInfo != unit.civInfo && unit.civInfo.isAtWarWith(firstUnit.civInfo))

core/src/com/unciv/logic/trade/TradeLogic.kt

@@ -91,7 +91,7 @@ class TradeLogic(val ourCivilization:CivilizationInfo, val otherCivilization: Ci
                     city.getCenterTile().getUnits().toList().forEach { it.movement.teleportToClosestMoveableTile() }
                     for (tile in city.getTiles()) {
                         for (unit in tile.getUnits().toList()) {
-                            if (!unit.civInfo.canEnterTiles(to)) unit.movement.teleportToClosestMoveableTile()
+                            if (!unit.civInfo.canPassThroughTiles(to)) unit.movement.teleportToClosestMoveableTile()
                         }
                     }
                     to.updateViewableTiles()