fix [firststrike] special with apply_to=opponent crashes Wesnoth 1.17.x

data/test/maps/4p_single_castle.map

@@ -0,0 +1,8 @@
+Gg, Gg, Gg, Gg, Gg, Gg, Gg, Gg, Gg, Gg
+Gg, Gg, Gg, Gg, Gg, Gg, Gg, Gg, Gg, Gg
+Gg, Gg, Gg, Gg, Gg, Gg, Gg, Gg, Gg, Gg
+Gg, Gg, Gg, Gg, 1 Ke, Gg, Gg, Gg, Gg, Gg
+Gg, Gg, Gg, 3 Ke, 2 Ke, 4 Ke, Gg, Gg, Gg, Gg
+Gg, Gg, Gg, Gg, Gg, Gg, Gg, Gg, Gg, Gg
+Gg, Gg, Gg, Gg, Gg, Gg, Gg, Gg, Gg, Gg
+Gg, Gg, Gg, Gg, Gg, Gg, Gg, Gg, Gg, Gg

data/test/scenarios/firststrike_and_laststrike.cfg

@@ -0,0 +1,203 @@
+#textdomain wesnoth-test
+
+#####
+# API(s) being tested: [firststrike]
+##
+# Actions:
+# This uses a "common keep" map, with Alice and Bob already in position to attack any of the other units.
+# In this test they're all Orcish Grunts.
+# Set everyone to 1 hp, so that a single hit will kill.
+# Give Alice negatestrike.
+# Give Charlie firststrike.
+# Give Dave laststrike.
+# Simulate various combats to check that negatestrike ignores both firststrike and laststrike.
+# Give Alice firststrike.
+# Simulate various combats to check that firststrike goes first.
+# Finally, give Bob laststrike and simulate combat with Dave.
+##
+# Expected end state:
+# If either unit has negatestrike, the attacker has the advantage.
+# Firststrike gives an advantage.
+# Laststrike gives a disadvantage.
+# Two units that both have firststrike act the same as neither having it.
+# Two units that both have laststrike act the same as neither having it.
+#####
+[test]
+    name = _ "Unit Test firststrike_and_laststrike"
+    map_file=test/maps/4p_single_castle.map
+    turns = unlimited
+    id = firststrike_and_laststrike
+    is_unit_test = yes
+
+    {DAWN}
+
+    [side]
+        side=1
+        controller=human
+        [leader]
+            name = _ "Alice"
+            type = Orcish Grunt
+            id=alice
+        [/leader]
+    [/side]
+    [side]
+        side=2
+        controller=human
+        [leader]
+            name = _ "Bob"
+            type = Orcish Grunt
+            id=bob
+        [/leader]
+    [/side]
+    [side]
+        side=3
+        controller=human
+        [leader]
+            name = _ "Charlie"
+            type = Orcish Grunt
+            id=charlie
+        [/leader]
+    [/side]
+    [side]
+        side=4
+        controller=human
+        [leader]
+            name = _ "Dave"
+            type = Orcish Grunt
+            id=dave
+        [/leader]
+    [/side]
+
+    [event]
+        name=start
+
+        [object]
+            [filter]
+                id=alice
+            [/filter]
+            [effect]
+                apply_to=attack
+                [set_specials]
+                    mode=append
+                    [firststrike]
+                        id=negatestrike
+                        name= _ "negate strike"
+                        description= _ "Ignores firststrike - in combats with this unit, the attacker always strikes first."
+                        apply_to=both
+                    [/firststrike]
+                [/set_specials]
+            [/effect]
+        [/object]
+
+        [object]
+            [filter]
+                id=charlie
+            [/filter]
+            [effect]
+                apply_to=attack
+                [set_specials]
+                    mode=append
+                    {WEAPON_SPECIAL_FIRSTSTRIKE}
+                [/set_specials]
+            [/effect]
+        [/object]
+
+        [object]
+            [filter]
+                id=dave
+            [/filter]
+            [effect]
+                apply_to=attack
+                [set_specials]
+                    mode=append
+                    [firststrike]
+                        id=laststrike
+                        name= _ "last strike"
+                        description= _ "Opposite of first strike — this unit strikes last, even on offense."
+                        apply_to=opponent
+                    [/firststrike]
+                [/set_specials]
+            [/effect]
+        [/object]
+
+        [lua]
+            code=<<
+                local alice = wesnoth.units.find({id="alice"})[1]
+                local bob = wesnoth.units.find({id="bob"})[1]
+                local charlie = wesnoth.units.find({id="charlie"})[1]
+                local dave = wesnoth.units.find({id="dave"})[1]
+
+                alice.hitpoints = 1
+                bob.hitpoints = 1
+                charlie.hitpoints = 1
+                dave.hitpoints = 1
+
+                -- Everybody's an orcish grunt, and they're all on 60% terrain. As they only have 1 hp, the order of attacks is significant.
+                -- Whoever strikes first, their chance to survive is the sum of
+                -- hit first time
+                -- both combatants miss once each, then a hit
+                -- four consecutive misses
+                local first_survival_chance = 0.4 + (0.6 ^ 2 * 0.4) + 0.6 ^ 4
+
+                -- Whoever strikes second has lower odds. They need:
+                -- miss followed by hit
+                -- three consecutive misses
+                local second_survival_chance = 0.6 * 0.4 + 0.6 ^ 3
+
+                -- The chances calculated above are expected to match the 'unscathed' combat stat,
+                -- however bug #6590 is that that stat does not take the expected value when both
+                -- units could die in the combat. Instead the 'average_hp' stat is used, because
+                -- for a setup where both combatants start with 1hp the average matches the chance
+                -- of surviving.
+
+                -- Alice starts with "negate strike", which means all of the combats should be "attacker swings first"
+                local att_stats, def_stats = wesnoth.simulate_combat(alice, bob)
+                unit_test.assert_approx_equal(att_stats.average_hp, first_survival_chance, 0.01, "negatestrike: Alice v Bob test failed")
+                att_stats, def_stats = wesnoth.simulate_combat(alice, charlie)
+                unit_test.assert_approx_equal(att_stats.average_hp, first_survival_chance, 0.01, "negatestrike: Alice v Charlie test failed")
+                att_stats, def_stats = wesnoth.simulate_combat(alice, dave)
+                unit_test.assert_approx_equal(att_stats.average_hp, first_survival_chance, 0.01, "negatestrike: Alice v Dave test failed")
+                att_stats, def_stats = wesnoth.simulate_combat(bob, alice)
+                unit_test.assert_approx_equal(def_stats.average_hp, second_survival_chance, 0.01, "negatestrike: Bob v Alice test failed")
+                att_stats, def_stats = wesnoth.simulate_combat(charlie, alice)
+                unit_test.assert_approx_equal(def_stats.average_hp, second_survival_chance, 0.01, "negatestrike: Charlie v Alice test failed")
+                att_stats, def_stats = wesnoth.simulate_combat(dave, alice)
+                unit_test.assert_approx_equal(def_stats.average_hp, second_survival_chance, 0.01, "negatestrike: Dave v Alice test failed")
+
+                -- Give Alice the normal firststrike ability
+                alice.attacks[1] = charlie.attacks[1]
+
+                -- Alice has firststrike, so she should get first strike on any combat where she's the attacker
+                att_stats, def_stats = wesnoth.simulate_combat(alice, bob)
+                unit_test.assert_approx_equal(att_stats.average_hp, first_survival_chance, 0.01, "Alice v Bob test failed")
+                att_stats, def_stats = wesnoth.simulate_combat(alice, charlie)
+                unit_test.assert_approx_equal(att_stats.average_hp, first_survival_chance, 0.01, "Alice v Charlie test failed")
+                att_stats, def_stats = wesnoth.simulate_combat(alice, dave)
+                unit_test.assert_approx_equal(att_stats.average_hp, first_survival_chance, 0.01, "Alice v Dave test failed")
+
+                -- When Alice is the defender, she should still get first strike against anyone except Charlie (who also has firststrike)
+                att_stats, def_stats = wesnoth.simulate_combat(bob, alice)
+                unit_test.assert_approx_equal(def_stats.average_hp, first_survival_chance, 0.01, "Bob v Alice test failed")
+                att_stats, def_stats = wesnoth.simulate_combat(charlie, alice)
+                unit_test.assert_approx_equal(def_stats.average_hp, second_survival_chance, 0.01, "Charlie v Alice test failed")
+                att_stats, def_stats = wesnoth.simulate_combat(dave, alice)
+                unit_test.assert_approx_equal(def_stats.average_hp, first_survival_chance, 0.01, "Dave v Alice test failed")
+
+                -- Attacker or defender, Dave always goes last, even against Bob
+                att_stats, def_stats = wesnoth.simulate_combat(bob, dave)
+                unit_test.assert_approx_equal(att_stats.average_hp, first_survival_chance, 0.01, "Bob v Dave test failed")
+                att_stats, def_stats = wesnoth.simulate_combat(dave, bob)
+                unit_test.assert_approx_equal(def_stats.average_hp, first_survival_chance, 0.01, "Dave v Bob test failed")
+
+                -- Final test is two units that both have laststrike
+                bob.attacks[1] = dave.attacks[1]
+                att_stats, def_stats = wesnoth.simulate_combat(bob, dave)
+                unit_test.assert_approx_equal(att_stats.average_hp, first_survival_chance, 0.01, "Both with laststrike: Bob v Dave failed")
+                att_stats, def_stats = wesnoth.simulate_combat(dave, bob)
+                unit_test.assert_approx_equal(def_stats.average_hp, second_survival_chance, 0.01, "Both with laststrike: Dave v Bob failed")
+            >>
+        [/lua]
+
+        {SUCCEED}
+    [/event]
+[/test]

