Fix ruleset error crash (#9666)

* Harden NewGameScreen against exceptions from getComplexRuleset

* Miscellaneous tweaks to NewGameScreen - e.g. allow it to show last used custom map

android/assets/jsons/translations/template.properties

@@ -360,6 +360,7 @@ Domination =
 Cultural = 
 Diplomatic = 
 Time = 
+Your previous options needed to be reset to defaults. = 
 
 # Used for random nation indicator in empire selector and unknown nation icons in various overview screens.
 # Should be a single character, or at least visually square.

core/src/com/unciv/ui/screens/newgamescreen/MapFileSelectTable.kt

@@ -10,9 +10,9 @@ import com.unciv.UncivGame
 import com.unciv.logic.files.MapSaver
 import com.unciv.logic.map.MapParameters
 import com.unciv.models.ruleset.RulesetCache
-import com.unciv.ui.components.input.onChange
 import com.unciv.ui.components.extensions.pad
 import com.unciv.ui.components.extensions.toLabel
+import com.unciv.ui.components.input.onChange
 import com.unciv.ui.screens.basescreen.BaseScreen
 import com.unciv.ui.screens.victoryscreen.LoadMapPreview
 import com.unciv.utils.Concurrency
@@ -32,7 +32,7 @@ class MapFileSelectTable(
     // The SelectBox auto displays the text a object.toString(), which on the FileHandle itself includes the folder path.
     //  So we wrap it in another object with a custom toString()
     private class MapWrapper(val fileHandle: FileHandle, val mapParameters: MapParameters) {
-        override fun toString(): String = mapParameters.baseRuleset + " | "+ fileHandle.name()
+        override fun toString(): String = mapParameters.baseRuleset + " | " + fileHandle.name()
     }
     private val mapWrappers= ArrayList<MapWrapper>()
 
@@ -44,10 +44,11 @@ class MapFileSelectTable(
         add(mapFileLabel).left()
         add(mapFileSelectBox)
             // because SOME people gotta give the hugest names to their maps
-            .width(columnWidth * 2 / 3)
+            .width(columnWidth - 40f - mapFileLabel.prefWidth)
             .right().row()
         add(miniMapWrapper)
             .pad(15f)
+            .maxWidth(columnWidth - 20f)
             .colspan(2).center().row()
 
         mapFileSelectBox.onChange { onSelectBoxChange() }
@@ -86,7 +87,6 @@ class MapFileSelectTable(
             .sortedWith(compareBy(UncivGame.Current.settings.getCollatorFromLocale()) { it.toString() })
             .sortedByDescending { it.mapParameters.baseRuleset == newGameScreen.gameSetupInfo.gameParameters.baseRuleset }
             .forEach { mapFiles.add(it) }
-        mapFileSelectBox.items = mapFiles
 
         // Pre-select: a) map saved within last 15min or b) map named in mapParameters or c) alphabetically first
         // This is a kludge - the better way would be to have a "play this map now" menu button in the editor
@@ -97,9 +97,21 @@ class MapFileSelectTable(
                     ?: mapFiles.firstOrNull { it.fileHandle.name() == mapParameters.name }
                     ?: mapFiles.firstOrNull()
                     ?: return
+
+        // since mapFileSelectBox.selection.setProgrammaticChangeEvents() defaults to true, this would
+        // kill the mapParameters.name we would like to look for when determining what to pre-select -
+        // so do it ***after*** getting selectedItem - but control programmaticChangeEvents anyway
+        // to avoid the overhead of doing a full updateRuleset + updateTables + startMapPreview
+        // (all expensive) for something that will be overthrown momentarily
+        mapFileSelectBox.selection.setProgrammaticChangeEvents(false)
+        mapFileSelectBox.items = mapFiles
+
+        // Now, we want this ON because the event removes map selections which are pulling mods
+        // that trip updateRuleset - so that code should still be active for the pre-selection
+        mapFileSelectBox.selection.setProgrammaticChangeEvents(true)
         mapFileSelectBox.selected = selectedItem
-        mapParameters.name = selectedItem.toString()
+        // In the event described above, we now have: mapFileSelectBox.selected != selectedItem
-        newGameScreen.gameSetupInfo.mapFile = selectedItem.fileHandle
+        // Do NOT try to put back the "bad" preselection!
     }
 
     private fun onSelectBoxChange() {
@@ -111,10 +123,20 @@ class MapFileSelectTable(
         newGameScreen.gameSetupInfo.gameParameters.mods = LinkedHashSet(mapMods.second)
         newGameScreen.gameSetupInfo.gameParameters.baseRuleset = mapMods.first.firstOrNull()
             ?: mapFileSelectBox.selected.mapParameters.baseRuleset
-        newGameScreen.updateRuleset()
+        val success = newGameScreen.updateRuleset()
         newGameScreen.updateTables()
         hideMiniMap()
-        startMapPreview(mapFile)
+        if (success) {
+            startMapPreview(mapFile)
+        } else {
+            // Mod error - the options have been reset by updateRuleset
+            // Note SelectBox doesn't react sensibly to _any_ clear - Group, Selection or items
+            val items = mapFileSelectBox.items
+            items.removeIndex(mapFileSelectBox.selectedIndex)
+            // Changing the array itself is not enough, SelectBox gets out of sync, need to call setItems()
+            mapFileSelectBox.items = items
+            // Note - this will have triggered a nested onSelectBoxChange()!
+        }
     }
 
     private fun startMapPreview(mapFile: FileHandle) {
@@ -124,7 +146,7 @@ class MapFileSelectTable(
                 if (!isActive) return@run
                 map.setTransients(newGameScreen.ruleset, false)
                 if (!isActive) return@run
-                // ReplyMap still paints outside its bounds - so we subtract padding and a little extra
+                // ReplayMap still paints outside its bounds - so we subtract padding and a little extra
                 val size = (columnWidth - 40f).coerceAtMost(500f)
                 val miniMap = LoadMapPreview(map, size, size)
                 if (!isActive) return@run

core/src/com/unciv/ui/screens/newgamescreen/MapOptionsTable.kt

@@ -2,11 +2,11 @@ package com.unciv.ui.screens.newgamescreen
 
 import com.badlogic.gdx.scenes.scene2d.ui.Table
 import com.unciv.logic.map.MapGeneratedMainType
-import com.unciv.ui.components.input.onChange
 import com.unciv.ui.components.extensions.toLabel
+import com.unciv.ui.components.input.onChange
 import com.unciv.ui.screens.basescreen.BaseScreen
 
-class MapOptionsTable(private val newGameScreen: NewGameScreen): Table() {
+class MapOptionsTable(private val newGameScreen: NewGameScreen, isReset: Boolean = true): Table() {
 
     private val mapParameters = newGameScreen.gameSetupInfo.mapParameters
     private var mapTypeSpecificTable = Table()
@@ -22,7 +22,14 @@ class MapOptionsTable(private val newGameScreen: NewGameScreen): Table() {
 
         val mapTypes = arrayListOf(MapGeneratedMainType.generated, MapGeneratedMainType.randomGenerated)
         if (savedMapOptionsTable.isNotEmpty()) mapTypes.add(MapGeneratedMainType.custom)
-        mapTypeSelectBox = TranslatedSelectBox(mapTypes, "Generated", BaseScreen.skin)
+
+        // Pre-select custom if any map saved within last 15 minutes
+        val chooseCustom = !isReset && (
+                savedMapOptionsTable.recentlySavedMapExists() ||
+                savedMapOptionsTable.isNotEmpty() && mapParameters.type == MapGeneratedMainType.custom && mapParameters.name.isNotEmpty()
+            )
+        val mapTypeDefault = if (chooseCustom) MapGeneratedMainType.custom else MapGeneratedMainType.generated
+        mapTypeSelectBox = TranslatedSelectBox(mapTypes, mapTypeDefault, BaseScreen.skin)
 
         fun updateOnMapTypeChange() {
             mapTypeSpecificTable.clear()
@@ -50,11 +57,6 @@ class MapOptionsTable(private val newGameScreen: NewGameScreen): Table() {
             newGameScreen.updateTables()
         }
 
-        // Pre-select custom if any map saved within last 15 minutes
-        if (savedMapOptionsTable.recentlySavedMapExists())
-            mapTypeSelectBox.selected =
-                    TranslatedSelectBox.TranslatedString(MapGeneratedMainType.custom)
-
         // activate once, so when we had a file map before we'll have the right things set for another one
         updateOnMapTypeChange()
 

core/src/com/unciv/ui/screens/newgamescreen/NewGameScreen.kt

@@ -11,12 +11,15 @@ import com.unciv.UncivGame
 import com.unciv.logic.GameInfo
 import com.unciv.logic.GameStarter
 import com.unciv.logic.IdChecker
+import com.unciv.logic.UncivShowableException
 import com.unciv.logic.civilization.PlayerType
 import com.unciv.logic.files.MapSaver
 import com.unciv.logic.map.MapGeneratedMainType
 import com.unciv.logic.multiplayer.OnlineMultiplayer
 import com.unciv.logic.multiplayer.storage.FileStorageRateLimitReached
+import com.unciv.models.metadata.BaseRuleset
 import com.unciv.models.metadata.GameSetupInfo
+import com.unciv.models.ruleset.Ruleset
 import com.unciv.models.ruleset.RulesetCache
 import com.unciv.models.translations.tr
 import com.unciv.ui.components.ExpanderTab
@@ -44,14 +47,16 @@ import com.unciv.utils.launchOnGLThread
 import kotlinx.coroutines.coroutineScope
 import java.net.URL
 import java.util.UUID
+import kotlin.math.floor
 import com.unciv.ui.components.AutoScrollPane as ScrollPane
 
 class NewGameScreen(
-    _gameSetupInfo: GameSetupInfo? = null
+    defaultGameSetupInfo: GameSetupInfo? = null,
+    isReset: Boolean = false
 ): IPreviousScreen, PickerScreen(), RecreateOnResize {
 
-    override val gameSetupInfo = _gameSetupInfo ?: GameSetupInfo.fromSettings()
+    override var gameSetupInfo = defaultGameSetupInfo ?: GameSetupInfo.fromSettings()
-    override var ruleset = RulesetCache.getComplexRuleset(gameSetupInfo.gameParameters) // needs to be set because the GameOptionsTable etc. depend on this
+    override val ruleset = Ruleset()  // updateRuleset will clear and add
     private val newGameOptionsTable: GameOptionsTable
     private val playerPickerTable: PlayerPickerTable
     private val mapOptionsTable: MapOptionsTable
@@ -60,7 +65,6 @@ class NewGameScreen(
         val isPortrait = isNarrowerThan4to3()
 
         updateRuleset()  // must come before playerPickerTable so mod nations from fromSettings
-        // Has to be initialized before the mapOptionsTable, since the mapOptionsTable refers to it on init
 
         // remove the victory types which are not in the rule set (e.g. were in the recently disabled mod)
         gameSetupInfo.gameParameters.victoryTypes.removeAll { it !in ruleset.victories.keys }
@@ -78,7 +82,7 @@ class NewGameScreen(
             updatePlayerPickerTable = { desiredCiv -> playerPickerTable.update(desiredCiv) },
             updatePlayerPickerRandomLabel = { playerPickerTable.updateRandomNumberLabel() }
         )
-        mapOptionsTable = MapOptionsTable(this)
+        mapOptionsTable = MapOptionsTable(this, isReset)
         closeButton.onActivation {
             mapOptionsTable.cancelBackgroundJobs()
             game.popScreen()
@@ -101,7 +105,7 @@ class NewGameScreen(
                     "Are you sure you want to reset all game options to defaults?",
                     "Reset to defaults",
                 ) {
-                    game.replaceCurrentScreen(NewGameScreen(GameSetupInfo()))
+                    game.replaceCurrentScreen(NewGameScreen(GameSetupInfo(), isReset = true))
                 }.open(true)
             }
         }
@@ -231,7 +235,7 @@ class NewGameScreen(
 
     /** Subtables may need an upper limit to their width - they can ask this function. */
     // In sync with isPortrait in init, here so UI details need not know about 3-column vs 1-column layout
-    internal fun getColumnWidth() = stage.width / (if (isNarrowerThan4to3()) 1 else 3)
+    internal fun getColumnWidth() = floor(stage.width / (if (isNarrowerThan4to3()) 1 else 3))
 
     private fun initLandscape() {
         scrollPane.setScrollingDisabled(true,true)
@@ -354,11 +358,37 @@ class NewGameScreen(
         }
     }
 
-    fun updateRuleset() {
+    /** Updates our local [ruleset] from [gameSetupInfo], guarding against exceptions.
+     *
+     *  Note: The options reset on failure is not propagated automatically to the Widgets -
+     *  the caller must ensure that.
+     *
+     *  @return Success - failure means gameSetupInfo was reset to defaults and the Ruleset was reverted to G&K
+     */
+    fun updateRuleset(): Boolean {
+        var success = true
+        fun handleFailure(message: String): Ruleset {
+            success = false
+            gameSetupInfo = GameSetupInfo()
+            ToastPopup(message, this, 5000)
+            return RulesetCache[BaseRuleset.Civ_V_GnK.fullName]!!
+        }
+
+        val newRuleset = try {
+            // this can throw with non-default gameSetupInfo, e.g. when Mods change or we change the impact of Mod errors
+            RulesetCache.getComplexRuleset(gameSetupInfo.gameParameters)
+        } catch (ex: UncivShowableException) {
+            handleFailure("«YELLOW»{Your previous options needed to be reset to defaults.}«»\n\n${ex.localizedMessage}")
+        } catch (ex: Throwable) {
+            Log.debug("updateRuleset failed", ex)
+            handleFailure("«RED»{Your previous options needed to be reset to defaults.}«»")
+        }
+
         ruleset.clear()
-        ruleset.add(RulesetCache.getComplexRuleset(gameSetupInfo.gameParameters))
+        ruleset.add(newRuleset)
         ImageGetter.setNewRuleset(ruleset)
         game.musicController.setModList(gameSetupInfo.gameParameters.getModsAndBaseRuleset())
+        return success
     }
 
     fun lockTables() {