Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#4143 closed defect (fixed)

[PATCH] Last Man Standing / Diplomacy support for AI

Reported by: elexis Owned by:
Priority: Nice to Have Milestone: Alpha 22
Component: AI Keywords: patch
Cc: Patch:

Description

The new diplomacy mode "Last Man Standing" means that only one player can win, i.e. the game continues if a group of allies is left. The AI should declare war on one enemy then. Don't know if the AI has code to change the diplomacy state with unlocked teams in general, but a good strategy is to ally everyone against the strongest and if one is the strongest player, destroy the weakest or most dangerous, when some opportunity arises (backstabbing). For regicide mode the hero location is known when having an ally.

Attachments (5)

petra_lastmanstanding_v1.patch (8.1 KB) - added by Sandarac 3 years ago.
Petra will turn against the ally with the highest population when only allies remain.
petra_lastmanstanding_v1.1.patch (13.1 KB) - added by Sandarac 3 years ago.
Some fixes.
petra_lastmanstanding_v1.2.patch (13.8 KB) - added by Sandarac 3 years ago.
More fixes.
petra_lastmanstanding_v1.3.patch (13.6 KB) - added by Sandarac 3 years ago.
Use getEntities instead of getAllyEntities when calculating the amount of entities a non-enemy player has.
petra_lastmanstanding_v1.3_rebased.patch (15.7 KB) - added by elexis 3 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 3 years ago by elexis

In 18772:

Petra AI: Use ally instead of team chat, since it should eventually be able to use diplomacy. Based on patch by bb, refs #4143, #3102, #4069.

Changed 3 years ago by Sandarac

Petra will turn against the ally with the highest population when only allies remain.

comment:2 Changed 3 years ago by Sandarac

Keywords: rfc patch added
Milestone: BacklogAlpha 22
Summary: Last Man Standing / Diplomacy support for AI[PATCH] Last Man Standing / Diplomacy support for AI

comment:3 Changed 3 years ago by elexis

Cc: mimo added

comment:4 Changed 3 years ago by mimo

Thanks for the patch which looks globally good. Here are some comments

ChatHelper?: In the patch, the AI which breaks the alliance warns its allies, but we should also have a message from the AI whose alliance has been broken in case it has different allies: if A is allied with B and C, and B is allied with A and D, when A breaks its alliance with B, D will not be warned in the patch.

DiplomacyManager?, lines 108-110: no need to loop, which could result in multiple calls to lastManStandingCheck. You could use

if (events.DiplomacyChanged.length || events.PlayerDefeated.length)
this.lastManStandingCheck(gameState);

function lastmanstandingcheck: no need to count the number of units if there is no ally or already an enemy. You could use (after adding hasEnemies on the same model as hasAllies)

if (gameState.hasEnemies())
{
this.waitingToBetray = false;
return;
}
if (!gameState.hasAllies()) return;

just after the return when getAlliedVictory is false

and when needing to count them, you can use entitycollection.length

you could test if you want to betray and that the lapsetime is done before choosing your enemy to avoid useless steps

i don't understand the condition if (this.allyBetrayLapseTime != -1 && this.waitingToBetray) both conditions seem to be equivalent, and futhermore, if you arrive there this.waitingToBetray is necessarily true

and in the update function, would be clearer to test on this.waitingToBetray rather than this.allyBetrayLapseTime != -1

DiplomacyManager?.Deserialize: maybe replace by a loop now that there are more properties for (let key in data) this[key] = data[key];

Finally, you should also early return in lastmanstandingcheck when in ceasefire, and call this function when the ceasefire ends.

Changed 3 years ago by Sandarac

Some fixes.

comment:5 Changed 3 years ago by Sandarac

Thank you for the quick response mimo, this patch should address the remarks. I added hasEnemies and modified hasAllies to ignore gaia and defeated players, as otherwise there would need to be more loops in lastManStandingCheck. I know that the AI treats gaia as an enemy, especially in other areas of the code, but hasAllies is not used currently by Petra.

Also, I forgot to update the tests in the first patch, but they are in the new one.

comment:6 Changed 3 years ago by mimo

Thanks for the new patch.

I think my first previous comments (some allies of the player that has been turned to an enemy may not be warned) still hold.

I forgot the possibility of neutrals players in my previous comments: line 133 should become if (!gameState.hasAllies() && !gameState.hasNeutrals())

In the loop 156-167, turning against neutrals should have priority compared to turning against ally, even if less units than an ally.

line 172: the processing of the command may not be done on the next turn: maybe we should prevent any other betrayal during the 30s following a previous betrayal?

line 85 of headquarter: this initial call would be better in gameAnalysis (in startingStrategy.js). And calling lastmanstandingcheck here is a bit too specific. It would be better to call here a new more generic diplomacyManager function (i.e. diplomacyCheck) which would itself be:

m.DiplomacyManager.prototype.diplomacyCheck(gameState)
{
if (gameState.getAlliedVictory() || gameState.isCeasefireActive())
this.lastManStandingCheck(gameState);
}

in case we add different game types later.

In addition, something which is not part of this patch, but would be nice to add now or later:

  • describe in one or two sentences what's done in http://trac.wildfiregames.com/wiki/PetraBot
  • be more restrictive when giving tribute to an ally when lastmanstanding. That's done in the function tributes of the diplomacyManager. For example, do not give tribute to an ally which has more units, and when less units, scale the tribute by (1 - allyUnitCount/ourUnitCount) or whatever better idea.

