Dugy / Legend_of_the_Invincibles

An add-on campaign for the Battle for Wesnoth game
GNU General Public License v3.0
40 stars 22 forks source link

Elvish Assassin's advancement "executing enemy bosses better" does nothing #259

Closed edwardspec closed 1 year ago

edwardspec commented 5 years ago

This advancement is supposed to increase the damage of "execution" attack, but the damage is unchanged (was 45-1, remains 45-1).

Dugy commented 5 years ago

This is actually a far greater problem than I expected. The Execution attack is supposed to be swapped by an advancement that grants the same, but as a bonus attack merging the attacks into one based on the regular bow attack. This is added into a WML variable holding the unit by an event call done at the beginning of the stats update and the unit's WML stored in the variables is correctly saved. It starts as a part of [modifications], but it isn't recognised later and lost somewhere.

It might be fixable by adding the same advancement into his advancement list, but it feels like a workaround.

It seems to be a problem with the implementation of the loti.item.something functions that don't recognise these. Do you have any insight on this?

edwardspec commented 5 years ago

The problem appeared in the commit that replaced old stats.cfg with stats.lua. There was some logic related to this attack in stats.cfg (stats.lua doesn't have this).

Although the code in main.lua seems to do the same (remove "execution" attack, add "execution" advancement), my guess is that it's somehow undone when the unit is recreated in update_stats().

Dugy commented 5 years ago

How does the current implementation of the effects() iterator handle unknown advancements? I think neither of us remembered that this unit has a special advancement obtained with a dirty hack like this (shame we don't have encapsulation in WML).

It could be totally me who broke it, but I can't find the cause in my code.

The code for adding that attack as a bonus attack rather than a normal attack can also make that property act as an [object] (that is handled correctly at the moment). It was impossible before when bonus attacks could only be added as advancements. This way, we can avoid this problem completely.

edwardspec commented 5 years ago

It has nothing to do with effects(). Here is a simple test: 1) start Gladiators with Elvish Assassin, 2) do :unit advances=1 to level it up, select "executing enemy bosses better", 3) check the unit via :inspect. You will see that [modifications] don't contain a modification that does "remove attack, add attack" logic. They only contain the modification with [effect] apply_to=improve_bonus_attack name=execution_bonus_attack increase_damage=10 [/effect]

How does the current implementation of the effects() iterator handle unknown advancements?

Unknown advancement (advancement not listed in the unit type) is treated as { id = "name" } with nothing else: https://github.com/Dugy/Legend_of_the_Invincibles/commit/f92895caad637f72e341646fbb9a7658af5e6627#diff-4baf816582e0940cfff59d79f7c3c623R74

edwardspec commented 5 years ago

Rather than investigating this complex workaround, I suggest an alternate solution: 1) completely remove this [attack] from Elvish_Assassin.cfg, 2) have [advancement]id=execution0 ...here we add the attack...[/advancement] in Elvish_Assassin.cfg - can just copy it from Predator.cfg. 3) in some code that gets called for all units, e.g. in update_stats() or in unit placed event, if the unit is Elvish Assassin and it doesn't have execution0, then add execution0 to this unit.

So it would work as any normal advancement (without hacks), except all Elvish Assassin units will have it.

Dugy commented 5 years ago

Unknown advancement (advancement not listed in the unit type) is treated as { id = "name" } with nothing else: f92895c#diff-4baf816582e0940cfff59d79f7c3c623R74

This is a problem. That advancement isn't id-only. It has some effects that must be active for it to work and they cannot be obtained from anywhere.

Your alternate solution isn't applicable, because the attack, being a bonus attack, derives its damage from the damage multiplied by attack count of the main bow attack. The attack that appears there is there just to make automated systems list it. I added it because people thought it was a bug.

That advancement is added in an event called by update_stats(), at its very start, so that it's processed with other effects. The only way it differs from your 'alternate solution' is that the advancement is also listed in advancements, which I mentioned earlier as a workaround I would like to avoid.

Toranks commented 1 year ago

I just happened to check the Elvish assassin and it works fine for me. Attack "Execution" increases in damage. Should this be closed then?

Dugy commented 1 year ago

The code has been altered a few times since the conversation took place. How certain are you that it scales as it should?

Toranks commented 1 year ago

unit type=AdvancingElvish Assassin (resilient, strong) 45-1 > 45-1 Doesn't work

unit type=Elvish Assassin (resilient, dextrous) 42-1 > 49-1

Dugy commented 1 year ago

I have tested it also with items that increase damage or number of attacks and also with AMLA upgrading this attack and I am confirming it works.