data/test/scenarios/poison_opponent.cfg

@@ -0,0 +1,114 @@
+# this unit test what 'poison' applied to opponent work perfectly after fixing.
+{GENERIC_UNIT_TEST "poison_opponent" (
+    [event]
+        name=start
+        [modify_unit]
+            [filter]
+            [/filter]
+            max_hitpoints=100
+            hitpoints=100
+            attacks_left=1
+        [/modify_unit]
+        [object]
+            silent=yes
+            [effect]
+                apply_to=attack
+                [set_specials]
+                    mode=append
+                    [poison]
+                        apply_to=opponent
+                    [/poison]
+                    [attacks]
+                        value=1
+                    [/attacks]
+                    [damage]
+                        value=1
+                    [/damage]
+                    [chance_to_hit]
+                        value=100
+                    [/chance_to_hit]
+                [/set_specials]
+            [/effect]
+            [filter]
+                id=bob
+            [/filter]
+        [/object]
+        [object]
+            silent=yes
+            [effect]
+                apply_to=attack
+                [set_specials]
+                    mode=append
+                    [poison]
+                        apply_to=opponent
+                    [/poison]
+                    [attacks]
+                        value=1
+                    [/attacks]
+                    [damage]
+                        value=1
+                    [/damage]
+                    [chance_to_hit]
+                        value=100
+                    [/chance_to_hit]
+                [/set_specials]
+            [/effect]
+            [filter]
+                id=alice
+            [/filter]
+        [/object]
+
+        [store_unit]
+            [filter]
+                id=alice
+            [/filter]
+            variable=a
+            kill=yes
+        [/store_unit]
+        [store_unit]
+            [filter]
+                id=bob
+            [/filter]
+            variable=b
+        [/store_unit]
+        [unstore_unit]
+            variable=a
+            find_vacant=yes
+            x,y=$b.x,$b.y
+        [/unstore_unit]
+        [store_unit]
+            [filter]
+                id=alice
+            [/filter]
+            variable=a
+        [/store_unit]
+
+        [do_command]
+            [attack]
+                weapon=0
+                defender_weapon=0
+                [source]
+                    x,y=$a.x,$a.y
+                [/source]
+                [destination]
+                    x,y=$b.x,$b.y
+                [/destination]
+            [/attack]
+        [/do_command]
+        [store_unit]
+            [filter]
+                id=alice
+            [/filter]
+            variable=a
+        [/store_unit]
+        [store_unit]
+            [filter]
+                id=bob
+            [/filter]
+            variable=b
+        [/store_unit]
+        {ASSERT ({VARIABLE_CONDITIONAL a.status.poisoned boolean_equals yes})}
+        {ASSERT ({VARIABLE_CONDITIONAL b.status.poisoned boolean_equals yes})}
+        {SUCCEED}
+    [/event]
+)}

