yairm210 / Unciv

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

Battle modifier uniques review / roadmap #4777

Closed SomeTroglodyte closed 2 years ago

SomeTroglodyte commented 3 years ago

Inventory / current state of affairs

placeholder Att/Def Unit/Civ suggested notes
+[]% Strength vs [] * U,C []% Strength vs [] isProbablySiegeUnit will need to check param0 when this is generalized
-[]% Strength vs [] * U,C - see above
+[]% Combat Strength * U []% Combat Strength
[] units deal +[]% damage * C [] units deal []% damage
Bonus for units in 2 tile radius 15% * U Bonus for units in [] tile radius []% modChecker should ensure param1 in 1..5
[]% Strength when stacked with [] * U OK implementation should check if a mod makes param1 apply to the unit itself
[]% Strength for [] units which have another [] unit in an adjacent tile * C OK
[]% Strength for enemy [] units in adjacent [] tiles * U OK
+10% Strength for all units during Golden Age * C []% Strength for all units during Golden Age
+30% Strength when fighting City-State units and cities * C []% Strength when fighting City-State units and cities
+[]% Strength in [] * U []% Strength in [] tileFilter for defender's tile
+[]% Strength for units fighting in [] * C []% Strength for units fighting in [] tileFilter for defender's tile
+[]% Strength if within [] tiles of a [] * C []% Strength if within [] tiles of a [] tileFilter for defender's tile
+[]% Strength when attacking A U []% Strength when attacking
+[]% attack strength to all [] units for [] turns A C []% attack strength to all [] units for [] turns
+15% Combat Strength for all units when attacking Cities A C []% Combat Strength for all units when attacking [cityFilter]
+[]% attacking strength for cities with garrisoned units A City []% attacking strength for [cityFilter] with garrisoned units
[]% attacking Strength for cities A City []% Offensive Strength for [cityFilter] for consistency with "Defensive"
+[]% Strength when defending D U []% Strength when defending
[]% Strength when defending vs [] units D U OK
+[]% Defensive strength for cities D City []% Defensive strength for [cityFilter]
Defense bonus when embarked D U Defence bonus when embarked Works exactly like the next one - but - display wording
Embarked units can defend themselves D C OK
+[]% defence in [] tiles D U []% defence in [] tiles not in getTileSpecificModifiers?

Not counting:

Hardcoded values

Ideas how to achieve moddability?

Additional considerations:

SomeTroglodyte commented 3 years ago

Thanks @lishaoxia1985 for prompting!

SomeTroglodyte commented 3 years ago

If there's Pull Requests for specific entries - please just mention #4777 without a 'resolves' 'fixes' or 'solves' prefix and I'll ensure a link.

yairm210 commented 3 years ago

I do have an idea, not sure if it's any good. We could add Attack/Defence/Both as a parameter, combatFilter, and change uniques to look like:

As of now, our goal should be unification.

I see several immediate candidates:

Also, "City-states" should be a recognized unitFilter like "Barbarians", to allow "+30% Strength when fighting City-State units and cities" to be deprecated with "[]% Strength [in Combat] vs [City-states]"

yairm210 commented 3 years ago

And I forgot to mention, why Melee and Ranged? Because that solves the existing Accuracy+Barrage vs Shock+Drill problem, where if melee units upgrade from/to ranged units they can get both sets

SomeTroglodyte commented 3 years ago

:+1: You know, this was just inventory by code inspection (except for the 'suggestion' column) to get the ball rolling. Because I did/do the Civilopedia kawaii-ification and the Battering Rams have that +-... Yes that one is to blame for this.

I'll give it time, then collate.

More food for thought: Less more generic uniques can also complicate life for the translators and force them into kludgy compromises. Which is why for example I would have left the City State thing alone at first... "in Combat" is a very elegant way to have a combat situation filter say "off, don't test, apply always", but that may not work so nicely in other languages. Language is the key here. If we didn't display uniques, this would not be a problem at all -> filters for situation, attacker type, defender type, attacker tile, defender tile, time of day, weather, mood and micropayment level all rolled into one, no problem. Except no-one would want to read that on screen for many combinations of filters...

