Allow improvements that don't need removal to build (#11299)

* Fix improvements that remove features only not checking for all terrains

* Allow improvements that don't need removal to build without improving resource

* spot the missing import

* Add in tests

* typo

* Assert Forest is still there

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

@@ -443,7 +443,7 @@ class WorkerAutomation(
         return tile.tileResource.getImprovements().any { resourceImprovementName ->
             if (resourceImprovementName !in potentialTileImprovements) return@any false
             val resourceImprovement = potentialTileImprovements[resourceImprovementName]!!
-            tile.terrainFeatures.any { resourceImprovement.isAllowedOnFeature(it) }
+            tile.terrainFeatureObjects.any { resourceImprovement.isAllowedOnFeature(it) }
         }
     }
 

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

@@ -495,7 +495,7 @@ class Tile : IsPartOfGameInfoSerialization {
     }
 
     fun matchesTerrainFilter(filter: String, observingCiv: Civilization? = null): Boolean {
-        return MultiFilter.multiFilter(filter, {matchesSingleTerrainFilter(it, observingCiv)})
+        return MultiFilter.multiFilter(filter, { matchesSingleTerrainFilter(it, observingCiv) })
     }
 
     /** Implements [UniqueParameterType.TerrainFilter][com.unciv.models.ruleset.unique.UniqueParameterType.TerrainFilter] */
