#3970 closed enhancement (fixed)
[PATCH] Last man standing - gamesetup option to disable allied victory
Reported by: | elexis | Owned by: | elexis |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 21 |
Component: | UI & Simulation | Keywords: | patch |
Cc: | Patch: |
Description
Diplomacy games (teams not locked) would become more entertaining if it were possible to disable the allied-victory mechanism.
This would result in a "last man standing" gametype, forcing players to break the alliance in order to win. Players were incentivized to ally the weaker players, in order to counterbalance the player who is most likely to win (instead of allying that one and be done with the game while making it even harder to beat the strongest).
It should be implemented as a separate boolean option, not a victory condition. This way the mode can be played with all gametypes like wonder-games.
As players might assume allying all players means having ended the game, it might be an idea to show a messagebox that the game isn't over yet.
If teams are set to locked, the gamesetup checkbox should be hidden and the allied-victory mechanism must be enabled.
The EndGameManager.js
has a method to disable allied-victory already:
// Allied victory means allied players can win if victory conditions are met for each of them // Would be false for a "last man standing" game (when diplomacy is fully implemented)
The feature should be possible to implement with changing probably less than 5 files. Feel free to ask on IRC for details (@wiki:GettingStartedProgrammers#QuestionsandSuggestions)!
Attachments (4)
Change History (17)
by , 8 years ago
Attachment: | lastmanstanding.patch added |
---|
comment:1 by , 8 years ago
Keywords: | review patch added |
---|---|
Milestone: | Backlog → Alpha 21 |
Summary: | Last man standing - gamesetup option to disable allied victory → [PATCH] Last man standing - gamesetup option to disable allied victory |
comment:2 by , 8 years ago
Thanks, it's a must have for alpha 21 in my opinion! Didn't test the patch yet, logic looks good from reading
Logic:
- What should happen if only allies are remaining? Nothing? I see players either wondering why the game didn't end yet or assuming that all of them won. We could show the notification as long as all are allied to reminding them. (Setting diplomacies back to neutral or enemy might be a bit too harsh).
Codestyle remarks:
- Either use
LastManStanding
ordisableAlliedVictory
, but not both (and also notLastMan
). ThoseoptionFoo
things should likely be consistent with the controls it contains (so we haveoptionFoo
which containsfoo
andfooText
). Should probably beLastManStanding
as that's the string that would also be displayed to the user. - The first part of the tooltip
Toggle the "Last Man Standing" option (only one player can win).
is redundant (it's obvious that it triggers that option since it's called like that). So only mention the non-obvious things that it does. when diplomacy is fully implemented
-> it is as far as I know, so nuke that[1,2,3,4,5,6,7,8]
-> oh no, you don't. If you need that, construct it usingg_MaxPlayers
(#4004), see alsoplayersArray
ingamesetup.js
. But it also doesn't take observers into account, so maybe you're just better off not passing the players argument?Last civilization remaining wins
-> it's the player that wins (there might be 2 players with the same civ). Not a big issue, but for wonder-games the first player to build a wonder wins. (That message is only shown for some seconds after the gamestart?)- L112 unneeded parenthesis as
&&
has a higher operator precedence than||
(just like*
and+
): https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence You could also add a linebreak after the second||
by , 8 years ago
Attachment: | lastmanstanding_v2.patch added |
---|
All code in EndGameManager.js is called in OnGlobalPlayerDefeated()
comment:3 by , 8 years ago
Keywords: | review removed |
---|
- Trailing whitespace in the first changed XML line
optionLastManStanding
should be hidden if teams are locked- "Last Man Standing: No" seems to be counterintuitive in comparison to "Last Man Standing: Yes" and "Allied Victory: Yes", this could only be solved by using a dropdown with two choices "Last Man Standing" and "Allied Victory". But as we don't use dropdowns for binary choices, we'll have to go with only one title, so it's okay this way I guess.
delete g_GameAttributes.settings.LastManStanding;
that line totally needs to be wrapped in braces, otherwise it's not part of the if-statement.!this.alliedVictory && cmpPlayers.filter(pData => pData.GetState() == "active").length != 1)
-> use 4 spaces instead of a tab so it's right below that other condition, regardless of how the tabs are displayed- When starting the match with allies only, I don't get a notification. Neither do I get one when changing the diplomacies so that all are allied.
- Ideally the notification would be shown as long as all players are allied, as 5 seconds are over fast and people often don't read the notification in that time frame. You can probably do it the way the wonder victory does it (
AddTimeNotification
with maybe a 90 minute limit, not showing the time in that string and then callingDeleteTimeNotification
)
comment:4 by , 8 years ago
Any updates? You can identify diplomacy changes in OnDiplomacyChanged
(maybe OnGlobalDiplomacyChanged
) and then just call the same code.
You could add a function to the EndGameManager
that does the onlyAlliesLeft
check and adding of a timeNotification and call that from the DiplomacyChanged / PlayerDefeated events.
by , 8 years ago
Attachment: | lastmanstanding_v3.patch added |
---|
Thank you for the tips, this should do it all, but the code could be cleaner
comment:5 by , 8 years ago
Keywords: | review added |
---|
comment:6 by , 8 years ago
Keywords: | review removed |
---|
Just wanted to make sure you didn't abort ;)
You are right, few more cleaning and the code should be done:
- gamesetup.js L513 newline
Toggle if only one player can win (disables allied victory).
: "allied victory" seems a bit technical, maybe `Toggle whether the last remaining player or the last remaining set of allies wins" ?
About DisableAlliedVictoryCheck
:
- Rename
DisableAlliedVictoryCheck
toLastManStandingCheck
- Any reason for not using
this
instead ofcmpEndGameManager
? - Add an early return
!cmpGuiInterface || !cmpPlayerManager
- Add an 'early return' function
this.alliedVictory
(and also delete the notification, so that the code supports changing the setting in a running game, although that is not being used yet) - That
i == msg.playerId
check seems wrong, since you call the function also for the diplomacy changed message. Is that check needed for the defeat message actually or is cmpPlayers[i].GetState() == "active"already the case
OnGlobalPlayerDefat`? (In that case you could just delete it) - Don't copy code. Reuse it by moving the shared part to a function. For example you could add a function
OnlyAlliesLeft
if you need that check in multiple places (it should return false or thatallies
array). - I would say merge L111 L112, but L112 is actually not needed if you take a look at
GUIInterface.js
- L106 newline (same goes for L104)
- L107 following, wrong indentation
10 * 60 * 9000
-> that was split because it ought to be readable, if you want 90min, make that90 * 60 * 1000
instead (minutes * seconds/minute * milliseconds/second)messages
is not needed, just accesscmpEndGameManager.disabledAlliedVictoryMessages
cmpEndGameManager.disabledAlliedVictoryMessages
will never contain anything more thannoAlliesLeftMessage
, so just usecmpEndGameManager.noAlliesLeftMessage
Last player remaining wins
-> add an exclamation mark or period- Unneeded variable
numPlayers
OnGlobalPlayerDefeated
should just call theLastManStandingCheck
instead of introducing any duplicate lines- Since both the
LastManStandingCheck
andOnGlobalPlayerDefeated
need theallies
array, you could just doLastManStandingCheck(OnlyAlliesLeft()))
in the init and diplomacy-changed function andlet allies = OnlyAlliesLeft(); LastManStandingCheck(allies);
and then theSetWon
part.
by , 8 years ago
Attachment: | lastmanstanding_v4.patch added |
---|
comment:7 by , 8 years ago
Keywords: | review added |
---|
comment:9 by , 8 years ago
Keywords: | simple review removed |
---|
Thanks for the patch!
I had to change few things though, mostly style issues but also a bug in the simulation (it doesn't mark players as won if the game is started with allies only).
gamesetup.js
!this.checked ? this.checked : !this.checked
is equal to!this.checked
However I set it tofalse
instead, so it's unchecked by default when disabling locked teams.- Moved that
optionLastManStanding
to theoptionWonderDuration.hidden
part
gamesetup.xml
- The
hideMoreOptions
button also needs to be moved 30 pixels down (though that error is corrected byresizeMoreOptionsWindow
anyway) Apparently #3234 also forgot that.
EndGameManager.js
:
- let instead of var
- added those functions to the protoype, so it's consistent and uses
this
instead ofcmpEndGameManager
noAlliesLeftMessage
->lastManStandingMessage
- Removed the two
alliedVictory
checks since we already have that early return ensuring the value above, thus end up with if-else, - moved the
DeleteTimeNotification
out of the if-statement since it's done in both cases - fixed indentation of
AddTimeNotification
- Removed unused
cmpPlayerManager
fromLastManStandingCheck
- Not really needed to have an array
cmpPlayers
, just usingcmpPlayer
(after swappingallies[0]
andi
when checking forIsMutualAlly
) - The
i == msg.playerId
check is still wrong as you passmsg.playerId
also if that player changed the diplomacy but wasn't defeated. It is also not needed for checking the defeated message as the playerstate is already "defeated" once the message arrives (found by testing) - Removed unused variable
onlyAlliesLeft
- Using
cmpPlayerManager.GetNumPlayers()
directly to get rid ofnumPlayers
which is only used once Last player remaining wins
->Last remaining player wins
- Actually there is a bug with the current code - it should also do the
LastManStandingCheck
test on diplomacy change and on init too, otherwise it would wait until a player has resigned to decide whether the game is won due to only allies remaining. Thus theOnlyAlliesLeft
can be merged withLastManStandingCheck
(Yeah, could have noticed that before)
- Furthermore for clarity I renamed "skip" to "skipAlliedVictoryCheck"
- Added
lastManStandingMessage
toInit
. not strictly required but I think it's nice to have all variables used defined there. - That
cmpPlayers.filter
loops over the players while you just did a whole loop before (inOnlyAlliesLeft
in your patch). That check should be equivalent toallies.length == 1
.
Adds check box in More Options