wesnoth / wesnoth

An open source, turn-based strategy game with a high fantasy theme.
https://www.wesnoth.org/
GNU General Public License v2.0
5.51k stars 1.02k forks source link

EI: Survivor Trait Doesn't Trigger on Defence When Survivor Unit Doesn't Have a Useable Weapon #8633

Closed IskarJarak01 closed 6 months ago

IskarJarak01 commented 6 months ago

Game and System Information

Description of the bug

The Survivor trait gives a unit -1 damage from undead, bats and necromancers.

However, the trait only triggers when the Survivor has a useable weapon.

In practice, this means Undead ranged attacks against units without ranged weapons do not have their damage reduced by 1.

Steps to reproduce the behavior

No response

Expected behavior

No response

Additional context

No response

Dalas121 commented 6 months ago

I believe the survivor trait got rewritten sometime during the EI PR - sounds like the rewrite wasn't fully tested? The old implementation was ugly but IIRC was played by a number of people without noticing any weird bugs.

My vote would be to use the original implementation, but I don't remember who removed it or why so I'm not sure if I'd be allowed to just put it back in.

@CelticMinstrel, @Pentarctagon, @stevecotton, @knyghtmare, @Wedge009. Apologies for the mass ping; not sure who made the original change so I want to check with everyone. Let me know if this is inappropriate.

The original, ugly implementation:

#define RESIST_DAMAGE DAMAGE PERCENTAGE
    [resistance]
        affect_self=yes
        [filter_self]
            [filter_adjacent]
                # ok, ok, yes, I know, this will trigger even if you're attacking non-undead as long as you're adjacent to undead.
                # but that's such an unusual edge case, and I don't know how to filter this by enemy
                trait=undead

                [or]
                    type_adv_tree=Dark Adept,Vampire Bat
                [/or]
            [/filter_adjacent]
        [/filter_self]
        [filter_second_weapon]
            damage={DAMAGE}
        [/filter_second_weapon]
        add={PERCENTAGE}
        max_value=100 # required by WML
    [/resistance]
#enddef
#define TRAIT_SURVIVOR
    [trait]
        id=survivor
        male_name= _ "survivor"
        female_name= _ "female^survivor"
        description= _ "<span color='#00FF00'>+1</span> damage to and <span color='#00FF00'>-1</span> damage from undead, necromancers, and bats.

