Switch to excluding given modifier effect targets rather than using a positive filter

9 files changed, 133 insertions, 38 deletions

diff --git a/src/openvic-simulation/country/CountryInstance.cpp b/src/openvic-simulation/country/CountryInstance.cpp
index aceae11..417ca0a 100644
--- a/src/openvic-simulation/country/CountryInstance.cpp
+++ b/src/openvic-simulation/country/CountryInstance.cpp
@@ -1028,9 +1028,7 @@ void CountryInstance::update_modifier_sum(Date today, StaticModifierCache const&
 void CountryInstance::contribute_province_modifier_sum(ModifierSum const& province_modifier_sum) {
 	using enum ModifierEffect::target_t;
 
-	static constexpr ModifierEffect::target_t NOT_PROVINCE = ALL_TARGETS & ~PROVINCE;
-
-	modifier_sum.add_modifier_sum_filter_targets(province_modifier_sum, NOT_PROVINCE);
+	modifier_sum.add_modifier_sum_exclude_targets(province_modifier_sum, PROVINCE);
 }
 
 fixed_point_t CountryInstance::get_modifier_effect_value(ModifierEffect const& effect) const {
diff --git a/src/openvic-simulation/map/ProvinceInstance.cpp b/src/openvic-simulation/map/ProvinceInstance.cpp
index 57a33a4..236efd2 100644
--- a/src/openvic-simulation/map/ProvinceInstance.cpp
+++ b/src/openvic-simulation/map/ProvinceInstance.cpp
@@ -255,7 +255,7 @@ fixed_point_t ProvinceInstance::get_modifier_effect_value(ModifierEffect const&
 		using enum ModifierEffect::target_t;
 
 		if (owner != nullptr) {
-			if ((effect.get_targets() & PROVINCE) == NO_TARGETS) {
+			if (ModifierEffect::excludes_targets(effect.get_targets(), PROVINCE)) {
 				// Non-province targeted effects are already added to the country modifier sum
 				return owner->get_modifier_effect_value(effect);
 			} else {
@@ -285,7 +285,7 @@ void ProvinceInstance::push_contributing_modifiers(
 		using enum ModifierEffect::target_t;
 
 		if (owner != nullptr) {
-			if ((effect.get_targets() & PROVINCE) == NO_TARGETS) {
+			if (ModifierEffect::excludes_targets(effect.get_targets(), PROVINCE)) {
 				// Non-province targeted effects are already added to the country modifier sum
 				owner->push_contributing_modifiers(effect, contributions);
 			} else {
diff --git a/src/openvic-simulation/modifier/ModifierEffect.cpp b/src/openvic-simulation/modifier/ModifierEffect.cpp
index 8daf563..2ffb9a5 100644
--- a/src/openvic-simulation/modifier/ModifierEffect.cpp
+++ b/src/openvic-simulation/modifier/ModifierEffect.cpp
@@ -4,6 +4,38 @@
 
 using namespace OpenVic;
 
+std::string ModifierEffect::target_to_string(target_t target) {
+	using enum target_t;
+
+	if (target == NO_TARGETS) {
+		return "NO TARGETS";
+	}
+
+	if (target == ALL_TARGETS) {
+		return "ALL TARGETS";
+	}
+
+	static constexpr std::string_view SEPARATOR = " | ";
+
+	std::string ret;
+
+	const auto append_target = [target, &ret](target_t check_target, std::string_view target_str) -> void {
+		if (!ModifierEffect::excludes_targets(target, check_target)) {
+			if (!ret.empty()) {
+				ret += SEPARATOR;
+			}
+			ret += target_str;
+		}
+	};
+
+	append_target(COUNTRY, "COUNTRY");
+	append_target(PROVINCE, "PROVINCE");
+	append_target(UNIT, "UNIT");
+	append_target(~ALL_TARGETS, "INVALID TARGET");
+
+	return ret;
+}
+
 std::string ModifierEffect::make_default_modifier_effect_localisation_key(std::string_view identifier) {
 	return "MODIFIER_" + StringUtils::string_toupper(identifier);
 }
diff --git a/src/openvic-simulation/modifier/ModifierEffect.hpp b/src/openvic-simulation/modifier/ModifierEffect.hpp
index 8eac65b..ff82acf 100644
--- a/src/openvic-simulation/modifier/ModifierEffect.hpp
+++ b/src/openvic-simulation/modifier/ModifierEffect.hpp
@@ -27,6 +27,10 @@ namespace OpenVic {
 			ALL_TARGETS = (1 << 3) - 1
 		};
 
+		static constexpr bool excludes_targets(target_t targets, target_t excluded_target);
+
+		static std::string target_to_string(target_t target);
+
 		static std::string make_default_modifier_effect_localisation_key(std::string_view identifier);
 
 	private:
@@ -50,4 +54,10 @@ namespace OpenVic {
 	};
 
 	template<> struct enable_bitfield<ModifierEffect::target_t> : std::true_type {};
+
+	constexpr bool ModifierEffect::excludes_targets(target_t targets, target_t excluded_targets) {
+		using enum target_t;
+
+		return (targets & excluded_targets) == NO_TARGETS;
+	}
 }
diff --git a/src/openvic-simulation/modifier/ModifierManager.cpp b/src/openvic-simulation/modifier/ModifierManager.cpp
index c9072c8..f21712d 100644
--- a/src/openvic-simulation/modifier/ModifierManager.cpp
+++ b/src/openvic-simulation/modifier/ModifierManager.cpp
@@ -7,11 +7,18 @@ bool ModifierManager::add_modifier_effect(
 	ModifierEffect const*& effect_cache, std::string_view identifier, bool positive_good, ModifierEffect::format_t format,
 	ModifierEffect::target_t targets, std::string_view localisation_key
 ) {
+	using enum ModifierEffect::target_t;
+
 	if (identifier.empty()) {
 		Logger::error("Invalid modifier effect identifier - empty!");
 		return false;
 	}
 
+	if (targets == NO_TARGETS) {
+		Logger::error("Invalid targets for modifier effect \"", identifier, "\" - none!");
+		return false;
+	}
+
 	if (effect_cache != nullptr) {
 		Logger::error(
 			"Cache variable for modifier effect \"", identifier, "\" is already filled with modifier effect \"",
diff --git a/src/openvic-simulation/modifier/ModifierSum.cpp b/src/openvic-simulation/modifier/ModifierSum.cpp
index a3b8cd5..676e872 100644
--- a/src/openvic-simulation/modifier/ModifierSum.cpp
+++ b/src/openvic-simulation/modifier/ModifierSum.cpp
@@ -2,8 +2,27 @@
 
 #include "openvic-simulation/modifier/Modifier.hpp"
 
+#include "openvic-simulation/country/CountryInstance.hpp"
+#include "openvic-simulation/map/ProvinceInstance.hpp"
+
 using namespace OpenVic;
 
+std::string_view ModifierSum::source_to_string(modifier_source_t const& source) {
+	return std::visit(
+		[](HasGetIdentifier auto const* has_identifier) -> std::string_view {
+			return has_identifier->get_identifier();
+		},
+		source
+	);
+}
+
+std::string ModifierSum::modifier_entry_t::to_string() const {
+	return StringUtils::append_string_views(
+		"[", modifier->get_identifier(), ", ", multiplier.to_string(), ", ", source_to_string(source), ", ",
+		ModifierEffect::target_to_string(excluded_targets), "]"
+	);
+}
+
 void ModifierSum::clear() {
 	modifiers.clear();
 	value_sum.clear();
@@ -26,21 +45,25 @@ bool ModifierSum::has_effect(ModifierEffect const& effect) const {
 }
 
 void ModifierSum::add_modifier(
-	Modifier const& modifier, modifier_source_t const& source, fixed_point_t multiplier, ModifierEffect::target_t targets
+	Modifier const& modifier, modifier_source_t const& source, fixed_point_t multiplier,
+	ModifierEffect::target_t excluded_targets
 ) {
 	using enum ModifierEffect::target_t;
 
-	if (multiplier != fixed_point_t::_0() && targets != NO_TARGETS) {
-		modifiers.emplace_back(&modifier, multiplier, source, targets);
-		value_sum.multiply_add_filter(modifier, multiplier, targets);
+	// We could test that excluded_targets != ALL_TARGETS, but in practice it's always
+	// called with an explcit/hardcoded value and so won't ever exclude everything.
+	if (multiplier != fixed_point_t::_0()) {
+		modifiers.emplace_back(&modifier, multiplier, source, excluded_targets);
+		value_sum.multiply_add_exclude_targets(modifier, multiplier, excluded_targets);
 	}
 }
 
 void ModifierSum::add_modifier_nullcheck(
-	Modifier const* modifier, modifier_source_t const& source, fixed_point_t multiplier, ModifierEffect::target_t targets
+	Modifier const* modifier, modifier_source_t const& source, fixed_point_t multiplier,
+	ModifierEffect::target_t excluded_targets
 ) {
 	if (modifier != nullptr) {
-		add_modifier(*modifier, source, multiplier, targets);
+		add_modifier(*modifier, source, multiplier, excluded_targets);
 	}
 }
 
@@ -49,22 +72,25 @@ void ModifierSum::add_modifier_sum(ModifierSum const& modifier_sum) {
 	value_sum += modifier_sum.value_sum;
 }
 
-void ModifierSum::add_modifier_sum_filter_targets(ModifierSum const& modifier_sum, ModifierEffect::target_t targets) {
-	using enum ModifierEffect::target_t;
-
+void ModifierSum::add_modifier_sum_exclude_targets(
+	ModifierSum const& modifier_sum, ModifierEffect::target_t excluded_targets
+) {
+	// We could test that excluded_targets != ALL_TARGETS, but in practice it's always
+	// called with an explcit/hardcoded value and so won't ever exclude everything.
 	for (modifier_entry_t const& modifier_entry : modifier_sum.modifiers) {
-		ModifierEffect::target_t new_targets = modifier_entry.targets & targets;
-
-		if (new_targets != NO_TARGETS) {
-			add_modifier(*modifier_entry.modifier, modifier_entry.source, modifier_entry.multiplier, new_targets);
-		}
+		add_modifier(
+			*modifier_entry.modifier, modifier_entry.source, modifier_entry.multiplier,
+			modifier_entry.excluded_targets | excluded_targets
+		);
 	}
 }
 
 void ModifierSum::add_modifier_sum_exclude_source(ModifierSum const& modifier_sum, modifier_source_t const& excluded_source) {
 	for (modifier_entry_t const& modifier_entry : modifier_sum.modifiers) {
 		if (modifier_entry.source != excluded_source) {
-			add_modifier(*modifier_entry.modifier, modifier_entry.source, modifier_entry.multiplier, modifier_entry.targets);
+			add_modifier(
+				*modifier_entry.modifier, modifier_entry.source, modifier_entry.multiplier, modifier_entry.excluded_targets
+			);
 		}
 	}
 }
@@ -77,7 +103,7 @@ void ModifierSum::push_contributing_modifiers(
 	using enum ModifierEffect::target_t;
 
 	for (modifier_entry_t const& modifier_entry : modifiers) {
-		if ((modifier_entry.targets & effect.get_targets()) != NO_TARGETS) {
+		if (ModifierEffect::excludes_targets(effect.get_targets(), modifier_entry.excluded_targets)) {
 			bool effect_found = false;
 			const fixed_point_t value = modifier_entry.modifier->get_effect(effect, &effect_found);
 
diff --git a/src/openvic-simulation/modifier/ModifierSum.hpp b/src/openvic-simulation/modifier/ModifierSum.hpp
index 7c1b238..858ca71 100644
--- a/src/openvic-simulation/modifier/ModifierSum.hpp
+++ b/src/openvic-simulation/modifier/ModifierSum.hpp
@@ -13,16 +13,32 @@ namespace OpenVic {
 	struct ModifierSum {
 		using modifier_source_t = std::variant<CountryInstance const*, ProvinceInstance const*>;
 
+		static std::string_view source_to_string(modifier_source_t const& source);
+
 		struct modifier_entry_t {
 			Modifier const* modifier;
 			fixed_point_t multiplier;
 			modifier_source_t source;
-			ModifierEffect::target_t targets;
+			ModifierEffect::target_t excluded_targets;
 
 			constexpr modifier_entry_t(
-				Modifier const* new_modifier, fixed_point_t new_multiplier, modifier_source_t const& new_source,
-				ModifierEffect::target_t new_targets
-			) : modifier { new_modifier }, multiplier { new_multiplier }, source { new_source }, targets { new_targets } {}
+				Modifier const* new_modifier,
+				fixed_point_t new_multiplier,
+				modifier_source_t const& new_source,
+				ModifierEffect::target_t new_excluded_targets
+			) : modifier { new_modifier },
+				multiplier { new_multiplier },
+				source { new_source },
+				excluded_targets { new_excluded_targets } {}
+
+			constexpr bool operator==(modifier_entry_t const& other) const {
+				return modifier == other.modifier
+					&& multiplier == other.multiplier
+					&& source_to_string(source) == source_to_string(other.source)
+					&& excluded_targets == other.excluded_targets;
+			}
+
+			std::string to_string() const;
 		};
 
 	private:
@@ -45,14 +61,14 @@ namespace OpenVic {
 
 		void add_modifier(
 			Modifier const& modifier, modifier_source_t const& source, fixed_point_t multiplier = fixed_point_t::_1(),
-			ModifierEffect::target_t targets = ModifierEffect::target_t::ALL_TARGETS
+			ModifierEffect::target_t excluded_targets = ModifierEffect::target_t::ALL_TARGETS
 		);
 		void add_modifier_nullcheck(
 			Modifier const* modifier, modifier_source_t const& source, fixed_point_t multiplier = fixed_point_t::_1(),
-			ModifierEffect::target_t targets = ModifierEffect::target_t::ALL_TARGETS
+			ModifierEffect::target_t excluded_targets = ModifierEffect::target_t::ALL_TARGETS
 		);
 		void add_modifier_sum(ModifierSum const& modifier_sum);
-		void add_modifier_sum_filter_targets(ModifierSum const& modifier_sum, ModifierEffect::target_t targets);
+		void add_modifier_sum_exclude_targets(ModifierSum const& modifier_sum, ModifierEffect::target_t excluded_targets);
 		void add_modifier_sum_exclude_source(ModifierSum const& modifier_sum, modifier_source_t const& excluded_source);
 
 		void push_contributing_modifiers(ModifierEffect const& effect, std::vector<modifier_entry_t>& contributions) const;
diff --git a/src/openvic-simulation/modifier/ModifierValue.cpp b/src/openvic-simulation/modifier/ModifierValue.cpp
index eec34bb..e3e7b0b 100644
--- a/src/openvic-simulation/modifier/ModifierValue.cpp
+++ b/src/openvic-simulation/modifier/ModifierValue.cpp
@@ -108,27 +108,31 @@ ModifierValue ModifierValue::operator*(fixed_point_t const& right) const {
 	return copy *= right;
 }
 
-void ModifierValue::apply_target_filter(ModifierEffect::target_t targets) {
+void ModifierValue::apply_exclude_targets(ModifierEffect::target_t excluded_targets) {
 	using enum ModifierEffect::target_t;
 
+	// We could test if excluded_targets is NO_TARGETS (and so we do nothing) or ALL_TARGETS (and so we clear everything),
+	// but so long as this is always called with an explicit/hardcoded value then we'll never have either of those cases.
 	erase_if(
 		values,
-		[targets](effect_map_t::value_type const& value) -> bool {
-			return (value.first->get_targets() & targets) == NO_TARGETS;
+		[excluded_targets](effect_map_t::value_type const& value) -> bool {
+			return !ModifierEffect::excludes_targets(value.first->get_targets(), excluded_targets);
 		}
 	);
 }
 
-void ModifierValue::multiply_add_filter(
-	ModifierValue const& other, fixed_point_t multiplier, ModifierEffect::target_t targets
+void ModifierValue::multiply_add_exclude_targets(
+	ModifierValue const& other, fixed_point_t multiplier, ModifierEffect::target_t excluded_targets
 ) {
 	using enum ModifierEffect::target_t;
 
-	if (multiplier == fixed_point_t::_1() && targets == ALL_TARGETS) {
+	if (multiplier == fixed_point_t::_1() && excluded_targets == NO_TARGETS) {
 		*this += other;
-	} else if (multiplier != fixed_point_t::_0() && targets != NO_TARGETS) {
+	} else if (multiplier != fixed_point_t::_0()) {
+		// We could test that excluded_targets != ALL_TARGETS, but in practice it's always
+		// called with an explcit/hardcoded value and so won't ever exclude everything.
 		for (effect_map_t::value_type const& value : other.values) {
-			if ((value.first->get_targets() & targets) != NO_TARGETS) {
+			if (ModifierEffect::excludes_targets(value.first->get_targets(), excluded_targets)) {
 				values[value.first] += value.second * multiplier;
 			}
 		}
diff --git a/src/openvic-simulation/modifier/ModifierValue.hpp b/src/openvic-simulation/modifier/ModifierValue.hpp
index 23c0ba5..6e6a29b 100644
--- a/src/openvic-simulation/modifier/ModifierValue.hpp
+++ b/src/openvic-simulation/modifier/ModifierValue.hpp
@@ -40,8 +40,10 @@ namespace OpenVic {
 		ModifierValue& operator*=(fixed_point_t const& right);
 		ModifierValue operator*(fixed_point_t const& right) const;
 
-		void apply_target_filter(ModifierEffect::target_t targets);
-		void multiply_add_filter(ModifierValue const& other, fixed_point_t multiplier, ModifierEffect::target_t targets);
+		void apply_exclude_targets(ModifierEffect::target_t excluded_targets);
+		void multiply_add_exclude_targets(
+			ModifierValue const& other, fixed_point_t multiplier, ModifierEffect::target_t excluded_targets
+		);
 
 		friend std::ostream& operator<<(std::ostream& stream, ModifierValue const& value);
 	};