Add support for one-sided alliances #1680

kubaau · 2024-07-23T12:08:15Z

Adds supported for one-sided alliances exactly (I hope) how they worked in S2. This makes it possible to achieve the same alliance logic as in the campaigns designed for S2 (tested with the Roman campaign and the FANpaign). FANpaign in particular makes almost no sense to play without such alliances being possible. See video1 and video2 for a short demonstration.

Adds an addon for catapults to attack allies, which was the case in S2 (defaults to disabled in RttR). This also makes old campaigns more viable since many scenarios rely on a slow catapult creep without starting a war.

These should probably be split into 2 PR's, but I am modifying campaign scripts in both so I worked on one branch and the addon does not make much sense without modifying campaign alliances.

Related to issues #743 and #1177

Spikeone · 2024-07-25T14:54:54Z

Great work! Just two questions: Does this work in multiplayer? Is it possible to break such pact via diplomacy window as well?

kubaau · 2024-07-26T22:02:31Z

Great work! Just two questions: Does this work in multiplayer? Is it possible to break such pact via diplomacy window as well?

This type of pact does not appear in the diplomacy window, so you cannot suggest or break such an alliance this way.

I was designing this with only single player in mind, thus the human can always attack (even their allies), as it was in S2. As it stands the pact does not make sense in multiplayer unless AIs are also involved and it can only be made through scripts. I think it could be made to work with multiplayer by adding one additional if statement in bool GamePlayer::CanAttack(const unsigned char otherPlayerId) const, I just didn't think much about multiplayer other than disallowing suggesting such a pact.

Flamefire

This might be a bit harder than it looks. Especially the definition of "allied to" isn't that clear any more. We might need to check where and how this is used.

Flamefire · 2024-07-30T12:50:31Z

libs/s25main/ai/AIInterface.h

-    /// Test whether a player is attackable or not (alliances, etc)
-    bool IsPlayerAttackable(unsigned char playerID) const { return player_.IsAttackable(playerID); }
+    /// Can the current player (AI) attack the other player?
+    bool CanAttack(unsigned char otherPlayerId) const { return gwb.GetPlayer(GetPlayerId()).CanAttack(otherPlayerId); }


Why the extra call?

Suggested change

bool CanAttack(unsigned char otherPlayerId) const { return gwb.GetPlayer(GetPlayerId()).CanAttack(otherPlayerId); }

bool CanAttack(unsigned char otherPlayerId) const { return player_.CanAttack(otherPlayerId); }

Flamefire · 2024-07-30T12:54:39Z

libs/s25main/buildings/nobBaseMilitary.h

@@ -121,7 +121,7 @@ class nobBaseMilitary : public noBuilding
    bool IsUnderAttack() const { return !aggressors.empty(); };

    /// Return whether this building can be attacked by the given player.
-    virtual bool IsAttackable(unsigned playerIdx) const;
+    virtual bool CanBeAttackedBy(unsigned playerIdx) const;


I like having such getters start with get or is. Is IsAttackableBy better? Avoids another "fill-word" there.
I do like the addition of "by" though.

Flamefire · 2024-07-30T12:55:37Z

libs/s25main/figures/nofAttacker.cpp

@@ -673,7 +673,7 @@ void nofAttacker::OrderAggressiveDefender()
            continue;
        // We only send a defender if we are allied with the attacked player and can attack the attacker (no pact etc)
        GamePlayer& bldOwner = world->GetPlayer(bldOwnerId);
-        if(bldOwner.IsAlly(attacked_goal->GetPlayer()) && bldOwner.IsAttackable(player))
+        if(bldOwner.IsAlly(attacked_goal->GetPlayer()) && world->GetPlayer(bldOwnerId).CanAttack(player))


Suggested change

if(bldOwner.IsAlly(attacked_goal->GetPlayer()) && world->GetPlayer(bldOwnerId).CanAttack(player))

if(bldOwner.IsAlly(attacked_goal->GetPlayer()) && bldOwner.CanAttack(player))

Flamefire · 2024-07-30T12:56:33Z

doc/lua/functions.md

+    - P1 cannot attack P2
+- if P1 attacks P2 `or` if P1 breaks their alliance towards P2, `then`
+    - P2 breaks their alliance towards P1 `and`
+    - all players who are allied to P2 `and` to whom P2 is allied to break their alliance towards P1


Currently only one-sided alliances are broken. What is the intention?

Flamefire · 2024-07-30T13:03:45Z

libs/s25main/GamePlayer.cpp

+        if(!IsAlly(i))
+            continue;
+
+        auto& player = world.GetPlayer(i);
+
+        // skip if they are not allied to us
+        if(!player.IsAlly(thisPlayerId))


I'm not sure I understand that. In which case can we be allied to the other player but not the other way round?

maybe instead of commenting on the lines (e.g. "skip ourselves" is obvious from the code, similar for others) extend the comment above the loop. It looks like: "Break one-sided alliances from all our allies to the attacking player"

And maybe the loop can be even simpler: if(allied) -> reset one-sided pact to attacker
--> No need to check for self or do it above the loop as we are allied to ourselves, hence also removing any "continue"

Flamefire · 2024-07-30T14:36:09Z

libs/s25main/world/GameWorld.cpp

+        {
+            GetLua().EventAttack(player_attacker, attacked_building.GetPlayer(), counter);
+        }


Suggested change

{

GetLua().EventAttack(player_attacker, attacked_building.GetPlayer(), counter);

}

GetLua().EventAttack(player_attacker, attacked_building.GetPlayer(), counter);

Flamefire · 2024-07-30T14:43:55Z

tests/s25Main/integration/testPacts.cpp