Against all odds, this unit has endured the terrors and tortures of Mal-Ravanal's prisons and lived to tell the tale. Survivors bear a heavy burden from their time in captivity, but have also learned much of the strength and weaknesses of their former captors."

        # nice and simple, just give +1 damage vs undead
        [effect]
            apply_to=attack # all attacks
            [set_specials]
                mode=append
                [damage]
                    apply_to=self
                    [filter_opponent] # note that I also use this filter in the implementation for the Sentinel shield
                        trait=undead

                        [or]
                            type_adv_tree=Dark Adept,Vampire Bat
                        [/or]
                    [/filter_opponent]
                    add=1
                [/damage]
            [/set_specials]
        [/effect]

        # resistance only works by percentages, so to implement -1 damage
        # I need a separate hard-coded percentage for each possible attack
        [effect]
            apply_to=new_ability
            [abilities]
                {RESIST_DAMAGE  1 100.00}
                {RESIST_DAMAGE  2 50.00}
                {RESIST_DAMAGE  3 33.33}
                {RESIST_DAMAGE  4 25.00}
                {RESIST_DAMAGE  5 20.00}
                {RESIST_DAMAGE  6 16.67}
                {RESIST_DAMAGE  7 14.29}
                {RESIST_DAMAGE  8 12.50}
                {RESIST_DAMAGE  9 11.11}
                {RESIST_DAMAGE 10 10.00}
                {RESIST_DAMAGE 11  9.09}
                {RESIST_DAMAGE 12  8.33}
                {RESIST_DAMAGE 13  7.69}
                {RESIST_DAMAGE 14  7.14}
                {RESIST_DAMAGE 15  6.67}
                {RESIST_DAMAGE 16  6.25}
                {RESIST_DAMAGE 17  5.88}
                {RESIST_DAMAGE 18  5.56}
                {RESIST_DAMAGE 19  5.26}
                {RESIST_DAMAGE 20  5.00}
                {RESIST_DAMAGE 21  4.76}
                {RESIST_DAMAGE 22  4.55}
                {RESIST_DAMAGE 23  4.35}
                {RESIST_DAMAGE 24  4.17}
                {RESIST_DAMAGE 25  4.00}
                {RESIST_DAMAGE 26  3.85}
                {RESIST_DAMAGE 27  3.70}
                {RESIST_DAMAGE 28  3.57}
                {RESIST_DAMAGE 29  3.45}
                {RESIST_DAMAGE 30  3.33}
                {RESIST_DAMAGE 31  3.23}
                {RESIST_DAMAGE 32  3.13}
                {RESIST_DAMAGE 33  3.03}
                {RESIST_DAMAGE 34  2.94}
                {RESIST_DAMAGE 35  2.86}
                {RESIST_DAMAGE 36  2.78}
                {RESIST_DAMAGE 37  2.70}
                {RESIST_DAMAGE 38  2.63}
                {RESIST_DAMAGE 39  2.56}
                {RESIST_DAMAGE 40  2.50}
                {RESIST_DAMAGE 41  2.44}
                {RESIST_DAMAGE 42  2.38}
                {RESIST_DAMAGE 43  2.33}
                {RESIST_DAMAGE 44  2.27}
                {RESIST_DAMAGE 45  2.22}
                {RESIST_DAMAGE 46  2.17}
                {RESIST_DAMAGE 47  2.13}
                {RESIST_DAMAGE 48  2.08}
                {RESIST_DAMAGE 49  2.04}
                {RESIST_DAMAGE 50  2.00}
                {RESIST_DAMAGE 51  1.96}
                {RESIST_DAMAGE 52  1.92}
                {RESIST_DAMAGE 53  1.89}
                {RESIST_DAMAGE 54  1.85}
                {RESIST_DAMAGE 55  1.82}
                {RESIST_DAMAGE 56  1.79}
                {RESIST_DAMAGE 57  1.75}
                {RESIST_DAMAGE 58  1.72}
                {RESIST_DAMAGE 59  1.69}
                {RESIST_DAMAGE 60  1.67}
                {RESIST_DAMAGE 61  1.64}
                {RESIST_DAMAGE 62  1.61}
                {RESIST_DAMAGE 63  1.59}
                {RESIST_DAMAGE 64  1.56}
                {RESIST_DAMAGE 65  1.54}
                {RESIST_DAMAGE 66  1.52}
                {RESIST_DAMAGE 67  1.49}
                {RESIST_DAMAGE 68  1.47}
                {RESIST_DAMAGE 69  1.45}
                {RESIST_DAMAGE 70  1.43}
                {RESIST_DAMAGE 71  1.41}
                {RESIST_DAMAGE 72  1.39}
                {RESIST_DAMAGE 73  1.37}
                {RESIST_DAMAGE 74  1.35}
                {RESIST_DAMAGE 75  1.33}
                {RESIST_DAMAGE 76  1.32}
                {RESIST_DAMAGE 77  1.30}
                {RESIST_DAMAGE 78  1.28}
                {RESIST_DAMAGE 79  1.27}
                {RESIST_DAMAGE 80  1.25}
                {RESIST_DAMAGE 81  1.23}
                {RESIST_DAMAGE 82  1.22}
                {RESIST_DAMAGE 83  1.20}
                {RESIST_DAMAGE 84  1.19}
                {RESIST_DAMAGE 85  1.18}
                {RESIST_DAMAGE 86  1.16}
                {RESIST_DAMAGE 87  1.15}
                {RESIST_DAMAGE 88  1.14}
                {RESIST_DAMAGE 89  1.12}
                {RESIST_DAMAGE 90  1.11}
                {RESIST_DAMAGE 91  1.10}
                {RESIST_DAMAGE 92  1.09}
                {RESIST_DAMAGE 93  1.08}
                {RESIST_DAMAGE 94  1.06}
                {RESIST_DAMAGE 95  1.05}
                {RESIST_DAMAGE 96  1.04}
                {RESIST_DAMAGE 97  1.03}
                {RESIST_DAMAGE 98  1.02}
                {RESIST_DAMAGE 99  1.01}
                {RESIST_DAMAGE 100-9999 1.00}
            [/abilities]
        [/effect]
    [/trait]
