Merge pull request #663 from OpenVicProject/typed_identifier_registry_and_fixes

wvpm · web-flow · commit b0d2f4290701 · 2025-11-23T21:11:26.000+01:00
IdentifierRegistry uses typed indices + fixes
diff --git a/src/headless/main.cpp b/src/headless/main.cpp
@@ -240,7 +240,10 @@ static bool run_headless(fs::path const& root, memory::vector<memory::string>& m
 
 	SPDLOG_INFO("===== Setting up instance... =====");
 	ret &= game_manager.setup_instance(
-		game_manager.get_definition_manager().get_history_manager().get_bookmark_manager().get_bookmark_by_index(0)
+		game_manager.get_definition_manager()
+			.get_history_manager()
+			.get_bookmark_manager()
+			.get_front_bookmark()
 	);
 
 	print_memory_usage("Instance Setup");
diff --git a/src/openvic-simulation/GameManager.cpp b/src/openvic-simulation/GameManager.cpp
@@ -1,7 +1,6 @@
 #include "GameManager.hpp"
 
 #include <chrono>
-#include <cstddef>
 #include <string_view>
 
 #include <range/v3/algorithm/contains.hpp>
@@ -169,7 +168,7 @@ bool GameManager::load_definitions(Dataloader::localisation_callback_t localisat
 	return ret;
 }
 
-bool GameManager::setup_instance(Bookmark const* bookmark) {
+bool GameManager::setup_instance(Bookmark const& bookmark) {
 	if (instance_manager) {
 		spdlog::error_s("Trying to setup a new game instance while one is already setup!");
 		return false;
@@ -187,7 +186,7 @@ bool GameManager::setup_instance(Bookmark const* bookmark) {
 
 	bool ret = instance_manager->setup();
 
-	SPDLOG_INFO("Loading bookmark \"{}\" for new game instance.", bookmark ? bookmark->get_identifier() : "<NULL>");
+	SPDLOG_INFO("Loading bookmark \"{}\" for new game instance.", bookmark.get_identifier());
 
 	ret &= instance_manager->load_bookmark(bookmark);
 
diff --git a/src/openvic-simulation/GameManager.hpp b/src/openvic-simulation/GameManager.hpp
@@ -59,7 +59,7 @@ namespace OpenVic {
 
 		bool load_definitions(Dataloader::localisation_callback_t localisation_callback);
 
-		bool setup_instance(Bookmark const* bookmark);
+		bool setup_instance(Bookmark const& bookmark);
 		bool is_game_instance_setup() const;
 		bool is_bookmark_loaded() const;
 
diff --git a/src/openvic-simulation/InstanceManager.cpp b/src/openvic-simulation/InstanceManager.cpp
@@ -215,7 +215,7 @@ bool InstanceManager::setup() {
 	return true;
 }
 
-bool InstanceManager::load_bookmark(Bookmark const* new_bookmark) {
+bool InstanceManager::load_bookmark(Bookmark const& new_bookmark) {
 	if (is_bookmark_loaded()) {
 		spdlog::error_s("Cannot load bookmark - already loaded!");
 		return false;
@@ -226,12 +226,7 @@ bool InstanceManager::load_bookmark(Bookmark const* new_bookmark) {
 		return false;
 	}
 
-	if (new_bookmark == nullptr) {
-		spdlog::critical_s("Cannot load bookmark - null!");
-		return false;
-	}
-
-	bookmark = new_bookmark;
+	bookmark = &new_bookmark;
 
 	SPDLOG_INFO("Loading bookmark {} with start date {}", bookmark->get_name(), bookmark->date);
 
diff --git a/src/openvic-simulation/InstanceManager.hpp b/src/openvic-simulation/InstanceManager.hpp
@@ -92,7 +92,7 @@ namespace OpenVic {
 		}
 
 		bool setup();
-		bool load_bookmark(Bookmark const* new_bookmark);
+		bool load_bookmark(Bookmark const& new_bookmark);
 		bool start_game_session();
 		bool update_clock();
 		void force_tick_and_update();
diff --git a/src/openvic-simulation/economy/BuildingType.hpp b/src/openvic-simulation/economy/BuildingType.hpp
@@ -82,7 +82,7 @@ namespace OpenVic {
 	private:
 		IdentifierRegistry<BuildingType> IDENTIFIER_REGISTRY(building_type);
 		string_set_t PROPERTY(building_type_types);
-		memory::vector<BuildingType const*> PROPERTY(province_building_types);
+		memory::vector<BuildingType const*> SPAN_PROPERTY(province_building_types);
 		BuildingType const* PROPERTY(port_building_type);
 
 	public:
diff --git a/src/openvic-simulation/map/MapDefinition.cpp b/src/openvic-simulation/map/MapDefinition.cpp
@@ -39,12 +39,12 @@ MapDefinition::MapDefinition() {}
 ProvinceDefinition* MapDefinition::get_province_definition_from_number(
 	decltype(std::declval<ProvinceDefinition>().get_province_number())province_number
 ) {
-	return province_definitions.get_item_by_index(type_safe::get(ProvinceDefinition::get_index_from_province_number(province_number)));
+	return province_definitions.get_item_by_index(ProvinceDefinition::get_index_from_province_number(province_number));
 }
 ProvinceDefinition const* MapDefinition::get_province_definition_from_number(
 	decltype(std::declval<ProvinceDefinition>().get_province_number())province_number
 ) const {
-	return province_definitions.get_item_by_index(type_safe::get(ProvinceDefinition::get_index_from_province_number(province_number)));
+	return province_definitions.get_item_by_index(ProvinceDefinition::get_index_from_province_number(province_number));
 }
 
 RiverSegment::RiverSegment(uint8_t new_size, memory::vector<ivec2_t>&& new_points)
@@ -93,7 +93,7 @@ bool MapDefinition::add_province_definition(std::string_view identifier, colour_
 	}
 
 	ProvinceDefinition const& new_province = province_definitions.back();
-	colour_index_map[new_province.get_colour()] = province_index_t(new_province.get_province_number());
+	colour_index_map[new_province.get_colour()] = new_province.index;
 	return true;
 }
 
@@ -860,15 +860,24 @@ bool MapDefinition::load_map_images(fs::path const& province_path, fs::path cons
 	uint8_t const* province_data = province_bmp.get_pixel_data().data();
 	uint8_t const* terrain_data = terrain_bmp.get_pixel_data().data();
 
-	memory::FixedVector<fixed_point_map_t<TerrainType const*>> _terrain_type_pixels_list(province_definitions.size());
+	memory::FixedVector<fixed_point_map_t<TerrainType const*>> _terrain_type_pixels_list(
+		province_definitions.size(),
+		[](const size_t i) { return fixed_point_map_t<TerrainType const*>{}; }
+	);
 	TypedSpan<province_index_t, fixed_point_map_t<TerrainType const*>> terrain_type_pixels_list { _terrain_type_pixels_list };
 
 	bool ret = true;
 	ordered_set<colour_t> unrecognised_province_colours;
 
-	memory::FixedVector<fixed_point_t> _pixels_per_province(province_definitions.size());
+	memory::FixedVector<fixed_point_t> _pixels_per_province(
+		province_definitions.size(),
+		[](const size_t i) { return fixed_point_t::_0; }
+	);
 	TypedSpan<province_index_t, fixed_point_t> pixels_per_province { _pixels_per_province };
-	memory::FixedVector<fvec2_t> _pixel_position_sum_per_province(province_definitions.size());
+	memory::FixedVector<fvec2_t> _pixel_position_sum_per_province(
+		province_definitions.size(),
+		[](const size_t i) { return fvec2_t{}; }
+	);
 	TypedSpan<province_index_t, fvec2_t> pixel_position_sum_per_province { _pixel_position_sum_per_province };
 
 	for (ivec2_t pos {}; pos.y < get_height(); ++pos.y) {
diff --git a/src/openvic-simulation/map/MapDefinition.hpp b/src/openvic-simulation/map/MapDefinition.hpp
@@ -79,13 +79,13 @@ namespace OpenVic {
 		}
 
 		IDENTIFIER_REGISTRY_NON_CONST_ACCESSORS(province_definition);
+		ProvinceDefinition* get_province_definition_from_number(
+			const ProvinceDefinition::province_number_t province_number
+		);
 
 	public:
 		MapDefinition();
 
-		ProvinceDefinition* get_province_definition_from_number(
-			const ProvinceDefinition::province_number_t province_number
-		);
 		ProvinceDefinition const* get_province_definition_from_number(
 			const ProvinceDefinition::province_number_t province_number
 		) const;
diff --git a/src/openvic-simulation/map/ProvinceInstance.cpp b/src/openvic-simulation/map/ProvinceInstance.cpp
@@ -172,15 +172,16 @@ bool ProvinceInstance::remove_core(CountryInstance& core_to_remove, bool warn) {
 	return true;
 }
 
-bool ProvinceInstance::expand_building(building_type_index_t building_type_index) {
-	BuildingInstance* building = buildings.get_item_by_index(type_safe::get(building_type_index));
+bool ProvinceInstance::expand_building(const building_instance_index_t index) {
+	BuildingInstance* building = buildings.get_item_by_index(type_safe::get(index));
 	if (building == nullptr) {
 		spdlog::error_s(
 			"Trying to expand non-existent building index {} in province {}",
-			building_type_index, *this
+			index, *this
 		);
 		return false;
 	}
+	
 	return building->expand();
 }
 
diff --git a/src/openvic-simulation/map/ProvinceInstance.hpp b/src/openvic-simulation/map/ProvinceInstance.hpp
@@ -158,7 +158,7 @@ namespace OpenVic {
 			return owner == nullptr;
 		}
 
-		bool expand_building(building_type_index_t building_type_index);
+		bool expand_building(const building_instance_index_t index);
 
 		bool add_pop(Pop&& pop);
 		bool add_pop_vec(
diff --git a/src/openvic-simulation/misc/GameAction.cpp b/src/openvic-simulation/misc/GameAction.cpp
@@ -44,15 +44,15 @@ bool GameActionManager::VariantVisitor::operator() (set_ai_argument_t const& arg
 
 // Production
 bool GameActionManager::VariantVisitor::operator() (expand_province_building_argument_t const& argument) const {
-	const auto [province_index, building_type_index] = argument;
+	const auto [province_index, building_instance_index] = argument;
 	ProvinceInstance* province = instance_manager.get_map_instance().get_province_instance_by_index(province_index);
 
 	if (OV_unlikely(province == nullptr)) {
 		spdlog::error_s("GAME_ACTION_EXPAND_PROVINCE_BUILDING called with invalid province index: {}", province_index);
 		return false;
 	}
 
-	return province->expand_building(building_type_index);
+	return province->expand_building(building_instance_index);
 }
 
 // Budget
@@ -65,7 +65,7 @@ bool GameActionManager::VariantVisitor::operator() (set_strata_tax_argument_t co
 		return false;
 	}
 
-	Strata const* strata = instance_manager.definition_manager.get_pop_manager().get_strata_by_index(type_safe::get(strata_index));
+	Strata const* strata = instance_manager.definition_manager.get_pop_manager().get_strata_by_index(strata_index);
 
 	if (OV_unlikely(strata == nullptr)) {
 		spdlog::error_s("GAME_ACTION_SET_STRATA_TAX called with invalid strata index: {}", strata_index);
@@ -193,7 +193,7 @@ bool GameActionManager::VariantVisitor::operator() (start_research_argument_t co
 	Technology const* technology = instance_manager.definition_manager
 		.get_research_manager()
 		.get_technology_manager()
-		.get_technology_by_index(type_safe::get(technology_index));
+		.get_technology_by_index(technology_index);
 
 	if (OV_unlikely(technology == nullptr)) {
 		spdlog::error_s("GAME_ACTION_START_RESEARCH called with invalid technology index: {}", technology_index);
diff --git a/src/openvic-simulation/misc/GameAction.hpp b/src/openvic-simulation/misc/GameAction.hpp
@@ -44,7 +44,7 @@ X(tick, std::monostate) \
 X(set_pause, bool) \
 X(set_speed, int64_t) \
 X(set_ai, country_index_t, bool) \
-X(expand_province_building, province_index_t, building_type_index_t) \
+X(expand_province_building, province_index_t, building_instance_index_t) \
 X(set_strata_tax, country_index_t, strata_index_t, fixed_point_t) \
 X(set_army_spending, country_index_t, fixed_point_t) \
 X(set_navy_spending, country_index_t, fixed_point_t) \
diff --git a/src/openvic-simulation/population/PopManager.cpp b/src/openvic-simulation/population/PopManager.cpp
@@ -376,7 +376,7 @@ bool PopManager::load_delayed_parse_pop_type_data(
 	bool ret = true;
 	for (size_t index = 0; index < delayed_parse_nodes.size(); ++index) {
 		const auto [rebel_units, equivalent, promote_to_node, issues_node] = delayed_parse_nodes[index];
-		PopType* pop_type = pop_types.get_item_by_index(index);
+		PopType* pop_type = pop_types.get_item_by_index(pop_type_index_t(index));
 
 		pop_type->promote_to = std::move(decltype(PopType::promote_to){get_pop_types()});
 
diff --git a/src/openvic-simulation/types/FixedVector.hpp b/src/openvic-simulation/types/FixedVector.hpp
@@ -45,12 +45,12 @@ namespace OpenVic::_detail {
 		requires (!specialization_of<std::remove_cvref_t<std::invoke_result_t<GeneratorTemplateType, size_t>>, std::tuple>)
 		// The type must be constructible from the generator's single return value
 		&& std::constructible_from<T, decltype(std::declval<GeneratorTemplateType>()(std::declval<size_t>()))>
-		FixedVector(size_t capacity, GeneratorTemplateType&& generator)
-			: _max_size(capacity),
-			_size(capacity),
+		FixedVector(size_t size, GeneratorTemplateType&& generator)
+			: _max_size(size),
+			_size(size),
 			_allocator(),
-			_data_start_ptr(allocator_traits::allocate(_allocator, capacity)) {
-			for (size_t i = 0; i < capacity; ++i) {
+			_data_start_ptr(allocator_traits::allocate(_allocator, size)) {
+			for (size_t i = 0; i < size; ++i) {
 				allocator_traits::construct(
 					_allocator,
 					begin()+i,
@@ -74,12 +74,12 @@ namespace OpenVic::_detail {
 				)
 			};
 		}
-		FixedVector(size_t max_size, GeneratorTemplateType&& generator)
-			: _max_size(max_size),
-			_size(max_size),
+		FixedVector(size_t size, GeneratorTemplateType&& generator)
+			: _max_size(size),
+			_size(size),
 			_allocator(),
-			_data_start_ptr(allocator_traits::allocate(_allocator, max_size)) {
-			for (size_t i = 0; i < max_size; ++i) {
+			_data_start_ptr(allocator_traits::allocate(_allocator, size)) {
+			for (size_t i = 0; i < size; ++i) {
 				std::apply(
 					[this, i](auto&&... args) {
 						allocator_traits::construct(
diff --git a/src/openvic-simulation/types/IdentifierRegistry.hpp b/src/openvic-simulation/types/IdentifierRegistry.hpp
@@ -1,5 +1,6 @@
 #pragma once
 
+#include <concepts>
 #include <vector>
 
 #include "openvic-simulation/dataloader/NodeTools.hpp"
@@ -173,11 +174,24 @@ namespace OpenVic {
 
 	private:
 		using StorageInfo = _StorageInfo<item_type>;
-		using index_type = typename StorageInfo::index_type;
-		using identifier_index_map_t = template_string_map_t<index_type, Case>;
+		using internal_storage_index_type = typename StorageInfo::index_type;
+		using identifier_index_map_t = template_string_map_t<internal_storage_index_type, Case>;
+		
+		//helper to avoid error 'index_t': is not a member of external_value_type
+		template<typename EXTERNAL_VALUE_TYPE, typename = void>
+		struct get_index_t {
+			using type = std::size_t;
+		};
+
+		template<has_index EXTERNAL_VALUE_TYPE>
+		struct get_index_t<EXTERNAL_VALUE_TYPE, std::void_t<typename EXTERNAL_VALUE_TYPE::index_t>> {
+			using type = typename EXTERNAL_VALUE_TYPE::index_t;
+		};
 
 	public:
 		using storage_type = typename StorageInfo::storage_type;
+		using index_t = typename get_index_t<external_value_type>::type;
+
 		static constexpr bool storage_type_reservable = reservable<storage_type>;
 
 	private:
@@ -187,6 +201,16 @@ namespace OpenVic {
 		bool PROPERTY_CUSTOM_PREFIX(locked, is, false);
 		identifier_index_map_t identifier_index_map;
 
+		constexpr void emplace_identifier_index() {
+			const internal_storage_index_type index = StorageInfo::get_back_index(items);
+
+			/* Get item's identifier via index rather than using new_identifier, as it may have been invalidated item's move. */
+			identifier_index_map.emplace(
+				ValueInfo::get_identifier(ItemInfo::get_value(StorageInfo::get_item_from_index(items, index))),
+				StorageInfo::get_back_index(items)
+			);
+		}
+
 	public:
 		constexpr UniqueKeyRegistry(std::string_view new_name, bool new_log_lock = true)
 			: name { new_name }, log_lock { new_log_lock } {}
@@ -210,15 +234,7 @@ namespace OpenVic {
 			}
 
 			items.emplace_back(std::move(item));
-
-			const index_type index = StorageInfo::get_back_index(items);
-
-			/* Get item's identifier via index rather than using new_identifier, as it may have been invalidated item's move. */
-			identifier_index_map.emplace(
-				ValueInfo::get_identifier(ItemInfo::get_value(StorageInfo::get_item_from_index(items, index))),
-				StorageInfo::get_back_index(items)
-			);
-
+			emplace_identifier_index();
 			return true;
 		}
 
@@ -238,15 +254,7 @@ namespace OpenVic {
 			}
 
 			items.emplace_back(std::forward<Args>(args)...);
-
-			const index_type index = StorageInfo::get_back_index(items);
-
-			/* Get item's identifier via index rather than using new_identifier, as it may have been invalidated item's move. */
-			identifier_index_map.emplace(
-				ValueInfo::get_identifier(ItemInfo::get_value(StorageInfo::get_item_from_index(items, index))),
-				StorageInfo::get_back_index(items)
-			);
-
+			emplace_identifier_index();
 			return true;
 		}
 		
@@ -363,12 +371,15 @@ namespace OpenVic {
 		} \
 		return nullptr; \
 	} \
-	constexpr external_value_type CONST* get_item_by_index(std::size_t index) CONST { \
-		if (index < items.size()) { \
-			return std::addressof(ValueInfo::get_external_value(ItemInfo::get_value(items[index]))); \
-		} else { \
+	constexpr external_value_type CONST* get_item_by_index(const index_t typed_index) CONST { \
+		std::size_t index; \
+		if constexpr (std::is_same_v<index_t, std::size_t>) { \
+			index = typed_index; \
+		} else { index = type_safe::get(typed_index); } \
+		if (index < 0 || index >= items.size()) { \
 			return nullptr; \
 		} \
+		return std::addressof(ValueInfo::get_external_value(ItemInfo::get_value(items[index]))); \
 	} \
 	constexpr NodeTools::Callback<std::string_view> auto expect_item_str( \
 		NodeTools::Callback<external_value_type CONST&> auto callback, bool allow_empty, bool warn = false \
@@ -690,8 +701,8 @@ public: \
 	constexpr T const_kw* get_cast_##singular##_by_identifier(std::string_view identifier) const_kw { \
 		return registry.get_cast_item_by_identifier<T>(identifier); \
 	} \
-	constexpr decltype(registry)::external_value_type const_kw* get_##singular##_by_index(std::size_t index) const_kw { \
-		return index >= 0 ? registry.get_item_by_index(index) : nullptr; \
+	constexpr decltype(registry)::external_value_type const_kw* get_##singular##_by_index(decltype(registry)::index_t index) const_kw { \
+		return registry.get_item_by_index(index); \
 	} \
 	constexpr decltype(registry)::storage_type const_kw& get_##plural() const_kw { \
 		return registry.get_items(); \
diff --git a/src/openvic-simulation/types/TypedIndices.hpp b/src/openvic-simulation/types/TypedIndices.hpp
@@ -26,6 +26,7 @@
 #define TYPED_INDEX(name) TYPED_INDEX_CUSTOM(name, std::size_t)
 
 TYPED_INDEX(bookmark_index_t)
+TYPED_INDEX(building_instance_index_t)
 TYPED_INDEX(building_type_index_t)
 TYPED_INDEX(country_index_t)
 TYPED_INDEX(crime_index_t)
diff --git a/src/openvic-simulation/utility/Concepts.hpp b/src/openvic-simulation/utility/Concepts.hpp

Original file line number	Diff line number	Diff line change
`@@ -92,7 +92,7 @@ namespace OpenVic {`
`92`	`92`	`}`
`93`	`93`
`94`	`94`	`bool setup();`
`95`		`- bool load_bookmark(Bookmark const* new_bookmark);`
	`95`	`+ bool load_bookmark(Bookmark const& new_bookmark);`
`96`	`96`	`bool start_game_session();`
`97`	`97`	`bool update_clock();`
`98`	`98`	`void force_tick_and_update();`
Original file line number	Diff line number	Diff line change
`@@ -172,15 +172,16 @@ bool ProvinceInstance::remove_core(CountryInstance& core_to_remove, bool warn) {`
`172`	`172`	`return true;`
`173`	`173`	`}`
`174`	`174`
`175`		`-bool ProvinceInstance::expand_building(building_type_index_t building_type_index) {`
`176`		`- BuildingInstance* building = buildings.get_item_by_index(type_safe::get(building_type_index));`
	`175`	`+bool ProvinceInstance::expand_building(const building_instance_index_t index) {`
	`176`	`+ BuildingInstance* building = buildings.get_item_by_index(type_safe::get(index));`
`177`	`177`	`if (building == nullptr) {`
`178`	`178`	`spdlog::error_s(`
`179`	`179`	`"Trying to expand non-existent building index {} in province {}",`
`180`		`- building_type_index, *this`
	`180`	`+ index, *this`
`181`	`181`	`);`
`182`	`182`	`return false;`
`183`	`183`	`}`
	`184`	`+`
`184`	`185`	`return building->expand();`
`185`	`186`	`}`
`186`	`187`
Original file line number	Diff line number	Diff line change
`@@ -158,7 +158,7 @@ namespace OpenVic {`
`158`	`158`	`return owner == nullptr;`
`159`	`159`	`}`
`160`	`160`
`161`		`- bool expand_building(building_type_index_t building_type_index);`
	`161`	`+ bool expand_building(const building_instance_index_t index);`
`162`	`162`
`163`	`163`	`bool add_pop(Pop&& pop);`
`164`	`164`	`bool add_pop_vec(`