search

LoneGazebo / Community-Patch-DLL

Community Patch for Civilization V - Brave New World
Other
286 stars 158 forks source link

DealAI Rework #6883

Closed LoneGazebo closed 4 years ago

LoneGazebo commented 4 years ago

https://github.com/LoneGazebo/Community-Patch-DLL/commit/21e1ab112e954e3fdd903969036b09effdfb27c7

This version has a lot of changes to DealAI - I sanitized the terrible code as best I could, and cleaned up a lot of the logic and redundant code. I'd appreciate some extra eyes and tests on this version.

Iamblichos commented 4 years ago

I'll play around with this today.

RecursiveVision commented 4 years ago

@LoneGazebo Made a couple comments on your first commit, still looking through the dealAI changes and the second commit. Won't have that much to offer though, I can't test things while on vacation or read the code well either.

Am glad to see DealAI getting some much-needed cleanup though. :)

LoneGazebo commented 4 years ago

It was so bad.

RecursiveVision commented 4 years ago

@LoneGazebo Agreed.

I made comments on a couple of individual lines, as you know.

Additional observations:

  1. I recall a problem where the AI would say a deal was impossible even when there were valid deals it was willing to accept. Has that been resolved? It looks like it'll loop a few times to make checks now.

  2. Demand/help request logic could use revision, it's a total mess from what I remember, especially demand logic.

Issues with vassalage specifically:

  1. It's not added in DoAddItemsToUs/Them, nor is revoking it.

  2. AI has no function to inform the human player when they're willing to become a voluntary vassal (and I think vassal willingness logic in the diplo AI itself could use revision IMO, but I can't work on that right now). Many players on the forums have expressed a desire to be informed of when the AI is willing to become a vassal.

Same goes for revoking vassalage - and perhaps the AI should be making offers to vassalize other players, or for the other player to revoke their vassals.

Probably a fair amount of work to implement, but vassals are arguably a core part of the mod now and I think the players would appreciate it.

  1. It would be nice if you could force a defeated civ with vassals to capitulate - their vassals could either be freed or become the conqueror's vassals. Having to go to war twice and wait is just a pain.
LoneGazebo commented 4 years ago

I saw your posts, I responded to most/all.

  1. I hope so. A big part of the issue is that the AI would bork deals on their own and then complain that the deal was unfair.
  2. It's a bit better now.

Vassals: That's a bigger todo- voluntary vassalage is tricky, it can be really swingy.

LoneGazebo commented 4 years ago

Alright, vassalage stuff is integrated. I'll test.

RecursiveVision commented 4 years ago

Awesome!

Iamblichos commented 4 years ago

Peace deals in the trade screen are working better. I once got a completely empty trade proposal.

LoneGazebo commented 4 years ago

@Iamblichos really? interesting.

LoneGazebo commented 4 years ago

Do you know what the trade context was, possibly?

Iamblichos commented 4 years ago

I thinking he was trying to buy a luxury. Is this in the logs?

LoneGazebo commented 4 years ago

Should be in logs

Iamblichos commented 4 years ago

Next time I get it, I'll check the logs. Should be fairly reproducible again.

RecursiveVision commented 4 years ago

Happy to see voluntary vassalage be included. :)

RecursiveVision commented 4 years ago

And the code is a LOT easier to read now.

LoneGazebo commented 4 years ago

Yeah, happy with the changes overall. The renewal system is a wee bit of kludge but it works.