PR feedback + Logger::warning

11 files changed, 95 insertions, 86 deletions

diff --git a/src/openvic-simulation/GameManager.cpp b/src/openvic-simulation/GameManager.cpp
index ee5050d..0b42230 100644
--- a/src/openvic-simulation/GameManager.cpp
+++ b/src/openvic-simulation/GameManager.cpp
@@ -13,12 +13,11 @@ void GameManager::set_needs_update() {
 }
 
 void GameManager::update_state() {
-	if (needs_update) {
-		Logger::info("Update: ", today);
-		map.update_state(today);
-		if (state_updated) state_updated();
-		needs_update = false;
-	}
+	if (!needs_update) return;
+	Logger::info("Update: ", today);
+	map.update_state(today);
+	if (state_updated) state_updated();
+	needs_update = false;
 }
 
 /* REQUIREMENTS:
diff --git a/src/openvic-simulation/dataloader/Dataloader.cpp b/src/openvic-simulation/dataloader/Dataloader.cpp
index 334d5b8..25641e2 100644
--- a/src/openvic-simulation/dataloader/Dataloader.cpp
+++ b/src/openvic-simulation/dataloader/Dataloader.cpp
@@ -11,15 +11,15 @@ using namespace OpenVic;
 using namespace OpenVic::NodeTools;
 using namespace ovdl;
 
-bool Dataloader::set_roots(std::vector<std::filesystem::path> new_roots) {
+bool Dataloader::set_roots(path_vector_t new_roots) {
 	if (!roots.empty()) {
 		Logger::error("Overriding existing dataloader roots!");
 		roots.clear();
 	}
 	bool ret = true;
-	for (std::reverse_iterator<std::vector<std::filesystem::path>::const_iterator> it = new_roots.crbegin(); it != new_roots.crend(); ++it) {
+	for (std::reverse_iterator<path_vector_t::const_iterator> it = new_roots.crbegin(); it != new_roots.crend(); ++it) {
 		if (std::find(roots.begin(), roots.end(), *it) == roots.end()) {
-			if (std::filesystem::is_directory(*it)) {
+			if (fs::is_directory(*it)) {
 				Logger::info("Adding dataloader root: ", *it);
 				roots.push_back(*it);
 			} else {
@@ -38,10 +38,10 @@ bool Dataloader::set_roots(std::vector<std::filesystem::path> new_roots) {
 	return ret;
 }
 
-std::filesystem::path Dataloader::lookup_file(std::filesystem::path const& path) const {
-	for (std::filesystem::path const& root : roots) {
-		const std::filesystem::path composed = root / path;
-		if (std::filesystem::is_regular_file(composed)) {
+fs::path Dataloader::lookup_file(fs::path const& path) const {
+	for (fs::path const& root : roots) {
+		const fs::path composed = root / path;
+		if (fs::is_regular_file(composed)) {
 			return composed;
 		}
 	}
@@ -49,27 +49,27 @@ std::filesystem::path Dataloader::lookup_file(std::filesystem::path const& path)
 	return {};
 }
 
-const std::filesystem::path Dataloader::TXT = ".txt";
+const fs::path Dataloader::TXT = ".txt";
 
-static bool contains_file_with_name(std::vector<std::filesystem::path> const& paths,
-	std::filesystem::path const& name) {
+static bool contains_file_with_name(Dataloader::path_vector_t const& paths,
+	fs::path const& name) {
 
-	for (std::filesystem::path const& path : paths) {
+	for (fs::path const& path : paths) {
 		if (path.filename() == name) return true;
 	}
 	return false;
 }
 
-std::vector<std::filesystem::path> Dataloader::lookup_files_in_dir(std::filesystem::path const& path,
-	std::filesystem::path const* extension) const {
+Dataloader::path_vector_t Dataloader::lookup_files_in_dir(fs::path const& path,
+	fs::path const* extension) const {
 
-	std::vector<std::filesystem::path> ret;
-	for (std::filesystem::path const& root : roots) {
-		const std::filesystem::path composed = root / path;
+	path_vector_t ret;
+	for (fs::path const& root : roots) {
+		const fs::path composed = root / path;
 		std::error_code ec;
-		for (std::filesystem::directory_entry const& entry : std::filesystem::directory_iterator { composed, ec }) {
+		for (fs::directory_entry const& entry : fs::directory_iterator { composed, ec }) {
 			if (entry.is_regular_file()) {
-				const std::filesystem::path file = entry;
+				const fs::path file = entry;
 				if (extension == nullptr || file.extension() == *extension) {
 					if (!contains_file_with_name(ret, file.filename())) {
 						ret.push_back(file);
@@ -81,19 +81,19 @@ std::vector<std::filesystem::path> Dataloader::lookup_files_in_dir(std::filesyst
 	return ret;
 }
 
-bool Dataloader::apply_to_files_in_dir(std::filesystem::path const& path,
-	std::function<bool(std::filesystem::path const&)> callback,
-	std::filesystem::path const* extension) const {
+bool Dataloader::apply_to_files_in_dir(fs::path const& path,
+	std::function<bool(fs::path const&)> callback,
+	fs::path const* extension) const {
 
 	bool ret = true;
-	for (std::filesystem::path const& file : lookup_files_in_dir(path, extension)) {
+	for (fs::path const& file : lookup_files_in_dir(path, extension)) {
 		ret &= callback(file);
 	}
 	return ret;
 }
 
 template<std::derived_from<detail::BasicParser> Parser, bool(Parser::*parse_func)()>
-static Parser _run_ovdl_parser(std::filesystem::path const& path) {
+static Parser _run_ovdl_parser(fs::path const& path) {
 	Parser parser;
 	std::string buffer;
 	auto error_log_stream = ovdl::detail::CallbackStream {
@@ -130,18 +130,18 @@ static Parser _run_ovdl_parser(std::filesystem::path const& path) {
 	return parser;
 }
 
-static v2script::Parser _parse_defines(std::filesystem::path const& path) {
+static v2script::Parser _parse_defines(fs::path const& path) {
 	return _run_ovdl_parser<v2script::Parser, &v2script::Parser::simple_parse>(path);
 }
 
-static csv::Windows1252Parser _parse_csv(std::filesystem::path const& path) {
+static csv::Windows1252Parser _parse_csv(fs::path const& path) {
 	return _run_ovdl_parser<csv::Windows1252Parser, &csv::Windows1252Parser::parse_csv>(path);
 }
 
-bool Dataloader::_load_pop_types(PopManager& pop_manager, std::filesystem::path const& pop_type_directory) const {
+bool Dataloader::_load_pop_types(PopManager& pop_manager, fs::path const& pop_type_directory) const {
 	const bool ret = apply_to_files_in_dir(pop_type_directory,
-		[&pop_manager](std::filesystem::path const& file) -> bool {
-			return pop_manager.load_pop_type_file(file, _parse_defines(file).get_file_node());
+		[&pop_manager](fs::path const& file) -> bool {
+			return pop_manager.load_pop_type_file(file.stem().string(), _parse_defines(file).get_file_node());
 		}
 	);
 	if (!ret) {
@@ -151,8 +151,8 @@ bool Dataloader::_load_pop_types(PopManager& pop_manager, std::filesystem::path
 	return ret;
 }
 
-bool Dataloader::_load_map_dir(Map& map, std::filesystem::path const& map_directory) const {
-	static const std::filesystem::path defaults_filename = "default.map";
+bool Dataloader::_load_map_dir(Map& map, fs::path const& map_directory) const {
+	static const fs::path defaults_filename = "default.map";
 	static const std::string default_definitions = "definition.csv";
 	static const std::string default_provinces = "provinces.bmp";
 	static const std::string default_positions = "positions.txt";
@@ -233,12 +233,12 @@ bool Dataloader::_load_map_dir(Map& map, std::filesystem::path const& map_direct
 }
 
 bool Dataloader::load_defines(GameManager& game_manager) const {
-	static const std::filesystem::path good_file = "common/goods.txt";
-	static const std::filesystem::path pop_type_directory = "poptypes";
-	static const std::filesystem::path graphical_culture_type_file = "common/graphicalculturetype.txt";
-	static const std::filesystem::path culture_file = "common/cultures.txt";
-	static const std::filesystem::path religion_file = "common/religion.txt";
-	static const std::filesystem::path map_directory = "map";
+	static const fs::path good_file = "common/goods.txt";
+	static const fs::path pop_type_directory = "poptypes";
+	static const fs::path graphical_culture_type_file = "common/graphicalculturetype.txt";
+	static const fs::path culture_file = "common/cultures.txt";
+	static const fs::path religion_file = "common/religion.txt";
+	static const fs::path map_directory = "map";
 
 	bool ret = true;
 
@@ -270,9 +270,9 @@ bool Dataloader::load_defines(GameManager& game_manager) const {
 	return ret;
 }
 
-bool Dataloader::load_pop_history(GameManager& game_manager, std::filesystem::path const& path) const {
+bool Dataloader::load_pop_history(GameManager& game_manager, fs::path const& path) const {
 	return apply_to_files_in_dir(path,
-		[&game_manager](std::filesystem::path const& file) -> bool {
+		[&game_manager](fs::path const& file) -> bool {
 			return expect_dictionary(
 				[&game_manager](std::string_view province_key, ast::NodeCPtr province_node) -> bool {
 					Province* province = game_manager.map.get_province_by_identifier(province_key);
diff --git a/src/openvic-simulation/dataloader/Dataloader.hpp b/src/openvic-simulation/dataloader/Dataloader.hpp
index f723803..20538a6 100644
--- a/src/openvic-simulation/dataloader/Dataloader.hpp
+++ b/src/openvic-simulation/dataloader/Dataloader.hpp
@@ -5,31 +5,36 @@
 #include <vector>
 
 namespace OpenVic {
+	namespace fs = std::filesystem;
+
 	struct GameManager;
 	struct PopManager;
 	struct Map;
 
 	class Dataloader {
-		std::vector<std::filesystem::path> roots;
+	public:
+		using path_vector_t = std::vector<fs::path>;
+	private:
+		path_vector_t roots;
 
-		bool _load_pop_types(PopManager& pop_manager, std::filesystem::path const& pop_type_directory) const;
-		bool _load_map_dir(Map& map, std::filesystem::path const& map_directory) const;
+		bool _load_pop_types(PopManager& pop_manager, fs::path const& pop_type_directory) const;
+		bool _load_map_dir(Map& map, fs::path const& map_directory) const;
 
 	public:
 		Dataloader() = default;
 
 		/* In reverse-load order, so base defines first and final loaded mod last */
