Fix [resistance_defaults] and [terrain_defaults] (issue #5308)

The bug introduced in fcd0ced is fixed by ensuring that movetype::merge() is
called with the strings "movement_costs", "vision_costs" and "jamming_costs"
instead of "movement", "vision" and "jamming".

Both [terrain_defaults][movement] and [terrain_defaults][movement_costs] are
supported and equivalent, as added to the master branch in 3522eb2.

These now work again:

    [resistance_defaults]
        id="special_res_for_test"
        default="30"
    [/resistance_defaults]

    [resistance_defaults]
        id="copy_of_arcane"
        default="(arcane)"
    [/resistance_defaults]

    [terrain_defaults]
        id="special_terrain_for_test"
        [movement]
            default="(swamp_water + 1)"
            orcishfoot="(swamp_water * 2)"
        [/movement]
    [/terrain_defaults]

Formulas can now access other parts of the movetype too, as this allowed the
code to be the same as in 1.15:

    [terrain_defaults]
        id="special_terrain_for_test"
        [movement_costs]
            default="(swamp_water + 1)"
            orcishfoot="(vision_costs.swamp_water * 2)"
        [/movement_costs]
    [/terrain_defaults]
    [terrain_defaults]
        id="special_terrain_for_test"
        [defense]
            default="(20 + 7 * movement_costs.special_terrain_for_test)"
        [/defense]
    [/terrain_defaults]

The formula handling will recognise "resistance", "movement_costs",
"vision_costs", "jamming_costs" and "defense". For [resistance_defaults], the
formula will recognise both "(arcane)" and "(resistance.arcane)" as equivalent,
similarly for [terrain_defaults] "(swamp_water)" is a shorthand for whichever
subtag is being patched.

A [terrain_defaults] tag may use data added in a previous [terrain_defaults],
as in the examples above where the second tag's [defense] is based on the first
tag's [movement_costs], this gives orcish grunts on the special terrain a 62%
chance to be hit. However, relying on data in the same [terrain_defaults] that
creates or changes it is unsupported - if the [movement_costs] and [defense]
were in a single [terrain_defaults] tag then the result would be implementation
defined, because no guarantee is made of the order in which the children of the
tag are processed.

The unit tests for [terrain_defaults] and [resistance_defaults] must stay
out-of-tree until we support some method of testing them without affecting the
core units' stats during other tests. The test added here is limited to
checking the ways that WML can read the stats, it's also a framework for the
out-of-tree parts to use.

This is a squashed cherry pick of these commits from 1.15:
* 72863e5 (the unit test)
* 0ba4332
* 3522eb2 (adding the aliases)
* 0035b77 (fix for empty children)

changelog.md

@@ -17,6 +17,7 @@
    * Added support to wmlxgettext for double-quote characters in translatable raw strings
    * Fixed an error message and the AI leader potentially not moving when it cannot reach a keep because it's occupied by an allied unit
    * Fixed display zoom not being taken into account when using the `x`, `y`, `directional_x` and `directional_y` attributes in unit animations.
+   * Fixed `[terrain_defaults]` and `[resistance_defaults]` (issue #5308).
 
 ## Version 1.14.15
  ### Add-ons client

data/test/scenarios/test_resistances.cfg

@@ -0,0 +1,46 @@
+# wmllint: no translatables
+
+{GENERIC_UNIT_TEST "test_resistances" (
+    [event]
+        name = start
+
+        [store_unit]
+            [filter]
+                id=alice
+            [/filter]
+            variable=alice
+        [/store_unit]
+
+        # Elvish archers have a slight malus against arcane
+        {ASSERT {VARIABLE_CONDITIONAL alice.resistance.arcane equals 110}}
+        {ASSERT {VARIABLE_CONDITIONAL alice.resistance.impact equals 100}}
+        {ASSERT {VARIABLE_CONDITIONAL alice.resistance.length equals 1}}
+
+        [store_unit]
+            [filter]
+                id=bob
+            [/filter]
+            variable=bob
+        [/store_unit]
+
+        {ASSERT {VARIABLE_CONDITIONAL bob.resistance.arcane equals 100}}
+        {ASSERT {VARIABLE_CONDITIONAL bob.resistance.impact equals 100}}
+
+        # Check that there are exactly 6 resistance types, therefore that
+        # none have been added by a [resistance_defaults] tag.
+        {ASSERT {VARIABLE_CONDITIONAL bob.resistance.length equals 1}}
+        {VARIABLE key_count -1}
+        [lua]
+            code = <<
+                local key_count = 0
+                for k,v in pairs(wml.variables["bob.resistance[0]"]) do
+                    key_count = key_count + 1
+                end
+                wml.variables["key_count"] = key_count
+                >>
+        [/lua]
+        {ASSERT {VARIABLE_CONDITIONAL key_count equals 6}}
+
+        {SUCCEED}
+    [/event]
+)}

src/movetype.cpp

@@ -889,6 +889,9 @@ void movetype::merge(const config & new_cfg, const std::string & applies_to, boo
 	else if(applies_to == "resistance") {
 		resist_.merge(new_cfg, overwrite);
 	}
+	else {
+		ERR_CF << "movetype::merge with unknown applies_to: " << applies_to << std::endl;
+	}
 }
 
 /**

src/units/types.cpp

@@ -19,6 +19,7 @@
 
 #include "units/types.hpp"
 
+#include "formula/callable_objects.hpp"
 #include "game_config.hpp"
 #include "game_errors.hpp" //thrown sometimes
 //#include "gettext.hpp"
@@ -32,7 +33,7 @@
 #include "gui/auxiliary/typed_formula.hpp"
 #include "gui/dialogs/loading_screen.hpp"
 
-#include <boost/regex.hpp>
+#include <boost/range/algorithm_ext/erase.hpp>
 
 #include <locale>
 
@@ -1075,33 +1076,79 @@ void handle_variations(config& ut_cfg)
 	ut_cfg.splice_children(variations, "variation");
 }
 
-const boost::regex fai_identifier("[a-zA-Z_]+");
-
-template<typename MoveT>
-void patch_movetype(
-		MoveT& mt, const std::string& type, const std::string& new_key, const std::string& formula_str, int default_val, bool replace)
+/**
+ * Insert a new value into a movetype, possibly calculating the value based on
+ * the existing values in the target movetype.
+ */
+void patch_movetype(movetype& mt,
+	const std::string& type_to_patch,
+	const std::string& new_key,
+	const std::string& formula_str,
+	int default_val,
+	bool replace)
 {
-	config temp_cfg, original_cfg;
-	mt.write(original_cfg);
+	LOG_CONFIG << "Patching " << new_key << " into movetype." << type_to_patch << std::endl;
+	config mt_cfg;
+	mt.write(mt_cfg);
 
-	if(!replace && !original_cfg[new_key].blank()) {
-		// Don't replace if the key already exists in the config (even if empty).
+	if(!replace && mt_cfg.child_or_empty(type_to_patch).has_attribute(new_key)) {
+		// Don't replace if this type already exists in the config
 		return;
 	}
 
-	gui2::typed_formula<int> formula(formula_str);
-	wfl::map_formula_callable original;
+	// Make movement_costs.flat, defense.castle, etc available to the formula.
+	// The formula uses config_callables which take references to data in mt;
+	// the enclosing scope is to run all the destructors before calling mt's
+	// non-const merge() function. Probably unnecessary, but I'd rather write
+	// it this way than debug it afterwards.
+	config temp_cfg;
+	{
+		// Instances of wfl::config_callable take a reference to a config,
+		// which means that the "cumulative_values" variable below needs to be
+		// copied so that movement costs aren't overwritten by vision costs
+		// before the formula is evaluated.
+		std::list<config> config_copies;
+
+		gui2::typed_formula<int> formula(formula_str, default_val);
+		wfl::map_formula_callable original;
+
+		// These three need to follow movetype's fallback system, where values for
+		// movement costs are used for vision too.
+		const auto fallback_children = std::array<std::string, 3>{{"movement_costs", "vision_costs", "jamming_costs"}};
+		config cumulative_values;
+		for(const auto& x : fallback_children) {
+			if(mt_cfg.has_child(x)) {
+				cumulative_values.merge_with(mt_cfg.child(x));
+			}
+			config_copies.emplace_back(cumulative_values);
+			auto val = std::make_shared<wfl::config_callable>(config_copies.back());
+			original.add(x, val);
 
-	boost::sregex_iterator m(formula_str.begin(), formula_str.end(), fai_identifier);
-	for(const boost::sregex_iterator::value_type& p : std::make_pair(m, boost::sregex_iterator())) {
-		const std::string var_name = p.str();
+			// Allow "flat" to work as "vision_costs.flat" when patching vision_costs, etc
+			if(type_to_patch == x) {
+				original.set_fallback(val);
+			}
+		}
 
-		wfl::variant val(original_cfg[var_name].to_int(default_val));
-		original.add(var_name, val);
-	}
+		// These don't need the fallback system
+		const auto child_names = std::array<std::string, 2>{{"defense", "resistance"}};
+		for(const auto& x : child_names) {
+			if(mt_cfg.has_child(x)) {
+				const auto& subtag = mt_cfg.child(x);
+				auto val = std::make_shared<wfl::config_callable>(subtag);
+				original.add(x, val);
+
+				// Allow "arcane" to work as well as "resistance.arcane", etc
+				if(type_to_patch == x) {
+					original.set_fallback(val);
+				}
+			}
+		}
 
-	temp_cfg[new_key] = formula(original);
-	mt.merge(temp_cfg, type, true);
+		LOG_CONFIG << " formula=" << formula_str << ", resolves to " << formula(original) << std::endl;
+		temp_cfg[new_key] = formula(original);
+	}
+	mt.merge(temp_cfg, type_to_patch, true);
 }
 } // unnamed namespace
 
@@ -1131,9 +1178,9 @@ void unit_type_data::set_config(config& cfg)
 	}
 
 	// Movetype resistance patching
+	DBG_CF << "Start of movetype patching" << std::endl;
 	for(const config& r : cfg.child_range("resistance_defaults")) {
 		const std::string& dmg_type = r["id"];
-		config temp_cfg;
 
 		for(const config::attribute& attr : r.attribute_range()) {
 			const std::string& mt = attr.first;
@@ -1142,7 +1189,8 @@ void unit_type_data::set_config(config& cfg)
 				continue;
 			}
 
-			patch_movetype(movement_types_[mt], "resistances", dmg_type, attr.second, 100, true);
+			DBG_CF << "Patching specific movetype " << mt << std::endl;
+			patch_movetype(movement_types_[mt], "resistance", dmg_type, attr.second, 100, true);
 		}
 
 		if(r.has_attribute("default")) {
@@ -1151,25 +1199,50 @@ void unit_type_data::set_config(config& cfg)
 				if(r.has_attribute(mt.first)) {
 					continue;
 				}
+				// The "none" movetype expects everything to have the default value (to be UNREACHABLE)
+				if(mt.first == "none") {
+					continue;
+				}
 
-				patch_movetype(mt.second, "resistances", dmg_type, r["default"], 100, false);
+				patch_movetype(mt.second, "resistance", dmg_type, r["default"], 100, false);
 			}
 		}
 	}
