Page MenuHomeWildfire Games

Do not reset the capture the relic countdown if a non-allied player changes diplomacy
ClosedPublic

Authored by Sandarac on Apr 9 2017, 9:35 AM.

Details

Summary

The capture the relic countdown should only be reset if a player on the team-to-win tries to switch diplomacy with another player.

This should fix the concern raised in rP19345.

Test Plan

Capture all relics, switch the perspective to a player you are not allied with, and change the diplomacy with any other player. The countdown will not reset.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Sandarac created this revision.Apr 9 2017, 9:35 AM
Vulcan added a subscriber: Vulcan.Apr 9 2017, 10:54 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/719/ for more details.

elexis requested changes to this revision.Apr 11 2017, 4:12 AM

Ok the question is, what behavior do we intend in diplomacy mode?

What if another player joins the team that owns all relics (mutually)? The timer might be reset. [That's the case]
What if a player of the team that owns all relics leaves? The timer might continue. [Not the case, but okay too]

Notice it's also reset if we change the alliance from ally to ally. Those should be fixed in the Player component, or Commands.js if that isn't possible, probably in a separate diff. IIRC there was an old rejected GUI patch on trac for the diplomacy manager.

I noticed one bug though. First wololo-captured relics and setup alliance, then broke the alliance and set it back again, but the timer didn't start again.

This revision now requires changes to proceed.Apr 11 2017, 4:12 AM
In D305#12392, @elexis wrote:

Ok the question is, what behavior do we intend in diplomacy mode?

What if another player joins the team that owns all relics (mutually)? The timer might be reset. [That's the case]
What if a player of the team that owns all relics leaves? The timer might continue. [Not the case, but okay too]

I think it makes sense to reset the timer any time a player on the team-to-win tries to change their diplomacy (as is done in the patch), as changing the diplomacy causes the original "team" (that started the countdown) to no longer exist (from my point of view).

Notice it's also reset if we change the alliance from ally to ally. Those should be fixed in the Player component, or Commands.js if that isn't possible, probably in a separate diff. IIRC there was an old rejected GUI patch on trac for the diplomacy manager.

I believe this is #3198, and yes, these useless DiplomacyChanged messages that are sent are a real pain (they were also really annoying to deal with when implementing diplomacy requests for Petra). An attempt to deal with them in this patch would just be a hack, and as you said, they should be fixed/handled elsewhere.

I noticed one bug though. First wololo-captured relics and setup alliance, then broke the alliance and set it back again, but the timer didn't start again.

Yes, || !this.relicsVictoryCountdownPlayers.length was missing in line 107 (needed to handle cases like this). But there are still cases when a non-allied player switches their diplomacy with a player on the team-to-win to a negative state (i.e. neutral to enemy), which causes the other player to switch diplomacy as well because of the automatic "worsening of relations" in SetDiplomacyIndex() in Player.js. So this can also cause the countdown to reset.

elexis updated the Trac tickets for this revision.Apr 21 2017, 4:31 PM
Sandarac updated this revision to Diff 1617.May 3 2017, 10:56 PM
Sandarac edited edge metadata.

Handle cases when neutral players become enemies.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/980/ for more details.

Sandarac updated this revision to Diff 1670.May 5 2017, 10:48 PM

Update after rP19518.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1023/ for more details.

Sandarac planned changes to this revision.May 7 2017, 4:31 AM

@elexis believes that the timer shouldn't be reset when a player on the team-to-win changes their diplomacy with some other player from neutral to enemy - that makes sense. But what if player A is allied with player B, but player B has their diplomacy stance with player A set to enemy, and then later, when player A's team starts the countdown, B switches to become allies with A? Should the countdown reset then? Maybe not, but it certainly comes down to a design decision.

Another possibility is to save all mutual allies when the countdown starts, and then only reset the timer if one of these saved players is no longer an ally with any other of the saved players (regardless of any players that later "join" the team). In this case then only the saved players would be marked as won. This approach may be misleading.

Sandarac updated this revision to Diff 1747.May 8 2017, 1:56 AM

Reset only if the original team of mutual allies is altered, removing the need to modify the player component.

Vulcan added a comment.May 8 2017, 5:02 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1077/ for more details.

elexis accepted this revision.Jun 17 2017, 2:58 PM

From testing:
The timer is reset if a relic becomes destroyed while the teams are still the same.

The timer is not reset if one of the teamplayers adds a new mutual ally.
That new ally will marked as a winner only if all of other allies that own relics have it mutually allied.

(This effect isn't clear from the string (Player1's team will have won in T and Capture all relics spread across the map and keep them for a certain time to win the game.).
Maybe it should mentioned exactly the players that are stored in the timer, no matter what logic determined those.)

From reading the code:
Since MarkPlayerAsWon is called for the lowest playerID whose mutual allies have captured all relics, we get a different result depending on the playerID.
This seems wrong since it shouldn't matter which playerID a player is assigned to.

i.e. if 1 is mutually allied with 2-7 all of them (i.e. 1-7) will win,
but if 3 had captured all relics and built up an allience with only 1, still 1-7 will win.

(i.e. if we made a graph where the vertices are players and the edges mutual alliances, then we check for a star in the current code, but should probably look for a subgraph that is complete).

Wonder victory doesn't have this issue since there is only one wonder, so checking for the mutual allies of that player is easier to grasp.

Patch still seems a better behavior than before, so I'd be ok with committing it.

binaries/data/mods/public/maps/scripts/CaptureTheRelic.js
90 ↗(On Diff #1747)

The added comment doesn't match the actual result unless we change the last player to ally.

108 ↗(On Diff #1747)

The data check seems unneeded. If theres a malformed diplomacy message, that is an error and having it error out here would seem fine.
If we want to keep the check (for diplomacy changes without diplomacy change?) maybe this check would have to be done only once instead of in a loop.

This revision is now accepted and ready to land.Jun 17 2017, 2:58 PM

Incorrectness:
Two replays that show that having a lower playerID helps with getting marked as a winner of the game:

Player 1 is only mutual ally with 2. Player 2 is mutual ally with everyone but 8. Player 1 captures the relic and only 1+2 win.

Player 2 is only mutual ally with 1. Player 1 is mutual ally with everyone but 8. Player 2 captures the relic and everyone but 8 will be marked as winners (even if 2 had fought the entire game against player 3 to 7).

Correctness:
The timer is reset the timer if one of the original allies changed the diplomatic relation.
The timer is not reset if a player that wasn't one of the original allies changed the diplomatic relation.
The timer is (reset and) started if a player has all relic owners mutually allied and if there wasn't a timer yet.
Since the diplomacy behavior itself is what we want to achieve, the patch is correct and complete. The playerID dependency can be considered an independent issue.

binaries/data/mods/public/maps/scripts/CaptureTheRelic.js
108 ↗(On Diff #1747)

Removing the !data check, none of the other subscription I had looked at has it and I'd be happy to show modders an error if there is a malformed diplomacy message, so that message can be fixed

elexis added inline comments.Jun 22 2017, 12:45 PM
binaries/data/mods/public/maps/scripts/CaptureTheRelic.js
90 ↗(On Diff #1747)

Replacing team-to-win with "original allies", since a player can be part of the team-to-win without being one of the original allies and it only considers the original allies changing the diplomatic relation.

elexis added inline comments.Jun 22 2017, 12:59 PM
binaries/data/mods/public/maps/scripts/CaptureTheRelic.js
108 ↗(On Diff #1747)

Ah well, the ownership sent data always exists, but this function is also called from CheckCaptureTheRelicVictory upon capturing without data, so nvm.

This revision was automatically updated to reflect the committed changes.