yairm210 / Unciv

Open-source Android/Desktop remake of Civ V
Mozilla Public License 2.0
8.49k stars 1.57k forks source link

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

Closed SomeTroglodyte closed 7 months ago

SomeTroglodyte commented 1 year ago

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:

Side comments:

SeventhM commented 1 year ago

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 commented 1 year ago

(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: ```patch 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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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[bot] commented 7 months ago

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 commented 7 months ago

Can close?

yairm210 commented 7 months ago

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