Effect "<for [amount] turns> <upon being defeated>" not working

[SpacedOutChicken]

Is there an existing issue for this?

• [X] I have searched the existing issues

Game Version

4.10.11

Describe the bug

I attempted to give a unit a temporary civ-wide effect that kicks in when the unit gets defeated: "[+100]% Strength <for [Military] units> <for [50] turns> <upon being defeated>" It isn't working. After the unit is defeated, the strength boost fails to take effect.

Steps to Reproduce

This mod contains a simple unit that has the specified effect. TestUponDefeat.zip

Screenshots

No response

Link to save file

No response

Operating System

Windows

Additional Information

No response

[SomeTroglodyte]

Imagine without conditions: You wouldn't expect a +20% on one unit to affect another unit just because the first exists, no? So why should the base Unique suddenly change to Civ scope???

[SpacedOutChicken]

In that case, is there a functional way of doing what I'm trying to do? I'd love to be able to kick-start other temporary effects by expending units. Imagine something like "[+2 Science] [in all cities] <for [10] turns> <by consuming this unit>". Can that be done with existing uniques? If not, I'll make it a modding request.

[SomeTroglodyte]

I'm not the modder with a complete overlook over what exists what not... And that up there was an uninformed guess. Your example "[+2 Science] [in all cities] <for [10] turns> " obviously survives the unit - if it works which I assume is the case or you wouldn't mention it.... Wait, there was something.... Here:

civInfo.temporaryUniques.add(TemporaryUnique(unique, timingConditional.params[0].toInt())) in triggerUnique

Every working application of must go through that, and then it has civ scope no matter where it came from. But the implementation must still work from Civ scope... So your original one, shortened to ""[+100]% Strength <for [Military] units>" - if that works on a Nation, it should work exactly as shown from a unit? Except if the TemporaryUnique constructor holds surprises. I never really studied the trigger system before.

... and there is a surprise: unique = uniqueObject.text.replaceFirst("<$turnsText>", "") fails to trim ending or double spaces. RulesetValidator would complain about the result if it ever saw it. No idea if that has an effect...

So - yes worth debugging, not just a misunderstanding.

Demo? The demo mod is up there, but a save to make debugging easy?

Aside: If we get that working it will be quite probably cumulative - lose 20 units in one turn -> have +2000% bonus.

[SpacedOutChicken]

Sorry for not including a saved game, but the setup is easy enough. You build the Sacrificial Lamb unit, which has a strength of 1 and the specified unique, and then you send the Sacrificial Lamb out to get killed by a Barbarian unit. I can attach a saved game later if you need one.

[SpacedOutChicken]

Aside: If we get that working it will be quite probably cumulative - lose 20 units in one turn -> have +2000% bonus.

Sounds like fun to me. It's not likely to come up in what I'm working on, but it would be interesting to have an OP faction that just gets stronger and stronger as wars progress...

[SomeTroglodyte]

You build the Sacrificial Lamb

I saw. cost 1. Now give it movement 42 and it will find its butcher even fastah.

[SomeTroglodyte]

stronger and stronger as wars progress

+0.1% for 1000 turns and then start collecting. Only- the way it's implemented they also accumulate save size and memory footprint and perf hit. Oh, also makes the boni fold multiplicatively so +2000% was wrong.

[SomeTroglodyte]

out to get killed by a Barbarian

Too easy:

But with "Never appears as a Barbarian unit" it gets easier - and it turns out no triggering code ever runs because the entire unique never made it into uniqueMap. Which means - you led me a merry chase with a mod that doesn't pass validation??? No. The transfer of UniqueObjects from the BaseUnit to the tempUniqueMap of the MapUnit omits it.

... because UniqueMap.addUnique decides it doesn't want to see Uniques with a timing condition at all: if (unique.conditionals.any { it.type == UniqueType.ConditionalTimedUnique }) return. Makes sense, but then using that UniqueMap to look for triggered uniques will just not see any with a ConditionalTimedUnique.

Since my brain can't quite 'get it', not the big picture, when dealing with these Unique scope issues, we need a second opinion. UniqueMap.addUnique is inefficient and wastes a memory allocation. Its source iterator in MapUnit.updateUniques could be a reusable, so one could more easily get the entire set without the ConditionalTimedUnique suppression. Lastly, triggerDefeatUniques and triggerVictoryUniques have a vastly different architecture but should effectively - be one function.

