5 files changed, 89 insertions, 61 deletions

diff --git a/src/openvic-simulation/military/Deployment.cpp b/src/openvic-simulation/military/Deployment.cpp
index bd176be..27cd124 100644
--- a/src/openvic-simulation/military/Deployment.cpp
+++ b/src/openvic-simulation/military/Deployment.cpp
@@ -144,7 +144,7 @@ bool DeploymentManager::load_oob_file(
 						return false;
 					}
 
-					army_regiments.push_back({regiment_name, *regiment_type, regiment_home});
+					army_regiments.push_back({ regiment_name, *regiment_type, regiment_home });
 
 					return ret;
 				},
diff --git a/src/openvic-simulation/military/LeaderTrait.cpp b/src/openvic-simulation/military/LeaderTrait.cpp
index da7331a..e00deec 100644
--- a/src/openvic-simulation/military/LeaderTrait.cpp
+++ b/src/openvic-simulation/military/LeaderTrait.cpp
@@ -1,5 +1,8 @@
 #include "LeaderTrait.hpp"
 
+#include "openvic-simulation/dataloader/NodeTools.hpp"
+#include "openvic-simulation/modifier/ModifierManager.hpp"
+
 using namespace OpenVic;
 using namespace OpenVic::NodeTools;
 
@@ -30,17 +33,14 @@ bool LeaderTraitManager::load_leader_traits_file(ModifierManager const& modifier
 		return expect_dictionary_reserve_length(
 			leader_traits,
 			[this, &modifier_manager, type](std::string_view trait_identifier, ast::NodeCPtr value) -> bool {
-				static const string_set_t allowed_modifiers = {
-					"attack", "defence", "morale", "organisation", "reconnaissance",
-					"speed", "attrition", "experience", "reliability"
-				};
-
 				ModifierValue modifiers;
-				bool ret = modifier_manager.expect_whitelisted_modifier_value(
-					move_variable_callback(modifiers), allowed_modifiers
+
+				bool ret = NodeTools::expect_dictionary(
+					modifier_manager.expect_leader_modifier(modifiers)
 				)(value);
 
 				ret &= add_leader_trait(trait_identifier, type, std::move(modifiers));
+
 				return ret;
 			}
 		);
diff --git a/src/openvic-simulation/military/UnitType.cpp b/src/openvic-simulation/military/UnitType.cpp
index 71bc5c3..c185154 100644
--- a/src/openvic-simulation/military/UnitType.cpp
+++ b/src/openvic-simulation/military/UnitType.cpp
@@ -1,7 +1,9 @@
 #include "UnitType.hpp"
 
 #include "openvic-simulation/country/CountryInstance.hpp"
+#include "openvic-simulation/dataloader/NodeTools.hpp"
 #include "openvic-simulation/map/TerrainType.hpp"
+#include "openvic-simulation/modifier/ModifierManager.hpp"
 
 using namespace OpenVic;
 using namespace OpenVic::NodeTools;
@@ -28,8 +30,17 @@ UnitType::UnitType(
 	build_time { unit_args.build_time },
 	build_cost { std::move(unit_args.build_cost) },
 	supply_consumption { unit_args.supply_consumption },
-	supply_cost { std::move(unit_args.supply_cost) },
-	terrain_modifiers { std::move(unit_args.terrain_modifiers) } {}
+	supply_cost { std::move(unit_args.supply_cost) } {
+
+	using enum Modifier::modifier_type_t;
+
+	for (auto [terrain, modifier_value] : mutable_iterator(unit_args.terrain_modifier_values)) {
+		terrain_modifiers.emplace(terrain, Modifier {
+			StringUtils::append_string_views(new_identifier, " ", terrain->get_identifier()), std::move(modifier_value),
+			UNIT_TERRAIN
+		});
+	}
+}
 
 bool UnitTypeBranched<LAND>::allowed_cultures_check_culture_in_country(
 	allowed_cultures_t allowed_cultures, Culture const& culture, CountryInstance const& country
@@ -105,8 +116,8 @@ static bool _check_shared_parameters(std::string_view identifier, UnitType::unit
 		return false;
 	}
 
-	if (unit_args.icon <= 0) {
-		Logger::error("Invalid icon for unit ", identifier, " - ", unit_args.icon, " (must be positive)");
+	if (unit_args.icon < 0) {
+		Logger::error("Invalid icon for unit ", identifier, " - ", unit_args.icon, " (must be >= 0)");
 		return false;
 	}
 
@@ -231,16 +242,18 @@ bool UnitTypeManager::load_unit_type_file(
 				good_definition_manager.expect_good_definition_decimal_map(move_variable_callback(unit_args.supply_cost))
 		);
 
-		auto add_terrain_modifier = [&unit_args, &terrain_type_manager, &modifier_manager](
+		auto add_terrain_modifier_value = [&unit_args, &terrain_type_manager, &modifier_manager](
 			std::string_view default_key, ast::NodeCPtr default_value
 		) -> bool {
 			TerrainType const* terrain_type = terrain_type_manager.get_terrain_type_by_identifier(default_key);
+
 			if (terrain_type != nullptr) {
-				// TODO - restrict what modifier effects can be used here
-				return modifier_manager.expect_modifier_value(
-					map_callback(unit_args.terrain_modifiers, terrain_type)
+				ModifierValue& modifier_value = unit_args.terrain_modifier_values[terrain_type];
+				return expect_dictionary(
+					modifier_manager.expect_unit_terrain_modifier(modifier_value, terrain_type->get_identifier())
 				)(default_value);
 			}
+
 			return key_value_invalid_callback(default_key, default_value);
 		};
 
@@ -274,7 +287,7 @@ bool UnitTypeManager::load_unit_type_file(
 				regiment_type_args.allowed_cultures = RegimentType::allowed_cultures_t::ALL_CULTURES;
 			}
 
-			ret &= expect_dictionary_key_map_and_default(key_map, add_terrain_modifier)(value);
+			ret &= expect_dictionary_key_map_and_default(key_map, add_terrain_modifier_value)(value);
 
 			ret &= add_regiment_type(key, unit_args, regiment_type_args);
 
@@ -301,7 +314,7 @@ bool UnitTypeManager::load_unit_type_file(
 				"torpedo_attack", ZERO_OR_ONE, expect_fixed_point(assign_variable_callback(ship_type_args.torpedo_attack))
 			);
 
-			ret &= expect_dictionary_key_map_and_default(key_map, add_terrain_modifier)(value);
+			ret &= expect_dictionary_key_map_and_default(key_map, add_terrain_modifier_value)(value);
 
 			ret &= add_ship_type(key, unit_args, ship_type_args);
 
@@ -319,59 +332,71 @@ bool UnitTypeManager::generate_modifiers(ModifierManager& modifier_manager) cons
 	bool ret = true;
 
 	const auto generate_stat_modifiers = [&modifier_manager, &ret](
-		std::string_view identifier, UnitType::branch_t branch
+		std::derived_from<ModifierEffectCache::unit_type_effects_t> auto unit_type_effects, std::string_view identifier
 	) -> void {
+		using enum ModifierEffect::format_t;
+
 		const auto stat_modifier = [&modifier_manager, &ret, &identifier](
-			std::string_view suffix, bool is_positive_good, ModifierEffect::format_t format, std::string_view localisation_key
+			ModifierEffect const*& effect_cache, std::string_view suffix, bool is_positive_good,
+			ModifierEffect::format_t format, std::string_view localisation_key
 		) -> void {
-			ret &= modifier_manager.add_modifier_effect(
-				ModifierManager::get_flat_identifier(identifier, suffix), is_positive_good, format,
+			ret &= modifier_manager.register_technology_modifier_effect(
+				effect_cache, ModifierManager::get_flat_identifier(identifier, suffix), is_positive_good, format,
 				StringUtils::append_string_views("$", identifier, "$: $", localisation_key, "$")
 			);
 		};
 
-		using enum ModifierEffect::format_t;
-
 		ret &= modifier_manager.register_complex_modifier(identifier);
 
-		stat_modifier("attack", true, RAW_DECIMAL, "ATTACK");
-		stat_modifier("defence", true, RAW_DECIMAL, "DEFENCE");
-		stat_modifier("default_organisation", true, RAW_DECIMAL, "DEFAULT_ORG");
-		stat_modifier("maximum_speed", true, RAW_DECIMAL, "MAXIMUM_SPEED");
-		stat_modifier("build_time", false, INT, "BUILD_TIME");
-		stat_modifier("supply_consumption", false, PROPORTION_DECIMAL, "SUPPLY_CONSUMPTION");
+		stat_modifier(unit_type_effects.attack, "attack", true, RAW_DECIMAL, "ATTACK");
+		stat_modifier(unit_type_effects.defence, "defence", true, RAW_DECIMAL, "DEFENCE");
+		stat_modifier(unit_type_effects.default_organisation, "default_organisation", true, RAW_DECIMAL, "DEFAULT_ORG");
+		stat_modifier(unit_type_effects.maximum_speed, "maximum_speed", true, RAW_DECIMAL, "MAXIMUM_SPEED");
+		stat_modifier(unit_type_effects.build_time, "build_time", false, INT, "BUILD_TIME");
+		stat_modifier(
+			unit_type_effects.supply_consumption, "supply_consumption", false, PROPORTION_DECIMAL, "SUPPLY_CONSUMPTION"
+		);
 
-		switch (branch) {
-		case LAND:
-			stat_modifier("reconnaissance", true, RAW_DECIMAL, "RECONAISSANCE");
-			stat_modifier("discipline", true, PROPORTION_DECIMAL, "DISCIPLINE");
-			stat_modifier("support", true, PROPORTION_DECIMAL, "SUPPORT");
-			stat_modifier("maneuver", true, INT, "Maneuver");
-			stat_modifier("siege", true, RAW_DECIMAL, "SIEGE");
-			break;
-		case NAVAL:
-			stat_modifier("colonial_points", true, INT, "COLONIAL_POINTS_TECH");
-			stat_modifier("supply_consumption_score", false, INT, "SUPPLY_LOAD");
-			stat_modifier("hull", true, RAW_DECIMAL, "HULL");
-			stat_modifier("gun_power", true, RAW_DECIMAL, "GUN_POWER");
-			stat_modifier("fire_range", true, RAW_DECIMAL, "FIRE_RANGE");
-			stat_modifier("evasion", true, PROPORTION_DECIMAL, "EVASION");
-			stat_modifier("torpedo_attack", true, RAW_DECIMAL, "TORPEDO_ATTACK");
-			break;
-		default:
+		if constexpr (std::same_as<decltype(unit_type_effects), ModifierEffectCache::regiment_type_effects_t>) {
+			stat_modifier(unit_type_effects.reconnaissance, "reconnaissance", true, RAW_DECIMAL, "RECONAISSANCE");
+			stat_modifier(unit_type_effects.discipline, "discipline", true, PROPORTION_DECIMAL, "DISCIPLINE");
+			stat_modifier(unit_type_effects.support, "support", true, PROPORTION_DECIMAL, "SUPPORT");
+			stat_modifier(unit_type_effects.maneuver, "maneuver", true, INT, "Maneuver");
+			stat_modifier(unit_type_effects.siege, "siege", true, RAW_DECIMAL, "SIEGE");
+		} else if constexpr(std::same_as<decltype(unit_type_effects), ModifierEffectCache::ship_type_effects_t>) {
+			stat_modifier(unit_type_effects.colonial_points, "colonial_points", true, INT, "COLONIAL_POINTS_TECH");
+			stat_modifier(unit_type_effects.supply_consumption_score, "supply_consumption_score", false, INT, "SUPPLY_LOAD");
+			stat_modifier(unit_type_effects.hull, "hull", true, RAW_DECIMAL, "HULL");
+			stat_modifier(unit_type_effects.gun_power, "gun_power", true, RAW_DECIMAL, "GUN_POWER");
+			stat_modifier(unit_type_effects.fire_range, "fire_range", true, RAW_DECIMAL, "FIRE_RANGE");
+			stat_modifier(unit_type_effects.evasion, "evasion", true, PROPORTION_DECIMAL, "EVASION");
+			stat_modifier(unit_type_effects.torpedo_attack, "torpedo_attack", true, RAW_DECIMAL, "TORPEDO_ATTACK");
+		} else {
 			/* Unreachable - unit types are only added via add_regiment_type or add_ship_type which set branch to LAND or NAVAL. */
-			Logger::error("Invalid branch for unit ", identifier, ": ", static_cast<int>(branch));
+			Logger::error("Invalid branch for unit ", identifier, " - not LAND or NAVAL!");
 		}
 	};
 
-	generate_stat_modifiers("army_base", LAND);
+	generate_stat_modifiers(modifier_manager.modifier_effect_cache.army_base_effects, "army_base");
+
+	IndexedMap<RegimentType, ModifierEffectCache::regiment_type_effects_t>& regiment_type_effects =
+		modifier_manager.modifier_effect_cache.regiment_type_effects;
+
+	regiment_type_effects.set_keys(&get_regiment_types());
+
 	for (RegimentType const& regiment_type : get_regiment_types()) {
-		generate_stat_modifiers(regiment_type.get_identifier(), LAND);
+		generate_stat_modifiers(regiment_type_effects[regiment_type], regiment_type.get_identifier());
 	}
 
-	generate_stat_modifiers("navy_base", NAVAL);
+	generate_stat_modifiers(modifier_manager.modifier_effect_cache.navy_base_effects, "navy_base");
+
+	IndexedMap<ShipType, ModifierEffectCache::ship_type_effects_t>& ship_type_effects =
+		modifier_manager.modifier_effect_cache.ship_type_effects;
+
+	ship_type_effects.set_keys(&get_ship_types());
+
 	for (ShipType const& ship_type : get_ship_types()) {
-		generate_stat_modifiers(ship_type.get_identifier(), NAVAL);
+		generate_stat_modifiers(ship_type_effects[ship_type], ship_type.get_identifier());
 	}
 
 	return ret;
diff --git a/src/openvic-simulation/military/UnitType.hpp b/src/openvic-simulation/military/UnitType.hpp
index 3fa9af5..9527041 100644
--- a/src/openvic-simulation/military/UnitType.hpp
+++ b/src/openvic-simulation/military/UnitType.hpp
@@ -20,7 +20,7 @@ namespace OpenVic {
 
 	struct UnitType : HasIdentifier {
 		using icon_t = uint32_t;
-		using terrain_modifiers_t = ordered_map<TerrainType const*, ModifierValue>;
+		using terrain_modifiers_t = ordered_map<TerrainType const*, Modifier>;
 
 		enum struct branch_t : uint8_t { INVALID_BRANCH, LAND, NAVAL };
 		enum struct unit_category_t : uint8_t {
@@ -28,6 +28,8 @@ namespace OpenVic {
 		};
 
 		struct unit_type_args_t {
+			using terrain_modifier_values_t = ordered_map<TerrainType const*, ModifierValue>;
+
 			icon_t icon = 0;
 			unit_category_t unit_category = unit_category_t::INVALID_UNIT_CATEGORY;
 			// TODO defaults for move_sound and select_sound
@@ -38,7 +40,7 @@ namespace OpenVic {
 				supply_consumption = 0;
 			Timespan build_time;
 			GoodDefinition::good_definition_map_t build_cost, supply_cost;
-			terrain_modifiers_t terrain_modifiers;
+			terrain_modifier_values_t terrain_modifier_values;
 
 			unit_type_args_t() = default;
 			unit_type_args_t(unit_type_args_t&&) = default;
diff --git a/src/openvic-simulation/military/Wargoal.cpp b/src/openvic-simulation/military/Wargoal.cpp
index 773e791..ce6b153 100644
--- a/src/openvic-simulation/military/Wargoal.cpp
+++ b/src/openvic-simulation/military/Wargoal.cpp
@@ -86,6 +86,7 @@ bool WargoalTypeManager::load_wargoal_file(ovdl::v2script::Parser const& parser)
 			}
 
 			using enum WargoalType::peace_options_t;
+			using enum scope_type_t;
 
 			std::string_view war_name;
 			Timespan available {}, truce {};
@@ -94,12 +95,12 @@ bool WargoalTypeManager::load_wargoal_file(ovdl::v2script::Parser const& parser)
 				mutual = false, all_allowed_states = false, always = false;
 			WargoalType::peace_options_t peace_options = NO_PEACE_OPTIONS;
 			WargoalType::peace_modifiers_t modifiers;
-			ConditionScript can_use { scope_t::COUNTRY, scope_t::COUNTRY, scope_t::COUNTRY };
-			ConditionScript is_valid { scope_t::COUNTRY, scope_t::COUNTRY, scope_t::COUNTRY };
-			ConditionScript allowed_states { scope_t::STATE, scope_t::COUNTRY, scope_t::COUNTRY };
-			ConditionScript allowed_substate_regions { scope_t::STATE, scope_t::COUNTRY, scope_t::COUNTRY };
-			ConditionScript allowed_states_in_crisis { scope_t::STATE, scope_t::COUNTRY, scope_t::COUNTRY };
-			ConditionScript allowed_countries { scope_t::COUNTRY, scope_t::COUNTRY, scope_t::COUNTRY };
+			ConditionScript can_use { COUNTRY, COUNTRY, COUNTRY };
+			ConditionScript is_valid { COUNTRY, COUNTRY, COUNTRY };
+			ConditionScript allowed_states { STATE, COUNTRY, COUNTRY };
+			ConditionScript allowed_substate_regions { STATE, COUNTRY, COUNTRY };
+			ConditionScript allowed_states_in_crisis { STATE, COUNTRY, COUNTRY };
+			ConditionScript allowed_countries { COUNTRY, COUNTRY, COUNTRY };
 			EffectScript on_add, on_po_accepted; //country as default scope for both
 
 			const auto expect_peace_option = [&peace_options](WargoalType::peace_options_t peace_option) -> node_callback_t {