RFC: Status quo / possibilities when a unit has more than one FoundCity unique

[SomeTroglodyte]

This is relevant only to weird mods... Imagine one where a Settler can gain a Promotion through - say, a Fountain-of-Youth-alike. That promotion grants Founds a new city <[3] times> <after which this unit is consumed>. The idea of course - make the unit better...

Status quo: The second Unique is ignored. That Settler can still only found once.
Expectation: The second Unique is of "higher quality" somehow and should take precedence.

Cause: The firstOrNull in getFoundCityAction, UnitActions.L183, goes by whatever order MapUnit.getMatchingUniques decides (UniqueMap addition order which is baseUnit, type, promotions)

Possible approaches:

• Leave as is and drop a wiki hint somewhere
• Fix by making that a maxByOrNull over a quality - complicated as that should take usagesLeft into account, and that a mod may decide the unit survives after using up the founding action - now if there is a 3 times + survival and another for 3 times + death - gets complicated a little, right?
• Semi-Fix by nailing down the assumption promotions should always improve -> simply change UniqueMap addition order

Side comments:

• MapUnit.updateUniques allocates and discards an ArrayList - sounds needless to me, or is there a reason?
• Create improvement instantly works differently - there'll be more than one Action on the menu instead.
• Looks like some conditional check styles repeat quite often - enough to warrant some more helpers like hasConditional(UniqueType)?

[SeventhM]

Expectation: The second Unique is of "higher quality" somehow and should take precedence

Other similar things of this nature (see here) usually spawns the same prompt multiple times (once for each instance). I would assume expected behavior is that multiple found city uniques would show all of them

[SomeTroglodyte]

(see here)

I know - that's how I came up with checking this. But that multiple prompt isn't really optimal is it? Would the same logic improve that UX over there as well?

With this patch, retrofitted to improvement dropper, the suicidal Seastead would be hidden until the others are used up:

Subject: [PATCH] Prioritize multiple FoundCity UnitActions
---
Index: core/src/com/unciv/ui/screens/worldscreen/unit/actions/UnitActions.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/src/com/unciv/ui/screens/worldscreen/unit/actions/UnitActions.kt b/core/src/com/unciv/ui/screens/worldscreen/unit/actions/UnitActions.kt
--- a/core/src/com/unciv/ui/screens/worldscreen/unit/actions/UnitActions.kt	
+++ b/core/src/com/unciv/ui/screens/worldscreen/unit/actions/UnitActions.kt	
@@ -178,13 +178,22 @@
      * (no movement left, too close to another city).
       */
     fun getFoundCityAction(unit: MapUnit, tile: Tile): UnitAction? {
+        if (tile.isWater || tile.isImpassible()) return null
+        fun getFoundingUniqueQuality(unique: Unique): Int {
+            if (unique.conditionals.any { it.type == UniqueType.UnitActionConsumeUnit })
+                return -1
+            val survives = unique.conditionals.none { it.type == UniqueType.UnitActionAfterWhichConsumed }
+            val maxUses = getMaxUsages(unit, unique) ?: 99
+            val isExpended = usagesLeft(unit, unique) == 0
+            return maxUses + (if (isExpended) 0 else 200) + (if (survives) 100 else 0)
+        }
         val unique = unit.getMatchingUniques(UniqueType.FoundCity)
             .filter { unique -> unique.conditionals.none { it.type == UniqueType.UnitActionExtraLimitedTimes } }
-            .firstOrNull()
-        if (unique == null || tile.isWater || tile.isImpassible()) return null
+            .maxByOrNull(::getFoundingUniqueQuality)
+            ?: return null
         // Spain should still be able to build Conquistadors in a one city challenge - but can't settle them
         if (unit.civ.isOneCityChallenger() && unit.civ.hasEverOwnedOriginalCapital) return null
-        if (usagesLeft(unit, unique)==0) return null
+        if (usagesLeft(unit, unique) == 0) return null
 
         if (unit.currentMovement <= 0 || !tile.canBeSettled())
             return UnitAction(UnitActionType.FoundCity, action = null)
@@ -735,7 +744,7 @@
     }
 
     /** Returns 'null' if usages are not limited */
-    fun usagesLeft(unit:MapUnit, actionUnique: Unique): Int?{
+    fun usagesLeft(unit: MapUnit, actionUnique: Unique): Int? {
         val usagesTotal = getMaxUsages(unit, actionUnique) ?: return null
         val usagesSoFar = unit.abilityToTimesUsed[actionUnique.placeholderText] ?: 0
         return usagesTotal - usagesSoFar

[SeventhM]

I'm not sure how important it is to police mod makers on what shows up or what doesn't. While this may work fine for something like UnitActionConsumeUnit, how would this work if the mod maker has both one for that and one that uses movement instead? What if we add unit action conditionals? That seems like something that could end up as a can of worms fast
I can see centralizing some of the code for actions to all function the same (or at least similarly), and maybe some optimization regarding actions to group up ones that aren't hyper specific. But otherwise, I think maybe the better solution might be to just show each one and request mod makers to address the layout themselves

[yairm210]

I am also against this. The new system supports adding additional 'charges', the old does not, we shouldn't retrofit to allow combinations of the two

[SomeTroglodyte]

Against what? Multiple UniqueType.FoundCity behave entirely different in getFoundCityAction than multiple ConstructImprovementInstantly in getImprovementConstructionActions. Sync direction? Fix which one to match the other?

[yairm210]

I don't think they do, though.
The root of the problem is that getMaxUsages() doesn't consider 'by consuming this unit' to be a one time thing, thus when you ask 'how many times can it X' it says just the extra amount.
What we should be doing is treating "" THE SAME AS " ", in both getMaxUsages AND getSideEffectString AND activateSideEffects.

Not sure what the best course of action would be though - just replace all instances of A with B and C would work, but not exactly clear, but then again I can't think of another cleaner solution right now

[github-actions]

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]

Can close?

[yairm210]

Yes
A base game can specify "can found once after which is consumed" if it wants to enable mods to add founding charges