@@ -508,9 +508,6 @@ class Tile : IsPartOfGameInfoSerialization {
             "Land" -> isLand
             Constants.coastal -> isCoastalTile()
             Constants.river -> isAdjacentToRiver()
-            naturalWonder -> true
-            "Open terrain" -> !isRoughTerrain()
-            "Rough terrain" -> isRoughTerrain()
 
             "your" -> observingCiv != null && getOwner() == observingCiv
             "Foreign Land", "Foreign" -> observingCiv != null && !isFriendlyTerritory(observingCiv)
@@ -520,13 +517,11 @@ class Tile : IsPartOfGameInfoSerialization {
             resource -> observingCiv != null && hasViewableResource(observingCiv)
             "resource" -> observingCiv != null && hasViewableResource(observingCiv)
             "Water resource" -> isWater && observingCiv != null && hasViewableResource(observingCiv)
-            "Natural Wonder" -> naturalWonder != null
             "Featureless" -> terrainFeatures.isEmpty()
             Constants.freshWaterFilter -> isAdjacentTo(Constants.freshWater, observingCiv)
             
-            in terrainFeatures -> true
             else -> {
-                if (terrainUniqueMap.containsFilteringUnique(filter)) return true
+                if (allTerrains.any { it.matchesFilter(filter) }) return true
                 if (getOwner()?.matchesFilter(filter) == true) return true
 
                 // Resource type check is last - cannot succeed if no resource here

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

@@ -79,10 +79,9 @@ class TileInfoImprovementFunctions(val tile: Tile) {
                     .any { civInfo.getResourceAmount(it.params[1]) < it.params[0].toInt() })
             yield(ImprovementBuildingProblem.MissingResources)
 
-        val knownFeatureRemovals = tile.ruleset.tileImprovements.values
+        val knownFeatureRemovals = tile.ruleset.tileRemovals
             .filter { rulesetImprovement ->
-                rulesetImprovement.name.startsWith(Constants.remove)
+                        RoadStatus.values().none { it.removeAction == rulesetImprovement.name }
-                        && RoadStatus.values().none { it.removeAction == rulesetImprovement.name }
                         && (rulesetImprovement.techRequired == null || civInfo.tech.isResearched(rulesetImprovement.techRequired!!))
             }
 
@@ -107,16 +106,17 @@ class TileInfoImprovementFunctions(val tile: Tile) {
         ): Boolean {
             val topTerrain = tile.lastTerrain
             // We can build if we are specifically allowed to build on this terrain
-            if (isAllowedOnFeature(topTerrain.name)) return true
+            if (isAllowedOnFeature(topTerrain)) return true
 
             // Otherwise, we can if this improvement removes the top terrain
             if (!hasUnique(UniqueType.RemovesFeaturesIfBuilt, stateForConditionals)) return false
-            val removeAction = tile.ruleset.tileImprovements[Constants.remove + topTerrain.name] ?: return false
+            if (knownFeatureRemovals == null) return false
-            // and we have the tech to remove that top terrain
+            val featureRemovals = tile.terrainFeatures.map {
-            if (removeAction.techRequired != null && (knownFeatureRemovals == null || removeAction !in knownFeatureRemovals)) return false
+                feature -> tile.ruleset.tileRemovals.firstOrNull{ it.name == Constants.remove + feature } }
-            // and we can build it on the tile without the top terrain
+            if (featureRemovals.any { it != null && it !in knownFeatureRemovals }) return false
             val clonedTile = tile.clone()
-            clonedTile.removeTerrainFeature(topTerrain.name)
+            clonedTile.setTerrainFeatures(tile.terrainFeatures.filterNot {
+                feature -> featureRemovals.any{ it?.name?.removePrefix(Constants.remove) == feature } })
             return clonedTile.improvementFunctions.canImprovementBeBuiltHere(improvement, resourceIsVisible, knownFeatureRemovals, stateForConditionals)
         }
 
@@ -175,7 +175,7 @@ class TileInfoImprovementFunctions(val tile: Tile) {
             // At this point we know this is a normal improvement and that there is no reason not to allow it to be built.
 
             // Lastly we check if the improvement may be built on this terrain or resource
-            improvement.canBeBuiltOn(tile.lastTerrain.name) -> true
+            improvement.isAllowedOnFeature(tile.lastTerrain) -> true
             tile.isLand && improvement.canBeBuiltOn("Land") -> true
             tile.isWater && improvement.canBeBuiltOn("Water") -> true
             // DO NOT reverse this &&. isAdjacentToFreshwater() is a lazy which calls a function, and reversing it breaks the tests.
@@ -213,14 +213,14 @@ class TileInfoImprovementFunctions(val tile: Tile) {
         if (improvementObject != null && improvementObject.hasUnique(UniqueType.RemovesFeaturesIfBuilt)) {
             // Remove terrainFeatures that a Worker can remove
             // and that aren't explicitly allowed under the improvement
-            val removableTerrainFeatures = tile.terrainFeatures.filter { feature ->
+            val removableTerrainFeatures = tile.terrainFeatureObjects.filter { feature ->
-                val removingAction = "${Constants.remove}$feature"
+                val removingAction = "${Constants.remove}${feature.name}"
 
                 removingAction in tile.ruleset.tileImprovements // is removable
                     && !improvementObject.isAllowedOnFeature(feature) // cannot coexist
             }
 
-            tile.setTerrainFeatures(tile.terrainFeatures.filterNot { it in removableTerrainFeatures })
+            tile.setTerrainFeatures(tile.terrainFeatures.filterNot { feature -> removableTerrainFeatures.any { it.name == feature } })
         }
 
         if (civToActivateBroaderEffects != null && improvementObject != null

core/src/com/unciv/models/ruleset/Ruleset.kt

@@ -1,6 +1,7 @@
 package com.unciv.models.ruleset
 
 import com.badlogic.gdx.files.FileHandle
+import com.unciv.Constants
 import com.unciv.json.fromJsonFile
 import com.unciv.json.json
 import com.unciv.logic.BackwardCompatibility.updateDeprecations
@@ -77,6 +78,8 @@ class Ruleset {
         units.values.filter { it.hasUnique(UniqueType.GreatPersonFromCombat, StateForConditionals.IgnoreConditionals) }
     }
 
+    val tileRemovals by lazy { tileImprovements.values.filter { it.name.startsWith(Constants.remove) } }
+
     /** Contains all happiness levels that moving *from* them, to one *below* them, can change uniques that apply */
     val allHappinessLevelsThatAffectUniques by lazy {
         sequence {

core/src/com/unciv/models/ruleset/tile/Terrain.kt

@@ -2,6 +2,7 @@ package com.unciv.models.ruleset.tile
 
 import com.badlogic.gdx.graphics.Color
 import com.unciv.Constants
+import com.unciv.logic.MultiFilter
 import com.unciv.models.ruleset.Belief
 import com.unciv.models.ruleset.Ruleset
 import com.unciv.models.ruleset.RulesetStatsObject
@@ -143,6 +144,27 @@ class Terrain : RulesetStatsObject() {
         return textList
     }
 
+    fun matchesFilter(filter: String): Boolean {
+        return MultiFilter.multiFilter(filter, { matchesSingleFilter(it) })
+    }
+
+    /** Implements [UniqueParameterType.TerrainFilter][com.unciv.models.ruleset.unique.UniqueParameterType.TerrainFilter] */
+    fun matchesSingleFilter(filter: String): Boolean {
+        return when (filter) {
+            in Constants.all -> true
+            name -> true
+            "Terrain" -> true
+            in Constants.all -> true
+            "Open terrain" -> !isRough()
+            "Rough terrain" -> isRough()
+            type.name -> true
+            "Natural Wonder" -> type == TerrainType.NaturalWonder
+            "Terrain Feature" -> type == TerrainType.TerrainFeature
+
+            else -> uniques.contains(filter)
+        }
+    }
+
     fun setTransients() {
         damagePerTurn = getMatchingUniques(UniqueType.DamagesContainingUnits).sumOf { it.params[0].toInt() }
     }

core/src/com/unciv/models/ruleset/tile/TileImprovement.kt

@@ -75,6 +75,9 @@ class TileImprovement : RulesetStatsObject() {
     fun canBeBuiltOn(terrain: String): Boolean {
         return terrain in terrainsCanBeBuiltOn
     }
+    fun canBeBuiltOn(terrain: Terrain): Boolean {
+        return terrainsCanBeBuiltOn.any{ terrain.matchesFilter(it) }
+    }
 
     /**
      * Check: Is this improvement allowed on a [given][name] terrain feature?
@@ -86,7 +89,8 @@ class TileImprovement : RulesetStatsObject() {
      * so this check is done in conjunction - for the user, success means he does not need to remove
      * a terrain feature, thus the unique name.
      */
-    fun isAllowedOnFeature(name: String) = terrainsCanBeBuiltOn.contains(name) || getMatchingUniques(UniqueType.NoFeatureRemovalNeeded).any { it.params[0] == name }
+    fun isAllowedOnFeature(terrain: Terrain) = canBeBuiltOn(terrain)
+        || getMatchingUniques(UniqueType.NoFeatureRemovalNeeded).any { terrain.matchesFilter(it.params[0]) }
 
     /** Implements [UniqueParameterType.ImprovementFilter][com.unciv.models.ruleset.unique.UniqueParameterType.ImprovementFilter] */
     fun matchesFilter(filter: String): Boolean {

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

@@ -30,12 +30,11 @@ class TileImprovementConstructionTests {
         testGame.makeHexagonalMap(4)
         tileMap = testGame.tileMap
         civInfo = testGame.addCiv()
-        civInfo.tech.researchedTechnologies.addAll(testGame.ruleset.technologies.values)
+        for (tech in testGame.ruleset.technologies.values)
-        civInfo.tech.techsResearched.addAll(testGame.ruleset.technologies.keys)
+            civInfo.tech.addTechnology(tech.name)
         city = testGame.addCity(civInfo, tileMap[0,0])
     }
 
-
     @Test
     fun allTerrainSpecificImprovementsCanBeBuilt() {
         for (improvement in testGame.ruleset.tileImprovements.values) {
@@ -214,6 +213,39 @@ class TileImprovementConstructionTests {
         assert(tile.improvement == "Camp") // Camp can be both on Forest AND on Plains, so not removed
     }
 
+    @Test
+    fun improvementCannotBuildWhenNotAllowed() {
+        val tile = tileMap[1,1]
+        tile.baseTerrain ="Grassland"
+        tile.addTerrainFeature("Forest")
+
+        val improvement = testGame.createTileImprovement()
+        Assert.assertFalse("Forest doesn't allow building unless allowed",
+            tile.improvementFunctions.canBuildImprovement(improvement, civInfo))
+
+
+        val allowedImprovement = testGame.createTileImprovement()
+        allowedImprovement.terrainsCanBeBuiltOn += "Forest"
+        Assert.assertTrue("Forest should allow building when allowed",
+            tile.improvementFunctions.canBuildImprovement(allowedImprovement, civInfo))
+        tile.changeImprovement(allowedImprovement.name)
+        Assert.assertTrue(tile.improvement == allowedImprovement.name)
+        Assert.assertTrue("Forest should not be removed with this improvement", tile.terrainFeatures.contains("Forest"))
+    }
+
+    @Test
+    fun improvementDoesntNeedRemovalCanBuildHere() {
+        val tile = tileMap[1,1]
+        tile.baseTerrain ="Grassland"
+        tile.addTerrainFeature("Forest")
+
+        val improvement = testGame.createTileImprovement("Does not need removal of [Forest]")
+        Assert.assertTrue(tile.improvementFunctions.canBuildImprovement(improvement, civInfo))
+        tile.changeImprovement(improvement.name)
+        Assert.assertTrue(tile.improvement == improvement.name)
+        Assert.assertTrue("Forest should not be removed with this improvement", tile.terrainFeatures.contains("Forest"))
+    }
+
     @Test
     fun statsDiffFromRemovingForestTakesRemovedLumberMillIntoAccount() {
         val tile = tileMap[1,1]

tests/src/com/unciv/uniques/GlobalUniquesTests.kt

@@ -116,6 +116,7 @@ class GlobalUniquesTests {
         city.cityStats.update()
         Assert.assertTrue(city.cityStats.finalStatList["Buildings"]!!.gold == 3f)
         tile.baseTerrain = Constants.grassland
+        tile.setTransients()
         city.cityStats.update()
         Assert.assertTrue(city.cityStats.finalStatList["Buildings"]!!.gold == 0f)
     }