Page MenuHomeWildfire Games

Petra: diplomacyManager cleanup
ClosedPublic

Authored by Sandarac on May 31 2017, 5:38 PM.

Details

Summary

With the recent changes in diplomacy, it can happen that AI players can become neutral with each other in-game, but if these players defeat the last enemy player renaming,
then they will stay neutral indefinitely, and no player(s) will have "won" (regardless of the gamesetup option allied victory / last man standing option). There should be only an early return in lastManStandingCheck if AlliedVictory and if the player has allies. Also, there should be an early return when teams are locked.

While at it, rename diplomacyRequests to receivedDiplomacyRequests, to match with sentDiplomacyRequests.

Test Plan

Read the code to verify.

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.May 31 2017, 5:38 PM
mimo added a subscriber: mimo.May 31 2017, 6:06 PM

Looks good, there is nonetheless the inline comment which could be added

binaries/data/mods/public/simulation/ai/petra/diplomacyManager.js
203 ↗(On Diff #2331)

Shouldn't we test here as done above for the sentRequest:
if (requestType=="ally" && !isAlly) || (requestType="Neutral" && isEnemy)

Vulcan added a subscriber: Vulcan.May 31 2017, 7:11 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!
Checking XML files...

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

Sandarac added inline comments.May 31 2017, 7:33 PM
binaries/data/mods/public/simulation/ai/petra/diplomacyManager.js
203 ↗(On Diff #2331)

Okay done, I arranged the conditions like in the block above, in order to match.

Sandarac updated this revision to Diff 2338.May 31 2017, 7:52 PM

Fix typo in rP19194 in chathelper.js that throws errors.

mimo added inline comments.May 31 2017, 8:10 PM
binaries/data/mods/public/simulation/ai/petra/chatHelper.js
79 ↗(On Diff #2338)

nice catch :)

binaries/data/mods/public/simulation/ai/petra/diplomacyManager.js
56 ↗(On Diff #2338)

i'm now wondering if we should not have an early return in handleDiplomacyRequest when teams are locked? or protect here these calls in the init function. From a quick look, it seems that these calls here are the only one which could happen when team locked.

Sandarac updated this revision to Diff 2339.May 31 2017, 8:44 PM

Remove typo fix that was committed, add early return if teamsLocked.

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!
Checking XML files...

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

Sandarac added inline comments.May 31 2017, 8:47 PM
binaries/data/mods/public/simulation/ai/petra/diplomacyManager.js
56 ↗(On Diff #2338)

I think it would be better to have an early return in handleDiplomacyRequest, in case this function is used somewhere else.

mimo accepted this revision.May 31 2017, 9:07 PM
This revision is now accepted and ready to land.May 31 2017, 9:07 PM
This revision was automatically updated to reflect the committed changes.

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!
Checking XML files...

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