#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)
Change History (18)
comment:1 by , 8 years ago
by , 7 years ago
Attachment: | petra_lastmanstanding_v1.patch added |
---|
Petra will turn against the ally with the highest population when only allies remain.
comment:2 by , 7 years ago
Keywords: | rfc patch added |
---|---|
Milestone: | Backlog → Alpha 22 |
Summary: | Last Man Standing / Diplomacy support for AI → [PATCH] Last Man Standing / Diplomacy support for AI |
comment:3 by , 7 years ago
Cc: | added |
---|
comment:4 by , 7 years ago
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.
comment:5 by , 7 years ago
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.
follow-up: 7 comment:6 by , 7 years ago
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
follow-up: 8 comment:7 by , 7 years ago
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 by , 7 years ago
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))
by , 7 years ago
Attachment: | petra_lastmanstanding_v1.3.patch added |
---|
Use getEntities instead of getAllyEntities when calculating the amount of entities a non-enemy player has.
comment:9 by , 7 years ago
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 by , 7 years ago
- Endgame Manager test:
- the
cmpPlayerManager
variable is unused - we don't need to load
Player.js
norPlayerManager.js
,GUIInterface.js
, we only test theEndGameManager.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
sinceIID_EndGameManager
is the only interface called by our test (viaEndGameManager.js
) - Other than that the test is fine.
- Stlye:
let gameTypeSettings = {}; gameTypeSettings.wonderDuration = wonderDuration * 60 * 1000;
should becomelet 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 withGetGameType
, 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
andAlliedVictoryCheck
behave correctly.
- the
- 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 thesvn add filename
andsvn 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.
by , 7 years ago
Attachment: | petra_lastmanstanding_v1.3_rebased.patch added |
---|
comment:12 by , 7 years ago
Cc: | removed |
---|---|
Keywords: | review removed |
Resolution: | → fixed |
Status: | new → closed |
fixed in r18945, thanks for the patch.
In 18772: