Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Traits do not affect [advancement] effects by default #4419

Open
shikadiqueen opened this issue Oct 1, 2019 · 12 comments

Comments

@shikadiqueen
Copy link
Member

commented Oct 1, 2019

As of version 1.14.9, a unit with traits that have some relevance to [effect]s added by AMLA modifications doesn't have those traits taken into account for said effects unless some additional effect (such as apply_to=type) or engine action (such as wesnoth.remove_modifications on an unrelated modification) causes the unit's stats to be forcibly recalculated.

Steps to reproduce:

  • Edit the test scenario data/scenario-test.cfg to add {TRAIT_DEXTROUS} to Kaleh.
  • Launch the test scenario, move Kaleh to 7,10 to fill his XP pool and choose the Hunter advancement — this will give him the bolas attack as 11-2 ranged impact.
  • Quit and edit data/campaigns/Under_the_Burning_Suns/units/quenoth/Youth.cfg removing both apply_to=type effects under the id=hunter_1 AMLA around line 78. Do note that in the WML the apply_to=new_attack effect defines his new attack as 10-2.
  • Launch the test scenario, move Kaleh to 7,10 and choose the Hunter advancement again — now he will be given a 10-2 attack instead.

It's not clear what the intended behaviour is, but the expected behaviour would be for traits to take precedence over AMLAs and other non-trait modifications, as the outcome of forcibly recalculating stats seems to suggest. Fixing this in any way whatsoever probably has too many ramifications to consider for 1.14, however.

Note: I have not confirmed yet whether this issue is specific to [advancement] or it affects all other modifications as well.

@shikadiqueen

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2019

It appears even a dummy variation effect like this forces stats to be recalculated:

[effect]
    apply_to=variation
    name=""
[/effect]

The downside is that when combined with a full-heal effect, the latter will only heal the unit back to its base HP with its unit type and traits considered without any AMLAs that increase total HP.

(Obviously it only counts as a dummy effect if the unit doesn't have a variation set already. It's also worth noting that using apply_to=type with the unit's current type id doesn't have the same effect.)

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

This is a known issue, when an effect is applied 'normally' the objects/advancements/traits are applied in the order in which they are added to the unit, but when a unit is rebuild advancements are applied first then effects and objects last.

So when you do for example

u:add_modification("object", { T.effect { apply_to="hitpoints", set_total="100"}})
u:add_modification("advancement", { T.effect { apply_to="hitpoints", increase_total="100"}})

the unit will have 200 hp, but a 'rebuild' decreases the units hp to 100 because when rebuilding the unit the second effect is applied first.

Not sure what to do actually, for your usecase it'd be enough to add a unit rebuild after every advancement, but that'd not fix the case i just mentioned, i basicially see three options:

  1. apply all effect in chronological order when a unit is rebuild
  2. when an trait/advancement is added and the unti already have a modification of lower priority rebuild the unti automaticially
  3. keep it as it is and rely on umc devs to rebuild the unit if needed.
@shikadiqueen

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2019

Known but not documented or reported to the bug tracker before, I take it.

The problem with 3 is that it violates the principle of least surprise. The unit rebuild can happen at some random point later without the player or even the author expecting it, depending on a number of internal and external factors (including Mod add-ons that may be installed). I originally came across this because of a call to wesnoth.remove_modifications on what was supposed to be a no-op AMLA causing the unit's stats to change. The wiki neglects mentioning any of this in https://wiki.wesnoth.org/EffectWML as well.

The fact that there may be two different sets of stats for any given unit depending on whether and how it has undergone a mysterious process is just problematic in general and seems like the logical conclusion to the flimsiness of the unit modifications mechanism as we know it. Perhaps a deeper overhaul is due, or at the very least a concerted effort to make the internals more transparent and easier to control by content authors.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

Known but not documented or reported to the bug tracker before, I take it.

heh, ye what i meant by that was basicially just that i also stumbled over this when writing my addon.

The problem with 3 is that it violates the principle of least surprise

Ye i agree, so any opinion on whether to do 1 or 2 or something completely different? I currently tend to 1, because well, rebuilding the unit will still erase all direct changes and forcing that as a side effect of add_modification doesn't seem that good to me. On the other hand, it does make sense that advancements are applied before traits, exactly because in cases like your first example it does make sense to have 11-2.

@shikadiqueen

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2019

It's a difficult question. I can think of cases where ignoring traits when applying AMLAs makes sense as well as cases where it doesn't (like the above). [object] modifications make the situation even more complicated.

I feel like ideally the unit should be forcefully rebuilt in all cases with a single strict order to avoid nasty surprises later down the line, like it already is (right?) during normal level-ups to new unit types — and in order to address the issue at hand, AMLAs and objects should be allowed to choose whether they ignore traits or allow themselves to be affected by those.

Also, incidentally, I just randomly came across a note on wesnoth.add_modification in the wiki which refers to a fourth parameter (with an unspecified default, I assume it's true?) as well as a no_write option in [object] without explaining the implications of either. It's not helped by the documentation for the latter option being entirely missing from the wiki.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2019

Also, incidentally, I just randomly came across a note on wesnoth.add_modification in the wiki which refers to a fourth parameter (with an unspecified default, I assume it's true?)

Yes its true by default, setting is to false means the modification is not added to the units wml but only its effects are applied, (which then also makes the first parameter irrelevant). This is for example used in the wiki example or wesnoth.effects https://wiki.wesnoth.org/LuaWML/Units#wesnoth.effects

as well as a no_write option in [object] without explaining the implications of either

Iirc this option was deprecated in favour of [modify_unit][effect]

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Oct 5, 2019

I'd probably support option 1, as that seems like it should guarantee that they're applied in the same order on a rebuild as they were when added to the unit.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2019

what we could also do is go with 1. but add an option to add_modification that is when set to true, inserts the modification at the 'correct' position and rebuilds the unit.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Oct 5, 2019

What is the "correct" position? I don't understand. You mentioned something about priority earlier too; what determines the priority of a modification?

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2019

What is the "correct" position?

Currently modifications are applies on the following order when a unit is rebuild 1) advancement, 2) triat, 3) object . So the plan would be to change that that they are just applied in the order in which they appear in the units wml (which is currenly the order in which they were added), but to give an option to add_modification to insert a modification at a concreate position, when that option is used add_modification would also rebuild the unit

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2019

like in particular the c++ advancement code migth teh be changed to use that to insert the new [advancement] not at the end but behind the last [advancement] of the unit.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Oct 5, 2019

That doesn't really answer my question about priority, but... I guess basically what you're saying is that by default, option 1 would be used; but then there would also be some optional keys like at_index or after_id or before_id or after_tags or before_tags that can be used for finer-grained control of the order?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.