[SpacedOutChicken]

I swear that I'm not proposing these things to be difficult. I'm just trying to come up with something cool.

[SomeTroglodyte]

I swear that I'm not rambling to be funny. No clown here. Deadpan serious. Go put on the Dunce hat and stand in the Shame®️ corner for being difficult!!!!!

No, really good catch. Could have eluded us for a long time.

Wait did I say "Barbarian unit"???? I meant "Baaaaaaaahrbaaaaahrian sheep"!

Also, TemporaryUnique leaving double spaces in the text is not a problem, the generated Unique and its conditionals are all typed properly.

Also -

screenie

 (two lambs massacred)

patch

```patch Subject: [PATCH] Fix ships unable to enter single-tile lakes via a city --- Index: core/src/com/unciv/ui/screens/devconsole/DevConsolePopup.kt IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/core/src/com/unciv/ui/screens/devconsole/DevConsolePopup.kt b/core/src/com/unciv/ui/screens/devconsole/DevConsolePopup.kt --- a/core/src/com/unciv/ui/screens/devconsole/DevConsolePopup.kt (revision 6c87bef1cf13e11985f7b261f836566251d1df82) +++ b/core/src/com/unciv/ui/screens/devconsole/DevConsolePopup.kt (date 1708542666090) @@ -36,7 +36,7 @@ open(true) - keyShortcuts.add(KeyCharAndCode.ESC) { close() } + keyShortcuts.add(KeyCharAndCode.BACK) { close() } keyShortcuts.add(KeyCharAndCode.TAB) { val textToAdd = getAutocomplete() Index: core/src/com/unciv/ui/components/input/KeyCharAndCode.kt IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/core/src/com/unciv/ui/components/input/KeyCharAndCode.kt b/core/src/com/unciv/ui/components/input/KeyCharAndCode.kt --- a/core/src/com/unciv/ui/components/input/KeyCharAndCode.kt (revision 6c87bef1cf13e11985f7b261f836566251d1df82) +++ b/core/src/com/unciv/ui/components/input/KeyCharAndCode.kt (date 1708542666082) @@ -46,7 +46,7 @@ // Convenience shortcuts for frequently used constants /** Android back, assigns ESC automatically as well */ val BACK = KeyCharAndCode(Input.Keys.BACK) - /** Automatically assigned for [BACK] */ + /** Automatically assigned for [BACK] (use that in client code, not ESC) */ val ESC = KeyCharAndCode(Input.Keys.ESCAPE) /** Assigns [NUMPAD_ENTER] automatically as well */ val RETURN = KeyCharAndCode(Input.Keys.ENTER) Index: core/src/com/unciv/logic/battle/Battle.kt IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/core/src/com/unciv/logic/battle/Battle.kt b/core/src/com/unciv/logic/battle/Battle.kt --- a/core/src/com/unciv/logic/battle/Battle.kt (revision 6c87bef1cf13e11985f7b261f836566251d1df82) +++ b/core/src/com/unciv/logic/battle/Battle.kt (date 1708543856966) @@ -150,30 +150,12 @@ if (!defender.isDefeated() && defender is MapUnitCombatant && defender.unit.isExploring()) defender.unit.action = null - fun triggerVictoryUniques(ourUnit:MapUnitCombatant, enemy:MapUnitCombatant) { - val stateForConditionals = StateForConditionals(civInfo = ourUnit.getCivInfo(), - ourCombatant = ourUnit, theirCombatant=enemy, tile = attackedTile) - for (unique in ourUnit.unit.getTriggeredUniques(UniqueType.TriggerUponDefeatingUnit, stateForConditionals)) - if (unique.conditionals.any { it.type == UniqueType.TriggerUponDefeatingUnit - && enemy.unit.matchesFilter(it.params[0]) }) - UniqueTriggerActivation.triggerUnique(unique, ourUnit.unit, triggerNotificationText = "due to our [${ourUnit.getName()}] defeating a [${enemy.getName()}]") - } - // Add culture when defeating a barbarian when Honor policy is adopted, gold from enemy killed when honor is complete // or any enemy military unit with Sacrificial captives unique (can be either attacker or defender!) if (defender.isDefeated() && defender is MapUnitCombatant && !defender.unit.isCivilian()) { - tryEarnFromKilling(attacker, defender) - tryHealAfterKilling(attacker) - - if (attacker is MapUnitCombatant) triggerVictoryUniques(attacker, defender) - triggerDefeatUniques(defender, attacker, attackedTile) - + triggerVictoryEffects(attacker, defender, attackedTile) } else if (attacker.isDefeated() && attacker is MapUnitCombatant && !attacker.unit.isCivilian()) { - tryEarnFromKilling(defender, attacker) - tryHealAfterKilling(defender) - - if (defender is MapUnitCombatant) triggerVictoryUniques(defender, attacker) - triggerDefeatUniques(attacker, defender, attackedTile) + triggerVictoryEffects(defender, attacker, attackedTile) } if (attacker is MapUnitCombatant) { @@ -201,12 +183,34 @@ return damageDealt + interceptDamage } + private fun triggerVictoryEffects(victor: ICombatant, loser: MapUnitCombatant, attackedTile: Tile) { + tryEarnFromKilling(victor, loser) + tryHealAfterKilling(victor) + if (victor is MapUnitCombatant) triggerVictoryUniques(victor, loser, attackedTile) + triggerDefeatUniques(loser, victor, attackedTile) + } + internal fun triggerDefeatUniques(ourUnit: MapUnitCombatant, enemy: ICombatant, attackedTile: Tile) { val stateForConditionals = StateForConditionals(civInfo = ourUnit.getCivInfo(), - ourCombatant = ourUnit, theirCombatant=enemy, tile = attackedTile) - for (unique in ourUnit.unit.getTriggeredUniques(UniqueType.TriggerUponDefeat, stateForConditionals)) + ourCombatant = ourUnit, theirCombatant = enemy, tile = attackedTile) + for (unique in ourUnit.unit.getAllUniquesUncached()) { + // If we use normal getTriggeredUniques here, we can't combine TriggerUponDefeat with ConditionalTimedUnique + if (unique.conditionals.none { it.type == UniqueType.TriggerUponDefeat }) continue + if (!unique.conditionalsApply(stateForConditionals)) continue UniqueTriggerActivation.triggerUnique(unique, ourUnit.unit, triggerNotificationText = "due to our [${ourUnit.getName()}] being defeated by a [${enemy.getName()}]") + } } + + private fun triggerVictoryUniques(ourUnit: MapUnitCombatant, enemy: MapUnitCombatant, attackedTile: Tile) { + val stateForConditionals = StateForConditionals(civInfo = ourUnit.getCivInfo(), + ourCombatant = ourUnit, theirCombatant = enemy, tile = attackedTile) + for (unique in ourUnit.unit.getAllUniquesUncached()) { + // If we use normal getTriggeredUniques here, we can't combine TriggerUponDefeat with ConditionalTimedUnique + if (unique.conditionals.none { it.type == UniqueType.TriggerUponDefeatingUnit && enemy.unit.matchesFilter(it.params[0]) }) continue + if (!unique.conditionalsApply(stateForConditionals)) continue + UniqueTriggerActivation.triggerUnique(unique, ourUnit.unit, triggerNotificationText = "due to our [${ourUnit.getName()}] defeating a [${enemy.getName()}]") + } + } private fun tryEarnFromKilling(civUnit: ICombatant, defeatedUnit: MapUnitCombatant) { val unitStr = max(defeatedUnit.unit.baseUnit.strength, defeatedUnit.unit.baseUnit.rangedStrength) @@ -526,7 +530,7 @@ if (civilianUnit != null) BattleUnitCapture.captureCivilianUnit(attacker, MapUnitCombatant(civilianUnit!!), checkDefeat = false) for (airUnit in airUnits.toList()) airUnit.destroy() } - + val stateForConditionals = StateForConditionals(civInfo = attackerCiv, city=city, unit = attacker.unit, ourCombatant = attacker, attackedTile = city.getCenterTile()) for (unique in attacker.getMatchingUniques(UniqueType.CaptureCityPlunder, stateForConditionals, true)) { attackerCiv.addStat( Index: core/src/com/unciv/logic/map/mapunit/MapUnit.kt IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/core/src/com/unciv/logic/map/mapunit/MapUnit.kt b/core/src/com/unciv/logic/map/mapunit/MapUnit.kt --- a/core/src/com/unciv/logic/map/mapunit/MapUnit.kt (revision 6c87bef1cf13e11985f7b261f836566251d1df82) +++ b/core/src/com/unciv/logic/map/mapunit/MapUnit.kt (date 1708542666046) @@ -589,22 +589,18 @@ } fun updateUniques() { - val uniques = ArrayList() - val baseUnit = baseUnit() - uniques.addAll(baseUnit.uniqueObjects) - uniques.addAll(type.uniqueObjects) - - for (promotion in promotions.getPromotions()) { - uniques.addAll(promotion.uniqueObjects) - } - tempUniquesMap = UniqueMap().apply { - addUniques(uniques) + addUniques(getAllUniquesUncached().asIterable()) } cache.updateUniques() } + fun getAllUniquesUncached(): Sequence = + baseUnit.uniqueObjects.asSequence() + + type.uniqueObjects + + promotions.getPromotions().flatMap { it.uniqueObjects } + fun copyStatisticsTo(newUnit: MapUnit) { newUnit.health = health newUnit.instanceName = instanceName ```

