Fix broken random nations pool (#8763)

core/src/com/unciv/logic/GameStarter.kt

@@ -221,11 +221,10 @@ object GameStarter {
 
     private fun addCivilizations(newGameParameters: GameParameters, gameInfo: GameInfo, ruleset: Ruleset, existingMap: Boolean) {
         val availableCivNames = Stack<String>()
-        // CityState or Spectator civs are not available for Random pick
         if (gameSetupInfo.gameParameters.enableRandomNationsPool) {
-            for (nation in gameSetupInfo.gameParameters.randomNations)
+            availableCivNames.addAll(gameSetupInfo.gameParameters.randomNations.shuffled())
-                availableCivNames.add(nation.name)
         } else
+            // CityState or Spectator civs are not available for Random pick
             availableCivNames.addAll(ruleset.nations.filter { it.value.isMajorCiv() }.keys.shuffled())
 
         availableCivNames.removeAll(newGameParameters.players.map { it.chosenCiv }.toSet())
@@ -248,12 +247,12 @@ object GameStarter {
             val max = newGameParameters.maxNumberOfPlayers.coerceAtLeast(newGameParameters.minNumberOfPlayers)
             var playerCount = (min..max).random()
 
-            val humanPlayerCount = newGameParameters.players.filter {
+            val humanPlayerCount = newGameParameters.players.count {
                 it.playerType === PlayerType.Human
-            }.count()
+            }
-            val spectatorCount = newGameParameters.players.filter {
+            val spectatorCount = newGameParameters.players.count {
                 it.chosenCiv === Constants.spectator
-            }.count()
+            }
             playerCount = playerCount.coerceAtLeast(humanPlayerCount + spectatorCount)
 
             if (newGameParameters.players.size < playerCount) {
@@ -265,6 +264,7 @@ object GameStarter {
                     it.playerType === PlayerType.AI
                 }.shuffled().subList(0, extraPlayers)
 
+                @Suppress("ConvertArgumentToSet")  // Not worth it for a handful entries
                 newGameParameters.players.removeAll(playersToRemove)
             }
         }

core/src/com/unciv/models/metadata/GameParameters.kt

@@ -3,8 +3,9 @@ package com.unciv.models.metadata
 import com.unciv.logic.IsPartOfGameInfoSerialization
 import com.unciv.logic.civilization.PlayerType
 import com.unciv.models.ruleset.Speed
-import com.unciv.models.ruleset.nation.Nation
 
+
+@Suppress("EnumEntryName")  // These merit unusual names
 enum class BaseRuleset(val fullName:String){
     Civ_V_Vanilla("Civ V - Vanilla"),
     Civ_V_GnK("Civ V - Gods & Kings"),
@@ -29,7 +30,7 @@ class GameParameters : IsPartOfGameInfoSerialization { // Default values are the
     var numberOfCityStates = 6
 
     var enableRandomNationsPool = false
-    var randomNations = arrayListOf<Nation>()
+    var randomNations = arrayListOf<String>()
 
     var noCityRazing = false
     var noBarbarians = false
@@ -54,21 +55,28 @@ class GameParameters : IsPartOfGameInfoSerialization { // Default values are the
         val parameters = GameParameters()
         parameters.difficulty = difficulty
         parameters.speed = speed
-        parameters.players = ArrayList(players)
         parameters.randomNumberOfPlayers = randomNumberOfPlayers
         parameters.minNumberOfPlayers = minNumberOfPlayers
         parameters.maxNumberOfPlayers = maxNumberOfPlayers
+        parameters.players = ArrayList(players)
         parameters.randomNumberOfCityStates = randomNumberOfCityStates
         parameters.minNumberOfCityStates = minNumberOfCityStates
         parameters.maxNumberOfCityStates = maxNumberOfCityStates
         parameters.numberOfCityStates = numberOfCityStates
+        parameters.enableRandomNationsPool = enableRandomNationsPool
+        parameters.randomNations = ArrayList(randomNations)
+        parameters.noCityRazing = noCityRazing
         parameters.noBarbarians = noBarbarians
         parameters.ragingBarbarians = ragingBarbarians
         parameters.oneCityChallenge = oneCityChallenge
+        // godMode intentionally reset on clone
         parameters.nuclearWeaponsEnabled = nuclearWeaponsEnabled
+        parameters.espionageEnabled = espionageEnabled
+        parameters.noStartBias = noStartBias
         parameters.victoryTypes = ArrayList(victoryTypes)
         parameters.startingEra = startingEra
         parameters.isOnlineMultiplayer = isOnlineMultiplayer
+        parameters.anyoneCanSpectate = anyoneCanSpectate
         parameters.baseRuleset = baseRuleset
         parameters.mods = LinkedHashSet(mods)
         parameters.maxTurns = maxTurns

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

@@ -28,6 +28,7 @@ import com.unciv.ui.components.extensions.keyShortcuts
 import com.unciv.ui.components.extensions.onActivation
 import com.unciv.ui.components.extensions.onChange
 import com.unciv.ui.components.extensions.onClick
+import com.unciv.ui.components.extensions.pad
 import com.unciv.ui.components.extensions.surroundWithCircle
 import com.unciv.ui.components.extensions.toCheckBox
 import com.unciv.ui.components.extensions.toLabel
@@ -43,9 +44,6 @@ class GameOptionsTable(
     var locked = false
     var modCheckboxes: ModCheckboxTable? = null
         private set
-    var keepAdvancedTabOpenForNationsPool = false
-    var keepAdvancedTabOpenForRandomNations = false
-    var keepAdvancedTabOpenForRandomCityStates = false
 
     init {
         getGameOptionsTable()
@@ -92,8 +90,6 @@ class GameOptionsTable(
         }).row()
         addVictoryTypeCheckboxes()
 
-
-
         val checkboxTable = Table().apply { defaults().left().pad(2.5f) }
         checkboxTable.addIsOnlineMultiplayerCheckbox()
         if (gameParameters.isOnlineMultiplayer)
@@ -102,7 +98,10 @@ class GameOptionsTable(
 
         val expander = ExpanderTab(
             "Advanced Settings",
-            startsOutOpened = keepAdvancedTabOpenForNationsPool || keepAdvancedTabOpenForRandomNations || keepAdvancedTabOpenForRandomCityStates) {
+            startsOutOpened = gameParameters.enableRandomNationsPool,
+            persistenceID = "GameOptionsTable.Advanced"
+        ) {
+            it.defaults().pad(5f, 0f)
             it.addNoCityRazingCheckbox()
             it.addNoBarbariansCheckbox()
             it.addRagingBarbariansCheckbox()
@@ -118,11 +117,10 @@ class GameOptionsTable(
                 it.addNationsSelectTextButton()
             }
         }
-        add(expander).pad(10f).padTop(10f).growX().row()
+        add(expander).pad(10f).row()
-
 
         if (!isPortrait)
-            add(modCheckboxes).row()
+            add(modCheckboxes).padTop(0f).row()
 
         pack()
     }
@@ -178,8 +176,7 @@ class GameOptionsTable(
     private fun Table.addRandomNationsPoolCheckbox() =
             addCheckbox("Set available nations for random pool", gameParameters.enableRandomNationsPool) {
                 gameParameters.enableRandomNationsPool = it
-                keepAdvancedTabOpenForNationsPool = it
+                update()  // To show the button opening the chooser popup
-                update()
             }
 
     private fun Table.addNationsSelectTextButton() {
@@ -209,16 +206,14 @@ class GameOptionsTable(
             addCheckbox("Random number of Civilizations", gameParameters.randomNumberOfPlayers)
             {
                 gameParameters.randomNumberOfPlayers = it
-                keepAdvancedTabOpenForRandomNations = it
+                update()  // To see the new sliders
-                update()
             }
 
     private fun Table.addRandomCityStatesCheckbox() =
             addCheckbox("Random number of City-States", gameParameters.randomNumberOfCityStates)
             {
                 gameParameters.randomNumberOfCityStates = it
-                keepAdvancedTabOpenForRandomCityStates = it
+                update()  // To see the changed sliders
-                update()
             }
 
     private fun Table.addMinPlayersSlider() {
@@ -459,12 +454,14 @@ private class RandomNationPickerPopup(
     private val selectedNationsListTable = Table()
     private val selectedNationsListScroll = AutoScrollPane(selectedNationsListTable)
     private var bannedNations = gameParameters.randomNations
-    var availableNations = arrayListOf<Nation>()
+    var availableNations = arrayListOf<String>()
+
+    private val ruleset = previousScreen.ruleset
 
     init {
         var nationListScrollY = 0f
-        availableNations += previousScreen.ruleset.nations.values.asSequence()
+        availableNations += ruleset.nations.values.asSequence()
-            .filter { it.isMajorCiv() }
+            .filter { it.isMajorCiv() }.map { it.name }
         nationListScroll.setOverscroll(false, false)
         // +10, because the nation table has a 5f pad, for a total of +10f
         if (stageToShowOn.isNarrowerThan4to3()) {
@@ -515,10 +512,11 @@ private class RandomNationPickerPopup(
         nationListTable.clear()
         bannedNations = gameParameters.randomNations
         availableNations -= bannedNations.toSet()
-        availableNations = availableNations.sortedWith(compareBy(UncivGame.Current.settings.getCollatorFromLocale()) { it.name.tr() }).toMutableList() as ArrayList<Nation>
+        availableNations = availableNations.sortedWith(compareBy(UncivGame.Current.settings.getCollatorFromLocale()) { it.tr() }).toMutableList() as ArrayList<String>
 
         var currentY = 0f
-        for (nation in availableNations) {
+        for (nationName in availableNations) {
+            val nation = ruleset.nations[nationName] ?: continue
             val nationTable = NationTable(nation, civBlocksWidth, 0f) // no need for min height
             val cell = nationListTable.add(nationTable)
             currentY += cell.padBottom + cell.prefHeight + cell.padTop
@@ -528,15 +526,7 @@ private class RandomNationPickerPopup(
             }
         }
 
-        if (bannedNations.isNotEmpty()) {
+        updateNationListTable()
-            selectedNationsListTable.clear()
-
-            for (currentNation in bannedNations) {
-                val nationTable = NationTable(currentNation, civBlocksWidth, 0f)
-                nationTable.onClick { removeNationFromPool(currentNation) }
-                selectedNationsListTable.add(nationTable).row()
-            }
-        }
     }
 
     private fun String.toImageButton(overColor: Color): Group {
@@ -553,7 +543,8 @@ private class RandomNationPickerPopup(
     private fun updateNationListTable() {
         selectedNationsListTable.clear()
 
-        for (currentNation in bannedNations) {
+        for (nationName in bannedNations) {
+            val currentNation = ruleset.nations[nationName] ?: continue
             val nationTable = NationTable(currentNation, civBlocksWidth, 0f)
             nationTable.onClick { removeNationFromPool(currentNation) }
             selectedNationsListTable.add(nationTable).row()
@@ -561,18 +552,17 @@ private class RandomNationPickerPopup(
     }
 
     private fun addNationToPool(nation: Nation) {
-        bannedNations.add(nation)
+        availableNations.remove(nation.name)
+        bannedNations.add(nation.name)
 
         update()
-        updateNationListTable()
     }
 
     private fun removeNationFromPool(nation: Nation) {
-        availableNations.add(nation)
+        availableNations.add(nation.name)
-        bannedNations.remove(nation)
+        bannedNations.remove(nation.name)
 
         update()
-        updateNationListTable()
     }
 
     private fun returnSelected() {
@@ -588,8 +578,5 @@ private class RandomNationPickerPopup(
         gameParameters.randomNations = bannedNations
 
         update()
-        updateNationListTable()
     }
 }
-
-