+BOOST_FIXTURE_TEST_CASE(OneSidedAlliances_AICanAttackAnyoneByDefault, EmptyWorldFixture2P)
+{
+    auto& p0 = world.GetPlayer(0);
+    auto& p1 = world.GetPlayer(1);
+    p0.ps = PlayerState::AI;
+    p1.ps = PlayerState::AI;
+
+    BOOST_TEST_REQUIRE(p0.CanAttack(1) == true);
+    BOOST_TEST_REQUIRE(p1.CanAttack(0) == true);
+}
+
+BOOST_FIXTURE_TEST_CASE(OneSidedAlliances_AICannotAttackAllies, EmptyWorldFixture2P)
+{
+    auto& p0 = world.GetPlayer(0);
+    auto& p1 = world.GetPlayer(1);
+    p0.ps = PlayerState::AI;
+    p1.ps = PlayerState::AI;
+
+    p0.MakeOneSidedAllianceTo(1);
+
+    BOOST_TEST_REQUIRE(p0.CanAttack(1) == false);
+    BOOST_TEST_REQUIRE(p1.CanAttack(0) == true);
+}
+
+BOOST_FIXTURE_TEST_CASE(OneSidedAlliances_HumansCanAttackAnyone, EmptyWorldFixture2P)
+{
+    auto& p0 = world.GetPlayer(0);
+    auto& p1 = world.GetPlayer(1);
+    p1.ps = PlayerState::AI;
+
+    p0.MakeOneSidedAllianceTo(1);
+    p1.MakeOneSidedAllianceTo(0);
+
+    BOOST_TEST_REQUIRE(p0.CanAttack(1) == true);
+    BOOST_TEST_REQUIRE(p1.CanAttack(0) == false);
+}


Those 3 can be combined, i.e.

AI can attack

after one-sided alliance it cannot

If it was a human it can attack AI (and humans), but not the other way round

This would more clearly highlight the differences

Flamefire · 2024-07-30T14:45:29Z

tests/s25Main/integration/testPacts.cpp

+    BOOST_TEST_REQUIRE(p0.CanAttack(1) == true);
+}
+
+BOOST_FIXTURE_TEST_CASE(OneSidedAlliances_AllianceIsBrokenBothWaysWhenAttacked, EmptyWorldFixture2P)


This includes the previous test, doesn't it? So OneSidedAlliances_AllianceIsBrokenWhenAttacked is not needed IMO

Flamefire · 2024-07-30T14:49:11Z

tests/s25Main/integration/testPacts.cpp

+    BOOST_TEST_REQUIRE(p2.CanAttack(1) == true);
+}
+
+BOOST_FIXTURE_TEST_CASE(OneSidedAlliances_BeingAttacked_DoesNotAlsoBreakAlliancesForPlayers_WhoAreNotOurAlliedToUs,


Similar here: It includes the former, so maybe rename to OneSidedAlliances_BeingAttacked_BreaksAlliancesForAlliedPlayers and extend the comments why which alliance is broken/not broken at the check

Flamefire · 2024-07-30T15:00:16Z

tests/s25Main/integration/testPacts.cpp

+    BOOST_TEST_REQUIRE(p2.CanAttack(1) == true);
+}
+
+BOOST_FIXTURE_TEST_CASE(OneSidedAlliances_BeingAttacked_DoesNotAlsoBreakAlliancesForPlayers_WhoWeAreNotAlliedTo,


I guess this needs resolving the definition of "allied to".

Say player X attacks player Y. Then of course player Y breaks the pact to player X.

And I guess every player Z allied to player Y should also break the pact to player X. Only question is whether this Z->Y should include one-sided pacts and if so which way: Z->Y, Y->Z, both or either. I would argue only Z->Y should be checked as Z "doesn't know" about Y->Z so why should it care about Y being attacked in that case?

Might also be worth merging with the above test case to focus on the differences and add the "DoesNotAlsoBreakAlliancesForPlayers_WhoWeAreNotAlliedTo" as a comment instead

kubaau added 2 commits July 19, 2024 22:02

Add support for one-sided alliances

d1c75cd

Addon: catapults attack allies

2d42eb6

Flamefire requested changes Jul 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for one-sided alliances #1680

Add support for one-sided alliances #1680

kubaau commented Jul 23, 2024

Spikeone commented Jul 25, 2024

kubaau commented Jul 26, 2024 •

edited

Loading

Flamefire left a comment

Flamefire Jul 30, 2024

Flamefire Jul 30, 2024

Flamefire Jul 30, 2024

Flamefire Jul 30, 2024

Flamefire Jul 30, 2024

Flamefire Jul 30, 2024

Flamefire Jul 30, 2024

Flamefire Jul 30, 2024

Flamefire Jul 30, 2024

Flamefire Jul 30, 2024

	bool CanAttack(unsigned char otherPlayerId) const { return gwb.GetPlayer(GetPlayerId()).CanAttack(otherPlayerId); }
	bool CanAttack(unsigned char otherPlayerId) const { return player_.CanAttack(otherPlayerId); }

	if(bldOwner.IsAlly(attacked_goal->GetPlayer()) && world->GetPlayer(bldOwnerId).CanAttack(player))
	if(bldOwner.IsAlly(attacked_goal->GetPlayer()) && bldOwner.CanAttack(player))

Add support for one-sided alliances #1680

Are you sure you want to change the base?

Add support for one-sided alliances #1680

Conversation

kubaau commented Jul 23, 2024

Spikeone commented Jul 25, 2024

kubaau commented Jul 26, 2024 • edited Loading

Flamefire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubaau commented Jul 26, 2024 •

edited

Loading