Compare improvements by tile yield calculation (#9873)

* Compare improvements by tile yield calculation

* Docstring

* Explicit documentation for which functions in Stats are and are not mutating functions

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

@@ -388,16 +388,6 @@ object Automation {
             if (distance > 3) score += 100
         }
 
-        // Improvements are good: less points
-        if (tile.improvement != null &&
-            tile.stats.getImprovementStats(
-                tile.getTileImprovement()!!,
-                city.civ,
-                city,
-                localUniqueCache
-            ).values.sum() > 0f
-        ) score -= 5
-
         if (tile.naturalWonder != null) score -= 105
 
         // Straight up take the sum of all yields

core/src/com/unciv/logic/automation/unit/WorkerAutomation.kt

@@ -391,18 +391,18 @@ class WorkerAutomation(
         if (potentialTileImprovements.isEmpty()) return null
 
         val cityUniqueCaches = HashMap<City, LocalUniqueCache>()
-        fun getRankingWithImprovement(improvementName: String): Float {
+        fun getImprovementRanking(improvementName: String): Float {
             val improvement = ruleSet.tileImprovements[improvementName]!!
             val city = tile.getCity()
             val cache =
                     if (city == null) LocalUniqueCache(false)
                     else cityUniqueCaches.getOrPut(city) { LocalUniqueCache() }
-            val stats = tile.stats.getImprovementStats(improvement, civInfo, tile.getCity(), cache)
+            val stats = tile.stats.getStatDiffForImprovement(improvement, civInfo, tile.getCity(), cache)
             return Automation.rankStatsValue(stats, unit.civ)
         }
 
         val bestBuildableImprovement = potentialTileImprovements.values.asSequence()
-            .map { Pair(it, getRankingWithImprovement(it.name)) }
+            .map { Pair(it, getImprovementRanking(it.name)) }
             .filter { it.second > 0f }
             .maxByOrNull { it.second }?.first
 
@@ -418,7 +418,7 @@ class WorkerAutomation(
                 && !tile.providesResources(civInfo)
                 && !isResourceImprovementAllowedOnFeature(tile, potentialTileImprovements) -> Constants.remove + lastTerrain.name
             else -> tile.tileResource.getImprovements().filter { it in potentialTileImprovements || it==tile.improvement }
-                .maxByOrNull { getRankingWithImprovement(it) }
+                .maxByOrNull { getImprovementRanking(it) }
         }
 
         // After gathering all the data, we conduct the hierarchy in one place
@@ -430,7 +430,7 @@ class WorkerAutomation(
             bestBuildableImprovement == null -> null
 
             tile.improvement != null &&
-                    getRankingWithImprovement(tile.improvement!!) > getRankingWithImprovement(bestBuildableImprovement.name)
+                    getImprovementRanking(tile.improvement!!) > getImprovementRanking(bestBuildableImprovement.name)
                 -> null // What we have is better, even if it's pillaged we should repair it
 
             lastTerrain.let {

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

@@ -193,22 +193,34 @@ class TileStatFunctions(val tile: Tile) {
             food + production + gold
         }
 
+    /** Returns the extra stats that we would get if we switched to this improvement
+     * Can be negative if we're switching to a worse improvement */
+    fun getStatDiffForImprovement(
+        improvement: TileImprovement,
+        observingCiv: Civilization,
+        city: City?,
+        cityUniqueCache: LocalUniqueCache = LocalUniqueCache(false)): Stats {
+
+        val currentStats = getTileStats(city, observingCiv, cityUniqueCache)
+
+        val tileClone = tile.clone()
+        tileClone.setTransients()
+
+        if (improvement.name.startsWith(Constants.remove))
+            tileClone.removeTerrainFeature(improvement.name.removePrefix(Constants.remove))
+        else tileClone.changeImprovement(improvement.name)
+        val futureStats = tileClone.stats.getTileStats(city, observingCiv, cityUniqueCache)
+
+        return futureStats.minus(currentStats)
+    }
 
     // Also multiplies the stats by the percentage bonus for improvements (but not for tiles)
-    fun getImprovementStats(
+    private fun getImprovementStats(
         improvement: TileImprovement,
         observingCiv: Civilization,
         city: City?,
         cityUniqueCache: LocalUniqueCache = LocalUniqueCache(false)
     ): Stats {
-        if (improvement.name.startsWith(Constants.remove)){
-            val currentTileStats = getTileStats(city, observingCiv, cityUniqueCache)
-            val tileClone = tile.clone()
-            tileClone.removeTerrainFeature(improvement.name.removePrefix(Constants.remove))
-            val tileStatsAfterRemoval = tileClone.stats.getTileStats(city, observingCiv, cityUniqueCache)
-            return tileStatsAfterRemoval.minus(currentTileStats)
-        }
-
         val stats = improvement.cloneStats()
         if (tile.hasViewableResource(observingCiv) && tile.tileResource.isImprovedBy(improvement.name)
                 && tile.tileResource.improvementStats != null

core/src/com/unciv/models/stats/Stats.kt

@@ -56,7 +56,8 @@ open class Stats(
                 && faith == otherStats.faith
     }
 
-    /** @return a new instance containing the same values as `this` */
+    /** **Non-Mutating function**
+     * @return a new instance containing the same values as `this` */
     fun clone() = Stats(production, food, gold, science, culture, happiness, faith)
 
     /** @return `true` if all values are zero */
@@ -80,7 +81,9 @@ open class Stats(
         faith = 0f
     }
 
-    /** Adds each value of another [Stats] instance to this one in place */
+    /** **Mutating function**
+     * Adds each value of another [Stats] instance to this one in place
+     * @return this for chaining */
     fun add(other: Stats): Stats {
         production += other.production
         food += other.food
@@ -92,20 +95,28 @@ open class Stats(
         return this
     }
 
-    /** @return a new [Stats] instance containing the sum of its operands value by value */
+    /** **Non-mutating function**
+     * @return a new [Stats] instance */
     operator fun plus(stats: Stats) = clone().apply { add(stats) }
+
+    /** **Non-mutating function**
+     * @return a new [Stats] instance */
     operator fun minus(stats: Stats) = clone().apply { add(stats.times(-1)) }
 
-    /** Adds the [value] parameter to the instance value specified by [stat] in place
+    /** **Mutating function**
+     * Adds the [value] parameter to the instance value specified by [stat] in place
      * @return `this` to allow chaining */
     fun add(stat: Stat, value: Float): Stats {
         set(stat, value + get(stat))
         return this
     }
 
-    /** @return The result of multiplying each value of this instance by [number] as a new instance */
+    /** **Non-Mutating function**
+     * @return a new [Stats] instance with the result of multiplying each value of this instance by [number] as a new instance */
     operator fun times(number: Int) = times(number.toFloat())
-    /** @return The result of multiplying each value of this instance by [number] as a new instance */
+
+    /** **Non-Mutating function**
+     * @return a new [Stats] instance with the result of multiplying each value of this instance by [number] as a new instance */
     operator fun times(number: Float) = Stats(
         production * number,
         food * number,
@@ -116,7 +127,8 @@ open class Stats(
         faith * number
     )
 
-    /** Multiplies each value of this instance by [number] in place */
+    /** **Mutating function**
+     * Multiplies each value of this instance by [number] in place */
     fun timesInPlace(number: Float) {
         production *= number
         food *= number
@@ -127,9 +139,12 @@ open class Stats(
         faith *= number
     }
 
+    /** **Non-Mutating function**
+     * @return a new [Stats] instance */
     operator fun div(number: Float) = times(1/number)
 
-    /** Apply weighting for Production Ranking */
+    /** **Mutating function**
+     * Apply weighting for Production Ranking */
     fun applyRankingWeights(){
         food *= 14
         production *= 12

core/src/com/unciv/ui/screens/cityscreen/CityScreen.kt

@@ -209,19 +209,16 @@ class CityScreen(
         fun isExistingImprovementValuable(tile: Tile, improvementToPlace: TileImprovement): Boolean {
             if (tile.improvement == null) return false
             val civInfo = city.civ
-            val existingStats = tile.stats.getImprovementStats(
+
+            val statDiffForNewImprovement = tile.stats.getStatDiffForImprovement(
                 tile.getTileImprovement()!!,
                 civInfo,
                 city,
                 cityUniqueCache
             )
-            val replacingStats = tile.stats.getImprovementStats(
-                improvementToPlace,
-                civInfo,
-                city,
-                cityUniqueCache
-            )
-            return Automation.rankStatsValue(existingStats, civInfo) > Automation.rankStatsValue(replacingStats, civInfo)
+
+            // If stat diff for new improvement is negative/zero utility, current improvement is valuable
+            return Automation.rankStatsValue(statDiffForNewImprovement, civInfo) <= 0
         }
 
         fun getPickImprovementColor(tile: Tile): Pair<Color, Float> {

core/src/com/unciv/ui/screens/pickerscreens/ImprovementPickerScreen.kt

@@ -157,24 +157,12 @@ class ImprovementPickerScreen(
             val statIcons = getStatIconsTable(provideResource, removeImprovement)
 
             // get benefits of the new improvement
-            val stats = tile.stats.getImprovementStats(
+            val stats = tile.stats.getStatDiffForImprovement(
                 improvement,
                 currentPlayerCiv,
                 tile.getCity(),
                 cityUniqueCache
             )
-            // subtract the benefits of the replaced improvement, if any
-            val existingImprovement = tile.getTileImprovement()
-            if (existingImprovement != null && removeImprovement) {
-                val existingStats = tile.stats.getImprovementStats(
-                    existingImprovement,
-                    currentPlayerCiv,
-                    tile.getCity(),
-                    cityUniqueCache
-                )
-                stats.add(existingStats.times(-1.0f))
-            }
-
             val statsTable = getStatsTable(stats)
             statIcons.add(statsTable).padLeft(13f)