Mod Compatibility Fixes #291
Conversation
| /// @brief Sets the directories the dataloader should pull content from. | ||
| /// | ||
| /// @param new_roots Dataloader roots in reverse-load order, so base defines first and final loaded mod last | ||
| /// @param new_replace_paths All base define paths that should be ignored entirely in favour of mods. | ||
| /// @return True if successful, false if failed. |
There was a problem hiding this comment.
Doxygen style comments are a bit inconsistent with other forms of documentation, we probably should decide how we'll handle documentation consistently.
| @@ -79,6 +79,7 @@ bool ConditionManager::setup_conditions(DefinitionManager const& definition_mana | |||
| ret &= add_condition("any_greater_power", GROUP, COUNTRY, COUNTRY); | |||
| ret &= add_condition("any_neighbor_country", GROUP, COUNTRY, COUNTRY); | |||
| ret &= add_condition("any_owned_province", GROUP, COUNTRY, PROVINCE); | |||
| ret &= add_condition("any_owned", GROUP, COUNTRY, PROVINCE); // alias? not in vanilla, but used by HPM poptypes, so used by a ton of derivative mods too | |||
There was a problem hiding this comment.
This is an effect scope. See https://vic2.paradoxwikis.com/List_of_scopes#any_owned
It is easily mistaken for a condition scope.
| @@ -230,6 +229,12 @@ bool ProductionTypeManager::load_production_types_file( | |||
| Logger::error("Failed get template identifier for ", key); | |||
| return false; | |||
| } | |||
| } else { | |||
There was a problem hiding this comment.
Can you provide a minimum working example?
Victoria 2 does nothing special with the name of a production type afaik.
Also, what is the point of having an unused template?
Is it not merely an unused production type?
BrickPi
force-pushed
the
improve-mod-loading
branch
from
0f5ab25 to
4e23937
Compare
| @@ -263,7 +263,7 @@ bool ModifierManager::setup_modifier_effects() { | |||
| modifier_effect_cache.land_attack_modifier, "land_attack_modifier", true, PROPORTION_DECIMAL, | |||
| ModifierEffect::make_default_modifier_effect_localisation_key("land_attack") | |||
| ); | |||
| ret &= register_technology_modifier_effect( | |||
| ret &= register_shared_tech_country_modifier_effect( | |||
There was a problem hiding this comment.
This is not valid as country modifier. It only works in tech.
| @@ -114,7 +116,9 @@ bool Icon::_fill_key_map(NodeTools::case_insensitive_key_map_t& key_map, UIManag | |||
|
|
|||
| "frame", ZERO_OR_ONE, expect_uint(assign_variable_callback(frame)), | |||
| "scale", ZERO_OR_ONE, expect_fixed_point(assign_variable_callback(scale)), | |||
| "rotation", ZERO_OR_ONE, expect_fixed_point(assign_variable_callback(rotation)) | |||
| "rotation", ZERO_OR_ONE, expect_fixed_point(assign_variable_callback(rotation)), | |||
| "maxWidth", ZERO_OR_ONE, success_callback, /* some mods use this, there's no effect afaik */ | |||
There was a problem hiding this comment.
Since it's not used, modders should receive a warning that their code does nothing.
Otherwise we pretend to understand it and then discard these values.
| if (!entry.ruling_party.has_value()) { | ||
| std::string_view def {}; | ||
| expect_identifier(assign_variable_callback(def))(value); | ||
| Logger::warning("In ", country.get_identifier(), " history: ruling_party ", def, " does NOT exist!"); |
There was a problem hiding this comment.
This results in an invalid state that OpenVic can't handle yet.
It's not a minor warning we can just ignore.
We'll either have to add support for a noParty or treat this as a game breaking error.
As a quickfix, we could just pick the first active party, if there is none, we still have a game breaking error.
| }, | ||
| "revolt", ZERO_OR_ONE, expect_dictionary_keys( | ||
| "type", ONE_EXACTLY, definition_manager.get_politics_manager().get_rebel_manager().expect_rebel_type_identifier(assign_variable_callback_pointer_opt(entry.revolt)), | ||
| "controller", ONE_EXACTLY, success_callback |
There was a problem hiding this comment.
Anything that is ignored should result in a warning.
We don't want to give modders the false impression that we do something with this.
| @@ -159,7 +160,11 @@ bool ProvinceHistoryMap::_load_history_entry( | |||
| } | |||
| } | |||
| return ret; | |||
| } | |||
| }, | |||
| "revolt", ZERO_OR_ONE, expect_dictionary_keys( | |||
There was a problem hiding this comment.
Could you provide a minimum working example?
I could not replicate it with
revolt = {
type = liberal_rebels
}
| @@ -546,6 +546,10 @@ bool PopManager::load_pop_bases_into_vector( | |||
| if (non_integer_size != nullptr && !size.is_integer()) { | |||
| *non_integer_size = true; | |||
| } | |||
| if (culture == nullptr || religion == nullptr) { | |||
| Logger::warning("No/invalid culture or relgion defined for pop of size ", size, ", ignored!"); | |||
There was a problem hiding this comment.
This warning should contain more information.
At least provide the province containing the pop. Preferably also provide the culture and/or religion text values.
replace_pathwhen searching for files