Much more accurate improvement stat effects

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

@@ -783,8 +783,6 @@ class CityConstructions : IsPartOfGameInfoSerialization {
         /**todo unify with [UnitActions.getImprovementConstructionActions] and [UnitTurnManager.workOnImprovement] - this won't allow e.g. a building to place a road */
         tileForImprovement.changeImprovement(improvement.name)
         city.civ.lastSeenImprovement[tileForImprovement.position] = improvement.name
-        city.cityStats.update()
-        city.civ.cache.updateCivResources()
         // If bought the worldscreen will not have been marked to update, and the new improvement won't show until later...
         GUI.setUpdateWorldOnNextRender()
     }

core/src/com/unciv/logic/map/mapunit/UnitTurnManager.kt

@@ -1,11 +1,9 @@
 package com.unciv.logic.map.mapunit
 
-import com.unciv.Constants
 import com.unciv.UncivGame
 import com.unciv.logic.civilization.LocationAction
 import com.unciv.logic.civilization.NotificationCategory
 import com.unciv.logic.civilization.NotificationIcon
-import com.unciv.logic.map.tile.RoadStatus
 import com.unciv.models.ruleset.unique.UniqueTriggerActivation
 import com.unciv.models.ruleset.unique.UniqueType
 
@@ -179,60 +177,12 @@ class UnitTurnManager(val unit: MapUnit) {
             UncivGame.Current.settings.addCompletedTutorialTask("Construct an improvement")
 
         val improvementInProgress = tile.improvementInProgress ?: return
-        when {
+        tile.changeImprovement(improvementInProgress, unit.civ)
-            improvementInProgress.startsWith(Constants.remove) -> {
-                val removedFeatureName = improvementInProgress.removePrefix(Constants.remove)
-                val tileImprovement = tile.getTileImprovement()
-                if (tileImprovement != null
-                        && tile.terrainFeatures.any {
-                            tileImprovement.terrainsCanBeBuiltOn.contains(it) && it == removedFeatureName
-                        }
-                        && !tileImprovement.terrainsCanBeBuiltOn.contains(tile.baseTerrain)
-                ) {
-                    // We removed a terrain (e.g. Forest) and the improvement (e.g. Lumber mill) requires it!
-                    tile.removeImprovement()
-                    if (tile.resource != null) unit.civ.cache.updateCivResources() // unlikely, but maybe a mod makes a resource improvement dependent on a terrain feature
-                }
-                if (RoadStatus.values().any { improvementInProgress == it.removeAction }) {
-                    tile.removeRoad()
-                } else {
-                    val removedFeatureObject = tile.ruleset.terrains[removedFeatureName]
-                    if (removedFeatureObject != null && removedFeatureObject.hasUnique(UniqueType.ProductionBonusWhenRemoved)) {
-                        tryProvideProductionToClosestCity(removedFeatureName)
-                    }
-                    tile.removeTerrainFeature(removedFeatureName)
-                }
-            }
-            improvementInProgress == RoadStatus.Road.name -> tile.addRoad(RoadStatus.Road, unit.civ)
-            improvementInProgress == RoadStatus.Railroad.name -> tile.addRoad(RoadStatus.Railroad, unit.civ)
-            improvementInProgress == Constants.repair -> tile.setRepaired()
-            else -> {
-                tile.changeImprovement(improvementInProgress, unit.civ)
-            }
-        }
 
         tile.improvementInProgress = null
         tile.getCity()?.updateCitizens = true
     }
 
 