As #5 said: Input! Input! Maybe we'll trigger the right ideas.

ravignir commented 3 years ago

Kind of offtopic but Siege units should have: "[]% Strength when attacking []", it should not work when defending.

SomeTroglodyte commented 3 years ago

Yes and as you said this is groundwork to make such a change easy in json (if it isn't already - Oh, I see an error in the table - fixed now, I see at once: not yet possible in json). Keep it in mind or ensure it's listed in that rules thread..

xlenstra commented 3 years ago

A little something that has been bugging me and was absent in your table: "Attacking Strength" vs "Strength when attacking" I have no specific opinion on which of these we should use, but in my opinion we should be consistent with them. This would automatically be the case when we use Yairm's proposal which is one of the reasons I'm a fan of it: forcing unification. On that proposal, it would need some kind of AND-logic, otherwise "+15% Strength when attacking for ranged combat for units fighting in open terrain" (equivalent to accuracy promotion) would not be able to fit this template. For these combinations I would also propose "In Melee Combat" instead of "for Melee attacks", also as the latter also feels like it should only apply when attacking. On the translation note raised by SomeTroglodyte, I again feel like we should move more towards uniques being a kind of computer language, meant to be parsed and not to be read, and its meaning being translated by uniqueText parameters to human language. This would, however, still force the same problems in language onto mod makers, we would not have the people to translate these specific uniqueTexts to every language.

avdstaaij commented 3 years ago

This is a very nice overview! I have a proposal as well.

tl;dr: [signedAmount]% ([strengthType]) Strength <for [unitFilter] units> ([combatType]) (vs [unitFilter]) (in [tileFilter])


There are some outliers, but generally, these are the independent elements that the strength bonus uniques consist of:

Both combatant (unit/city) and civ-wide:

Only civ-wide:

Note that there is a difference between the "attacking unit"/"defending unit" and "our unit"/"their unit" distinctions.

@SomeTroglodyte also mentioned the need for an "attacker tileFilter" condition:

We would need tile-specific modifiers using the attacker's tile for #4508 (...)

However, from the linked issue I gather that this condition is actually not needed: the original game only ever considers the defender's tile. It would of course still be nice to support an "attacker tileFilter" condition for mods.

The conditions "our unit tileFilter", "their unit tileFilter", "attacker unitFilter" and "defender unitFilter" are also imaginable for mods, but if we want to keep uniques "flowing language", I think that it will be impossible to include these without ambiguities. (And imagine the translation difficulties!)


My proposal for a unique "pattern" that contains all of the elements that our uniques use:

[signedAmount]% ([strengthType]) Strength <for [unitFilter] units> ([combatType]) (vs [unitFilter]) (in [tileFilter])

Parts between parentheses are optional. Explanation:

Component Purpose
[signedAmount]% Bonus amount
([strengthType]) melee/ranged
<for [unitFilter] units> our unit unitFilter, for civ-wide uniques
([combatType]) when attacking / when defending
(vs [unitFilter]) their unit unitFilter
(in [tileFilter]) defender tileFilter

This pattern covers most of the use cases, and allows for some combinations that weren't possible before (like "[+200]% Strength [when attacking] vs [City]"). Furthermore, every combination of conditions is grammatically correct. Of course, I do not know if this will be the case for every language, but the fact that every combination works in English is at least a good sign.

I tried to get an "attacker tileFilter" condition in there as well, but I couldn't figure out a way to fit it in independent of the other conditions and without causing ambiguity.

While we could just use this pattern to unify the strength uniques, it presents an additional opportunity. It might be possible to filter for all uniques that match the pattern (perhaps using regex), which would allow us to cleanly implement all of the condition combinations it offers at once, even ones that the base game doesn't use. It would be ideal if "optional fields" were integrated into the unique/translation system, but this is not required.

Some possible variations for the pattern:


Further notes:

yairm210 commented 3 years ago

We currently don't have a way to designate "optional" parts of uniques. This is a complex problem, from both a translation and syntax viewpoint. This connects to many other problems we've had in other places, which is the problem of multiple filters. In many many uniques we have this problem, and we mostly hardcode it. The problem in its entirety is: "How can we take an effect (say +1 str, +1 food, etc) and add an arbitrary amount of limiting clauses (in x cities, for unittype, when y researched, if empire happy) in a way that is A. Simple to write B. Parsable and Translatable

avdstaaij commented 3 years ago

We currently don't have a way to designate "optional" parts of uniques. This is a complex problem, from both a translation and syntax viewpoint.

I see, but do note that this is only a minor component of my proposal. The main goal is just unification. We can just use separate uniques, like "[signedAmount]% [attackType] Strength vs [unitFilter] and "[signedAmount]% Strength for all [unitFilter] units in [tileFilter]" The purpose of my pattern is mainly to ensure that there is only one clear and unified way to formulate these uniques, which is what I understood we were looking for in this issue.

An optional second step is then to regex for all uniques that match this pattern, in order to implement all of them at once, including ones that we don't use in the base game. We'd still only have the uniques we actually use in the jsons and the translations. And only if we do that, it would be an even more optional third step to integrate "optional values" in the unique system. Still, I do think integrated optional values should be possible in some cases, including this one.

The problem in its entirety is: "How can we take an effect (say +1 str, +1 food, etc) and add an arbitrary amount of limiting clauses (in x cities, for unittype, when y researched, if empire happy) in a way that is A. Simple to write B. Parsable and Translatable

Indeed. This problem is exactly what I tried to address for this specific case with my post. In the first part, I made a structured overview of the conditions/limiting clauses that the strength bonus uniques need, and in the second part, I proposed a unique pattern and explained how it supports all of them in a simple and translatable way.

Again, I'm just proposing another pattern for these strength uniques, based on a structured analysis of the requirements.

More concretely, this is what the uniques from @SomeTroglodyte's table would look like. | Original | Suggested | | :-- | :-- | | +[]% Strength vs [] | []% Strength vs [] | | -[]% Strength vs [] | []% Strength vs [] | | +[]% Combat Strength | []% Strength | | [] units deal +[]% damage | []% Strength for [] units | | Bonus for units in 2 tile radius 15% | not covered | | []% Strength when stacked with [] | not covered | | []% Strength for [] units which have another [] unit in an adjacent tile | not covered | | []% Strength for enemy [] units in adjacent [] tiles | not covered | | +10% Strength for all units during Golden Age | not covered | | +30% Strength when fighting City-State units and cities | [+30%] Strength vs [City-state] units | | +[]% Strength in [] | []% Strength in [] | | +[]% Strength for units fighting in [] | []% Strength for [All] units in [] | | +[]% Strength if within [] tiles of a [] | not covered | | +[]% Strength when attacking | []% Strength [when attacking] | | +[]% attack strength to all [] units for [] turns | not covered | | +15% Combat Strength for all units when attacking Cities | []% Strength for [All] units [when attacking] vs [City] | | +[]% attacking strength for cities with garrisoned units | not covered (but see "further notes") | | []% attacking Strength for cities | []% Strength for [City] units [when attacking] (yes, "city units" is kinda awkward, see "further notes") | | +[]% Strength when defending | []% Strength [when defending] | | []% Strength when defending vs [] units | []% Strength [when defending] vs [] units | | +[]% Defensive strength for cities | []% Strength for [City] units [when defending] | | Defense bonus when embarked | not actually covered, I just realized, but can be done with "[]% Strength [when defending] in [Coast]" and []% Strength [when defending] in [Ocean]" (is there a "water" tileFilter value?) | | Embarked units can defend themselves | Same as above | | +[]% defence in [] tiles | []% Strength [when defending] in [] | Note that in most "not covered" cases, my pattern can still be used partially.
yairm210 commented 3 years ago

I see I agree that having a unified textual rule makes it easier to decide what order and exact phrasing we should have, as well as hopefully making things slightly easier on the average player :)

SomeTroglodyte commented 2 years ago

We should be past this stage