No ruins undo (#10376)

* Encapsulate Undo functionality

* Fix Ruins-Undo exploit

* Reorg RuinsManager candidate determination

* Deep RuinsManager clone

* Revert "Fix Ruins-Undo exploit"

This reverts commit 6df6a1a071.

core/src/com/unciv/UncivGame.kt

@@ -38,6 +38,7 @@ import com.unciv.ui.screens.basescreen.BaseScreen
 import com.unciv.ui.screens.mainmenuscreen.MainMenuScreen
 import com.unciv.ui.screens.savescreens.LoadGameScreen
 import com.unciv.ui.screens.worldscreen.PlayerReadyScreen
+import com.unciv.ui.screens.worldscreen.UndoHandler.Companion.clearUndoCheckpoints
 import com.unciv.ui.screens.worldscreen.WorldMapHolder
 import com.unciv.ui.screens.worldscreen.WorldScreen
 import com.unciv.ui.screens.worldscreen.unit.UnitTable
@@ -111,6 +112,11 @@ object GUI {
         return UncivGame.Current.worldScreen!!.selectedCiv
     }
 
+    /** Disable Undo (as in: forget the way back, but allow future undo checkpoints) */
+    fun clearUndoCheckpoints() {
+        UncivGame.Current.worldScreen?.clearUndoCheckpoints()
+    }
+
     private var keyboardAvailableCache: Boolean? = null
     /** Tests availability of a physical keyboard */
     val keyboardAvailable: Boolean

core/src/com/unciv/logic/civilization/managers/ReligionManager.kt

@@ -141,10 +141,13 @@ class ReligionManager : IsPartOfGameInfoSerialization {
             UniqueTriggerActivation.triggerCivwideUnique(unique, civInfo)
     }
 
+    fun greatProphetsEarned(): Int = civInfo.civConstructions.boughtItemsWithIncreasingPrice[getGreatProphetEquivalent()?.name ?: ""]
+        // Counter.get never returns null, but it needs the correct key type, which is non-nullable
+
     // https://www.reddit.com/r/civ/comments/2m82wu/can_anyone_detail_the_finer_points_of_great/
     // Game files (globaldefines.xml)
     fun faithForNextGreatProphet(): Int {
-        val greatProphetsEarned = civInfo.civConstructions.boughtItemsWithIncreasingPrice[getGreatProphetEquivalent()!!.name]
+        val greatProphetsEarned = greatProphetsEarned()
 
         var faithCost =
             (200 + 100 * greatProphetsEarned * (greatProphetsEarned + 1) / 2f) *

core/src/com/unciv/logic/civilization/managers/RuinsManager.kt

@@ -1,5 +1,4 @@
 package com.unciv.logic.civilization.managers
-// Why is this the only file in its own package?
 
 import com.unciv.logic.IsPartOfGameInfoSerialization
 import com.unciv.logic.civilization.Civilization
@@ -10,49 +9,54 @@ import com.unciv.models.ruleset.unique.UniqueTriggerActivation
 import com.unciv.models.ruleset.unique.UniqueType
 import kotlin.random.Random
 
-class RuinsManager : IsPartOfGameInfoSerialization {
+class RuinsManager(
-    var lastChosenRewards: MutableList<String> = mutableListOf("", "")
+    private var lastChosenRewards: MutableList<String> = mutableListOf("", "")
-    private fun rememberReward(reward: String) {
+) : IsPartOfGameInfoSerialization {
-        lastChosenRewards[0] = lastChosenRewards[1]
-        lastChosenRewards[1] = reward
-    }
 
     @Transient
     lateinit var civInfo: Civilization
     @Transient
     lateinit var validRewards: List<RuinReward>
 
-    fun clone(): RuinsManager {
+    fun clone() = RuinsManager(ArrayList(lastChosenRewards))  // needs to deep-clone (the List, not the Strings) so undo works
-        val toReturn = RuinsManager()
-        toReturn.lastChosenRewards = lastChosenRewards
-        return toReturn
-    }
 
     fun setTransients(civInfo: Civilization) {
         this.civInfo = civInfo
         validRewards = civInfo.gameInfo.ruleset.ruinRewards.values.toList()
     }
 
+    private fun rememberReward(reward: String) {
+        lastChosenRewards[0] = lastChosenRewards[1]
+        lastChosenRewards[1] = reward
+    }
+
+    private fun getShuffledPossibleRewards(triggeringUnit: MapUnit): Iterable<RuinReward> {
+        val stateForOnlyAvailableWhen = StateForConditionals(civInfo, unit = triggeringUnit, tile = triggeringUnit.getTile())
+        val candidates =
+            validRewards.asSequence()
+            // Filter out what shouldn't be considered right now, before the random choice
+            .filterNot { possibleReward ->
+                possibleReward.name in lastChosenRewards
+                    || civInfo.gameInfo.difficulty in possibleReward.excludedDifficulties
+                    || possibleReward.hasUnique(UniqueType.HiddenWithoutReligion) && !civInfo.gameInfo.isReligionEnabled()
+                    || possibleReward.hasUnique(UniqueType.HiddenAfterGreatProphet) && civInfo.religionManager.greatProphetsEarned() > 0
+                    || possibleReward.getMatchingUniques(UniqueType.OnlyAvailableWhen, StateForConditionals.IgnoreConditionals)
+                    .any { !it.conditionalsApply(stateForOnlyAvailableWhen) }
+            }
+            // This might be a dirty way to do this, but it works (we do have randomWeighted in CollectionExtensions, but below we
+            // need to choose another when the first choice's TriggerActivations report failure, and that's simpler this way)
+            // For each possible reward, this feeds (reward.weight) copies of this reward to the overall Sequence to implement 'weight'.
+            .flatMap { reward -> generateSequence { reward }.take(reward.weight) }
+            // Convert to List since Sequence.shuffled would do one anyway, Mutable so shuffle doesn't need to pull a copy
+            .toMutableList()
+        // The resulting List now gets shuffled, using a tile-based random to thwart save-scumming.
+        // Note both Sequence.shuffled and Iterable.shuffled (with a 'd') always pull an extra copy of a MutableList internally, even if you feed them one.
+        candidates.shuffle(Random(triggeringUnit.getTile().position.hashCode()))
+        return candidates
+    }
+
     fun selectNextRuinsReward(triggeringUnit: MapUnit) {
-        val tileBasedRandom = Random(triggeringUnit.getTile().position.toString().hashCode())
+        for (possibleReward in getShuffledPossibleRewards(triggeringUnit)) {
-        val availableRewards = validRewards.filter { it.name !in lastChosenRewards }
-
-        // This might be a dirty way to do this, but it works.
-        // For each possible reward, this creates a list with reward.weight amount of copies of this reward
-        // These lists are then combined into a single list, and the result is shuffled.
-        val possibleRewards = availableRewards.flatMap { reward -> List(reward.weight) { reward } }.shuffled(tileBasedRandom)
-
-        for (possibleReward in possibleRewards) {
-            if (civInfo.gameInfo.difficulty in possibleReward.excludedDifficulties) continue
-            if (possibleReward.hasUnique(UniqueType.HiddenWithoutReligion) && !civInfo.gameInfo.isReligionEnabled()) continue
-            if (possibleReward.hasUnique(UniqueType.HiddenAfterGreatProphet)
-                && (civInfo.civConstructions.boughtItemsWithIncreasingPrice[civInfo.religionManager.getGreatProphetEquivalent()?.name] ?: 0) > 0
-            ) continue
-
-            if (possibleReward.getMatchingUniques(UniqueType.OnlyAvailableWhen, StateForConditionals.IgnoreConditionals)
-                        .any { !it.conditionalsApply(StateForConditionals(civInfo, unit=triggeringUnit, tile = triggeringUnit.getTile()) ) })
-                continue
-
             var atLeastOneUniqueHadEffect = false
             for (unique in possibleReward.uniqueObjects) {
                 atLeastOneUniqueHadEffect =

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

@@ -239,10 +239,7 @@ open class Tile : IsPartOfGameInfoSerialization {
         if (isExplored) {
             // Disable the undo button if a new tile has been explored
             if (!exploredBy.contains(player.civName)) {
-                if (GUI.isWorldLoaded()) {
+                GUI.clearUndoCheckpoints()
-                    val worldScreen = GUI.getWorldScreen()
-                    worldScreen.preActionGameInfo = worldScreen.gameInfo
-                }
                 exploredBy = exploredBy.withItem(player.civName)
             }
 

core/src/com/unciv/ui/screens/worldscreen/TechPolicyDiplomacyButtons.kt

@@ -20,7 +20,8 @@ import com.unciv.ui.screens.overviewscreen.EspionageOverviewScreen
 import com.unciv.ui.screens.pickerscreens.PolicyPickerScreen
 import com.unciv.ui.screens.pickerscreens.TechButton
 import com.unciv.ui.screens.pickerscreens.TechPickerScreen
-import com.unciv.utils.Concurrency
+import com.unciv.ui.screens.worldscreen.UndoHandler.Companion.canUndo
+import com.unciv.ui.screens.worldscreen.UndoHandler.Companion.restoreUndoCheckpoint
 
 
 /** A holder for Tech, Policies and Diplomacy buttons going in the top left of the WorldScreen just under WorldScreenTopBar */
@@ -118,7 +119,7 @@ class TechPolicyDiplomacyButtons(val worldScreen: WorldScreen) : Table(BaseScree
 
     private fun updateUndoButton() {
         // Don't show the undo button if there is no action to undo
-        if (worldScreen.gameInfo != worldScreen.preActionGameInfo && worldScreen.canChangeState) {
+        if (worldScreen.canUndo()) {
             undoButtonHolder.touchable = Touchable.enabled
             undoButtonHolder.actor = undoButton
         } else {
@@ -164,11 +165,6 @@ class TechPolicyDiplomacyButtons(val worldScreen: WorldScreen) : Table(BaseScree
 
     private fun handleUndo() {
         undoButton.disable()
-        Concurrency.run {
+        worldScreen.restoreUndoCheckpoint()
-            // Most of the time we won't load this, so we only set transients once we see it's relevant
-            worldScreen.preActionGameInfo.setTransients()
-            worldScreen.preActionGameInfo.isUpToDate = worldScreen.gameInfo.isUpToDate
-            game.loadGame(worldScreen.preActionGameInfo)
-        }
     }
 }

core/src/com/unciv/ui/screens/worldscreen/UndoHandler.kt

@@ -0,0 +1,39 @@
+package com.unciv.ui.screens.worldscreen
+
+import com.unciv.utils.Concurrency
+
+/** Encapsulates the Undo functionality.
+ *
+ *  Implementation is based on actively saving GameInfo clones and restoring them when needed.
+ *  For now, there's only one single Undo level (but the class signature could easily support more).
+ */
+class UndoHandler(private val worldScreen: WorldScreen) {
+    private var preActionGameInfo = worldScreen.gameInfo
+
+    fun canUndo() = preActionGameInfo != worldScreen.gameInfo && worldScreen.canChangeState
+
+    fun recordCheckpoint() {
+        preActionGameInfo = worldScreen.gameInfo.clone()
+    }
+
+    fun restoreCheckpoint() {
+        Concurrency.run {
+            // Most of the time we won't load this, so we only set transients once we see it's relevant
+            preActionGameInfo.setTransients()
+            preActionGameInfo.isUpToDate = worldScreen.gameInfo.isUpToDate  // Multiplayer!
+            worldScreen.game.loadGame(preActionGameInfo)
+        }
+    }
+
+    fun clearCheckpoints() {
+        preActionGameInfo = worldScreen.gameInfo
+    }
+
+    /** Simple readability proxies so the caller can pretend the interface exists directly on WorldScreen (imports ugly but calls neat) */
+    companion object {
+        fun WorldScreen.canUndo() = undoHandler.canUndo()
+        fun WorldScreen.recordUndoCheckpoint() = undoHandler.recordCheckpoint()
+        fun WorldScreen.restoreUndoCheckpoint() = undoHandler.restoreCheckpoint()
+        fun WorldScreen.clearUndoCheckpoints() = undoHandler.clearCheckpoints()
+    }
+}

core/src/com/unciv/ui/screens/worldscreen/WorldMapHolder.kt

@@ -32,8 +32,6 @@ import com.unciv.models.ruleset.unique.UniqueType
 import com.unciv.ui.audio.SoundPlayer
 import com.unciv.ui.components.MapArrowType
 import com.unciv.ui.components.MiscArrowTypes
-import com.unciv.ui.components.widgets.UnitGroup
-import com.unciv.ui.components.widgets.ZoomableScrollPane
 import com.unciv.ui.components.extensions.center
 import com.unciv.ui.components.extensions.colorFromRGB
 import com.unciv.ui.components.extensions.darken
@@ -48,9 +46,12 @@ import com.unciv.ui.components.tilegroups.TileGroup
 import com.unciv.ui.components.tilegroups.TileGroupMap
 import com.unciv.ui.components.tilegroups.TileSetStrings
 import com.unciv.ui.components.tilegroups.WorldTileGroup
+import com.unciv.ui.components.widgets.UnitGroup
+import com.unciv.ui.components.widgets.ZoomableScrollPane
 import com.unciv.ui.images.ImageGetter
 import com.unciv.ui.screens.basescreen.BaseScreen
 import com.unciv.ui.screens.basescreen.UncivStage
+import com.unciv.ui.screens.worldscreen.UndoHandler.Companion.recordUndoCheckpoint
 import com.unciv.ui.screens.worldscreen.bottombar.BattleTableHelpers.battleAnimation
 import com.unciv.utils.Concurrency
 import com.unciv.utils.Log
@@ -278,7 +279,7 @@ class WorldMapHolder(
             }
 
 
-            worldScreen.preActionGameInfo = worldScreen.gameInfo.clone()
+            worldScreen.recordUndoCheckpoint()
 
             launchOnGLThread {
                 try {

core/src/com/unciv/ui/screens/worldscreen/WorldScreen.kt

@@ -131,8 +131,7 @@ class WorldScreen(
 
     private var uiEnabled = true
 
-    var preActionGameInfo = gameInfo
+    internal val undoHandler = UndoHandler(this)
-
 
     init {
         // notifications are right-aligned, they take up only as much space as necessary.

core/src/com/unciv/ui/screens/worldscreen/bottombar/BattleTable.kt

@@ -28,6 +28,7 @@ import com.unciv.ui.components.input.onClick
 import com.unciv.ui.components.widgets.UnitGroup
 import com.unciv.ui.images.ImageGetter
 import com.unciv.ui.screens.basescreen.BaseScreen
+import com.unciv.ui.screens.worldscreen.UndoHandler.Companion.clearUndoCheckpoints
 import com.unciv.ui.screens.worldscreen.WorldScreen
 import com.unciv.ui.screens.worldscreen.bottombar.BattleTableHelpers.battleAnimation
 import com.unciv.ui.screens.worldscreen.bottombar.BattleTableHelpers.getHealthBar
@@ -285,7 +286,7 @@ class BattleTable(val worldScreen: WorldScreen): Table() {
         // There was a direct worldScreen.update() call here, removing its 'private' but not the comment justifying the modifier.
         // My tests (desktop only) show the red-flash animations look just fine without.
         worldScreen.shouldUpdate = true
-        worldScreen.preActionGameInfo = worldScreen.gameInfo // Reset - can no longer undo
+        worldScreen.clearUndoCheckpoints()
         //Gdx.graphics.requestRendering()  // Use this if immediate rendering is required
 
         if (!canStillAttack) return