last refactoring of terrain feature (#3702)

* last terrainFeature refactorings

* cut down terrainFeature as much as possible

* aerialDistanceTo fixed

wrapped distance should only be considered on wrapped worlds

* Adding spaces

* fixed tests not compiling

* prevent floodplains from spawning on oasis

* Check if terrain feature can be placed on tile in editor

* Adding reviewed changes

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

@@ -182,10 +182,10 @@ class WorkerAutomation(val unit: MapUnit) {
     private fun chooseImprovement(tile: TileInfo, civInfo: CivilizationInfo): TileImprovement? {
         val improvementStringForResource: String? = when {
             tile.resource == null || !tile.hasViewableResource(civInfo) -> null
-            tile.terrainFeature == Constants.marsh && !isImprovementOnFeatureAllowed(tile, civInfo) -> "Remove Marsh"
-            tile.terrainFeature == "Fallout" && !isImprovementOnFeatureAllowed(tile, civInfo) -> "Remove Fallout"    // for really mad modders
-            tile.terrainFeature == Constants.jungle && !isImprovementOnFeatureAllowed(tile, civInfo) -> "Remove Jungle"
-            tile.terrainFeature == Constants.forest && !isImprovementOnFeatureAllowed(tile, civInfo) -> "Remove Forest"
+            tile.terrainFeatures.contains(Constants.marsh) && !isImprovementOnFeatureAllowed(tile, civInfo) -> "Remove Marsh"
+            tile.terrainFeatures.contains("Fallout") && !isImprovementOnFeatureAllowed(tile, civInfo) -> "Remove Fallout"    // for really mad modders
+            tile.terrainFeatures.contains(Constants.jungle) && !isImprovementOnFeatureAllowed(tile, civInfo) -> "Remove Jungle"
+            tile.terrainFeatures.contains(Constants.forest) && !isImprovementOnFeatureAllowed(tile, civInfo) -> "Remove Forest"
             else -> tile.getTileResource().improvement
         }
 
@@ -207,11 +207,11 @@ class WorkerAutomation(val unit: MapUnit) {
             // I think we can assume that the unique improvement is better
             uniqueImprovement != null && tile.canBuildImprovement(uniqueImprovement, civInfo) -> uniqueImprovement.name
 
-            tile.terrainFeature == "Fallout" -> "Remove Fallout"
-            tile.terrainFeature == Constants.marsh -> "Remove Marsh"
-            tile.terrainFeature == Constants.jungle -> Constants.tradingPost
-            tile.terrainFeature == "Oasis" -> null
-            tile.terrainFeature == Constants.forest -> "Lumber mill"
+            tile.terrainFeatures.contains("Fallout") -> "Remove Fallout"
+            tile.terrainFeatures.contains(Constants.marsh) -> "Remove Marsh"
+            tile.terrainFeatures.contains(Constants.jungle) -> Constants.tradingPost
+            tile.terrainFeatures.contains("Oasis") -> null
+            tile.terrainFeatures.contains(Constants.forest) -> "Lumber mill"
             tile.isHill() -> "Mine"
             tile.baseTerrain in listOf(Constants.grassland, Constants.desert, Constants.plains) -> "Farm"
             tile.isAdjacentToFreshwater -> "Farm"
@@ -228,7 +228,7 @@ class WorkerAutomation(val unit: MapUnit) {
                 ?: return false
         val resourceImprovement = civInfo.gameInfo.ruleSet.tileImprovements[resourceImprovementName]
                 ?: return false
-        return resourceImprovement.resourceTerrainAllow.contains(tile.terrainFeature!!)
+        return tile.terrainFeatures.any { resourceImprovement.resourceTerrainAllow.contains(it) }
     }
 
     private fun isAcceptableTileForFort(tile: TileInfo, civInfo: CivilizationInfo): Boolean {

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

@@ -399,7 +399,7 @@ class MapUnit {
             tile.improvementInProgress!!.startsWith("Remove") -> {
                 val tileImprovement = tile.getTileImprovement()
                 if (tileImprovement != null
-                        && tileImprovement.terrainsCanBeBuiltOn.contains(tile.terrainFeature)
+                        && tile.terrainFeatures.any { tileImprovement.terrainsCanBeBuiltOn.contains(it) }
                         && !tileImprovement.terrainsCanBeBuiltOn.contains(tile.baseTerrain)) {
                     tile.improvement = null // We removed a terrain (e.g. Forest) and the improvement (e.g. Lumber mill) requires it!
                     if (tile.resource != null) civInfo.updateDetailedCivResources()        // unlikely, but maybe a mod makes a resource improvement dependent on a terrain feature
@@ -407,11 +407,12 @@ class MapUnit {
                 if (tile.improvementInProgress == "Remove Road" || tile.improvementInProgress == "Remove Railroad")
                     tile.roadStatus = RoadStatus.None
                 else {
-                    // We put "tile.terrainFeature!=null" because of a strange edge case that SHOULD be solved from 3.11.11+, so we should remove it then and see
-                    if (tile.terrainFeature != null && tile.tileMap.gameInfo.ruleSet.terrains[tile.terrainFeature!!]!!.uniques
-                                    .contains("Provides a one-time Production bonus to the closest city when cut down"))
-                        tryProvideProductionToClosestCity()
-                    tile.terrainFeature = null
+                    val removedFeature = tile.improvementInProgress!!.removePrefix("Remove ")
+                    if (tile.ruleset.terrains[removedFeature]!!.uniques
+                                    .contains("Provides a one-time Production bonus to the closest city when cut down")) {
+                        tryProvideProductionToClosestCity(removedFeature)
+                    }
+                    tile.terrainFeatures.remove(removedFeature)
                 }
             }
             tile.improvementInProgress == "Road" -> tile.roadStatus = RoadStatus.Road
@@ -424,7 +425,7 @@ class MapUnit {
         tile.improvementInProgress = null
     }
 
-    private fun tryProvideProductionToClosestCity() {
+    private fun tryProvideProductionToClosestCity(removedTerrainFeature: String) {
         val tile = getTile()
         val closestCity = civInfo.cities.minByOrNull { it.getCenterTile().aerialDistanceTo(tile) }
         if (closestCity == null) return
@@ -433,7 +434,7 @@ class MapUnit {
         if (tile.owningCity == null || tile.owningCity!!.civInfo != civInfo) productionPointsToAdd = productionPointsToAdd * 2 / 3
         if (productionPointsToAdd > 0) {
             closestCity.cityConstructions.addProductionPoints(productionPointsToAdd)
-            civInfo.addNotification("Clearing a [${tile.terrainFeature}] has created [$productionPointsToAdd] Production for [${closestCity.name}]", closestCity.location, Color.BROWN)
+            civInfo.addNotification("Clearing a [$removedTerrainFeature] has created [$productionPointsToAdd] Production for [${closestCity.name}]", closestCity.location, Color.BROWN)
         }
 
     }

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

@@ -53,36 +53,17 @@ open class TileInfo {
     lateinit var baseTerrain: String
     val terrainFeatures: ArrayList<String> = ArrayList()
 
-    //@Transient // So it won't be serialized from now on
-    @Deprecated(message = "Since 3.13.7 - gets replaced by terrainFeatures")
-    var terrainFeature: String? = null
-        get() {
-            //if terrainFeatures contains no terrainFeature maybe one got deserialized to field
-            if (terrainFeatures.firstOrNull() == null && field != null) {
-                terrainFeatures.add(field!!)
-                field = null
-            }
-            return terrainFeatures.firstOrNull()
-        }
-        set(value) {
-            if (terrainFeatures.isNotEmpty()) {
-                if (value == null) terrainFeatures.removeAt(0)
-                else terrainFeatures[0] = value
-            } else {
-                if (value != null) terrainFeatures.add(value)
-            }
-            field = null
-        }
-
+    // Deprecation level can't be increased because of convertTerrainFeatureToArray
+    // Can't be flagged transient because it won't deserialize then
+    // but it should not serialize because it always has the default value on serialization and is flagged optional
+    // Can be removed together with convertTerrainFeatureToArray to drop support for save files from version 3.13.7 and below
+    @Deprecated(message = "Since 3.13.7 - replaced by terrainFeatures")
+    private var terrainFeature: String? = null
     private fun convertTerrainFeatureToArray() {
-        if (terrainFeatures.firstOrNull() == null && terrainFeature != null)// -> terranFeature getter returns terrainFeature field
-            terrainFeature = terrainFeature // getter returns field, setter calls terrainFeatures.add()
-        //Note to Future GGGuenni
-        //TODO Use the following when terrainFeature got changed everywhere to support old saves in the future
-        //if (terrainFeature != null){
-        //terrainFeatures.add(terrainFeature)
-        //terrainFeature = null
-        //}
+        if (terrainFeature != null) {
+            terrainFeatures.add(terrainFeature!!)
+            terrainFeature = null
+        }
     }
 
     var naturalWonder: String? = null
@@ -437,12 +418,15 @@ open class TileInfo {
     fun aerialDistanceTo(otherTile: TileInfo): Int {
         val xDelta = position.x - otherTile.position.x
         val yDelta = position.y - otherTile.position.y
-        val distance = listOf(abs(xDelta), abs(yDelta), abs(xDelta - yDelta)).maxOrNull()!!
+        val distance = maxOf(abs(xDelta), abs(yDelta), abs(xDelta - yDelta))
 
-        val otherTileUnwrappedPos = tileMap.getUnWrappedPosition(otherTile.position)
-        val xDeltaWrapped = position.x - otherTileUnwrappedPos.x
-        val yDeltaWrapped = position.y - otherTileUnwrappedPos.y
-        val wrappedDistance = listOf(abs(xDeltaWrapped), abs(yDeltaWrapped), abs(xDeltaWrapped - yDeltaWrapped)).maxOrNull()!!
+        var wrappedDistance = Float.MAX_VALUE
+        if (tileMap.mapParameters.worldWrap) {
+            val otherTileUnwrappedPos = tileMap.getUnWrappedPosition(otherTile.position)
+            val xDeltaWrapped = position.x - otherTileUnwrappedPos.x
+            val yDeltaWrapped = position.y - otherTileUnwrappedPos.y
+            wrappedDistance = maxOf(abs(xDeltaWrapped), abs(yDeltaWrapped), abs(xDeltaWrapped - yDeltaWrapped))
+        }
 
         return min(distance, wrappedDistance).toInt()
     }

core/src/com/unciv/logic/map/mapgenerator/RiverGenerator.kt

@@ -22,7 +22,7 @@ class RiverGenerator(val randomness: MapGenerationRandomness){
 
         for(tile in map.values){
             if(tile.isAdjacentToRiver()){
-                if(tile.baseTerrain== Constants.desert && !tile.isHill()) tile.terrainFeatures.add(Constants.floodPlains)
+                if(tile.baseTerrain == Constants.desert && tile.terrainFeatures.isEmpty()) tile.terrainFeatures.add(Constants.floodPlains)
                 else if(tile.baseTerrain== Constants.snow) tile.baseTerrain = Constants.tundra
                 else if(tile.baseTerrain== Constants.tundra) tile.baseTerrain = Constants.plains
                 tile.setTerrainTransients()

core/src/com/unciv/ui/CivilopediaScreen.kt

@@ -166,7 +166,7 @@ class CivilopediaScreen(ruleset: Ruleset) : CameraStageBaseScreen() {
                 tileInfo.baseTerrain = terrain.turnsInto ?: Constants.grassland
             }
             TerrainType.TerrainFeature -> {
-                tileInfo.terrainFeature = terrain.name
+                tileInfo.terrainFeatures.add(terrain.name)
                 tileInfo.baseTerrain = terrain.occursOn.lastOrNull() ?: Constants.grassland
             }
             else ->

core/src/com/unciv/ui/mapeditor/TileEditorOptionsTable.kt

@@ -85,7 +85,7 @@ class TileEditorOptionsTable(val mapEditorScreen: MapEditorScreen): Table(Camera
         terrainFeaturesTable.add(getHex(getRedCross(50f, 0.6f)).apply {
             onClick {
                 tileAction = {
-                    it.terrainFeature = null
+                    it.terrainFeatures.clear()
                     it.naturalWonder = null
                     it.hasBottomRiver = false
                     it.hasBottomLeftRiver = false
@@ -325,7 +325,7 @@ class TileEditorOptionsTable(val mapEditorScreen: MapEditorScreen): Table(Camera
                     tileInfo.baseTerrain =
                             if (terrainObject.occursOn.isNotEmpty()) terrainObject.occursOn.first()
                             else "Grassland"
-                    tileInfo.terrainFeature = terrain
+                    tileInfo.terrainFeatures.add(terrain)
                 } else tileInfo.baseTerrain = terrain
 
                 tileInfo.resource = resource.name
@@ -346,7 +346,7 @@ class TileEditorOptionsTable(val mapEditorScreen: MapEditorScreen): Table(Camera
                     terrain.occursOn.isNotEmpty() -> terrain.occursOn.first()
                     else -> "Grassland"
                 }
-                tileInfo.terrainFeature = terrain.name
+                tileInfo.terrainFeatures.add(terrain.name)
             } else tileInfo.baseTerrain = terrain.name
             val group = makeTileGroup(tileInfo)
 
@@ -354,7 +354,10 @@ class TileEditorOptionsTable(val mapEditorScreen: MapEditorScreen): Table(Camera
                 tileAction = {
                     it.naturalWonder = null // If we're setting a base terrain it should remove the nat wonder
                     when (terrain.type) {
-                        TerrainType.TerrainFeature -> it.terrainFeature = terrain.name
+                        TerrainType.TerrainFeature -> {
+                            if (terrain.occursOn.contains(it.getLastTerrain().name))
+                                it.terrainFeatures.add(terrain.name)
+                        }
                         TerrainType.NaturalWonder -> it.naturalWonder = terrain.name
                         else -> it.baseTerrain = terrain.name
                     }

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

@@ -87,7 +87,8 @@ class TileMapTests {
         tile1.baseTerrain = Constants.hill
         tile1.setTerrainTransients()
         tile2.baseTerrain = Constants.grassland
-        tile2.terrainFeature = Constants.forest
+        tile2.terrainFeatures.clear()
+        tile2.terrainFeatures.add(Constants.forest)
         tile2.setTerrainTransients()
         tile3.baseTerrain = Constants.coast
         tile3.setTerrainTransients()
@@ -114,7 +115,8 @@ class TileMapTests {
     @Test
     fun canSeeMountainFromForestOverHills() {
         tile1.baseTerrain = Constants.grassland
-        tile1.terrainFeature = Constants.forest
+        tile1.terrainFeatures.clear()
+        tile1.terrainFeatures.add(Constants.forest)
         tile1.setTerrainTransients()
         tile2.baseTerrain = Constants.hill
         tile2.setTerrainTransients()
@@ -131,7 +133,8 @@ class TileMapTests {
         tile1.baseTerrain = Constants.hill
         tile1.setTerrainTransients()
         tile2.baseTerrain = Constants.grassland
-        tile2.terrainFeature = Constants.forest
+        tile2.terrainFeatures.clear()
+        tile2.terrainFeatures.add(Constants.forest)
         tile2.setTerrainTransients()
         tile3.baseTerrain = Constants.hill
         tile3.setTerrainTransients()
@@ -172,10 +175,12 @@ class TileMapTests {
     @Test
     fun canNOTSeeOutThroughForest() {
         tile1.baseTerrain = Constants.grassland
-        tile1.terrainFeature = Constants.forest
+        tile1.terrainFeatures.clear()
+        tile1.terrainFeatures.add(Constants.forest)
         tile1.setTerrainTransients()
         tile2.baseTerrain = Constants.grassland
-        tile2.terrainFeature = Constants.forest
+        tile2.terrainFeatures.clear()
+        tile2.terrainFeatures.add(Constants.forest)
         tile2.setTerrainTransients()
         tile3.baseTerrain = Constants.grassland
         tile3.setTerrainTransients()
@@ -190,7 +195,8 @@ class TileMapTests {
         tile1.baseTerrain = Constants.coast
         tile1.setTerrainTransients()
         tile2.baseTerrain = Constants.grassland
-        tile2.terrainFeature = Constants.jungle
+        tile2.terrainFeatures.clear()
+        tile2.terrainFeatures.add(Constants.jungle)
         tile2.setTerrainTransients()
         tile3.baseTerrain = Constants.coast
         tile3.setTerrainTransients()

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

@@ -97,7 +97,8 @@ class UnitMovementAlgorithmsTests {
     @Test
     fun canNOTEnterIce() {
         tile.baseTerrain = Constants.ocean
-        tile.terrainFeature = Constants.ice
+        tile.terrainFeatures.clear()
+        tile.terrainFeatures.add(Constants.ice)
         tile.setTransients()
 
         for (type in UnitType.values()) {