added 2 unique to change name/appearance of civ

[Emandac]

Draft PR to change name/appearance of civ using 2 different unique.

-NOTE i must write a doc on how this works for the modders

[SomeTroglodyte]

Seems unrelated. A simple explanation probably?

[Emandac]

I should change it back 😅

[SomeTroglodyte]

More trailing blanks. You don't let Android Studio clean them for you?
At least Ctrl-R, check regex, search=[ \t]+$, replace with ``, Replace all... Only in the files you worked on of course.

[Emandac]

I use intelij 😅

[SomeTroglodyte]

Surely has a comparable setting as it's the same thing just different wrapping.

[Emandac]

Even though i didn't manage to remove the white space with it but
It was still useful to replace displayCivName to getDisplayCivName() :D

[SomeTroglodyte]

👋

[Emandac]

😬

[SomeTroglodyte]

I agree with Yair's assessment that changing the "foreign key" relation between a Civ and its Nation mid-game opens a dangerous can of worms¹. But: With enough warning, why not offer modders a potentially game-breaking tool? No, I'd vote against it because a stable decoupling of Civ and Nation without a mid-game change should come first ¹. DisplayName is a first step. Abstracting appearance would be the next. Separating the "primary key" (which then could be anything, including a simple integer) of the Civ from its "Nation pointer key" another - at this point we could allow more than one Civ to play the same nation.

If you want to only change icon, colors, leader portrait and such, a "DisplayNation" might work. We'll then soon wish for a "unplayable" Unique to get true appearance-only Nations...

Footnotes

1. Think about how to support the concept of one Nation, several available Leaders as in certain non-5 Civ's. ↩ ↩²

[Emandac]

So tomorrow I will remove the appearance unique, and remove the white spaces.

[Emandac]

DisplayNation should be in it own file ?

[SomeTroglodyte]

The check you're failing is this unit test. You know you can debug them, right? But this one might be the first "Nope" I commented. Didn't really switch on brain, sorry. Or it may be related to the next point below, and to the fact that your constructor change is not guaranteed to always initialize the new field same as civName - deserialization uses the no-args constructor. So yes loading a save from a prior version pretty much guarantees trouble?

Idea: Initialize displayName as null. Gdx.Json will recognize the default and save space in the saved game unless it's actually set. Good for backwards compatibility too. Then, make the getter private too so all access will return displayName ?: civName instead. Now cloning is harder, so just make it a private field with explicit getter and setter... Can't explain well. Maybe as example:

Nice idea but needs sharp thinking because cloning would go through the getter and setter (but serialization/deserialization would not):

    var displayName: String? = null
        get() = field ?: civName
        set(value) { field = if (value == civName) null else value }

Safer:

    private var displayName: String? = null
    fun getDisplayName() = displayName ?: civName
    fun setDisplayName(value: String) { displayName = if (value == civName) null else value }

[SomeTroglodyte]

DisplayNation should be in it own file ?

Whut? Why? I imagined a new field in Civilization pretty much ticking the same way as displayName, not a new class.

[SomeTroglodyte]

The barest start to that DisplayNation idea would be:

Index: core/src/com/unciv/ui/screens/diplomacyscreen/LeaderIntroTable.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/src/com/unciv/ui/screens/diplomacyscreen/LeaderIntroTable.kt b/core/src/com/unciv/ui/screens/diplomacyscreen/LeaderIntroTable.kt
--- a/core/src/com/unciv/ui/screens/diplomacyscreen/LeaderIntroTable.kt	(revision b0f427a4469ca11a5f4e712d11cdde74bf0252a2)
+++ b/core/src/com/unciv/ui/screens/diplomacyscreen/LeaderIntroTable.kt	(date 1719508388471)
@@ -27,7 +27,7 @@
      */
     init {
         defaults().align(Align.center)
-        val nation = civInfo.nation
+        val nation = civInfo.getDisplayNation()
         val leaderPortraitFile = "LeaderIcons/" + nation.leaderName
         val leaderLabel = civInfo.getLeaderDisplayName().toLabel(fontSize = Constants.headingFontSize, hideIcons = true)
         val nationIndicator = ImageGetter.getNationPortrait(nation, 24f)
Index: core/src/com/unciv/ui/screens/worldscreen/topbar/WorldScreenTopBar.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/src/com/unciv/ui/screens/worldscreen/topbar/WorldScreenTopBar.kt b/core/src/com/unciv/ui/screens/worldscreen/topbar/WorldScreenTopBar.kt
--- a/core/src/com/unciv/ui/screens/worldscreen/topbar/WorldScreenTopBar.kt	(revision b0f427a4469ca11a5f4e712d11cdde74bf0252a2)
+++ b/core/src/com/unciv/ui/screens/worldscreen/topbar/WorldScreenTopBar.kt	(date 1734877708753)
@@ -235,7 +235,7 @@
             if (this.selectedCiv == newCiv) return
             this.selectedCiv = newCiv
 
-            selectedCivIcon = ImageGetter.getNationPortrait(worldScreen.selectedCiv.nation, 25f)
+            selectedCivIcon = ImageGetter.getNationPortrait(worldScreen.selectedCiv.getDisplayNation(), 25f)
             selectedCivIconCell.setActor(selectedCivIcon)
             selectedCivLabel.setText(newCiv.tr(hideIcons = true))
             invalidate()
Index: core/src/com/unciv/logic/civilization/Civilization.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/src/com/unciv/logic/civilization/Civilization.kt b/core/src/com/unciv/logic/civilization/Civilization.kt
--- a/core/src/com/unciv/logic/civilization/Civilization.kt	(revision b0f427a4469ca11a5f4e712d11cdde74bf0252a2)
+++ b/core/src/com/unciv/logic/civilization/Civilization.kt	(date 1743097917215)
@@ -157,6 +157,11 @@
     var civName = ""
         private set
 
+    private var displayNation: String? = null
+    fun getDisplayNationName() = displayNation ?: civName
+    fun getDisplayNation() = if (displayNation == null) nation else gameInfo.ruleset.nations[displayNation] ?: nation
+    fun setDisplayNation(value: String) { displayNation = if (value == civName) null else value }
+
     var tech = TechManager()
     var policies = PolicyManager()
     var civConstructions = CivConstructions()
@@ -305,6 +310,7 @@
         toReturn.hasMovedAutomatedUnits = hasMovedAutomatedUnits
         toReturn.statsHistory = statsHistory.clone()
         toReturn.resourceStockpiles = resourceStockpiles.clone()
+        toReturn.displayNation = displayNation
         return toReturn
     }

... You'd still need to find a ton of places where e.g. the nation colors are used - units on the map, city buttons,... Also, one would need to evaluate whether caching the getDisplayNation() result as transient would be advisable, if so the field's name isn't the best choice ...

[Emandac]

The only downside is that the modders have to create a dummy civ at the start era that has the default color of the civ.
to not get the other color era civ while loading up a old savefile

[Emandac]

Nvm I fixed it

[github-actions]

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[github-actions]

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

[github-actions]

This PR was closed because it has been stalled for 10 days with no activity.