Fix #8460 [effect] apply_to=variation (#8475)

* Fix #8460 [effect] apply_to=variation

Previously the code could apply the variation effects
last, so that codes like
```
[effect]
  apply_to=variation
  ..
[/effect]
[effect]
  apply_to=hitpoints
  heal_full=yes
[/effect]
```
Would not set the unit hitpoints to the new variations
hitpoints because the variation effect was applied after
the healing effect.

In 1.16 this worked because healing was applied a little
too often but that lead also to bugs like #8342

* f prev

* f prev

* f prev

* f prev

* f prev

* Create modification_effect_type_variation.cfg

* Update wml_test_schedule

data/test/scenarios/wml_tests/UnitsWML/modification_effect_type_variation.cfg

@@ -0,0 +1,86 @@
+#textdomain wesnoth-test
+
+#####
+# API(s) being tested: [effect]apply_to=variation,[effect]heal_full=yes
+##
+# Actions:
+# Turn Bob into a walking corpse (18 max hp)
+# Turn Bob into a drake walking corpse (23 max hp)
+# Apply a full heal
+##
+# Expected end state:
+# Bob's current hp is 23
+#####
+{GENERIC_UNIT_TEST "effect_type_variation_full_heal" (
+    [event]
+        name=side 1 turn 1
+
+        [object]
+            silent=yes
+            [filter]
+                id=bob
+            [/filter]
+            [effect]
+                apply_to=type
+                name="Walking Corpse"
+            [/effect]
+            [effect]
+                apply_to=variation
+                name="drake"
+            [/effect]
+            [effect]
+                apply_to=hitpoints
+                heal_full=yes
+            [/effect]
+        [/object]
+
+        {ASSERT (
+            [have_unit]
+                id=bob
+                formula="self.hitpoints=23"
+            [/have_unit]
+        )}
+
+        {SUCCEED}
+    [/event]
+)}
+
+#####
+# API(s) being tested: [effect]apply_to=variation,[effect]heal_full=yes
+##
+# Actions:
+# Turn Bob into a walking corpse (18 max hp)
+# Turn Bob into a drake walking corpse (23 max hp)
+##
+# Expected end state:
+# Bob's current hp is 18
+#####
+{GENERIC_UNIT_TEST "effect_type_variation_no_heal" (
+    [event]
+        name=side 1 turn 1
+
+        [object]
+            silent=yes
+            [filter]
+                id=bob
+            [/filter]
+            [effect]
+                apply_to=type
+                name="Walking Corpse"
+            [/effect]
+            [effect]
+                apply_to=variation
+                name="drake"
+            [/effect]
+        [/object]
+
+        {ASSERT (
+            [have_unit]
+                id=bob
+                formula="self.hitpoints=18"
+            [/have_unit]
+        )}
+
+        {SUCCEED}
+    [/event]
+)}

src/units/unit.cpp

@@ -2442,14 +2442,19 @@ void unit::add_modification(const std::string& mod_type, const config& mod, bool
 {
 	bool generate_description = mod["generate_description"].to_bool(true);
 
+	config* target = nullptr;
+
 	if(no_add == false) {
-		modifications_.add_child(mod_type, mod);
+		target = &modifications_.add_child(mod_type, mod);
+		target->remove_children("effect");
 	}
 
-	bool set_poisoned = false; // Tracks if the poisoned state was set after the type or variation was changed.
-	config type_effect, variation_effect;
 	std::vector<t_string> effects_description;
 	for(const config& effect : mod.child_range("effect")) {
+		if(target) {
+			//Store effects only after they are added to avoid double applying effects on advance with apply_to=variation.
+			target->add_child("effect", effect);
+		}
 		// Apply SUF.
 		if(auto afilter = effect.optional_child("filter")) {
 			assert(resources::filter_con);
@@ -2461,6 +2466,10 @@ void unit::add_modification(const std::string& mod_type, const config& mod, bool
 		int times = effect["times"].to_int(1);
 		t_string description;
 
+		if(no_add && (apply_to == "type" || apply_to == "variation")) {
+			continue;
+		}
+
 		if(effect["times"] == "per level") {
 			if(effect["apply_to"] == "level") {
 				WRN_UT << "[effect] times=per level is not allowed with apply_to=level, using default value of 1";
@@ -2474,20 +2483,6 @@ void unit::add_modification(const std::string& mod_type, const config& mod, bool
 		if(times) {
 			while (times > 0) {
 				times --;
-
-				bool was_poisoned = get_state(STATE_POISONED);
-				// Apply unit type/variation changes last to avoid double applying effects on advance.
-				if(apply_to == "type") {
-					set_poisoned = false;
-					type_effect = effect;
-					continue;
-				}
-				if(apply_to == "variation") {
-					set_poisoned = false;
-					variation_effect = effect;
-					continue;
-				}
-
 				std::string description_component;
 				if(resources::lua_kernel) {
 					description_component = resources::lua_kernel->apply_effect(apply_to, *this, effect, true);
@@ -2501,11 +2496,6 @@ void unit::add_modification(const std::string& mod_type, const config& mod, bool
 				if(!times) {
 					description += description_component;
 				}
-				if(!was_poisoned && get_state(STATE_POISONED)) {
-					set_poisoned = true;
-				} else if(was_poisoned && !get_state(STATE_POISONED)) {
-					set_poisoned = false;
-				}
 			} // end while
 		} else { // for times = per level & level = 0 we still need to rebuild the descriptions
 			if(resources::lua_kernel) {
@@ -2523,33 +2513,6 @@ void unit::add_modification(const std::string& mod_type, const config& mod, bool
 			effects_description.push_back(description);
 		}
 	}
-	// Apply variations -- only apply if we are adding this for the first time.
-	if((!type_effect.empty() || !variation_effect.empty()) && no_add == false) {
-		if(!type_effect.empty()) {
-			std::string description;
-			if(resources::lua_kernel) {
-				description = resources::lua_kernel->apply_effect(type_effect["apply_to"], *this, type_effect, true);
-			} else if(builtin_effects.count(type_effect["apply_to"])) {
-				apply_builtin_effect(type_effect["apply_to"], type_effect);
-				description = describe_builtin_effect(type_effect["apply_to"], type_effect);
-			}
-			effects_description.push_back(description);
-		}
-		if(!variation_effect.empty()) {
-			std::string description;
-			if(resources::lua_kernel) {
-				description = resources::lua_kernel->apply_effect(variation_effect["apply_to"], *this, variation_effect, true);
-			} else if(builtin_effects.count(variation_effect["apply_to"])) {
-				apply_builtin_effect(variation_effect["apply_to"], variation_effect);
-				description = describe_builtin_effect(variation_effect["apply_to"], variation_effect);
-			}
-			effects_description.push_back(description);
-		}
-		if(set_poisoned)
-			// An effect explicitly set the poisoned state, and this
-			// should override the unit being immune to poison.
-			set_state(STATE_POISONED, true);
-	}
 
 	t_string description;
 

wml_test_schedule

@@ -117,6 +117,8 @@
 0 modify_unit_which_recall_list
 0 modify_unit_recall_cost
 0 put_to_recall_and_modify
+0 effect_type_variation_full_heal
+0 effect_type_variation_no_heal
 0 event_handlers_in_events_1
 0 event_handlers_in_events_3
 0 event_handlers_in_events_2