Clearing a barbarian camp on a tile you own leaves the "improvement" behind

[SomeTroglodyte]

Is there an existing issue for this?

• [X] I have searched the existing issues

Game Version

from current source

Describe the bug

See title?

Steps to Reproduce

1. Found a city 2 tiles from a barbarian camp
2. Buy the tiles around the city including the camp
3. Buy a Knight and use it to clear that camp next turn
4. You get the clear notification and the gold, but the tile "improvement" stays.

Screenshots

Link to save file

Will need to try to repro on a smaller map

Operating System

Windows

Additional Information

Found #6560, but I doubt there's a connection.

The bogus improvement disappears on save/reload or next-turn. Console tile removeimprovement has no effect. Aha! That nasty lastSeenImprovement again! selectedTile.getOwner().lastSeenImprovement[selectedTile.position] - there it is.

[SeventhM]

I wonder if this lastSeenImprovenent problem is related to a similar issue: if you place an improvement via console, it won't update correctly unless you reload/go to next turn

[SomeTroglodyte]

I wonder if

It certainly is. Console also lacks a rule check - as in a call to that normalization function editor uses, and a stats/yield update for a possibly owning city. It's console after all. But all these are "fixed" by saving and reloading...

So - where did that code go that updated lastSeenImprovement from changeImprovement, not only from nextTurn?

And why does it seem to work at all when you "normally" clear a camp??????

[SomeTroglodyte]

Adding a 'all-human-civs, if tile visible, update lastseen' loop to changeImprovement only shows: Crashes mapgen as it is used all over the place - normalize tile, ruins, wonders. ~Which probably means mods with triggers firing from improvement placement or removal will also crash. Do we have such triggers?~ No, triggers happen to be ignored for changeImrpovement calls without a civToActivateBroaderEffects.

So, this patch:

Index: core/src/com/unciv/logic/map/TileMap.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/src/com/unciv/logic/map/TileMap.kt b/core/src/com/unciv/logic/map/TileMap.kt
--- a/core/src/com/unciv/logic/map/TileMap.kt	(revision 6f11f311a8fdf28f61d0fb06e3494a81fddabe97)
+++ b/core/src/com/unciv/logic/map/TileMap.kt	(date 1705647615821)
@@ -60,6 +60,7 @@
     /** Attention: lateinit will _stay uninitialized_ while in MapEditorScreen! */
     @Transient
     lateinit var gameInfo: GameInfo
+    fun hasGameInfo() = ::gameInfo.isInitialized
 
     /** Keep a copy of the [Ruleset] object passed to setTransients, for now only to allow subsequent setTransients without. Copied on [clone]. */
     @Transient
Index: core/src/com/unciv/models/ruleset/unique/UniqueTriggerActivation.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/src/com/unciv/models/ruleset/unique/UniqueTriggerActivation.kt b/core/src/com/unciv/models/ruleset/unique/UniqueTriggerActivation.kt
--- a/core/src/com/unciv/models/ruleset/unique/UniqueTriggerActivation.kt	(revision 6f11f311a8fdf28f61d0fb06e3494a81fddabe97)
+++ b/core/src/com/unciv/models/ruleset/unique/UniqueTriggerActivation.kt	(date 1705646254220)
@@ -605,10 +605,7 @@
                 for (explorableTile in explorableTiles) {
                     explorableTile.setExplored(civInfo, true)
                     positions += explorableTile.position
-                    if (explorableTile.improvement == null)
-                        civInfo.lastSeenImprovement.remove(explorableTile.position)
-                    else
-                        civInfo.lastSeenImprovement[explorableTile.position] = explorableTile.improvement!!
+                    civInfo.cache.updateLastSeenImprovement(explorableTile)
                 }
 
                 if (notification != null) {
Index: core/src/com/unciv/logic/map/tile/TileInfoImprovementFunctions.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/src/com/unciv/logic/map/tile/TileInfoImprovementFunctions.kt b/core/src/com/unciv/logic/map/tile/TileInfoImprovementFunctions.kt
--- a/core/src/com/unciv/logic/map/tile/TileInfoImprovementFunctions.kt	(revision 6f11f311a8fdf28f61d0fb06e3494a81fddabe97)
+++ b/core/src/com/unciv/logic/map/tile/TileInfoImprovementFunctions.kt	(date 1705647830324)
@@ -128,7 +128,7 @@
             // Can only remove roads if that road is actually there
             RoadStatus.values().any { it.removeAction == improvement.name } -> tile.roadStatus.removeAction == improvement.name
             // Can only remove features or improvement if that feature/improvement is actually there
-            improvement.name.startsWith(Constants.remove) -> tile.terrainFeatures.any { Constants.remove + it == improvement.name } 
+            improvement.name.startsWith(Constants.remove) -> tile.terrainFeatures.any { Constants.remove + it == improvement.name }
                 || Constants.remove + tile.improvement == improvement.name
             // Can only build roads if on land and they are better than the current road
             RoadStatus.values().any { it.name == improvement.name } -> !tile.isWater
@@ -207,26 +207,26 @@
             }
         }
 
-        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 removingAction = "${Constants.remove}$feature"
+        if (improvementObject != null) {
+            if (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 removingAction = "${Constants.remove}$feature"
 
-                removingAction in tile.ruleset.tileImprovements // is removable
-                    && !improvementObject.isAllowedOnFeature(feature) // cannot coexist
-            }
+                    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 { it in removableTerrainFeatures })
+            }
 
