-
Notifications
You must be signed in to change notification settings - Fork 67
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
Improve AI alliance logic #992
base: main
Are you sure you want to change the base?
Improve AI alliance logic #992
Conversation
Add diplomacy function Final pass
c19ad3b
to
ddc45f9
Compare
@@ -906,15 +936,29 @@ std::vector<dcon::political_party_id> get_active_political_parties(sys::state& s | |||
return parties; | |||
} | |||
|
|||
void monthly_adjust_relationship(sys::state& state, dcon::nation_id a, dcon::nation_id b, float delta) { | |||
dcon::diplomatic_relation_id get_diplomatic_relation(sys::state& state, dcon::nation_id a, dcon::nation_id b, bool force_create) { |
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 way that dcon works is that it is safe to look up values from the invalid handle, and such values will always be zero. Thus, when there was no relationship object between nations, looking up the relationship would return the invalid handle, and then looking up the relationship value from that would return 0. This helped cut down on the number of relationship objects. If we created a relationship object every time we checked the relations between two nations we would quickly head towards n squared such objects.
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 do see that you have force create as an extra parameter, so I realize that this won't actually be creating such relationships everywhere. It just makes my danger sense tingle. So my comment above isn't a blocking objection.
} | ||
|
||
// also returns true if one is the sphere leader of the other | ||
bool in_same_sphere(sys::state& state, dcon::nation_id a, dcon::nation_id b) { |
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 can be simplified to just return a_sphere == b || b_sphere == a || a_sphere == b_sphere && a_sphere;
under the assumption that we only pass valid handles
} | ||
|
||
bool are_neighbors(sys::state& state, dcon::nation_id a, dcon::nation_id b) { | ||
for(auto g : fatten(state.world, a).get_nation_adjacency()) { |
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 can be reduced to get_nation_adjacency_by_nation_adjacency_pair
. This does a (fast) hash lookup using the combined indices and will return a valid handle (true if cast to bool) if they are adjacent and an invalid handle if they are not
bool has_our_core(sys::state& state, dcon::nation_id n, dcon::nation_id other) { | ||
for(auto prov : state.world.nation_get_province_ownership(other)) { | ||
auto fat = fatten(state.world, prov).get_province(); | ||
for(auto c : fat.get_core_as_province()) { |
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.
get_core_by_prov_tag_key
does a hash map lookup for existing cores using the province and national identity id combination, which could replace this inner loop. However, it may be even faster to loop over all the cores of n's identity and check if any of those is owned by other.
uint32_t upper_gp_ally_limit = (state.military_definitions.great_wars_enabled) ? uint32_t(2) : uint32_t(1); | ||
uint32_t gp_ally_count = uint32_t(0); | ||
|
||
std::vector<dcon::nation_id> allies; |
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 would rather not have what feels like an unnecessary vector doing allocations here. Just looping over the diplomatic relationships and checking for gp allies directly would be more efficient. If you want to factor out duplicate code from that process, you can write an iterator that iterates over just the allies of a nation, allowing you to write for(auto a : alliesof(state, from)) { ... }
which then allows you to even break out early if you hit the upper_gp_ally_limit
uint32_t gp_ally_count = uint32_t(0); | ||
|
||
std::vector<dcon::nation_id> allies; | ||
nations::get_allies(state, n, allies); |
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.
see above
float number_of_alliances(sys::state& state, dcon::nation_id target, dcon::nation_id from) { | ||
uint32_t ally_count = 0; | ||
std::vector<dcon::nation_id> allies; | ||
nations::get_allies(state, target, allies); |
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.
see above
float current_wars(sys::state& state, dcon::nation_id target, dcon::nation_id from) { | ||
float value = 0.0f; | ||
std::vector<dcon::nation_id> allies; | ||
nations::get_allies(state, target, allies); |
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.
see above
} | ||
} | ||
|
||
if(military::are_in_common_war(state, from, target) && !military::are_at_war(state, from, target)) { |
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.
you may want military::are_allied_in_war
here -- that is true if they are on the same side of some ongoing war
continue; | ||
if((n.get_overlord_as_subject().get_ruler())) | ||
continue; | ||
if(state.world.nation_get_diplomatic_points(n) < state.defines.alliance_diplomatic_cost) |
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.
generally, we let AIs ignore diplomatic point costs. There has even been some talk of removing diplomatic points from the game entirely
} else { | ||
break; | ||
} | ||
for(auto other : prune_targets) { |
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.
It seems to me that this will make the AI drop allies without considering its overall strategic situation, causing it to drop allies it may very well need to deter an attack
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 main reason for this change was that I felt that this condition differed from the alliance forming logic, as the AI would make allies without considering their strongest neighbor's strength explicitly but then it could end alliances based on the strongest neighbor's strength which meant that a nation could meet the criteria for both. The hope was that the combination of distance penalty and strength differential would work as a crude strategic assessment, but I might play around with tweaking the new logic so the code I removed would be partially reincorporated (maybe giving a bonus/penalty based on how the new/current ally fares compared to the strongest neighbor).
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.
Alright, I am just concerned that, now that alliance formation is affected by "feelings," a nation could get mad at its strong ally, break the alliance, and then immediately get jumped. It is always very disappointing when this happens because it tends to stand out (you ally with X; an event makes X mad at you; X breaks the alliance; X immediately gets wrecked)
return false; | ||
|
||
// Has not surpassed infamy limit | ||
if(state.world.nation_get_infamy(target) >= state.defines.badboy_limit * 0.75f) | ||
return false; | ||
|
||
// vassals always ally liege |
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.
vassals don't ally their liege in PA because they are automatically called into wars with their overlord regardless of alliance status. It is thus easier to leave them as not allied
bool ai_will_accept_alliance(sys::state& state, dcon::nation_id target, dcon::nation_id from) { | ||
if(!state.world.nation_get_ai_is_threatened(target)) |
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 have you decided to remove this check? If the AI is not threatened, it means that it does not think that it needs more allies to deter an attack. At that point, additional allies provide no material benefit while limiting its expansion options.
else | ||
return a.index() > b.index(); | ||
}); | ||
command::execute_ask_for_alliance(state, n, alliance_targets[0]); |
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.
so, assuming that you have excluded human-controlled nations from the list of possibilities, the list of possibilities should only include valid targets that would accept the alliance. Thus you can go directly to make_alliance
and skip the redundant checks. The reason we always exclude human-controlled nations from the list of potential targets is to not annoy the human player. The human player is assumed to proactively ask for alliances that he or she wants, and if the human player isn't asking, then they are assumed not to want one. Hence, we don't annoy them with asks that are expected to be rejected.
Is work on this PR still ongoing? |
Yes and I have a bunch of your feedback already addressed, but I probably won't get around to another big push until this weekend. |
It's been a very long weekend but obviously too many things have changed and I can't even get the release build to work on Linux so I don't plan to continue this. I based most of this alliance logic on Victoria 2's values in the diplomacy screen, but between not keeping up with the game for a year and new non-V2 gameplay being added, I'm not sure what all is necessary/needed. I'll keep this open in case someone wants to pick it up, but if it's not needed then go ahead and close it. |
Mainly reworks AI alliance logic but also does some other clean up and implements a few missing pieces of logic regarding diplomacy actions changing relations. The alliance logic is based off of Victoria 2 to the best that I could recreate, which means changing the sequence of boolean checks with a running score which means alliances can be a bit more flexible. There are a few places where it wasn't obvious how the game was calculating everything but I tried to get as close to the original as possible.
The logic is shown in
ai_will_accept_alliance
in a pretty straightforward manner and I did try to be mindful of performance by adding in an early exit that can reduce the number of calculations. Overall the AI tends to have more stable alliances, and things like spherelings accepting alliances from other GPs, countries with negative relations forming alliances, and countries accepting alliances from nations who own their cores (except for vassal/substates) should be fixed which makes the gameplay more consistent with V2. The GP alliance limit is now active so nations can have only 1 GP ally before Great Wars are invented then they can have 2 (which is what I believe is the case in V2). The AI are now also required to have diplomatic points available for the make/cancel alliance action.The prune alliance logic was also redone to be consistent with the form alliance logic since I found that the AI was too capricious with making then breaking alliances.
I haven't tested this in multiplayer so I don't know if you'd want other devs to do testing with that before putting this into a release.