Refactor military to use reference_wrapper

src/openvic-simulation/country/CountryInstance.cpp

@@ -5,6 +5,7 @@
 #include <algorithm>
 #include <cstddef>
 #include <cstdint>
+#include <functional>
 #include <limits>
 
 #include <type_safe/strong_typedef.hpp>
@@ -705,10 +706,10 @@ bool CountryInstance::add_unit_instance_group(UnitInstanceGroup& group) {
 
 	switch (group.branch) {
 	case LAND:
-		armies.push_back(static_cast<ArmyInstance*>(&group));
+		armies.emplace_back(static_cast<ArmyInstance&>(group));
 		return true;
 	case NAVAL:
-		navies.push_back(static_cast<NavyInstance*>(&group));
+		navies.emplace_back(static_cast<NavyInstance&>(group));
 		return true;
 	default:
 		spdlog::error_s(
@@ -721,10 +722,13 @@ bool CountryInstance::add_unit_instance_group(UnitInstanceGroup& group) {
 
 bool CountryInstance::remove_unit_instance_group(UnitInstanceGroup const& group) {
 	const auto remove_from_vector = [this, &group]<unit_branch_t Branch>(
-		memory::vector<UnitInstanceGroupBranched<Branch>*>& unit_instance_groups
+		memory::vector<std::reference_wrapper<UnitInstanceGroupBranched<Branch>>>& unit_instance_groups
 	) -> bool {
-		const typename memory::vector<UnitInstanceGroupBranched<Branch>*>::const_iterator it =
-			std::find(unit_instance_groups.begin(), unit_instance_groups.end(), &group);
+		const auto it =	std::find(
+			unit_instance_groups.begin(),
+			unit_instance_groups.end(),
+			group
+		);
 
 		if (it != unit_instance_groups.end()) {
 			unit_instance_groups.erase(it);
@@ -759,10 +763,10 @@ bool CountryInstance::add_leader(LeaderInstance& leader) {
 
 	switch (leader.branch) {
 	case LAND:
-		generals.push_back(&leader);
+		generals.push_back(leader);
 		return true;
 	case NAVAL:
-		admirals.push_back(&leader);
+		admirals.push_back(leader);
 		return true;
 	default:
 		spdlog::error_s(
@@ -776,7 +780,7 @@ bool CountryInstance::add_leader(LeaderInstance& leader) {
 bool CountryInstance::remove_leader(LeaderInstance const& leader) {
 	using enum unit_branch_t;
 
-	memory::vector<LeaderInstance*>* leaders;
+	memory::vector<std::reference_wrapper<LeaderInstance>>* leaders;
 
 	switch (leader.branch) {
 	case LAND:
@@ -793,7 +797,7 @@ bool CountryInstance::remove_leader(LeaderInstance const& leader) {
 		return false;
 	}
 
-	const typename memory::vector<LeaderInstance*>::const_iterator it = std::find(leaders->begin(), leaders->end(), &leader);
+	const auto it = std::find(leaders->begin(), leaders->end(), leader);
 
 	if (it != leaders->end()) {
 		leaders->erase(it);
@@ -808,9 +812,11 @@ bool CountryInstance::remove_leader(LeaderInstance const& leader) {
 }
 
 bool CountryInstance::has_leader_with_name(std::string_view name) const {
-	const auto check_leaders = [&name](memory::vector<LeaderInstance*> const& leaders) -> bool {
-		for (LeaderInstance const* leader : leaders) {
-			if (leader->get_name() == name) {
+	const auto check_leaders = [name](
+		memory::vector<std::reference_wrapper<LeaderInstance>> const& leaders
+	) -> bool {
+		for (LeaderInstance const& leader : leaders) {
+			if (leader.get_name() == name) {
 				return true;
 			}
 		}
@@ -1595,24 +1601,24 @@ void CountryInstance::_update_diplomacy() {
 void CountryInstance::_update_military() {
 	regiment_count = 0;
 
-	for (ArmyInstance const* army : armies) {
-		regiment_count += army->get_unit_count();
+	for (ArmyInstance const& army : armies) {
+		regiment_count += army.get_unit_count();
 	}
 
 	ship_count = 0;
 	total_consumed_ship_supply = 0;
 
-	for (NavyInstance const* navy : navies) {
-		ship_count += navy->get_unit_count();
-		total_consumed_ship_supply += navy->get_total_consumed_supply();
+	for (NavyInstance const& navy : navies) {
+		ship_count += navy.get_unit_count();
+		total_consumed_ship_supply += navy.get_total_consumed_supply();
 	}
 
 	// Calculate military power from land, sea, and leaders
 
 	size_t deployed_non_mobilised_regiments = 0;
-	for (ArmyInstance const* army : armies) {
-		for (RegimentInstance const* regiment : army->get_regiment_instances()) {
-			if (!regiment->is_mobilised()) {
+	for (ArmyInstance const& army : armies) {
+		for (RegimentInstance const& regiment : army.get_regiment_instances()) {
+			if (!regiment.is_mobilised()) {
 				deployed_non_mobilised_regiments++;
 			}
 		}
@@ -1643,9 +1649,9 @@ void CountryInstance::_update_military() {
 	}
 
 	fixed_point_t military_power_from_sea_running_total = 0;
-	for (NavyInstance const* navy : navies) {
-		for (ShipInstance const* ship : navy->get_ship_instances()) {
-			ShipType const& ship_type = ship->get_ship_type();
+	for (NavyInstance const& navy : navies) {
+		for (ShipInstance const& ship : navy.get_ship_instances()) {
+			ShipType const& ship_type = ship.get_ship_type();
 
 			if (ship_type.is_capital) {
 
@@ -1908,7 +1914,7 @@ void CountryInstance::update_gamestate(const Date today, MapInstance& map_instan
 		for (ProvinceDefinition::adjacency_t const& adjacency : province_definition.get_adjacencies()) {
 			// TODO - should we limit based on adjacency type? Straits and impassable still work in game,
 			// and water provinces don't have an owner so they'll get caught by the later checks anyway.
-			CountryInstance* neighbour = map_instance.get_province_instance_by_definition(*adjacency.get_to()).get_owner();
+			CountryInstance* neighbour = map_instance.get_province_instance_by_definition(adjacency.get_to()).get_owner();
 			if (neighbour != nullptr && neighbour != this) {
 				neighbouring_countries.insert(neighbour);
 			}
@@ -1922,18 +1928,18 @@ void CountryInstance::update_gamestate(const Date today, MapInstance& map_instan
 
 	if (capital != nullptr) {
 		capital->set_connected_to_capital(true);
-		memory::vector<ProvinceInstance const*> province_checklist { capital };
+		memory::vector<std::reference_wrapper<const ProvinceInstance>> province_checklist { *capital };
 
-		for (size_t index = 0; index < province_checklist.size(); index++) {
-			ProvinceInstance const& province = *province_checklist[index];
+		for (size_t index = 0; index < province_checklist.size(); ++index) {
+			ProvinceInstance const& province = province_checklist[index];
 
 			for (ProvinceDefinition::adjacency_t const& adjacency : province.province_definition.get_adjacencies()) {
-				ProvinceInstance& adjacent_province = map_instance.get_province_instance_by_definition(*adjacency.get_to());
+				ProvinceInstance& adjacent_province = map_instance.get_province_instance_by_definition(adjacency.get_to());
 
 				if (adjacent_province.get_owner() == this && !adjacent_province.get_connected_to_capital()) {
 					adjacent_province.set_connected_to_capital(true);
 					adjacent_province.set_is_overseas(false);
-					province_checklist.push_back(&adjacent_province);
+					province_checklist.emplace_back(adjacent_province);
 				}
 			}
 		}

src/openvic-simulation/country/CountryInstance.hpp

@@ -1,5 +1,6 @@
 #pragma once
 
+#include <functional>
 #include <utility>
 
 #include <fmt/base.h>
@@ -353,10 +354,10 @@ namespace OpenVic {
 		OV_STATE_PROPERTY(fixed_point_t, military_power_from_sea);
 		OV_STATE_PROPERTY(fixed_point_t, military_power_from_leaders);
 		size_t PROPERTY(military_rank, 0);
-		memory::vector<LeaderInstance*> SPAN_PROPERTY(generals);
-		memory::vector<LeaderInstance*> SPAN_PROPERTY(admirals);
-		memory::vector<ArmyInstance*> SPAN_PROPERTY(armies);
-		memory::vector<NavyInstance*> SPAN_PROPERTY(navies);
+		memory::vector<std::reference_wrapper<LeaderInstance>> SPAN_PROPERTY(generals);
+		memory::vector<std::reference_wrapper<LeaderInstance>> SPAN_PROPERTY(admirals);
+		memory::vector<std::reference_wrapper<ArmyInstance>> SPAN_PROPERTY(armies);
+		memory::vector<std::reference_wrapper<NavyInstance>> SPAN_PROPERTY(navies);
 		size_t PROPERTY(regiment_count, 0);
 		size_t PROPERTY(mobilisation_potential_regiment_count, 0);
 		size_t PROPERTY(mobilisation_max_regiment_count, 0);

src/openvic-simulation/country/CountryInstanceManager.cpp

@@ -239,7 +239,7 @@ bool CountryInstanceManager::apply_history_to_countries(InstanceManager& instanc
 							map_instance
 						);
 
-						if (entry->get_initial_oob().has_value()) {
+						if (entry->get_initial_oob() != nullptr) {
 							oob_history_entry = entry.get();
 						}
 						if (entry->get_consciousness().has_value()) {
@@ -261,7 +261,7 @@ bool CountryInstanceManager::apply_history_to_countries(InstanceManager& instanc
 				}
 
 				if (oob_history_entry != nullptr) {
-					ret &= unit_instance_manager.generate_deployment(
+					ret &= oob_history_entry->get_initial_oob() != nullptr && unit_instance_manager.generate_deployment(
 						map_instance, country_instance, *oob_history_entry->get_initial_oob()
 					);
 				}

src/openvic-simulation/history/CountryHistory.hpp

@@ -43,7 +43,7 @@ namespace OpenVic {
 		std::optional<bool> PROPERTY_CUSTOM_PREFIX(civilised, is);
 		std::optional<fixed_point_t> PROPERTY(prestige);
 		ordered_set<Reform const*> PROPERTY(reforms);
-		std::optional<Deployment const*> PROPERTY(initial_oob);
+		Deployment const* PROPERTY(initial_oob, nullptr);
 		std::optional<TechnologySchool const*> PROPERTY(tech_school);
 		ordered_map<Technology const*, technology_unlock_level_t> PROPERTY(technologies);
 		ordered_map<Invention const*, bool> PROPERTY(inventions);

src/openvic-simulation/map/ProvinceDefinition.hpp

@@ -1,8 +1,8 @@
 #pragma once
 
+#include <functional>
 #include <optional>
 
-#include <type_safe/reference.hpp>
 #include <type_safe/strong_typedef.hpp>
 
 #include "openvic-simulation/dataloader/NodeTools.hpp"
@@ -53,7 +53,7 @@ namespace OpenVic {
 
 		private:
 			// fields are const after loading. They have to be mutable to support std::vector<adjacency_t>.erase(iterator)
-			type_safe::object_ref<const ProvinceDefinition> PROPERTY(to);
+			std::reference_wrapper<const ProvinceDefinition> PROPERTY(to);
 			ProvinceDefinition const* PROPERTY(through);
 			distance_t PROPERTY(distance);
 			type_t PROPERTY(type);

src/openvic-simulation/map/ProvinceInstance.cpp

@@ -340,23 +340,23 @@ void ProvinceInstance::update_gamestate(InstanceManager const& instance_manager)
 
 	MapInstance const& map_instance = instance_manager.get_map_instance();
 	for (ProvinceDefinition::adjacency_t const& adjacency : province_definition.get_adjacencies()) {
-		ProvinceDefinition const& province_definition = *adjacency.get_to();
-		ProvinceInstance const& province_instance = map_instance.get_province_instance_by_definition(province_definition);
+		ProvinceDefinition const& adjacent_to_definition = adjacency.get_to();
+		ProvinceInstance const& adjacent_to_instance = map_instance.get_province_instance_by_definition(adjacent_to_definition);
 
-		if (province_instance.is_empty()) {
+		if (adjacent_to_instance.is_empty()) {
 			has_empty_adjacent_province = true;
-		} else if (!province_definition.is_water()) {
-			adjacent_nonempty_land_provinces.push_back(&province_instance);
+		} else if (!adjacent_to_definition.is_water()) {
+			adjacent_nonempty_land_provinces.emplace_back(adjacent_to_instance);
 		}
 	}
 
 	land_regiment_count = 0;
-	for (ArmyInstance const* army : armies) {
-		land_regiment_count += army->get_unit_count();
+	for (ArmyInstance const& army : armies) {
+		land_regiment_count += army.get_unit_count();
 	}
-	for (NavyInstance const* navy : navies) {
-		for (ArmyInstance const* army : navy->get_carried_armies()) {
-			land_regiment_count += army->get_unit_count();
+	for (NavyInstance const& navy : navies) {
+		for (ArmyInstance const& army : navy.get_carried_armies()) {
+			land_regiment_count += army.get_unit_count();
 		}
 	}
 
@@ -409,10 +409,10 @@ bool ProvinceInstance::add_unit_instance_group(UnitInstanceGroup& group) {
 
 	switch (group.branch) {
 	case LAND:
-		armies.push_back(static_cast<ArmyInstance*>(&group));
+		armies.emplace_back(static_cast<ArmyInstance&>(group));
 		return true;
 	case NAVAL:
-		navies.push_back(static_cast<NavyInstance*>(&group));
+		navies.emplace_back(static_cast<NavyInstance&>(group));
 		return true;
 	default:
 		spdlog::error_s(
@@ -425,10 +425,13 @@ bool ProvinceInstance::add_unit_instance_group(UnitInstanceGroup& group) {
 
 bool ProvinceInstance::remove_unit_instance_group(UnitInstanceGroup const& group) {
 	const auto remove_from_vector = [this, &group]<unit_branch_t Branch>(
-		memory::vector<UnitInstanceGroupBranched<Branch>*>& unit_instance_groups
+		memory::vector<std::reference_wrapper<UnitInstanceGroupBranched<Branch>>>& unit_instance_groups
 	) -> bool {
-		const typename memory::vector<UnitInstanceGroupBranched<Branch>*>::const_iterator it =
-			std::find(unit_instance_groups.begin(), unit_instance_groups.end(), &group);
+		auto it = std::find(
+			unit_instance_groups.begin(),
+			unit_instance_groups.end(),
+			group
+		);
 
 		if (it != unit_instance_groups.end()) {
 			unit_instance_groups.erase(it);

src/openvic-simulation/map/ProvinceInstance.hpp

@@ -97,7 +97,7 @@ namespace OpenVic {
 		bool PROPERTY_RW(connected_to_capital, false);
 		bool PROPERTY_RW(is_overseas, false);
 		bool PROPERTY(has_empty_adjacent_province, false);
-		memory::vector<ProvinceInstance const*> SPAN_PROPERTY(adjacent_nonempty_land_provinces);
+		memory::vector<std::reference_wrapper<const ProvinceInstance>> SPAN_PROPERTY(adjacent_nonempty_land_provinces);
 		Crime const* PROPERTY_RW(crime, nullptr);
 		ResourceGatheringOperation PROPERTY(rgo);
 		memory::FixedVector<BuildingInstance> _buildings;
@@ -107,8 +107,8 @@ namespace OpenVic {
 			return buildings;
 		}
 	private:
-		memory::vector<ArmyInstance*> SPAN_PROPERTY(armies);
-		memory::vector<NavyInstance*> SPAN_PROPERTY(navies);
+		memory::vector<std::reference_wrapper<ArmyInstance>> SPAN_PROPERTY(armies);
+		memory::vector<std::reference_wrapper<NavyInstance>> SPAN_PROPERTY(navies);
 		// The number of land regiments currently in the province, including those being transported by navies
 		size_t PROPERTY(land_regiment_count, 0);
 		Timespan PROPERTY(occupation_duration);

src/openvic-simulation/military/Deployment.cpp

@@ -20,7 +20,7 @@ UnitDeployment<unit_branch_t::NAVAL>::UnitDeployment(std::string_view new_name,
 
 template<unit_branch_t Branch>
 UnitDeploymentGroup<Branch>::UnitDeploymentGroup(
-	std::string_view new_name, ProvinceDefinition const* new_location, memory::vector<_Unit>&& new_units,
+	std::string_view new_name, ProvinceDefinition const& new_location, memory::vector<_Unit>&& new_units,
 	std::optional<size_t> new_leader_index
 ) : name { new_name }, location { new_location }, units { std::move(new_units) }, leader_index { new_leader_index } {}
 
@@ -169,7 +169,7 @@ bool DeploymentManager::load_oob_file(
 
 			const size_t starting_general_count = general_count;
 
-			const bool ret = expect_dictionary_keys(
+			if(!expect_dictionary_keys(
 				"name", ONE_EXACTLY, expect_string(assign_variable_callback(army_name)),
 				"location", ONE_EXACTLY, map_definition.expect_province_definition_identifier(
 					assign_variable_callback_pointer(army_location)
@@ -201,14 +201,20 @@ bool DeploymentManager::load_oob_file(
 					return ret;
 				},
 				"leader", ZERO_OR_ONE, leader_callback
-			)(node);
+			)(node)) {
+				return false;
+			}
 
 			armies.emplace_back(
-				army_name, army_location, std::move(army_regiments),
-				starting_general_count < general_count ? std::optional { general_count - 1 } : std::nullopt
+				army_name,
+				*army_location,
+				std::move(army_regiments),
+				starting_general_count < general_count
+					? std::optional { general_count - 1 }
+					: std::nullopt
 			);
 
-			return ret;
+			return true;
 		},
 		"navy", ZERO_OR_MORE, [&admiral_count, &map_definition, &military_manager, &leader_callback](ast::NodeCPtr node) -> bool {
 			std::string_view navy_name {};
@@ -217,7 +223,7 @@ bool DeploymentManager::load_oob_file(
 
 			const size_t starting_admiral_count = admiral_count;
 
-			const bool ret = expect_dictionary_keys(
+			if(!expect_dictionary_keys(
 				"name", ONE_EXACTLY, expect_string(assign_variable_callback(navy_name)),
 				"location", ONE_EXACTLY, map_definition.expect_province_definition_identifier(
 					assign_variable_callback_pointer(navy_location)
@@ -242,14 +248,20 @@ bool DeploymentManager::load_oob_file(
 					return ret;
 				},
 				"leader", ZERO_OR_ONE, leader_callback
-			)(node);
+			)(node)) {
+				return false;
+			}
 
 			navies.emplace_back(
-				navy_name, navy_location, std::move(navy_ships),
-				starting_admiral_count < admiral_count ? std::optional { admiral_count - 1 } : std::nullopt
+				navy_name,
+				*navy_location,
+				std::move(navy_ships),
+				starting_admiral_count < admiral_count
+					? std::optional { admiral_count - 1 }
+					: std::nullopt
 			);
 
-			return ret;
+			return true;
 		}
 	)(Dataloader::parse_defines(lookedup_path).get_file_node());