-
Notifications
You must be signed in to change notification settings - Fork 66
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.
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
Devbranch7 into main #1911
base: main
Are you sure you want to change the base?
Devbranch7 into main #1911
Conversation
SneakBug8
commented
Feb 14, 2025
- Local commodities
- AI upgrades ships
- Ticking score adjustments
- disallow_naval_trade, disallow_land_trade modifiers for mods (uncivs)
- Technologies costing leadership for mods (military doctrines)
- Maneuver as a modifier (Vic2 compat)
- Provid console command
… into devbranch6
@@ -221,6 +221,7 @@ month;month | |||
a_port;a port | |||
national_rank;national rank | |||
att_technology;technology | |||
TECHNOLOGY_LEADERSHIP_POINTS;Leadership points: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the convention is that all PA keys are in lower case
@@ -1323,12 +1325,25 @@ price_origin;Origin price | |||
price_target;Target price | |||
trade_distance;Distance | |||
volume;Volume | |||
desired_volume; Desired Volume | |||
desired_volume;Actual Volume |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is correct, then the key itself should also be changed to avoid very confusing C++ code
@@ -23,6 +23,7 @@ And here is a list of some more specialized console functions that you can use | |||
- `clear` : removes old text form the console window | |||
- `spectate` : switches the game to spectator mode (use `change-tag` to resume playing) | |||
- `true fps` : turns the visible FPS counter on. A value of `false` will instead turn it off | |||
- `provid`: Turns on and off display of content province ids in tooltips |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably take a boolean parameter like the other options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it still will confuse vanilla V2 players expecting to type just provid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since all the other console commands have been changed, I imagine that they are already confused.
for(auto n : state.world.in_nation) { | ||
if(n.get_is_player_controlled()) | ||
continue; | ||
if(n.get_is_at_war() == false && nations::is_landlocked(state, n)) { | ||
for(auto v : n.get_navy_control()) { | ||
if(!v.get_navy().get_battle_from_navy_battle_participation()) { | ||
for(auto shp : v.get_navy().get_navy_membership()) { | ||
to_delete.push_back(shp.get_ship().id); | ||
state.world.delete_ship(shp.get_ship()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deleting ships while you are iterating over them is a bug
@@ -36,6 +36,20 @@ std::string get_tech_category_name(tech_category t) { | |||
return "none"; | |||
} | |||
|
|||
std::vector<culture::tech_category> get_active_tech_categories(sys::state& state) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this function exists, since we already have a vector of the tech categories in the form of state.culture_definitions.tech_folders, which you use below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Folders are created for all potential categories incl flavour from every mod. Previously scenario generator used hardcoded 5 categories and would crash when 6th was used. Now this method finds which categories actually have some tech in them.
auto B_bans_sea_trade = state.world.nation_get_modifier_values(n_B, sys::national_mod_offsets::disallow_naval_trade) > 0.f; | ||
auto sea_trade_banned = A_bans_sea_trade || B_bans_sea_trade; | ||
auto A_bans_land_trade = state.world.nation_get_modifier_values(n_A, sys::national_mod_offsets::disallow_land_trade) > 0.f; | ||
auto B_bans_land_trade = state.world.nation_get_modifier_values(n_B, sys::national_mod_offsets::disallow_land_trade) > 0.f; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you are using disallow_land_trade like a boolean rule, it should probably be a boolean rule. Otherwise, it will show up extremely weirdly in the ui
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have boolean modifiers (like other Pdx games do) and AFAIK modifiers can't change rules, so a true/false modifier type is necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fundamentally, modifiers are designed as an additive system. But you can't add one or more "bans trade" modifiers together in a meaningful way. I understand that putting it as a modifier is the easiest way to achieve the desired result. However, it isn't a good way to achieve the desired result. If you want to come up with a way to extend the political rules, I encourage you to do that.
@@ -1350,6 +1355,12 @@ object { | |||
tag{ scenario } | |||
tag{ save } | |||
|
|||
property{ | |||
name{ provid } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deliberately don't store the original province ids in the scenario since they are meaningless in game. When people say that they want to know the province id with a console command, they generally want a number that they can use with other console commands, not the number form the game files. Giving them the number from the game files will be frustrating since they can't actually use it to do change owner, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember people explicitly looking for Content province id to modify events/history files easier. Anyway, tooltip shows both PA id and content ID as two rows.
@@ -268,6 +270,7 @@ struct unit_variable_stats { | |||
float siege_or_torpedo_attack = 0.0f; | |||
float reconnaissance_or_fire_range = 0.0f; | |||
float discipline_or_evasion = 0.0f; | |||
float maneuver = 0.0f; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does any unit have both a maneuver value and a support value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it matter? We used base unit type values previously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, if no unit has both, it could double up, like many of the other values do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some units such as artillery (has 1 maneuver and support), and planes (has 2 manuever and support) has both maneuver and support by default.
@@ -44,6 +45,8 @@ std::string format_modifier_value(sys::state& state, float value, modifier_displ | |||
return(value >= 0.f ? "+" : "") + text::format_float(value, 2); | |||
case modifier_display_type::fp_three_places: | |||
return (value >= 0.f ? "+" : "") + text::format_float(value, 3); | |||
case modifier_display_type::yesno: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, but it will still be represented internally as a floating point, which means that you could have one modifier with a positive value showing "yes" and another with a negative value showing "no" and no way to know whether the sum will be "yes" or "no"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to extend the nation's rules in some way, you should extend the rules