-		bool set_roots(std::vector<std::filesystem::path> new_roots);
+		bool set_roots(path_vector_t new_roots);
 
-		std::filesystem::path lookup_file(std::filesystem::path const& path) const;
-		static const std::filesystem::path TXT;
-		std::vector<std::filesystem::path> lookup_files_in_dir(std::filesystem::path const& path,
-			std::filesystem::path const* extension = &TXT) const;
-		bool apply_to_files_in_dir(std::filesystem::path const& path,
-			std::function<bool(std::filesystem::path const&)> callback,
-			std::filesystem::path const* extension = &TXT) const;
+		fs::path lookup_file(fs::path const& path) const;
+		static const fs::path TXT;
+		path_vector_t lookup_files_in_dir(fs::path const& path,
+			fs::path const* extension = &TXT) const;
+		bool apply_to_files_in_dir(fs::path const& path,
+			std::function<bool(fs::path const&)> callback,
+			fs::path const* extension = &TXT) const;
 
 		bool load_defines(GameManager& game_manager) const;
-		bool load_pop_history(GameManager& game_manager, std::filesystem::path const& path) const;
+		bool load_pop_history(GameManager& game_manager, fs::path const& path) const;
 	};
 }
diff --git a/src/openvic-simulation/map/Map.cpp b/src/openvic-simulation/map/Map.cpp
index 3f86ccc..8f10410 100644
--- a/src/openvic-simulation/map/Map.cpp
+++ b/src/openvic-simulation/map/Map.cpp
@@ -68,8 +68,8 @@ bool Map::set_water_province(const std::string_view identifier) {
 		return false;
 	}
 	if (province->is_water()) {
-		Logger::error("Province ", identifier, " is already a water province!");
-		return false;
+		Logger::warning("Province ", identifier, " is already a water province!");
+		return true;
 	}
 	if (!water_provinces.add_province(province)) {
 		Logger::error("Failed to add province ", identifier, " to water province set!");
@@ -278,7 +278,7 @@ bool Map::generate_province_shape_image(size_t new_width, size_t new_height, uin
 				if (unrecognised_terrain_colours.find(terrain_colour) == unrecognised_terrain_colours.end()) {
 					unrecognised_terrain_colours.insert(terrain_colour);
 					if (detailed_errors) {
-						Logger::error("Unrecognised terrain colour ", colour_to_hex_string(terrain_colour),
+						Logger::warning("Unrecognised terrain colour ", colour_to_hex_string(terrain_colour),
 							" at (", x, ", ", y, ")");
 					}
 				}
@@ -309,7 +309,7 @@ bool Map::generate_province_shape_image(size_t new_width, size_t new_height, uin
 			if (unrecognised_province_colours.find(province_colour) == unrecognised_province_colours.end()) {
 				unrecognised_province_colours.insert(province_colour);
 				if (detailed_errors) {
-					Logger::error("Unrecognised province colour ", colour_to_hex_string(province_colour),
+					Logger::warning("Unrecognised province colour ", colour_to_hex_string(province_colour),
 						" at (", x, ", ", y, ")");
 				}
 			}
@@ -317,12 +317,10 @@ bool Map::generate_province_shape_image(size_t new_width, size_t new_height, uin
 		}
 	}
 	if (!unrecognised_province_colours.empty()) {
-		Logger::error("Province image contains ", unrecognised_province_colours.size(), " unrecognised province colours");
-		ret = false;
+		Logger::warning("Province image contains ", unrecognised_province_colours.size(), " unrecognised province colours");
 	}
 	if (!unrecognised_terrain_colours.empty()) {
-		Logger::error("Terrain image contains ", unrecognised_terrain_colours.size(), " unrecognised terrain colours");
-		ret = false;
+		Logger::warning("Terrain image contains ", unrecognised_terrain_colours.size(), " unrecognised terrain colours");
 	}
 
 	size_t missing = 0;
diff --git a/src/openvic-simulation/map/Province.cpp b/src/openvic-simulation/map/Province.cpp
index 43eddd1..8d4abcb 100644
--- a/src/openvic-simulation/map/Province.cpp
+++ b/src/openvic-simulation/map/Province.cpp
@@ -55,10 +55,10 @@ std::string Province::to_string() const {
 }
 
 bool Province::load_pop_list(PopManager const& pop_manager, ast::NodeCPtr root) {
-	return expect_list_reserve_length(
+	return expect_dictionary_reserve_length(
 		pops,
-		[this, &pop_manager](ast::NodeCPtr pop_node) -> bool {
-			return pop_manager.load_pop_into_province(*this, pop_node);
+		[this, &pop_manager](std::string_view pop_type_identifier, ast::NodeCPtr pop_node) -> bool {
+			return pop_manager.load_pop_into_province(*this, pop_type_identifier, pop_node);
 		}
 	)(root);
 }
diff --git a/src/openvic-simulation/pop/Pop.cpp b/src/openvic-simulation/pop/Pop.cpp
index 7b4dd60..f8e7254 100644
--- a/src/openvic-simulation/pop/Pop.cpp
+++ b/src/openvic-simulation/pop/Pop.cpp
@@ -126,7 +126,7 @@ bool PopManager::add_pop_type(const std::string_view identifier, colour_t colour
 	return pop_types.add_item({ identifier, colour, strata, sprite, max_size, merge_max_size, state_capital_only, demote_migrant, is_artisan, is_slave });
 }
 
-bool PopManager::load_pop_type_file(std::filesystem::path const& path, ast::NodeCPtr root) {
+bool PopManager::load_pop_type_file(const std::string_view filestem, ast::NodeCPtr root) {
 
 	// TODO - pop type loading
 
@@ -135,29 +135,24 @@ bool PopManager::load_pop_type_file(std::filesystem::path const& path, ast::Node
 	return true;
 }
 
-bool PopManager::load_pop_into_province(Province& province, ast::NodeCPtr root) const {
+bool PopManager::load_pop_into_province(Province& province, const std::string_view pop_type_identifier, ast::NodeCPtr pop_node) const {
 	static PopType const* type = get_pop_type_by_identifier("test_pop_type");
 	Culture const* culture = nullptr;
 	Religion const* religion = nullptr;
 	Pop::pop_size_t size = 0;
 
-	bool ret = expect_assign(
-		[this, &culture, &religion, &size](std::string_view, ast::NodeCPtr pop_node) -> bool {
-			return expect_dictionary_keys(
-				"culture", ONE_EXACTLY, culture_manager.expect_culture(culture),
-				"religion", ONE_EXACTLY, religion_manager.expect_religion(religion),
-				"size", ONE_EXACTLY, expect_uint(assign_variable_callback_uint("pop size", size)),
-				"militancy", ZERO_OR_ONE, success_callback,
-				"rebel_type", ZERO_OR_ONE, success_callback
-			)(pop_node);
-		}
-	)(root);
+	bool ret = expect_dictionary_keys(
+		"culture", ONE_EXACTLY, culture_manager.expect_culture(culture),
+		"religion", ONE_EXACTLY, religion_manager.expect_religion(religion),
+		"size", ONE_EXACTLY, expect_uint(assign_variable_callback_uint("pop size", size)),
+		"militancy", ZERO_OR_ONE, success_callback,
+		"rebel_type", ZERO_OR_ONE, success_callback
+	)(pop_node);
 
 	if (type != nullptr && culture != nullptr && religion != nullptr && size > 0) {
 		ret &= province.add_pop({ *type, *culture, *religion, size });
 	} else {
-		Logger::error("Some pop arguments are invalid: type = ", type, ", culture = ", culture, ", religion = ", religion, ", size = ", size);
-		ret = false;
+		Logger::warning("Some pop arguments are invalid: province = ", province, ", type = ", type, ", culture = ", culture, ", religion = ", religion, ", size = ", size);
 	}
 	return ret;
 }
diff --git a/src/openvic-simulation/pop/Pop.hpp b/src/openvic-simulation/pop/Pop.hpp
index 3c635a4..d01eb97 100644
--- a/src/openvic-simulation/pop/Pop.hpp
+++ b/src/openvic-simulation/pop/Pop.hpp
@@ -1,7 +1,5 @@
 #pragma once
 
-#include <filesystem>
-
 #include "openvic-simulation/pop/Culture.hpp"
 #include "openvic-simulation/pop/Religion.hpp"
 
@@ -95,7 +93,7 @@ namespace OpenVic {
 			bool is_artisan, bool is_slave);
 		IDENTIFIER_REGISTRY_ACCESSORS(PopType, pop_type)
 
-		bool load_pop_type_file(std::filesystem::path const& path, ast::NodeCPtr root);
-		bool load_pop_into_province(Province& province, ast::NodeCPtr root) const;
+		bool load_pop_type_file(const std::string_view filestem, ast::NodeCPtr root);
+		bool load_pop_into_province(Province& province, const std::string_view pop_type_identifier, ast::NodeCPtr pop_node) const;
 	};
 }
diff --git a/src/openvic-simulation/types/IdentifierRegistry.cpp b/src/openvic-simulation/types/IdentifierRegistry.cpp
index 6d5221b..e33bc29 100644
--- a/src/openvic-simulation/types/IdentifierRegistry.cpp
+++ b/src/openvic-simulation/types/IdentifierRegistry.cpp
@@ -13,6 +13,14 @@ std::string const& HasIdentifier::get_identifier() const {
 	return identifier;
 }
 
+std::ostream& OpenVic::operator<<(std::ostream& stream, HasIdentifier const& obj) {
+	return stream << obj.get_identifier();
+}
+
+std::ostream& OpenVic::operator<<(std::ostream& stream, HasIdentifier const* obj) {
+	return obj != nullptr ? stream << *obj : stream << "<NULL>";
+}
+
 HasColour::HasColour(colour_t const new_colour, bool can_be_null) : colour(new_colour) {
 	assert((can_be_null || colour != NULL_COLOUR) && colour <= MAX_COLOUR_RGB);
 }
diff --git a/src/openvic-simulation/types/IdentifierRegistry.hpp b/src/openvic-simulation/types/IdentifierRegistry.hpp
index 494ff3e..20eebb9 100644
--- a/src/openvic-simulation/types/IdentifierRegistry.hpp
+++ b/src/openvic-simulation/types/IdentifierRegistry.hpp
@@ -27,6 +27,9 @@ namespace OpenVic {
 		std::string const& get_identifier() const;
 	};
 
+	std::ostream& operator<<(std::ostream& stream, HasIdentifier const& obj);
+	std::ostream& operator<<(std::ostream& stream, HasIdentifier const* obj);
+
 	/*
 	 * Base class for objects with associated colour information.
 	 */
diff --git a/src/openvic-simulation/utility/Logger.cpp b/src/openvic-simulation/utility/Logger.cpp
index bf9a67c..fca08a5 100644
--- a/src/openvic-simulation/utility/Logger.cpp
+++ b/src/openvic-simulation/utility/Logger.cpp
@@ -6,6 +6,8 @@ using namespace OpenVic;
 
 Logger::log_func_t Logger::info_func {};
 Logger::log_queue_t Logger::info_queue {};
+Logger::log_func_t Logger::warning_func {};
+Logger::log_queue_t Logger::warning_queue {};
 Logger::log_func_t Logger::error_func {};
 Logger::log_queue_t Logger::error_queue {};
 
diff --git a/src/openvic-simulation/utility/Logger.hpp b/src/openvic-simulation/utility/Logger.hpp
index 417aba7..f9ebd5d 100644
--- a/src/openvic-simulation/utility/Logger.hpp
+++ b/src/openvic-simulation/utility/Logger.hpp
@@ -80,6 +80,7 @@ namespace OpenVic {
 		name(Ts&&...) -> name<Ts...>;
 
 		LOG_FUNC(info)
+		LOG_FUNC(warning)
 		LOG_FUNC(error)
 
 #undef LOG_FUNC