... but as I said I'm entirely unsure whether that is the way to go.

And no, that was not an Ultima reference.

[SeventhM]

I would probably need to read this further to fully understand what Trog is saying (kinda busy rn), but something tells me Trog is over complicating things <for [amount] turns> currently only adds a unique to the Civ's global uniques. If the unique for the unit doesn't check global uniques, it won't seem to work. Maybe the solution is to allow someway to allow units themselves to have temporary uniques? Sounds complicated but potentially doable, would need to check later and see if it creates any weirdness regarding assuming where a unique comes from

[SeventhM]

Imagine something like "[+2 Science] [in all cities] <for [10] turns> <by consuming this unit>"

I believe this already works. Would need to double check

[SomeTroglodyte]

<for [amount] turns> currently only adds a unique to the Civ's global uniques.

Summary: Nope it doesn't, because MapUnit.getTriggeredUniques doesn't see the entire Unique long before that code (the civInfo.temporaryUniques.add(TemporaryUnique... line) has a chance. The patch demonstrates everything else works - if you bypass the cache. The cache named MapUnit.tempUniquesMap.

[SeventhM]

Completely missed the patch. Why doesn't getTriggeredUniques work with ConditionalTimedUnique? Because it adds to civ uniques? Not sure that's reason enough

Actually, I need to check... Does that patch do... Anything? Seems like a refactor more than anything. Wait... Did I accidentally break conditional timed uniques further? Needs testing Edit: Nope, false alarm from me it seems

[SeventhM]

Would edit last message, but I'll be annoying and double post

because MapUnit.getTriggeredUniques doesn't see the entire Unique long before that code (the civInfo.temporaryUniques.add(TemporaryUnique... line) has a chance

Not true. The reason it's not seen is because apparently UniqueMap.addUnique strips out all uniques with timed conditionals. Usually a good idea, fails us here because we actually want the unique this time. Your patch works because getAllUniquesUncached() has the uniques before they are added to the UniqueMap. I suspect we should still find a way to cache both the map and the objects in MapUnit.

That said... that is an interesting way that getTriggeredUniques fails us, not just here. Probably there's a better way to cache this and use this within UniqueMap itself

Edit: not sure how you do the whole diff thing here (you mind showing me how? It's neat pretty neat, but I'm thinking:

In UniqueMap (Uniques.kt)

```kotlin fun addUnique(unique: Unique) { //if (unique.conditionals.any { it.type == UniqueType.ConditionalTimedUnique }) return val existingArrayList = get(unique.placeholderText) if (existingArrayList != null) existingArrayList.add(unique) else this[unique.placeholderText] = arrayListOf(unique) } fun getMatchingUniques(uniqueType: UniqueType, state: StateForConditionals) = getUniques(uniqueType) .filter { unique -> unique.conditionalsApply(state) && unique.conditionals.none { it.type == UniqueType.ConditionalTimedUnique } } ```

And then hunt down evil uses of getUniques that should be using getMatchingUniques

Also should we put the default for the state as IgnoreCondtitionals?

[SpacedOutChicken]

Imagine something like "[+2 Science] [in all cities] <for [10] turns> <by consuming this unit>"

I believe this already works. Would need to double check

I tested it out and it doesn't work. The game doesn't give you the option of expending the unit to create the temporary effect.

[SomeTroglodyte]

Not true. The reason it's not seen is because apparently UniqueMap.addUnique strips out all uniques with timed conditionals.

Actually, same thing, differing wording focusing on different parts of the flow.

Probably there's a better way to cache this

Glad you're doing the thinking. Yes, the getAllUniquesUncached thing is meant more as proof that the analysis is correct than actual (and efficient) solution. Side note: I did an experiment a few weeks ago (an issue I opened maybe? #10931) with a UniqueFlag preventing caching, because I found those mapgen-only uniques polluting the UniqueMaps during gameplay and suspected that may impact perf. Not pusued as I can't measure and it didn't feel quite right, as in "I'm missing something". Now - we got a modifier preventing caching. Maybe there's a way to get some synergy? a) don't hardcode ConditionalTimedUnique as skip-caching criterion, use flags. b) rethink a little c) use the "if someone asks for an uncached thing, bypass cache" idea - you could use the standard API to ask for any unique, it would simply check the flag to see if it should look in the map or do it the hard way. d) extend to expand cache the first time someone asks for an uncached. e) Could be extended to more than two levels. f) But this breaks when the uncached attribute is not directly on the unique but on any modifier..... Brain fail. I get more chaos than clarity.

Maybe drop the exclusion from UniqueMap and solve the original usecase of that exclusion as some filter instead? Include but mark maybe? Ah - your comment already goes in that direction.

not sure how you do the whole diff thing here (you mind showing me how

Not sure what precisely you mean (likely point 4 below), but here's a few tricks I took a long time learning:

• When I say 'git' in the following I mean the tool pane (menu: View-Tool Windows-Git, doesn't have a default key binding), the most important place in Studio bar none 🙃 .
• Compare w/: You know you can ask Studio to display diffs - interactive and easy to read. You can do so for varying scopes, and it's not always clear where it works well. Multiselect sometimes works sometimes not. e.g.: git-log-commit - no, but the "changed files" list per commit - yes
• Asking for a patch is closely related but not 100% found the same places - but if there's a diff offer chances for a patch offer are better. Most useful: git-Local Changes-right click on a Changelist, or on an individual file, or a multiselect of them IIRC. There's a Copy as patch to clipboard, or a Create Patch to get a text file. As for correlation to the diff feature: The "no" above is a "yes" for patches: it's near the top and only as dialog not as direct to clipboard shortcut. The dialog can still direct to the clipboard.
• Presenting a patch here in markdown can be done with the usual optional <details> and three-backtick blockquoting, and just as adding 'kotlin' or 'json' or many others to the end of the opening blockquote gets you syntax highlighting, you can use 'patch' to get diff highlighting. As collaborator you can maybe edit my posts? Just look how I did some...
• Using a patch is easy - menu: Git-Patch-Apply Patch from Clipboard among others, but that one works almost everywhere, and a patch knows its destination context so it doesn't need much help.
• The git Shelf is a chimaera serving as patch storage with some commit correlation - or is it a commit storage with patch semantics? Can lead to overcomplicated merges when shelved stuff too old, external Meld is often easier... But short-term also very useful. Mine is always overflowing. Allows patch export and import - another way to read a patch without applying it to a branch..
• A published PR can be displayed as raw patch file by appending .patch to the url here - the github mails lately have taken to supplying these links as well. .diff works too, but is less useful, can't be reapplied directly in Studio.

[SeventhM]

(likely point 4 below)

Yeah. That's neat. Willing look into setting it up at some point. Didn't even know that the blockquoting had a diff mode

[SomeTroglodyte]

tested it out and it doesn't work.

Well, the base unique is not a recognized UnitAction, there is no code to convert any Unique with the "for turns" modifer to a Triggerable on all levels, and not at the Validation or filtering level. So you do get the "which as a UnitActionModifier is only allowed on UnitAction uniques" warning, and it's right.

11181 will make it so it works - but not fix the warning or the glitch that such actions are allowed at 0 movement, or the delayed stats update (needs some additional trigger to show the increased science on world screen, like entering a city screen and changing focus)