+	DBG_CF << "Split between resistance and cost patching" << std::endl;
 
 	// Movetype move/defend patching
 	for(const config& terrain : cfg.child_range("terrain_defaults")) {
 		const std::string& ter_type = terrain["id"];
-		config temp_cfg;
 
-		static const std::string terrain_info_tags[] {"movement", "vision", "jamming", "defense"};
-
-		for(const std::string& tag : terrain_info_tags) {
-			if(!terrain.has_child(tag)) {
+		struct ter_defs_to_movetype
+		{
+			/** The data to read from is in [terrain_defaults][subtag], and corresponds to [movetype][subtag] */
+			std::string subtag;
+			/** Deprecated names used in 1.14.0's [terrain_defaults]. For [defense] the name didn't change. */
+			std::string alias;
+			int default_val;
+		};
+		const std::array<ter_defs_to_movetype, 4> terrain_info_tags{
+			ter_defs_to_movetype{{"movement_costs"}, {"movement"}, movetype::UNREACHABLE},
+			ter_defs_to_movetype{{"vision_costs"}, {"vision"}, movetype::UNREACHABLE},
+			ter_defs_to_movetype{{"jamming_costs"}, {"jamming"}, movetype::UNREACHABLE},
+			ter_defs_to_movetype{{"defense"}, {"defense"}, 100}
+		};
+
+		for(const auto& cost_type : terrain_info_tags) {
+			const std::string* src_tag = nullptr;
+			if(terrain.has_child(cost_type.subtag)) {
+				src_tag = &cost_type.subtag;
+			}
+			else if(terrain.has_child(cost_type.alias)) {
+				// Check for the deprecated name, no deprecation warnings are printed.
+				src_tag = &cost_type.alias;
+			}
+			if(!src_tag) {
 				continue;
 			}
 
-			const config& info = terrain.child(tag);
+			const config& info = terrain.child(*src_tag);
 
 			for(const config::attribute& attr : info.attribute_range()) {
 				const std::string& mt = attr.first;
@@ -1178,11 +1251,8 @@ void unit_type_data::set_config(config& cfg)
 					continue;
 				}
 
-				if(tag == "defense") {
-					patch_movetype(movement_types_[mt], tag, ter_type, attr.second, 100, true);
-				} else {
-					patch_movetype(movement_types_[mt], tag, ter_type, attr.second, 99, true);
-				}
+				patch_movetype(
+					movement_types_[mt], cost_type.subtag, ter_type, attr.second, cost_type.default_val, true);
 			}
 
 			if(info.has_attribute("default")) {
@@ -1191,16 +1261,18 @@ void unit_type_data::set_config(config& cfg)
 					if(info.has_attribute(mt.first)) {
 						continue;
 					}
-
-					if(tag == "defense") {
-						patch_movetype(mt.second, tag, ter_type, info["default"], 100, false);
-					} else {
-						patch_movetype(mt.second, tag, ter_type, info["default"], 99, false);
+					// The "none" movetype expects everything to have the default value
+					if(mt.first == "none") {
+						continue;
 					}
+
+					patch_movetype(
+						mt.second, cost_type.subtag, ter_type, info["default"], cost_type.default_val, false);
 				}
 			}
 		}
 	}
+	DBG_CF << "End of movetype patching" << std::endl;
 
 	// Apply base units.
 	for(config& ut : cfg.child_range("unit_type")) {

wml_test_schedule

@@ -140,6 +140,7 @@
 0 test_elf_longsighted_cave_vision
 0 test_elf_longsighted_cave_and_hills_vision
 0 test_elf_longsighted_cave_slow_cave_vision
+0 test_resistances
 #
 # Attack calculations & codepath tests
 #