Skip to content

Fix province number vs index issues #661

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wvpm merged 1 commit into from
Nov 22, 2025
Merged

Fix province number vs index issues #661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 54 additions & 38 deletions src/openvic-simulation/map/MapDefinition.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <algorithm>
#include <cstddef>
#include <cstdint>
#include <limits>
#include <system_error>
#include <vector>

Expand All @@ -21,6 +22,7 @@
#include "openvic-simulation/modifier/ModifierManager.hpp"
#include "openvic-simulation/types/Colour.hpp"
#include "openvic-simulation/types/OrderedContainersMath.hpp"
#include "openvic-simulation/types/TypedSpan.hpp"
#include "openvic-simulation/types/Vector.hpp"
#include "openvic-simulation/utility/BMP.hpp"
#include "openvic-simulation/utility/FormatValidate.hpp"
Expand Down Expand Up @@ -48,11 +50,14 @@ ProvinceDefinition const* MapDefinition::get_province_definition_from_number(
RiverSegment::RiverSegment(uint8_t new_size, memory::vector<ivec2_t>&& new_points)
: size { new_size }, points { std::move(new_points) } {}

//1 entry reserved for null province
static constexpr province_index_t MAX_PROVINCE_COUNT = std::numeric_limits<province_index_t>().max() - 1;

bool MapDefinition::add_province_definition(std::string_view identifier, colour_t colour) {
if (province_definitions.size() >= type_safe::get(max_provinces)) {
spdlog::error_s(
"The map's province list is full - maximum number of provinces is {} (this can be at most {})",
max_provinces, ProvinceDefinition::MAX_INDEX
max_provinces, MAX_PROVINCE_COUNT
);
return false;
}
Expand All @@ -71,24 +76,24 @@ bool MapDefinition::add_province_definition(std::string_view identifier, colour_
spdlog::error_s("Invalid province colour for {} - null! ({})", identifier, colour);
return false;
}
const ProvinceDefinition::index_t province_number = get_province_number_from_colour(colour);
if (province_number != ProvinceDefinition::NULL_INDEX) {
const ProvinceDefinition::province_number_t province_number = get_province_number_from_colour(colour);
if (province_number != ProvinceDefinition::NULL_PROVINCE_NUMBER) {
spdlog::error_s(
"Duplicate province colours: {} and {}",
get_province_definition_from_number(type_safe::get(province_number))->to_string(), identifier
get_province_definition_from_number(province_number)->to_string(), identifier
);
return false;
}

if (!province_definitions.emplace_item(
identifier,
identifier, colour, ProvinceDefinition::index_t(get_province_definition_count())
identifier, colour, province_index_t(get_province_definition_count())
)) {
return false;
}

ProvinceDefinition const& new_province = province_definitions.back();
colour_index_map[new_province.get_colour()] = ProvinceDefinition::index_t(new_province.get_province_number());
colour_index_map[new_province.get_colour()] = province_index_t(new_province.get_province_number());
return true;
}

Expand Down Expand Up @@ -468,34 +473,42 @@ bool MapDefinition::add_region(std::string_view identifier, memory::vector<Provi
return ret;
}

ProvinceDefinition::index_t MapDefinition::get_province_number_from_colour(colour_t colour) const {
ProvinceDefinition::province_number_t MapDefinition::get_province_number_from_colour(colour_t colour) const {
const colour_index_map_t::const_iterator it = colour_index_map.find(colour);
if (it != colour_index_map.end()) {
return it->second;
return ProvinceDefinition::get_province_number_from_index(it.value());
}
return ProvinceDefinition::NULL_INDEX;
return ProvinceDefinition::NULL_PROVINCE_NUMBER;
}

ProvinceDefinition::index_t MapDefinition::get_province_number_at(ivec2_t pos) const {
ProvinceDefinition::province_number_t MapDefinition::get_province_number_at(ivec2_t pos) const {
if (pos.nonnegative() && pos.is_within_bound(dims)) {
return province_shape_image[get_pixel_index_from_pos(pos)].province_number;
}
return ProvinceDefinition::NULL_INDEX;
return ProvinceDefinition::NULL_PROVINCE_NUMBER;
}

ProvinceDefinition* MapDefinition::get_province_definition_at(ivec2_t pos) {
return get_province_definition_from_number(type_safe::get(get_province_number_at(pos)));
return get_province_definition_from_number(get_province_number_at(pos));
}

ProvinceDefinition const* MapDefinition::get_province_definition_at(ivec2_t pos) const {
return get_province_definition_from_number(type_safe::get(get_province_number_at(pos)));
return get_province_definition_from_number(get_province_number_at(pos));
}

bool MapDefinition::set_max_provinces(ProvinceDefinition::index_t new_max_provinces) {
if (new_max_provinces <= ProvinceDefinition::NULL_INDEX) {
bool MapDefinition::set_max_provinces(province_index_t new_max_provinces) {
if (new_max_provinces <= province_index_t(0)) {
spdlog::error_s(
"Trying to set max province count to an invalid value {} (must be greater than 0)",
new_max_provinces
);
return false;
}
if (new_max_provinces > MAX_PROVINCE_COUNT) {
spdlog::error_s(
"Trying to set max province count to an invalid value {} (must be greater than {})",
new_max_provinces, ProvinceDefinition::NULL_INDEX
"Trying to set max province count to an invalid value {} (must <= {})",
new_max_provinces,
MAX_PROVINCE_COUNT
);
return false;
}
Expand Down Expand Up @@ -847,19 +860,22 @@ 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::vector<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());
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::vector<fixed_point_t> pixels_per_province(province_definitions.size());
memory::vector<fvec2_t> pixel_position_sum_per_province(province_definitions.size());
memory::FixedVector<fixed_point_t> _pixels_per_province(province_definitions.size());
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());
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) {
for (pos.x = 0; pos.x < get_width(); ++pos.x) {
const size_t pixel_index = get_pixel_index_from_pos(pos);
const colour_t province_colour = colour_at(province_data, pixel_index);
ProvinceDefinition::index_t province_number = ProvinceDefinition::NULL_INDEX;
ProvinceDefinition::province_number_t province_number = ProvinceDefinition::NULL_PROVINCE_NUMBER;

if (pos.x > 0) {
const size_t jdx = pixel_index - 1;
Expand All @@ -879,7 +895,7 @@ bool MapDefinition::load_map_images(fs::path const& province_path, fs::path cons

province_number = get_province_number_from_colour(province_colour);

if (province_number == ProvinceDefinition::NULL_INDEX && !unrecognised_province_colours.contains(province_colour)) {
if (province_number == ProvinceDefinition::NULL_PROVINCE_NUMBER && !unrecognised_province_colours.contains(province_colour)) {
unrecognised_province_colours.insert(province_colour);
if (detailed_errors) {
spdlog::warn_s(
Expand All @@ -892,17 +908,18 @@ bool MapDefinition::load_map_images(fs::path const& province_path, fs::path cons
index_found:
province_shape_image[pixel_index].province_number = province_number;

if (province_number != ProvinceDefinition::NULL_INDEX) {
const ProvinceDefinition::index_t array_index = province_number - 1;
pixels_per_province[type_safe::get(array_index)]++;
pixel_position_sum_per_province[type_safe::get(array_index)] += static_cast<fvec2_t>(pos);
if (province_number != ProvinceDefinition::NULL_PROVINCE_NUMBER) {
const province_index_t province_index = ProvinceDefinition::get_index_from_province_number(province_number);
pixels_per_province[province_index]++;
pixel_position_sum_per_province[province_index] += static_cast<fvec2_t>(pos);
}

const TerrainTypeMapping::index_t terrain = terrain_data[pixel_index];
TerrainTypeMapping const* mapping = terrain_type_manager.get_terrain_type_mapping_for(terrain);
if (mapping != nullptr) {
if (province_number != ProvinceDefinition::NULL_INDEX) {
terrain_type_pixels_list[type_safe::get(province_number - 1)][&mapping->type]++;
if (province_number != ProvinceDefinition::NULL_PROVINCE_NUMBER) {
const province_index_t province_index = ProvinceDefinition::get_index_from_province_number(province_number);
terrain_type_pixels_list[province_index][&mapping->type]++;
}
if (mapping->has_texture && terrain < terrain_type_manager.get_terrain_texture_limit()) {
province_shape_image[pixel_index].terrain = terrain + 1;
Expand All @@ -920,21 +937,20 @@ bool MapDefinition::load_map_images(fs::path const& province_path, fs::path cons
}

size_t missing = 0;
for (size_t array_index = 0; array_index < province_definitions.size(); ++array_index) {
ProvinceDefinition* province = province_definitions.get_item_by_index(array_index);

fixed_point_map_t<TerrainType const*> const& terrain_type_pixels = terrain_type_pixels_list[array_index];
for (ProvinceDefinition& province : province_definitions.get_items()) {
const province_index_t province_index = province.index;
fixed_point_map_t<TerrainType const*> const& terrain_type_pixels = terrain_type_pixels_list[province_index];
const fixed_point_map_const_iterator_t<TerrainType const*> largest = get_largest_item(terrain_type_pixels);
province->default_terrain_type = largest != terrain_type_pixels.end() ? largest->first : nullptr;
province.default_terrain_type = largest != terrain_type_pixels.end() ? largest->first : nullptr;

const fixed_point_t pixel_count = pixels_per_province[array_index];
province->on_map = pixel_count > 0;
const fixed_point_t pixel_count = pixels_per_province[province_index];
province.on_map = pixel_count > 0;

if (province->on_map) {
province->centre = pixel_position_sum_per_province[array_index] / pixel_count;
if (province.on_map) {
province.centre = pixel_position_sum_per_province[province_index] / pixel_count;
} else {
if (detailed_errors) {
spdlog::warn_s("Province missing from shape image: {}", province->to_string());
spdlog::warn_s("Province missing from shape image: {}", province.to_string());
}
missing++;
}
Expand Down
13 changes: 6 additions & 7 deletions src/openvic-simulation/map/MapDefinition.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,10 @@ namespace OpenVic {
* MAP-4
*/
struct MapDefinition {

#pragma pack(push, 1)
/* Used to represent tightly packed 3-byte integer pixel information. */
struct shape_pixel_t {
ProvinceDefinition::index_t province_number;
ProvinceDefinition::province_number_t province_number;
TerrainTypeMapping::index_t terrain;
};
#pragma pack(pop)
Expand All @@ -67,12 +66,12 @@ namespace OpenVic {
memory::vector<shape_pixel_t> PROPERTY(province_shape_image);
colour_index_map_t colour_index_map;

ProvinceDefinition::index_t PROPERTY(max_provinces, ProvinceDefinition::MAX_INDEX);
ProvinceDefinition::index_t PROPERTY(max_provinces);

PointMap PROPERTY_REF(path_map_land);
PointMap PROPERTY_REF(path_map_sea);

ProvinceDefinition::index_t get_province_number_from_colour(colour_t colour) const;
ProvinceDefinition::province_number_t get_province_number_from_colour(colour_t colour) const;
bool _generate_standard_province_adjacencies();

inline constexpr int32_t get_pixel_index_from_pos(ivec2_t pos) const {
Expand All @@ -85,10 +84,10 @@ namespace OpenVic {
MapDefinition();

ProvinceDefinition* get_province_definition_from_number(
decltype(std::declval<ProvinceDefinition>().get_province_number())province_number
const ProvinceDefinition::province_number_t province_number
);
ProvinceDefinition const* get_province_definition_from_number(
decltype(std::declval<ProvinceDefinition>().get_province_number())province_number
const ProvinceDefinition::province_number_t province_number
) const;

inline constexpr int32_t get_width() const { return dims.x; }
Expand All @@ -112,7 +111,7 @@ namespace OpenVic {
size_t get_land_province_count() const;
size_t get_water_province_count() const;

ProvinceDefinition::index_t get_province_number_at(ivec2_t pos) const;
ProvinceDefinition::province_number_t get_province_number_at(ivec2_t pos) const;

private:
ProvinceDefinition* get_province_definition_at(ivec2_t pos);
Expand Down
2 changes: 1 addition & 1 deletion src/openvic-simulation/map/Mapmode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ bool MapmodeManager::generate_mapmode_colours(

Mapmode::base_stripe_t* target_stripes = reinterpret_cast<Mapmode::base_stripe_t*>(target);

target_stripes[type_safe::get(ProvinceDefinition::NULL_INDEX)] = colour_argb_t::null();
target_stripes[ProvinceDefinition::NULL_PROVINCE_NUMBER] = colour_argb_t::null();

for (ProvinceInstance const& province : map_instance.get_province_instances()) {
target_stripes[province.province_definition.get_province_number()] = mapmode->get_base_stripe_colours(
Expand Down
4 changes: 2 additions & 2 deletions src/openvic-simulation/map/Mapmode.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ namespace OpenVic {
/* The mapmode colour image contains of a list of base colours and stripe colours. Each colour is four bytes
* in RGBA format, with the alpha value being used to interpolate with the terrain colour, so A = 0 is fully terrain
* and A = 255 is fully the RGB colour packaged with A. The base and stripe colours for each province are packed
* together adjacently, so each province's entry is 8 bytes long. The list contains ProvinceDefinition::MAX_INDEX + 1
* entries, that is the maximum allowed number of provinces plus one for the index-zero "null province". */
* together adjacently, so each province's entry is 8 bytes long.
* The list contains all provinces indexed by their number + index 0 for the "null province". */
bool generate_mapmode_colours(
MapInstance const& map_instance, Mapmode const* mapmode,
CountryInstance const* player_country, ProvinceInstance const* selected_province,
Expand Down
13 changes: 9 additions & 4 deletions src/openvic-simulation/map/ProvinceDefinition.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "openvic-simulation/types/TypedIndices.hpp"
#include "openvic-simulation/types/Vector.hpp"
#include "openvic-simulation/utility/Containers.hpp"
#include "type_safe/strong_typedef.hpp"

namespace OpenVic {

Expand Down Expand Up @@ -84,9 +85,10 @@ namespace OpenVic {
fixed_point_map_t<BuildingType const*> building_rotation;
};

static constexpr index_t NULL_INDEX { 0 }, MAX_INDEX = std::numeric_limits<index_t>::max();
static constexpr type_safe::underlying_type<index_t> NULL_PROVINCE_NUMBER { 0 };

private:
using province_number_t = type_safe::underlying_type<province_index_t>;
/* Immutable attributes (unchanged after initial game load) */
Region const* PROPERTY(region, nullptr);
Climate const* PROPERTY(climate, nullptr);
Expand Down Expand Up @@ -114,10 +116,13 @@ namespace OpenVic {
return region != nullptr;
}

constexpr std::size_t get_province_number() const {
return index.value_ + 1;
constexpr province_number_t get_province_number() const {
return get_province_number_from_index(index);
}
static constexpr index_t get_index_from_province_number(const std::size_t province_number) {
static constexpr province_number_t get_province_number_from_index(const province_index_t index) {
return type_safe::get(index) + 1;
}
static constexpr index_t get_index_from_province_number(const province_number_t province_number) {
return index_t(province_number - 1);
}

Expand Down