Expanding the relationship API #861
Conversation
…mplement actual per turn deals.
…ed the other party's player in when importing a deal, to log correctly
| warDeclarationCount = relationship.WarDeclarationCount, | ||
| // I don't think there is a way to figure this out for .sav or .biq files | ||
| // so by default we set this to 0 for these games | ||
| warDeclarationWithRoPActiveCount = 0, |
There was a problem hiding this comment.
Does civ3 treat "declaring war with your troops in the enemy civ's borders" differently than "declaring war with rop active"?
There was a problem hiding this comment.
I am not sure, but I feel like it does make a difference with the other AI, besides the one you are at war with, if you break some multi turn deal like RoP, alliances, MPP and stuff like that. Just to be clear, I am not aware of such a field existing in the biq.
Maybe we don't need/want such granular control, perhaps if we keep in a dictionary or something what we have broken and against whom would be enough for the AI to calculate some kind of score?
Should I add this as a TODO?
| public static bool IsInAnyWar(Player player, List<Player> players) { | ||
| if (players | ||
| // any player that is not barbarians, is not us, | ||
| .Any(other => !other.isBarbarians && other.id != player.id |
There was a problem hiding this comment.
Is this equivalent to players.Any(other => !other.isBarbarians && AtWar(player, other)?
There was a problem hiding this comment.
Actually yeah, thanks! You can probably guess the order in which these were implemented 😅
| // update whether the defender was sneak attacked | ||
| defender.playerRelationships[aggressor.id].wasSneakAttacked = sneakAttack; | ||
|
|
||
| // TODO: do we need the same for the aggressor? Perhaps a few turns less what the defender's is? |
There was a problem hiding this comment.
Yeah, I think that's reasonable - you could firm up the TODO, "we need to do the same for the aggressor ..."
| // add the deal for the other player | ||
| right.playerRelationships[left.id].multiTurnDeals.Add(mtd); | ||
| // right.playerRelationships[left.id].multiTurnDeals.Add(new MultiTurnDeal(mtd.dealType, mtd.dealSubType, | ||
| // mtd.dealDetails, mtd.goldPerTurn, mtd.resourcePerTurn, mtd.dealDuration, mtd.turnStartDeal, mtd.againstPlayer)); |
There was a problem hiding this comment.
Could you clean up the commented out code?
| } | ||
|
|
||
| /// <summary> | ||
| /// This automatically ends all deals except peace when they go over the initial agreed upon duration |
There was a problem hiding this comment.
Is it worth a TODO to add a popup if an AI/Human deal expires? I feel like that happens for some deal types in civ3
There was a problem hiding this comment.
You are right, I had totally forgotten about that, will do.
I think this is mostly about resources/luxuries? I can't remember a popup about an expired RoP for example, but sometimes(or is it always, not sure) it will open the deal window to renegotiate the expired deal, right? So i think this is actually 2 TODOs ?
I can see in the preferences a "Alaways renegotiate deals" option. So does this mean it has a random factor if it's off?
| // For example unless we have a good reason, as a human, receiving luxuries, gpt, | ||
| // or having an active RoP, doesn't hurt us. | ||
| log.Information("Checking to terminate any deals past their due duration"); | ||
| CheckForObsoleteDeals(gameData.players, GetTurnNumber()); |
There was a problem hiding this comment.
Should this happen player by player, rather than all at once? I can imagine this mattering for things like resource trades, where depending on the ordering another civ may or may not have resources available for new trades.
There was a problem hiding this comment.
Yeah that's a really good point. And the current implementation has a bug where UnRegisterMultiTurnDeal() only works for the "left" player, because the mtd object passed is their end of the deal, and not the object the "right" player has.
So for the other player the deal lingers untill it's their turn, where they are then the "left" player.
Even though there is a bug in my code, is this the correct behaviour? Or should we actually remove it from both when even 1 party terminates it?
I am afraid that if we do that, we will end up taking 1 round of the deal away from the second player, because it's getting yanked before it actually ends for them..
Perhaps this is what you meant too, besides not looping for every player, and check at their respective turn start instead.
I 'll drop a question in the discord channel if anyone has any information on this.
|
Very nice! I didn't patch + playtest this, but it seems like a reasonable way to deal with this stuff. I like how this PR unifies a war/peace, and the static functions definitely make this more readable. |
2 participants
This PR is a first pass at expanding the relationship API to make it easier to implement actual per turn deals.
I wanted to implement the Gold per turn deal, and I did, but in the process I found that I needed to alter the Relationship class to make it easier to work with. So I decided to do this first, kind of by itself.
So took this chance to also remove the boolean atWar, as I am not a huge fan of turning off and on properties to check if we are at war, so I 'd rather wrap this around a method that checks it.
Now we do this by checking if 2 civs have a relationship, but no multi turn deals, which in turn means that these 2 civs don't have a peace deal, and since they know each other, they are at war.
I have also found it quite confusing to work with stuff like p1.playerRelationships[p2].something so I wrapped what I could around a more (I think) comprehensive API that hides all the annoying things from the end user (us) and once they are set, we don't have to rack our brains everytime we use it.
That being said, it's a lot to cover, but I would love your feedback at this point (like that I overused the static keyword and microsoft will have me pay some kind of subscription to keep using it this month 😛)
I am going to try and write some unit tests for this as well, as it touches many areas, but after manually testing it with multiple scenarios, I would say most of it is ok.
Let me know what you think.