Merge pull request #135 from OpenVicProject/misc-changes

Miscellaneous bug fixes and format cleanup

14 files changed, 78 insertions, 50 deletions

diff --git a/src/openvic-simulation/GameManager.cpp b/src/openvic-simulation/GameManager.cpp
index 6435f05..a4cb5f4 100644
--- a/src/openvic-simulation/GameManager.cpp
+++ b/src/openvic-simulation/GameManager.cpp
@@ -195,10 +195,14 @@ bool GameManager::load_hardcoded_defines() {
 				// TODO - explore non-linear scaling to have more variation among non-massive provinces
 				// TODO - when selecting a province, only show the population of provinces controlled (or owned?)
 				// by the same country, relative to the most populous province in that set of provinces
-				const colour_argb_t::value_type val = colour_argb_t::colour_traits::component_from_fraction(
-					province.get_total_population(), map.get_highest_province_population() + 1, 0.1f, 1.0f
-				);
-				return colour_argb_t { 0, val, 0, ALPHA_VALUE };
+				if (!province.is_water()) {
+					const colour_argb_t::value_type val = colour_argb_t::colour_traits::component_from_fraction(
+						province.get_total_population(), map.get_highest_province_population() + 1, 0.1f, 1.0f
+					);
+					return colour_argb_t { 0, val, 0, ALPHA_VALUE };
+				} else {
+					return colour_argb_t::null();
+				}
 			}
 		},
 		{
diff --git a/src/openvic-simulation/dataloader/Dataloader.cpp b/src/openvic-simulation/dataloader/Dataloader.cpp
index 6a2a3bb..cce766f 100644
--- a/src/openvic-simulation/dataloader/Dataloader.cpp
+++ b/src/openvic-simulation/dataloader/Dataloader.cpp
@@ -109,10 +109,7 @@ fs::path Dataloader::lookup_image_file(std::string_view path) const {
 	return lookup_file(path);
 }
 
-template<typename _DirIterator, typename _UniqueKey>
-requires requires (_UniqueKey const& unique_key, std::string_view path) {
-	{ unique_key(path) } -> std::convertible_to<std::string_view>;
-}
+template<typename _DirIterator, UniqueFileKey _UniqueKey>
 Dataloader::path_vector_t Dataloader::_lookup_files_in_dir(
 	std::string_view path, fs::path const& extension, _UniqueKey const& unique_key
 ) const {
@@ -129,7 +126,7 @@ Dataloader::path_vector_t Dataloader::_lookup_files_in_dir(
 		for (fs::directory_entry const& entry : _DirIterator { root / dirpath, ec }) {
 			if (entry.is_regular_file()) {
 				fs::path file = entry;
-				if ((extension.empty() || file.extension() == extension)) {
+				if (extension.empty() || file.extension() == extension) {
 					const std::string full_path = file.string();
 					std::string_view relative_path = full_path;
 					relative_path.remove_prefix(root_len);
@@ -142,7 +139,7 @@ Dataloader::path_vector_t Dataloader::_lookup_files_in_dir(
 							ret.emplace_back(std::move(file));
 						} else if (it->second.root == &root) {
 							Logger::warning(
-								"Files in the same directory with conflicting keys: ", it->first, " - ", it->second.file,
+								"Files under the same root with conflicting keys: ", it->first, " - ", it->second.file,
 								" (accepted) and ", key, " - ", file, " (rejected)"
 							);
 						}
diff --git a/src/openvic-simulation/dataloader/Dataloader.hpp b/src/openvic-simulation/dataloader/Dataloader.hpp
index de72fcd..31e0904 100644
--- a/src/openvic-simulation/dataloader/Dataloader.hpp
+++ b/src/openvic-simulation/dataloader/Dataloader.hpp
@@ -11,6 +11,11 @@ namespace OpenVic {
 	struct GameManager;
 	class UIManager;
 
+	template<typename _UniqueFileKey>
+	concept UniqueFileKey = requires(_UniqueFileKey const& unique_key, std::string_view path) {
+		requires std::same_as<std::remove_cvref_t<decltype(unique_key(path))>, std::string_view>;
+	};
+
 	class Dataloader {
 	public:
 		using path_vector_t = std::vector<fs::path>;
@@ -34,10 +39,7 @@ namespace OpenVic {
 		/* _DirIterator is fs::directory_iterator or fs::recursive_directory_iterator. _UniqueKey is the type of a callable
 		 * which converts a string_view filepath with root removed into a string_view unique key. Any path whose key is empty
 		 * or matches an earlier found path's key is discarded, ensuring each looked up path's key is non-empty and unique. */
-		template<typename _DirIterator, typename _UniqueKey>
-		requires requires (_UniqueKey const& unique_key, std::string_view path) {
-			{ unique_key(path) } -> std::convertible_to<std::string_view>;
-		}
+		template<typename _DirIterator, UniqueFileKey _UniqueKey>
 		path_vector_t _lookup_files_in_dir(
 			std::string_view path, fs::path const& extension, _UniqueKey const& unique_key
 		) const;
diff --git a/src/openvic-simulation/dataloader/NodeTools.hpp b/src/openvic-simulation/dataloader/NodeTools.hpp
index 4b33c6d..0845e6c 100644
--- a/src/openvic-simulation/dataloader/NodeTools.hpp
+++ b/src/openvic-simulation/dataloader/NodeTools.hpp
@@ -357,17 +357,17 @@ namespace OpenVic {
 		}
 
 		template<typename... Args>
-		bool warn_or_error(bool warn, Args... args) {
+		bool warn_or_error(bool warn, Args&&... args) {
 			if (warn) {
-				Logger::warning(args...);
+				Logger::warning(std::forward<Args>(args)...);
 				return true;
 			} else {
-				Logger::error(args...);
+				Logger::error(std::forward<Args>(args)...);
 				return false;
 			}
 		}
 
-		template<typename T, typename U, typename...SetArgs>
+		template<typename T, typename U, typename... SetArgs>
 		Callback<T> auto set_callback(tsl::ordered_set<U, SetArgs...>& set, bool warn = false) {
 			return [&set, warn](T val) -> bool {
 				if (set.emplace(std::move(val)).second) {
@@ -377,7 +377,7 @@ namespace OpenVic {
 			};
 		}
 
-		template<std::derived_from<HasIdentifier> T, typename...SetArgs>
+		template<std::derived_from<HasIdentifier> T, typename... SetArgs>
 		Callback<T const&> auto set_callback_pointer(tsl::ordered_set<T const*, SetArgs...>& set, bool warn = false) {
 			return [&set, warn](T const& val) -> bool {
 				if (set.emplace(&val).second) {
diff --git a/src/openvic-simulation/military/Deployment.cpp b/src/openvic-simulation/military/Deployment.cpp
index d4a092b..c06ed95 100644
--- a/src/openvic-simulation/military/Deployment.cpp
+++ b/src/openvic-simulation/military/Deployment.cpp
@@ -82,22 +82,26 @@ bool DeploymentManager::load_oob_file(
 				"name", ONE_EXACTLY, expect_identifier_or_string(assign_variable_callback(leader_name)),
 				"date", ONE_EXACTLY, expect_identifier_or_string(expect_date_str(assign_variable_callback(leader_date))),
 				"type", ONE_EXACTLY, expect_identifier(UnitManager::expect_type_str(assign_variable_callback(leader_type))),
-				"personality", ONE_EXACTLY, game_manager.get_military_manager().get_leader_trait_manager()
-					.expect_leader_trait_identifier(assign_variable_callback_pointer(leader_personality)),
-				"background", ONE_EXACTLY, game_manager.get_military_manager().get_leader_trait_manager()
-					.expect_leader_trait_identifier(assign_variable_callback_pointer(leader_background)),
+				"personality", ONE_EXACTLY,
+					game_manager.get_military_manager().get_leader_trait_manager().expect_leader_trait_identifier_or_string(
+						assign_variable_callback_pointer(leader_personality)
+					),
+				"background", ONE_EXACTLY,
+					game_manager.get_military_manager().get_leader_trait_manager().expect_leader_trait_identifier_or_string(
+						assign_variable_callback_pointer(leader_background)
+					),
 				"prestige", ZERO_OR_ONE, expect_fixed_point(assign_variable_callback(leader_prestige)),
 				"picture", ZERO_OR_ONE, expect_identifier_or_string(assign_variable_callback(picture))
 			)(node);
 
-			if (!leader_personality->is_personality_trait()) {
+			if (leader_personality != nullptr && !leader_personality->is_personality_trait()) {
 				Logger::error(
 					"Leader ", leader_name, " has personality ", leader_personality->get_identifier(),
 					" which is not a personality trait!"
 				);
 				ret = false;
 			}
-			if (!leader_background->is_background_trait()) {
+			if (leader_background != nullptr && !leader_background->is_background_trait()) {
 				Logger::error(
 					"Leader ", leader_name, " has background ", leader_background->get_identifier(),
 					" which is not a background trait!"
diff --git a/src/openvic-simulation/misc/Event.cpp b/src/openvic-simulation/misc/Event.cpp
index 1796799..34743e8 100644
--- a/src/openvic-simulation/misc/Event.cpp
+++ b/src/openvic-simulation/misc/Event.cpp
@@ -213,7 +213,7 @@ bool EventManager::load_on_action_file(ast::NodeCPtr root) {
 					ret &= map_callback(weighted_events, event)(weight);
 				} else {
 					Logger::warning(
-						"Non-existing event ", event->get_identifier(), " loaded on action ", identifier, "with weight",
+						"Non-existing event ", event_node, " loaded on action ", identifier, " with weight ",
 						weight, "!"
 					);
 				}
diff --git a/src/openvic-simulation/misc/Modifier.cpp b/src/openvic-simulation/misc/Modifier.cpp
index e73d0e3..1fdc26e 100644
--- a/src/openvic-simulation/misc/Modifier.cpp
+++ b/src/openvic-simulation/misc/Modifier.cpp
@@ -105,7 +105,7 @@ bool ModifierManager::setup_modifier_effects() {
 	bool ret = true;
 
 	using enum ModifierEffect::format_t;
-	/* Tech/inventions only*/
+	/* Tech/inventions only */
 	ret &= add_modifier_effect("cb_creation_speed", true);
 	ret &= add_modifier_effect("combat_width", false);
 	ret &= add_modifier_effect("plurality", true, PERCENTAGE_DECIMAL);
@@ -214,7 +214,7 @@ bool ModifierManager::setup_modifier_effects() {
 	ret &= add_modifier_effect("unit_recruitment_time", false);
 	ret &= add_modifier_effect("war_exhaustion", false);
 
-	/* State only*/
+	/* State only */
 	ret &= add_modifier_effect("flashpoint_tension", true);
 	ret &= add_modifier_effect("railroads", true); // capitalist likelihood for railroads vs factories
 
@@ -371,9 +371,9 @@ key_value_callback_t ModifierManager::_modifier_effect_callback(
 	) -> bool {
 		if (effect_validator(*effect)) {
 			static const case_insensitive_string_set_t no_effect_modifiers {
-				"boost_strongest_party", "poor_savings_modifier",	"local_artisan_input",	  "local_artisan_throughput",
-				"local_artisan_output",	 "artisan_input",			"artisan_throughput",	  "artisan_output",
-				"import_cost",			 "unciv_economic_modifier", "unciv_military_modifier"
+				"boost_strongest_party", "poor_savings_modifier",   "local_artisan_input",     "local_artisan_throughput",
+				"local_artisan_output",  "artisan_input",           "artisan_throughput",      "artisan_output",
+				"import_cost",           "unciv_economic_modifier", "unciv_military_modifier"
 			};
 			if (no_effect_modifiers.contains(effect->get_identifier())) {
 				Logger::warning("This modifier does nothing: ", effect->get_identifier());
diff --git a/src/openvic-simulation/politics/Government.cpp b/src/openvic-simulation/politics/Government.cpp
index fc93cc3..49e0f2f 100644
--- a/src/openvic-simulation/politics/Government.cpp
+++ b/src/openvic-simulation/politics/Government.cpp
@@ -66,7 +66,8 @@ bool GovernmentTypeManager::load_government_types_file(IdeologyManager const& id
 
 			ret &= expect_dictionary(
 				[this, &ideology_manager, &ideologies, government_type_identifier](
-					std::string_view key, ast::NodeCPtr value) -> bool {
+					std::string_view key, ast::NodeCPtr value
+				) -> bool {
 					static const string_set_t reserved_keys = { "election", "duration", "appoint_ruling_party", "flagType" };
 					if (reserved_keys.contains(key)) {
 						return true;
diff --git a/src/openvic-simulation/pop/Pop.hpp b/src/openvic-simulation/pop/Pop.hpp
index 5a4cebf..4e173bb 100644
--- a/src/openvic-simulation/pop/Pop.hpp
+++ b/src/openvic-simulation/pop/Pop.hpp
@@ -184,7 +184,7 @@ namespace OpenVic {
 	template<> struct enable_bitfield<PopType::income_type_t> : std::true_type {};
 
 	/* This returns true if at least one income type is shared by both arguments. */
-	constexpr inline bool share_income_type(PopType::income_type_t lhs, PopType::income_type_t rhs) {
+	inline constexpr bool share_income_type(PopType::income_type_t lhs, PopType::income_type_t rhs) {
 		return (lhs & rhs) != PopType::income_type_t::NO_INCOME_TYPE;
 	}
 
diff --git a/src/openvic-simulation/scripts/Condition.hpp b/src/openvic-simulation/scripts/Condition.hpp
index e239142..5aa45e8 100644
--- a/src/openvic-simulation/scripts/Condition.hpp
+++ b/src/openvic-simulation/scripts/Condition.hpp
@@ -77,13 +77,13 @@ namespace OpenVic {
 	template<> struct enable_bitfield<identifier_type_t> : std::true_type {};
 
 	/* Returns true if the values have any bit in common. */
-	constexpr inline bool share_value_type(value_type_t lhs, value_type_t rhs) {
+	inline constexpr bool share_value_type(value_type_t lhs, value_type_t rhs) {
 		return (lhs & rhs) != value_type_t::NO_TYPE;
 	}
-	constexpr inline bool share_scope(scope_t lhs, scope_t rhs) {
+	inline constexpr bool share_scope(scope_t lhs, scope_t rhs) {
 		return (lhs & rhs) != scope_t::NO_SCOPE;
 	}
-	constexpr inline bool share_identifier_type(identifier_type_t lhs, identifier_type_t rhs) {
+	inline constexpr bool share_identifier_type(identifier_type_t lhs, identifier_type_t rhs) {
 		return (lhs & rhs) != identifier_type_t::NO_IDENTIFIER;
 	}
 
diff --git a/src/openvic-simulation/types/EnumBitfield.hpp b/src/openvic-simulation/types/EnumBitfield.hpp
index 2b0a1cf..8753449 100644
--- a/src/openvic-simulation/types/EnumBitfield.hpp
+++ b/src/openvic-simulation/types/EnumBitfield.hpp
@@ -27,58 +27,58 @@ namespace OpenVic {
 }
 
 template<OpenVic::EnumSupportBitfield T>
-[[nodiscard]] constexpr inline auto operator|(const T lhs, const T rhs) noexcept {
+[[nodiscard]] inline constexpr auto operator|(const T lhs, const T rhs) noexcept {
 	using underlying_type = std::underlying_type_t<T>;
 	return static_cast<T>(static_cast<underlying_type>(lhs) | static_cast<underlying_type>(rhs));
 }
 
 template<OpenVic::EnumSupportBitfield T>
-[[nodiscard]] constexpr inline auto operator&(const T lhs, const T rhs) noexcept {
+[[nodiscard]] inline constexpr auto operator&(const T lhs, const T rhs) noexcept {
 	using underlying_type = std::underlying_type_t<T>;
 	return static_cast<T>(static_cast<underlying_type>(lhs) & static_cast<underlying_type>(rhs));
 }
 
 template<OpenVic::EnumSupportBitfield T>
-[[nodiscard]] constexpr inline auto operator^(const T lhs, const T rhs) noexcept {
+[[nodiscard]] inline constexpr auto operator^(const T lhs, const T rhs) noexcept {
 	using underlying_type = std::underlying_type_t<T>;
 	return static_cast<T>(static_cast<underlying_type>(lhs) ^ static_cast<underlying_type>(rhs));
 }
 
 template<OpenVic::EnumSupportBitfield T>
-[[nodiscard]] constexpr inline auto operator~(const T lhs) noexcept {
+[[nodiscard]] inline constexpr auto operator~(const T lhs) noexcept {
 	using underlying_type = std::underlying_type_t<T>;
 	return static_cast<T>(~static_cast<underlying_type>(lhs));
 }
 
 template<OpenVic::EnumSupportBitfield T>
-constexpr inline decltype(auto) operator|=(T& lhs, const T rhs) noexcept {
+inline constexpr decltype(auto) operator|=(T& lhs, const T rhs) noexcept {
 	using underlying_type = std::underlying_type_t<T>;
 	lhs = static_cast<T>(static_cast<underlying_type>(lhs) | static_cast<underlying_type>(rhs));
 	return lhs;
 }
 
 template<OpenVic::EnumSupportBitfield T>
-constexpr inline decltype(auto) operator&=(T& lhs, const T rhs) noexcept {
+inline constexpr decltype(auto) operator&=(T& lhs, const T rhs) noexcept {
 	using underlying_type = std::underlying_type_t<T>;
 	lhs = static_cast<T>(static_cast<underlying_type>(lhs) & static_cast<underlying_type>(rhs));
 	return lhs;
 }
 
 template<OpenVic::EnumSupportBitfield T>
-constexpr inline decltype(auto) operator^=(T& lhs, const T rhs) noexcept {
+inline constexpr decltype(auto) operator^=(T& lhs, const T rhs) noexcept {
 	using underlying_type = std::underlying_type_t<T>;
 	lhs = static_cast<T>(static_cast<underlying_type>(lhs) ^ static_cast<underlying_type>(rhs));
 	return lhs;
 }
 
 template<OpenVic::EnumSupportBitfield T>
-[[nodiscard]] constexpr inline bool operator<<(const T lhs, const T rhs) noexcept {
+[[nodiscard]] inline constexpr bool operator<<(const T lhs, const T rhs) noexcept {
 	using underlying_type = std::underlying_type_t<T>;
 	return (lhs & rhs) == rhs;
 }
 
 template<OpenVic::EnumSupportBitfield T>
-[[nodiscard]] constexpr inline bool operator>>(const T lhs, const T rhs) noexcept {
+[[nodiscard]] inline constexpr bool operator>>(const T lhs, const T rhs) noexcept {
 	using underlying_type = std::underlying_type_t<T>;
 	return (lhs & rhs) == lhs;
 }
diff --git a/src/openvic-simulation/types/IdentifierRegistry.hpp b/src/openvic-simulation/types/IdentifierRegistry.hpp
index 251632b..68013d5 100644
--- a/src/openvic-simulation/types/IdentifierRegistry.hpp
+++ b/src/openvic-simulation/types/IdentifierRegistry.hpp
@@ -258,7 +258,7 @@ namespace OpenVic {
 			if (item != nullptr) { \
 				return callback(*item); \
 			} \
-			return NodeTools::warn_or_error(warn, "Invalid ", name, ": ", identifier); \
+			return NodeTools::warn_or_error(warn, "Invalid ", name, " identifier: ", identifier); \
 		}; \
 	} \
 	constexpr NodeTools::NodeCallback auto expect_item_identifier( \
@@ -266,6 +266,16 @@ namespace OpenVic {
 	) CONST { \
 		return NodeTools::expect_identifier(expect_item_str(callback, warn)); \
 	} \
+	constexpr NodeTools::NodeCallback auto expect_item_string( \
+		NodeTools::Callback<value_type CONST&> auto callback, bool warn \
+	) CONST { \
+		return NodeTools::expect_string(expect_item_str(callback, warn)); \
+	} \
+	constexpr NodeTools::NodeCallback auto expect_item_identifier_or_string( \
+		NodeTools::Callback<value_type CONST&> auto callback, bool warn \
+	) CONST { \
+		return NodeTools::expect_identifier_or_string(expect_item_str(callback, warn)); \
+	} \
 	constexpr NodeTools::NodeCallback auto expect_item_assign_and_default( \
 		NodeTools::KeyValueCallback auto default_callback, \
 		NodeTools::Callback<value_type CONST&, ast::NodeCPtr> auto callback \
@@ -506,6 +516,16 @@ private:
 	) const_kw { \
 		return registry.expect_item_identifier(callback, warn); \
 	} \
+	constexpr NodeTools::NodeCallback auto expect_##singular##_string( \
+		NodeTools::Callback<decltype(registry)::value_type const_kw&> auto callback, bool warn = false \
+	) const_kw { \
+		return registry.expect_item_string(callback, warn); \
+	} \
+	constexpr NodeTools::NodeCallback auto expect_##singular##_identifier_or_string( \
+		NodeTools::Callback<decltype(registry)::value_type const_kw&> auto callback, bool warn = false \
+	) const_kw { \
+		return registry.expect_item_identifier_or_string(callback, warn); \
+	} \
 	constexpr NodeTools::NodeCallback auto expect_##singular##_assign_and_default( \
 		NodeTools::KeyValueCallback auto default_callback, \
 		NodeTools::Callback<decltype(registry)::value_type const_kw&, ast::NodeCPtr> auto callback \
diff --git a/src/openvic-simulation/utility/StringUtils.hpp b/src/openvic-simulation/utility/StringUtils.hpp
index ee6d2f9..0df2952 100644
--- a/src/openvic-simulation/utility/StringUtils.hpp
+++ b/src/openvic-simulation/utility/StringUtils.hpp
@@ -198,7 +198,7 @@ namespace OpenVic::StringUtils {
 	}
 
 	template<typename... Args>
-	requires (std::is_same_v<std::string_view, Args> && ...)
+	requires(std::is_same_v<std::string_view, Args> && ...)
 	inline std::string _append_string_views(Args... args) {
 		std::string ret;
 		ret.reserve((args.size() + ...));
@@ -207,7 +207,7 @@ namespace OpenVic::StringUtils {
 	}
 
 	template<typename... Args>
-	requires (std::is_convertible_v<Args, std::string_view> && ...)
+	requires(std::is_convertible_v<Args, std::string_view> && ...)
 	inline std::string append_string_views(Args... args) {
 		return _append_string_views(std::string_view { args }...);
 	}
diff --git a/src/openvic-simulation/utility/Utility.hpp b/src/openvic-simulation/utility/Utility.hpp
index dbbcf8f..9951532 100644
--- a/src/openvic-simulation/utility/Utility.hpp
+++ b/src/openvic-simulation/utility/Utility.hpp
@@ -32,13 +32,13 @@ namespace OpenVic::utility {
 	}
 
 	template<class T>
-	constexpr inline void hash_combine(std::size_t& s, const T& v) {
+	inline constexpr void hash_combine(std::size_t& s, const T& v) {
 		std::hash<T> h;
 		s ^= h(v) + 0x9e3779b9 + (s << 6) + (s >> 2);
 	}
 
 	template<size_t Shift, class T>
-	constexpr inline void hash_combine_index(std::size_t& s, const T& v) {
+	inline constexpr void hash_combine_index(std::size_t& s, const T& v) {
 		std::hash<T> h;
 		if constexpr (Shift == 0) {
 			s = h(v);