#enddef
Pentarctagon commented 6 months ago

Well, it definitely wasn't me. You should be able to do a git blame from the web UI to see who it was?

Wedge009 commented 6 months ago

Wouldn't have been me.

stevecotton commented 6 months ago

I haven't seen the original implementation before, but it does look ugly. How about giving a defense-only weapon to units that don't have a ranged weapon?

CelticMinstrel commented 6 months ago
Okay, so here's the current implementation: ``` #define TRAIT_SURVIVOR [trait] id=survivor male_name= _ "survivor" female_name= _ "female^survivor" description= _ "+1 damage to and -1 damage from undead, necromancers, and bats. Against all odds, this unit has endured the terrors and tortures of Mal-Ravanal’s prisons and lived to tell the tale. Survivors bear a heavy burden from their time in captivity, but have also learned much of the strengths and weaknesses of their former captors." # nice and simple, just give +1 damage vs undead [effect] apply_to=attack # all attacks [set_specials] mode=append [damage] apply_to=self [filter_opponent] # note that I also use this filter in the implementation for the Sentinel shield trait=undead [or] type_adv_tree=Dark Adept,Vampire Bat [/or] [/filter_opponent] add=1 [/damage] [damage] apply_to=opponent [filter_opponent] trait=undead [or] type_adv_tree=Dark Adept,Vampire Bat [/or] [/filter_opponent] add=-1 [/damage] [/set_specials] [/effect] [/trait] #enddef ```

The issue is that the special that decreases an undead's damage by 1 is attached to the survivor's weapon. If the survivor has no weapon, there's no special.

With @stevecotton's suggestion, we'd have to double up the second [damage] tag. I think the better option is to make that second one into an ability instead of a weapon special, so it looks something like this (I stripped out the prose for this example:

Possible alternate implementation (untested): ``` # +1 damage for self against undead [effect] apply_to=attack # all attacks [set_specials] mode=append [damage] apply_to=self [filter_opponent] # note that I also use this filter in the implementation for the Sentinel shield trait=undead [or] type_adv_tree=Dark Adept,Vampire Bat [/or] [/filter_opponent] add=1 [/damage] [/set_specials] [/effect] # -1 damage for undead against self [effect] apply_to=new_ability [abilities] [damage] affect_enemies,affect_allies,affect_self=yes,no,no [filter_student] trait=undead [or] type_adv_tree=Dark Adept,Vampire Bat [/or] [/filter_student] add=-1 [/damage] [/abilities] [/effect] ```

I'm not sure if that works as written, but you can probably fiddle with it a little so that it works.

Dalas121 commented 6 months ago

After testing in-game, the alternate implementation doesn't seem to have any effect.

From the wiki, I don't see the [damage] tag as a valid ability, only as a weapon specials.

CelticMinstrel commented 6 months ago

From here:

All weapon specials except for [plague], [heal_on_hit], and [swarm] can be placed in the [abilities] tag. These "weapon specials as abilities" will give that weapon special to all attacks the unit has if matches with [filter_student/opponent/etc][filter_weapon].

So it is valid as an ability. The syntax I gave you is probably not quite right though. Unfortunately, the documentation on this isn't very good, but perhaps you can look at some of the other cases in mainline as reference. I think Li'sar's ability uses the feature, and there's probably some in unit tests as well…

Dalas121 commented 6 months ago

Got it, thanks! Bugfix PR opened: #8667