-        if (civToActivateBroaderEffects != null && improvementObject != null
-            && improvementObject.hasUnique(UniqueType.TakesOverAdjacentTiles)
-        )
-            takeOverTilesAround(civToActivateBroaderEffects, tile)
+            if (civToActivateBroaderEffects != null && improvementObject.hasUnique(UniqueType.TakesOverAdjacentTiles))
+                takeOverTilesAround(civToActivateBroaderEffects, tile)
 
-        if (civToActivateBroaderEffects != null && improvementObject != null)
-            triggerImprovementUniques(improvementObject, civToActivateBroaderEffects, unit)
+            if (civToActivateBroaderEffects != null)
+                triggerImprovementUniques(improvementObject, civToActivateBroaderEffects, unit)
+        }
 
         val city = tile.owningCity
         if (city != null) {
@@ -236,6 +236,14 @@
                 city.reassignPopulationDeferred()
             }
         }
+
+        // Update seen improvement for all humans that can see the tile
+        if (!tile.tileMap.hasGameInfo()) return // Mapgen calls us
+        for (civ in tile.tileMap.gameInfo.civilizations) {
+            if (!civ.isHuman()) continue
+            if (tile !in civ.viewableTiles) continue
+            civ.cache.updateLastSeenImprovement(tile)
+        }
     }
 
     private fun triggerImprovementUniques(
@@ -249,7 +257,7 @@
             }
             else UniqueTriggerActivation.triggerCivwideUnique(unique, civ, tile = tile)
 
-        if (unit != null){
+        if (unit != null) {
             for (unique in unit.getTriggeredUniques(UniqueType.TriggerUponBuildingImprovement)
                 .filter { improvement.matchesFilter(it.params[0]) })
                 UniqueTriggerActivation.triggerUnitwideUnique(unique, unit)
Index: core/src/com/unciv/logic/civilization/transients/CivInfoTransientCache.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/src/com/unciv/logic/civilization/transients/CivInfoTransientCache.kt b/core/src/com/unciv/logic/civilization/transients/CivInfoTransientCache.kt
--- a/core/src/com/unciv/logic/civilization/transients/CivInfoTransientCache.kt	(revision 6f11f311a8fdf28f61d0fb06e3494a81fddabe97)
+++ b/core/src/com/unciv/logic/civilization/transients/CivInfoTransientCache.kt	(date 1705646565473)
@@ -187,15 +187,22 @@
         civInfo.viewableTiles = newViewableTiles // to avoid concurrent modification problems
     }
 
-    private fun updateLastSeenImprovements() {
-        if (civInfo.playerType == PlayerType.AI) return // don't bother for AI, they don't really use the info anyway
-
-        for (tile in civInfo.viewableTiles) {
-            if (tile.improvement == null)
-                civInfo.lastSeenImprovement.remove(tile.position)
-            else
-                civInfo.lastSeenImprovement[tile.position] = tile.improvement!!
-        }
+    /** This updates what improvement this civ sees on [tile]
+     *  ...unless the civ is an AI (this is purely visual, the AI will only use the real info)
+     *
+     *  Does **not** include the visibility check - that _must_ be ensured by the caller.
+     *  Mutates [Civilization.lastSeenImprovement].
+     */
+    fun updateLastSeenImprovement(tile: Tile) {
+        if (civInfo.playerType == PlayerType.AI) return
+        if (tile.improvement == null)
+            civInfo.lastSeenImprovement.remove(tile.position)
+        else
+            civInfo.lastSeenImprovement[tile.position] = tile.improvement!!
+    }
+
+    private fun updateLastSeenImprovements() {
+        for (tile in civInfo.viewableTiles) updateLastSeenImprovement(tile)
     }
 
     private fun discoverNaturalWonders() {

..will indeed solve this and the console issue and possibly some more edge case visual glitches... But is that the right/best approach? I doubt.

[SeventhM]

And why does it seem to work at all when you "normally" clear a camp??????

I suspect it has something to do with unit movement. It's normally supposed to call MapUnit.updateVisibleTiles upon moving anywhere. Let's see, yep, there you find

// Don't bother updating if all previous and current viewable tiles are within our borders

I think your patch is probably fine, I'm not sure there's ever a situation where a Civ places an improvement and shouldn't update seen tiles, though I'm not sure if it's necessary to update the the viewed improvements for everyone and not just the player in question

[SomeTroglodyte]

your patch is probably fine

I don't feel really fine about it - namely why does mapgen use that high-level function in the first place? (It uses changeImprovement to place ruins when at this point you don't really want all context to be guaranteed - transients and potential worldscreen interaction? Should check - when bored. In other words: fun hasGameInfo() = ::gameInfo.isInitialized feels like a kludge.)

But big kudos for finding that comment - I didn't.

[SeventhM]

feels like a kludge.

Well, similar to improvement triggers, if you're only updating for 1 civ, rather than all civs, you don't need hasGameInfo. Hence me wondering why your patch does it when none of the other places that update improvements currently do. Probably a bigger discussion to be had about updating viewable tiles though. Probably stuff needs to be double checked, but I doubt there's that much harm in using the high level version instead of directly setting the improvement. It's not like addTechnology where we need to update eras and multiple caches

Editing note I thought about after replying: your patch needs a way to not trigger when we're using change improvement on a fake tile, such the getDiffImprovementStats function

[github-actions[bot]]

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 15 days.

[SeventhM]

Given its impact as a console bug, I'll unstale this here. I need to look over this again, But like I said before, Trog's patch, albeit edited a bit to not update everyone's vision unnecessarily and to work (or, uh, not work) with tile clones should be fine enough

[SomeTroglodyte]

impact as a console bug

Wasn't there an issue recently reproing without console? Something with ruins?

Trog's patch, albeit

Exactly. Wasn't having the "that's clean" feeling so I didn't PR It - go ahead and make it better (like The Beatles song.)