From 2b8e887bcf5f474f57612e5a71286f0ac7ce7e8a Mon Sep 17 00:00:00 2001 From: Steve Cotton Date: Thu, 26 Nov 2020 11:30:29 +0100 Subject: [PATCH] Fix [resistance_defaults] and [terrain_defaults] (issue #5308) The bug introduced in fcd0ceda 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 3522eb2c. 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: * 72863e578ba2153520488f096f88d024d7795340 (the unit test) * 0ba433203e508478fff7f79f7fcb64d186ba40e7 * 3522eb2c2a13c7d78480117a83f55946a0dee2d4 (adding the aliases) * 0035b776d717010fa7f1c9d72fbba79b56ab653c (fix for empty children) --- changelog.md | 1 + data/test/scenarios/test_resistances.cfg | 46 +++++++ src/movetype.cpp | 3 + src/units/types.cpp | 150 +++++++++++++++++------ wml_test_schedule | 1 + 5 files changed, 162 insertions(+), 39 deletions(-) create mode 100644 data/test/scenarios/test_resistances.cfg diff --git a/changelog.md b/changelog.md index 310a9fd00ec3..fd49f888259f 100644 --- a/changelog.md +++ b/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 diff --git a/data/test/scenarios/test_resistances.cfg b/data/test/scenarios/test_resistances.cfg new file mode 100644 index 000000000000..dca844f439a2 --- /dev/null +++ b/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] +)} diff --git a/src/movetype.cpp b/src/movetype.cpp index ca0d342d36ed..aa44b650582a 100644 --- a/src/movetype.cpp +++ b/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; + } } /** diff --git a/src/units/types.cpp b/src/units/types.cpp index 2aceb63ef84b..71d6375a495f 100644 --- a/src/units/types.cpp +++ b/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 +#include #include @@ -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 -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 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_copies; + + gui2::typed_formula 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{{"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(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{{"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(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 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")) { diff --git a/wml_test_schedule b/wml_test_schedule index b207f3f920af..3982256c15f6 100644 --- a/wml_test_schedule +++ b/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 #