src/units/abilities.cpp

@@ -1537,29 +1537,30 @@ bool attack_type::special_active_impl(const_attack_ptr self_attack, const_attack
 	temporary_facing self_facing(self, self_loc.get_relative_dir(other_loc));
 	temporary_facing other_facing(other, other_loc.get_relative_dir(self_loc));
 
-	// Filter poison, plague, drain, first strike
-	if (tag_name == "drains" && other && other->get_state("undrainable")) {
-		return false;
-	}
-	if (tag_name == "plague" && other &&
-		(other->get_state("unplagueable") ||
-		 resources::gameboard->map().is_village(other_loc))) {
+	// Filter poison, plague, drain, slow, petrifies
+	// True if "whom" corresponds to "self", false if "whom" is "other"
+	bool whom_is_self = ((whom == AFFECT_SELF) || ((whom == AFFECT_EITHER) && special_affects_self(special, is_attacker)));
+	unit_const_ptr them = whom_is_self ? other : self;
+	map_location their_loc = whom_is_self ? other_loc : self_loc;
+
+	if (tag_name == "drains" && them && them->get_state("undrainable")) {
 		return false;
 	}
-	if (tag_name == "poison" && other &&
-		(other->get_state("unpoisonable") || other->get_state(unit::STATE_POISONED))) {
+	if (tag_name == "plague" && them &&
+		(them->get_state("unplagueable") ||
+		 resources::gameboard->map().is_village(their_loc))) {
 		return false;
 	}
-	if (tag_name == "firststrike" && !is_attacker && other_attack &&
-		other_attack->has_special_or_ability("firststrike")) {
+	if (tag_name == "poison" && them &&
+		(them->get_state("unpoisonable") || them->get_state(unit::STATE_POISONED))) {
 		return false;
 	}
-	if (tag_name == "slow" && other &&
-		(other->get_state("unslowable") || other->get_state(unit::STATE_SLOWED))) {
+	if (tag_name == "slow" && them &&
+		(them->get_state("unslowable") || them->get_state(unit::STATE_SLOWED))) {
 		return false;
 	}
-	if (tag_name == "petrifies" && other &&
-		other->get_state("unpetrifiable")) {
+	if (tag_name == "petrifies" && them &&
+		them->get_state("unpetrifiable")) {
 		return false;
 	}
 
@@ -1572,6 +1573,14 @@ bool attack_type::special_active_impl(const_attack_ptr self_attack, const_attack
 	const_attack_ptr att_weapon = is_attacker ? self_attack : other_attack;
 	const_attack_ptr def_weapon = is_attacker ? other_attack : self_attack;
 
+	// Filter firststrike here, if both units have first strike then the effects cancel out. Only check
+	// the opponent if "whom" is the defender, otherwise this leads to infinite recursion.
+	if (tag_name == "firststrike") {
+		bool whom_is_defender = whom_is_self ? !is_attacker : is_attacker;
+		if (whom_is_defender && att_weapon && att_weapon->has_special_or_ability("firststrike"))
+			return false;
+	}
+
 	// Filter the units involved.
 	if (!special_unit_matches(self, other, self_loc, self_attack, special, is_for_listing, filter_self))
 		return false;