longturn / freeciv21

Develop your civilization from humble roots to a global empire
GNU General Public License v3.0
214 stars 39 forks source link

Add some protection measures for a players alliances #2266

Closed blabber closed 2 weeks ago

blabber commented 3 months ago

This is a follow-up PR for #2214 and #2239.

Closes #2177.

In games with complex alliance systems, declaring war can have unintended consequences. To protect the user from cancelling alliances by accident:

blabber commented 3 months ago

On Discord at least one voice was cast against these changes: https://discord.com/channels/378908274113904641/857286127165112370/1225396755789512715

In the previous PR, there was still a discussion about whether casus belli still had to be handled in a special way. This discussion was still ongoing and started here: https://github.com/longturn/freeciv21/pull/2239#issuecomment-2028815566

lmoureaux commented 3 months ago

I've likely missed the messages about this, sorry. What was the conclusion of the revert-or-fix discussion? This doesn't seem to be a revert, but a fix in a different way than #2239? Missing some context before reviewing

jwrober commented 3 months ago

As part of beta.1 blabber was kind enough to revert an earlier patch. He also closed the PR that was supposed to "fix" the patch. This PR includes the original + fixes.

I think we should look at setting up a game or something to test this out.

lmoureaux commented 3 months ago

Might be good for some LTX game?

jwrober commented 3 months ago

I was leaning more towards something small to do a really good edge case test of varying scenarios. Something short and to the point. Kind of like an autogame, but with humans.

lmoureaux commented 2 months ago

We could use Sketlux' Soviet Revolution scenario for that. It has many players that should naturally ally and many players that won't even meet.

jwrober commented 2 weeks ago

I've been playing with this on a local game off and on for the past few weeks. It seems to work as expected. Merging.