-    private fun tryProvideProductionToClosestCity(removedTerrainFeature: String) {
-        val tile = unit.getTile()
-        val closestCity = unit.civ.cities.minByOrNull { it.getCenterTile().aerialDistanceTo(tile) }
-        @Suppress("FoldInitializerAndIfToElvis")
-        if (closestCity == null) return
-        val distance = closestCity.getCenterTile().aerialDistanceTo(tile)
-        var productionPointsToAdd = if (distance == 1) 20 else 20 - (distance - 2) * 5
-        if (tile.owningCity == null || tile.owningCity!!.civ != unit.civ) productionPointsToAdd =
-                productionPointsToAdd * 2 / 3
-        if (productionPointsToAdd > 0) {
-            closestCity.cityConstructions.addProductionPoints(productionPointsToAdd)
-            val locations = LocationAction(tile.position, closestCity.location)
-            unit.civ.addNotification(
-                "Clearing a [$removedTerrainFeature] has created [$productionPointsToAdd] Production for [${closestCity.name}]",
-                locations, NotificationCategory.Production, NotificationIcon.Construction
-            )
-        }
-    }
 
 }

core/src/com/unciv/logic/map/tile/Tile.kt

@@ -316,14 +316,14 @@ open class Tile : IsPartOfGameInfoSerialization {
         improvementFunctions.changeImprovement(improvementStr, civToHandleCompletion)
 
     // function handling when adding a road to the tile
-    fun addRoad(roadType: RoadStatus, unitCivInfo: Civilization) {
+    fun addRoad(roadType: RoadStatus, creatingCivInfo: Civilization?) {
         roadStatus = roadType
         roadIsPillaged = false
-        if (getOwner() == null) {
+        if (getOwner() != null) {
-            roadOwner = unitCivInfo.civName // neutral tile, use building unit
-            unitCivInfo.neutralRoads.add(this.position)
-        } else {
             roadOwner = getOwner()!!.civName
+        } else if (creatingCivInfo != null) {
+            roadOwner = creatingCivInfo.civName // neutral tile, use building unit
+            creatingCivInfo.neutralRoads.add(this.position)
         }
     }
 

core/src/com/unciv/logic/map/tile/TileInfoImprovementFunctions.kt

@@ -2,6 +2,9 @@ package com.unciv.logic.map.tile
 
 import com.unciv.Constants
 import com.unciv.logic.civilization.Civilization
+import com.unciv.logic.civilization.LocationAction
+import com.unciv.logic.civilization.NotificationCategory
+import com.unciv.logic.civilization.NotificationIcon
 import com.unciv.models.ruleset.tile.TileImprovement
 import com.unciv.models.ruleset.unique.StateForConditionals
 import com.unciv.models.ruleset.unique.UniqueType
@@ -164,11 +167,26 @@ class TileInfoImprovementFunctions(val tile: Tile) {
     }
 
 
-    fun changeImprovement(improvementStr: String?, civToHandleCompletion:Civilization? = null) {
+    fun changeImprovement(improvementName: String?,
-        tile.improvementIsPillaged = false
+                          /** For road assignment and taking over tiles - DO NOT pass when simulating improvement effects! */
-        tile.improvement = improvementStr
+                          civToActivateBroaderEffects:Civilization? = null) {
+        val improvementObject = tile.ruleset.tileImprovements[improvementName]
+
+        when {
+            improvementName?.startsWith(Constants.remove) == true -> {
+                adtivateRemovalImprovement(improvementName, civToActivateBroaderEffects)
+            }
+            improvementName == RoadStatus.Road.name -> tile.addRoad(RoadStatus.Road, civToActivateBroaderEffects)
+            improvementName == RoadStatus.Railroad.name -> tile.addRoad(RoadStatus.Railroad, civToActivateBroaderEffects)
+            improvementName == Constants.repair -> tile.setRepaired()
+            else -> {
+                tile.improvementIsPillaged = false
+                tile.improvement = improvementName
+
+                removeCreatesOneImprovementMarker()
+            }
+        }
 
-        val improvementObject = tile.getTileImprovement()
         if (improvementObject != null && improvementObject.hasUnique(UniqueType.RemovesFeaturesIfBuilt)) {
             // Remove terrainFeatures that a Worker can remove
             // and that aren't explicitly allowed under the improvement
@@ -182,14 +200,62 @@ class TileInfoImprovementFunctions(val tile: Tile) {
             tile.setTerrainFeatures(tile.terrainFeatures.filterNot { it in removableTerrainFeatures })
         }
 
-        if (civToHandleCompletion != null && improvementObject != null
+        if (civToActivateBroaderEffects != null && improvementObject != null
             && improvementObject.hasUnique(UniqueType.TakesOverAdjacentTiles)
         )
-            UnitActions.takeOverTilesAround(civToHandleCompletion, tile)
+            UnitActions.takeOverTilesAround(civToActivateBroaderEffects, tile)
 
-        if (tile.owningCity != null) {
+        val city = tile.owningCity
-            tile.owningCity!!.civ.cache.updateCivResources()
+        if (city != null) {
-            tile.owningCity!!.reassignPopulationDeferred()
+            city.cityStats.update()
+            city.civ.cache.updateCivResources()
+            city.reassignPopulationDeferred()
+        }
+    }
+
+    private fun adtivateRemovalImprovement(
+        improvementName: String,
+        civToActivateBroaderEffects: Civilization?
+    ) {
+        val removedFeatureName = improvementName.removePrefix(Constants.remove)
+        val currentTileImprovement = tile.getTileImprovement()
+        // We removed a terrain (e.g. Forest) and the improvement (e.g. Lumber mill) requires it!
+        if (currentTileImprovement != null
+            && tile.terrainFeatures.any {
+                currentTileImprovement.terrainsCanBeBuiltOn.contains(it) && it == removedFeatureName
+            }
+            && !currentTileImprovement.terrainsCanBeBuiltOn.contains(tile.baseTerrain)
+        ) tile.removeImprovement()
+
+        if (RoadStatus.values().any { improvementName == it.removeAction }) {
+            tile.removeRoad()
+        } else {
+            val removedFeatureObject = tile.ruleset.terrains[removedFeatureName]
+            if (removedFeatureObject != null
+                && civToActivateBroaderEffects != null
+                && removedFeatureObject.hasUnique(UniqueType.ProductionBonusWhenRemoved)
+            )
+                tryProvideProductionToClosestCity(removedFeatureName, civToActivateBroaderEffects)
+
+            tile.removeTerrainFeature(removedFeatureName)
+        }
+    }
+
+    private fun tryProvideProductionToClosestCity(removedTerrainFeature: String, civ:Civilization) {
+        val closestCity = civ.cities.minByOrNull { it.getCenterTile().aerialDistanceTo(tile) }
+        @Suppress("FoldInitializerAndIfToElvis")
+        if (closestCity == null) return
+        val distance = closestCity.getCenterTile().aerialDistanceTo(tile)
+        var productionPointsToAdd = if (distance == 1) 20 else 20 - (distance - 2) * 5
+        if (tile.owningCity == null || tile.owningCity!!.civ != civ) productionPointsToAdd =
+            productionPointsToAdd * 2 / 3
+        if (productionPointsToAdd > 0) {
+            closestCity.cityConstructions.addProductionPoints(productionPointsToAdd)
+            val locations = LocationAction(tile.position, closestCity.location)
+            civ.addNotification(
+                "Clearing a [$removedTerrainFeature] has created [$productionPointsToAdd] Production for [${closestCity.name}]",
+                locations, NotificationCategory.Production, NotificationIcon.Construction
+            )
         }
     }
 

core/src/com/unciv/logic/map/tile/TileStatFunctions.kt

@@ -206,9 +206,7 @@ class TileStatFunctions(val tile: Tile) {
         val tileClone = tile.clone()
         tileClone.setTerrainTransients()
 
-        if (improvement.name.startsWith(Constants.remove))
+        tileClone.changeImprovement(improvement.name)
-            tileClone.removeTerrainFeature(improvement.name.removePrefix(Constants.remove))
-        else tileClone.changeImprovement(improvement.name)
         val futureStats = tileClone.stats.getTileStats(city, observingCiv, cityUniqueCache)
 
         return futureStats.minus(currentStats)

core/src/com/unciv/ui/screens/worldscreen/unit/actions/UnitActions.kt

@@ -491,9 +491,7 @@ object UnitActions {
                 title = actionTextWithSideEffects("Create [$improvementName]", unique, unit),
                 action = {
                     val unitTile = unit.getTile()
-                    unitTile.improvementFunctions.removeCreatesOneImprovementMarker()
                     unitTile.changeImprovement(improvementName, unit.civ)
-                    unitTile.stopWorkingOnImprovement()
 
                     // without this the world screen won't show the improvement because it isn't the 'last seen improvement'
                     unit.civ.cache.updateViewableTiles()

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

@@ -3,9 +3,11 @@ package com.unciv.logic.map
 
 import com.unciv.logic.city.City
 import com.unciv.logic.civilization.Civilization
+import com.unciv.logic.map.tile.RoadStatus
 import com.unciv.models.ruleset.tile.TerrainType
 import com.unciv.models.ruleset.unique.StateForConditionals
 import com.unciv.models.ruleset.unique.UniqueType
+import com.unciv.models.stats.Stats
 import com.unciv.testing.GdxTestRunner
 import com.unciv.uniques.TestGame
 import org.junit.Assert
@@ -172,4 +174,63 @@ class TileImprovementConstructionTests {
             Assert.assertFalse(improvement.name, canBeBuilt)
         }
     }
+
+    @Test
+    fun buildingRoadBuildsARoad(){
+        val tile = tileMap[1,1]
+        tile.improvementFunctions.changeImprovement("Road")
+        assert(tile.roadStatus == RoadStatus.Road)
+    }
+
+    @Test
+    fun removingRoadRemovesRoad(){
+        val tile = tileMap[1,1]
+        tile.roadStatus = RoadStatus.Road
+        tile.improvementFunctions.changeImprovement("Remove Road")
+        assert(tile.roadStatus == RoadStatus.None)
+    }
+
+    @Test
+    fun removingForestRemovesForestAndLumbermill(){
+        val tile = tileMap[1,1]
+        tile.addTerrainFeature("Forest")
+        tile.improvementFunctions.changeImprovement("Lumber mill")
+        assert(tile.getTileImprovement()!!.name == "Lumber mill")
+        tile.improvementFunctions.changeImprovement("Remove Forest")
+        assert(tile.terrainFeatures.isEmpty())
+        assert(tile.improvement == null) // Lumber mill can ONLY be on Forest, and is therefore removed
+    }
+
+    @Test
+    fun removingForestRemovesForestButNotCamp(){
+        val tile = tileMap[1,1]
+        tile.addTerrainFeature("Forest")
+        tile.resource = "Deer"
+        tile.baseTerrain = "Plains"
+        tile.improvementFunctions.changeImprovement("Camp")
+        assert(tile.getTileImprovement()!!.name == "Camp")
+        tile.improvementFunctions.changeImprovement("Remove Forest")
+        assert(tile.terrainFeatures.isEmpty())
+        assert(tile.improvement == "Camp") // Camp can be both on Forest AND on Plains, so not removed
+    }
+
+    @Test
+    fun statsDiffFromRemovingForestTakesRemovedLumberMillIntoAccount(){
+        val tile = tileMap[1,1]
+        tile.baseTerrain = "Grassland"
+        tile.addTerrainFeature("Forest")
+
+        val lumberMill = testGame.ruleset.tileImprovements["Lumber mill"]!!
+        tile.improvementFunctions.changeImprovement(lumberMill.name)
+        assert(tile.getTileImprovement() == lumberMill)
+
+        // 1f 1p from forest, 2p from lumber mill since all techs are researched
+        val tileStats = tile.stats.getTileStats(civInfo)
+        assert(tileStats.equals(Stats(production = 3f, food = 1f)))
+
+        val statsDiff = tile.stats.getStatDiffForImprovement(testGame.ruleset.tileImprovements["Remove Forest"]!!, civInfo, null)
+
+        // We'll be reverting back to grassland stats - 2f only
+        assert(statsDiff.equals(Stats(food = +1f, production = -3f)))
+    }
 }