PS i've not reviewed the changes on the tests

Changed 3 years ago by Sandarac

More fixes.

comment:7 in reply to:  6 ; Changed 3 years ago by Sandarac

The patch has been updated in response to the comments.

I think my first previous comments (some allies of the player that has been turned to an enemy may not be warned) still hold.

In v1.1 and v1.2 I removed the "/allies " string in chatNewDiplomacy so that all players will be notified (as they will all either be allies or neutral player up to that point)

line 172: the processing of the command may not be done on the next turn: maybe we should prevent any other betrayal during the 30s following a previous betrayal?

I am not sure that this is needed. In theory, if the AI is strong enough to turn against allies/neutral players, then it might not be necessary to set up another lapse timer like this.

In a "Last Man Standing" game, there should not really be a distinction between allies and neutral players, as the strongest at any one time should be considered the most dangerous. In any case, turning against neutral players will have higher priority in this patch.

comment:8 in reply to:  7 Changed 3 years ago by mimo

Replying to Sandarac:

The patch has been updated in response to the comments.

I think my first previous comments (some allies of the player that has been turned to an enemy may not be warned) still hold.

In v1.1 and v1.2 I removed the "/allies " string in chatNewDiplomacy so that all players will be notified (as they will all either be allies or neutral player up to that point)

Sorry, i didn't notice it. That's fine then

line 172: the processing of the command may not be done on the next turn: maybe we should prevent any other betrayal during the 30s following a previous betrayal?

I am not sure that this is needed. In theory, if the AI is strong enough to turn against allies/neutral players, then it might not be necessary to set up another lapse timer like this.

Not sure also, but there is a chance (although really small) that the player turn against two different allies in two consecutive turns if their respective number of units changed in between.

In a "Last Man Standing" game, there should not really be a distinction between allies and neutral players, as the strongest at any one time should be considered the most dangerous. In any case, turning against neutral players will have higher priority in this patch.

I proposed that because we may expect that our ally will help us against the ex-neutral while the opposite won't happen. So it may be better to start against neutrals, but that's not mandatory.

Then the code as it is would work, but it is a bit misleading to use gameState.getAllyEntities(i).length (new line 170) for neutrals. When we give a player argument, it is no more relevant if the player is an ally or not, we just want its entities. So instead of modifying getAllyEntities line 411, it would be better to modify getEntities line 378 (and possibly removing lines 438 and 441 after having changed the current calls to getEnemyEntities(i) to getEntities(i))

Changed 3 years ago by Sandarac

Use getEntities instead of getAllyEntities when calculating the amount of entities a non-enemy player has.

comment:9 Changed 3 years ago by mimo

Keywords: review added; rfc removed

The latest patch looks good to me, at least the part on the ai. Somebody should now review the test part, and commit it.

comment:10 Changed 3 years ago by elexis

  • Endgame Manager test:
    • the cmpPlayerManager variable is unused
    • we don't need to load Player.js nor PlayerManager.js, GUIInterface.js, we only test the EndGameManager.js component and mock the other components instead of using the code found in those scripts
    • We don't need to load interfaces/Player.js since IID_EndGameManager is the only interface called by our test (via EndGameManager.js)
    • Other than that the test is fine.
    • Stlye:
      let gameTypeSettings = {};
      gameTypeSettings.wonderDuration = wonderDuration * 60 * 1000;
      
      should become
      let gameTypeSettings = {
       "wonderDuration": wonderDuration * 60 * 1000
      
      as we are likely to extend in the future.
    • r14418 introduced CheckGameType to the EndGameManager?, but that is unused now, is redundant with GetGameType, thus should be removed entirely IMO
    • Since the purpose of the ticket is not to write a test for every situation of the EndGameManager? and since those 2 functions were not changed, the patch should be okay even if it doesn not test that MarkPlayerAsWon and AlliedVictoryCheck behave correctly.
  • Code stlye:
    • n my opinion we should avoid code and comments in the same line and capitalize the first letter of comments.
    • m.chatNewDiplomacy could use a ternary and nuke the variable + if.
    • getEntities has an unneeded "" + (unless JS surprises me)
    • Redundant comment:
          // wait a bit before turning 
          if (!this.waitingToBetray) 
      
  • The one committing the patch should make sure to either apply the patch with svn patch or do the svn add filename and svn propset svn:eol-style native filename manually to new files added to ensure that there will never be mixed windws and unix fileendings in those files.
  • We should add tickets for forming alliances and considering wonder victory if we don't do it in this patch.
  • I'd prefer if mimo commits this as long as we don't have anyone else qualified in that area. I didn't test (can do on request) nor review the AI part, but you can state that the tests were reviewed by me in the commit message if the unneeded lines are removed.

comment:11 Changed 3 years ago by elexis

In 18943:

Partial EndGameManager? test by Sandarac, refs #4143.

Changed 3 years ago by elexis

comment:12 Changed 3 years ago by mimo

Cc: mimo removed
Keywords: review removed
Resolution: fixed
Status: newclosed

fixed in r18945, thanks for the patch.

comment:13 Changed 3 years ago by elexis

In r18955:

petra: some more adjustements for lastManStanding games

Note: See TracTickets for help on using tickets.