#1950 closed defect (fixed)
[PATCH] Multiplayer pausing does not work as expected
Reported by: | Josh | Owned by: | echotangoecho |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 21 |
Component: | Network | Keywords: | patch |
Cc: | Patch: |
Description
Current Behavior: When any player in a multi-player game pauses their instance, all other player simulations freeze while rendering continues without player notification.
Expected Behavior: When any player in a multi-player game pauses their instance, all other players should enter the same simulation, rendering and UI state.
Attachments (8)
Change History (36)
comment:1 by , 11 years ago
comment:2 by , 11 years ago
Milestone: | Alpha 14 → Backlog |
---|
comment:3 by , 10 years ago
Personally, I think there should be a voting decided whether to pause or not. This is because I have played a few games where the losers just paused the game near the end so they wouldn't lose.
This could be done through voting commands in the chat.
Also when it does pause, a message should free displayed saying that the game has been paused.
comment:4 by , 9 years ago
In many many games I've experienced a lot of trolls, but never that someone pauses the game to avoid losing the game. Nor did I see someone otherwise abusing the pause function.
Sometimes when players disconnect it takes up to 30 seconds for the timeout to complete (see ENET_PEER_TIMEOUT_MAXIMUM = 30000
). In that case the game appears to be paused for that time.
A voting system might still be useful. See #2512 for a voting system ticket. I only use the pause button when I get a call or other time critical events happen. In that case I cannot wait for other players to participate in a vote for pausing.
But I agree that the game should display if someone has paused. If a client hits the pause button, then he (or the server) should broadcast a message to all clients which in return display a chat notification informing the players about the pause!
by , 9 years ago
Attachment: | t1950_show_who_paused_v1.patch added |
---|
Displays a chat notification if a player pauses or unpauses the game.
comment:5 by , 9 years ago
Keywords: | patch review added; multiplayer simulation pause freeze removed |
---|---|
Milestone: | Backlog → Alpha 19 |
Priority: | Must Have → Should Have |
Summary: | Multiplayer pausing does not work as expected → [PATCH] Multiplayer pausing does not work as expected |
The patch above displays a chat notification each time a player uses the pause / unpause function.
If a client uses that button, he sends a (enet) net message to the server which then broadcasts it to all clients. When the clients receive that message, they display the chat notification.
TODO: Displaying a chat message is not enough, since that message is displayed only for a short period. The information should be displayed for the whole duration of the pause.
Question: Should the pauseOverlay
be displayed while the game is paused for other clients too? It has the advantage that everyone is informed about the state of the game. It also has a place to show the player(s) which paused the game.
The downside would be that you cant
(a) issue new orders while waiting
(b) hover over units to see their health
(c) see all colors (and identify friends and foes) easily since everything is greyed
An alternative approach would be to use AddTimeNotification
(as in ceasefire #2749 or wonder victory message). It displays a text notification in the middle of the screen for the duration of the pause.
Question: What about the voting system? Done in a different ticket?
comment:6 by , 9 years ago
Keywords: | review removed |
---|
This is not how it should work.
We'd like everyone to see the same screen, like 'Game Paused', but in MP it would have an indication about who paused. Players should be able to chat still.
Adding a maximum pause time (5min?) would be great. Multiple pauses would probably not be difficult to implement and would be really nice too.
comment:7 by , 9 years ago
Milestone: | Alpha 19 → Alpha 20 |
---|
comment:8 by , 8 years ago
Milestone: | Alpha 20 → Backlog |
---|
comment:9 by , 8 years ago
Owner: | set to |
---|
comment:10 by , 8 years ago
A patch for this ticket should also in functions_global_object.js
remove:
// TODO: Players who paused the game should be added here
(That's the place I'd prefer for showing them, but that's open to debate).
by , 8 years ago
Attachment: | pausing.patch added |
---|
comment:11 by , 8 years ago
Keywords: | review added |
---|---|
Milestone: | Backlog → Alpha 21 |
Patch allows to see who paused and adds an overlay for every player similar to the overlay for players who paused. We should still decide how players should be notified if another player paused.
follow-up: 14 comment:13 by , 8 years ago
Keywords: | review removed |
---|
Thanks for the patch and the work you have put into this so far! This is a very important ticket to solve. Some edge cases need to be fixed in your patch:
- Add yourself to programming.json.
Logic:
- The code doesn't look like it handles rejoining clients.
- There is only a transition for NCS_INGAME. This means if a client is authenticating, loading or synchronizing while someone else has paused, it will trigger one of those (rather harmless) FSM errors (#3199, r18117). You shouldn't broadcast these messages but send them only to the clients that are actually in the right state (ingame, maybe others).
- Optimization: If a client leaves, then the server doesn't need to send the unpause-message, as the others could derive that from the disconnect itself. Furthermore sending information about a client that isn't connected seems wrong, as the GUID is only valid for the period the client is connected (new one picked on every connected).
GUI:
- If a player pauses the game, it would be nice if he could still see who else paused it. (Use case: someone paused for a long time, you pause too to make sure they dont unpause before you're ready, but you would resume if the other guy is back too). You should probably just hide the "click to unpause" label if the player itself didn't pause and not hide the thing onclick if someone else has paused as well.
- Why was
resumeGame()
changed?
Style:
- delete
PauseLocalSimulation
and use the existingSetPaused
. Maybe you can add a functionMultiplayerPause
that calls that function and sends that message.
- Are those
LOGERROR
messages really needed? Can't we just remove them and do as if nothing happened, or are there some sideffects?
g_PausingPlayers == null
that can never happen as far as I can see. If it would, you'd have to check that first before checking the length.g_PausingPlayers.length == 0
=>!g_PausingPlayers.length
- Names:
pauseOverlayNotPausing
->multiplayerPauseOverlay
?pausingListText
->multiplayerPauseText
?g_PausingPlayers
->g_PausingClients
(since technically observers can pause too)
Hope I found all things to improve, keep it up.
comment:14 by , 8 years ago
Replying to elexis:
- Why was
resumeGame()
changed?
Currently, because pauseGame()
only pauses in single player mode, the game will call the resumeGame()
function in multiplayer mode even though the game is not paused, resulting in an unneeded message being sent to the server.
- Are those
LOGERROR
messages really needed? Can't we just remove them and do as if nothing happened, or are there some sideffects?
It is a bit useless to pause the game when it is already paused or unpause when it is not paused, but they are not strictly needed and I will remove them.
Replying to stanislas69:
I need to ask, why did you only Doxygen some parts of your code ? :)
For everything, I followed the style of the surrounding code.
by , 8 years ago
Attachment: | pausing2.patch added |
---|
comment:15 by , 8 years ago
New patch should fix the issues pointed out by elexis and reorders some code. Additionally it fixes another issue in the previous patch due to wrong use of the splice()
method.
comment:16 by , 8 years ago
Keywords: | review added |
---|
comment:17 by , 8 years ago
Keywords: | review removed |
---|
Patch seemed to work, but the existing and newly added JS code is as ugly as sin. Had to rewrite all of that to make sense of it.
Your TODO: I don't see any reason why all the XML code was introduced. Reuse the existing objects. It might look nicer to have the names of the pausing players in a second line below "Game Paused" with a smaller font.
What changed in the new patch:
- In general the code should set object properties only in one line, for example
Engine.GetGUIObjectByName("foo").caption = paused ? translate("Pause") : translate("Resume");
instead of using if-else. - Furthermore this text should be set only in a single function, not in multiple functions. The existing function
resumeGame
now reusespauseGame
. - Using
pauseGame
for singleplayer andtogglePause
for multiplayerpause seems terrible. The behavior of the code to not pause when some message boxes are opened was clarified by adding an explicit argumentavoidMultiplayerPause
, thus allowing to reuse that function in singleplayer to nuke more duplicate code. - The logic (
togglePause
,pauseGame
) had to be separated from the GUI updates (updatePauseOverlay
). - The GUI shouldn't be used to store state information. The previous code checked if the pause overlay was hidden to check if the game was paused. This lead to the mixing of GUI access and logic code.
- Introduced
setClientPauseState
to further reduce duplicate calls. Game Paused by
->Game paused by
.join(", ")
had to be translated- Reusing
SetPaused
instead of introducingSetMultiplayerPaused
means not having to do an if-else construct updatePauseOverlayMultiplayer
should be inmenu.js
near the pause functions, not insession.js
- forEach -> map looks nicer there (only one line)
- moved
onPress
to XML (needs some logic change or revert though) - Since the
multiplayerPauseMessage
object contains the text "Click to resume", calling it "multiplayerResumeMessage" might make more sense.
- The
message.pause != 0
check is now done in c++ instead of JS - Could use
NMT_START_ARRAY
/NMT_END_ARRAY()
instead of sending N messages to rejoining clients, but not really a problem. CNetServerWorker::OnUserLeave
could useremove_if
, but somehow that code is not becoming shorter that way.- the comment
// Unpause for the leaving player.
nor the braces needed
Potential bugs in the last patch:
- Didn't check, but looks like
Engine.SetPaused
should also be called if a pausing player disconnects.
by , 8 years ago
Attachment: | pausing_3_wip.patch added |
---|
Rewrote all JS code to be less ugly. Needs the removal of XML duplicates and some fixing.
by , 8 years ago
Attachment: | pausing4.patch added |
---|
comment:19 by , 8 years ago
Keywords: | review added |
---|
comment:20 by , 8 years ago
- The peculiarity of
pauseGame
to not pause in singleplayer ifexplicitPause
isn't set should be commented, similarly to my WIP example if (enabled)
and the next check could use an early return, but- I don't really like those if's in the first place. If statements should be avoided where possible.
As far as I can see, you can just set the caption of
pausedByText
and setonPress
regardless, since it won't have an effect if its hidden anyway. g_Paused ? togglePause : function() {};
-> maybeg_Paused && togglePause
works already?- Why does that function have an argument at all? It should be able to derive from the current pausing state from
g_PausingClients
, no? In general mixing logic and GUI updates should be split as good as possible (IPO model).
by , 8 years ago
Attachment: | pausing5.patch added |
---|
comment:21 by , 8 years ago
- The comment for the explicit pausing only confuses me. It should state that in SP it pauses when message boxes are opened, while in MP it only pauses when explicitly ordered to.
translate("Paused by") + " " + playernames
should use asprintf
instead, as there might be a language which might pick a sentence where the playername list doesn't appear at the end.!!g_PausingClients.length
->g_PausingClients.length
as it's implicitly converted to boolean when calling the function.- If a client rejoins, it will synchronize after the loading screen vanished. But as the server only sends to players that have finished the synchronization already, the client won't be informed about players who paused in that timeframe, thus won't show the overlay while others paused. As the pausing screen should'nt be displayed while the game is synchronizing, you need to send the paused states once the client finished it (when the "player has rejoined the game" message is sent.)
- Speaking of syncing, the animations should be paused for that time, because moonwalking units don't really look appealing.
- Can I convince you to add an
options.json
entry to not show the pause overlay in case someone else paused (could be called "pause overlay"), so that one can still view the map while waiting for someone else? (animations should still be paused and playernames can be shown at the place where the TODO was removed)
comment:22 by , 8 years ago
Update: Actually the "player has rejoined" message is sent after the loading screen too, but should be delayed until the synchronization finished as well: r16614.
by , 8 years ago
Attachment: | pausing6.patch added |
---|
comment:23 by , 8 years ago
New patch sends the rejoined message from C++ instead of sending it from JS.
by , 8 years ago
Attachment: | pausing7.patch added |
---|
comment:26 by , 8 years ago
Keywords: | review removed |
---|
Thanks so much for the patch, completing a feature which has been missing since the beginning of the game, going through all the iterations, small optimizations and that one rewrite!
Here few things I changed from the last patch
- Split it into two commits, as the rejoin fix was solely related to #1949
- An
!!explicit
was needed to convertnull
tofalse
to pass it in non-explicit singleplayer pausing. - One
updatePauseOverlay
passed an unusedg_PausingClients.length
argument - Added a dash to the JSdoc @param
- 2 newlines in
updatePauseOverlay
and rearranged calls - Removed the whitespace cleaning of
updateHero
since it's highly unrelated and there is also a patch in the review queue changing that code.
Future enhancements:
- As mentioned before, it would be nice to have an optional "slim mode", so that the pausing players are shown in the top right corner (see the removed TODO) without the overlay. Might make a ticket sometime.
- Message box dialog inconsistencies: For some message boxes it is paused implicitly in singleplayer (f.e. resign question) while other dialogs (f.e. deleting a building) don't pause. That should be consistent unless there is some good reason not to (requiring the player to be quick enough with answering the delete dialog before the building is captured doesn't seem a good reason considering there should be a hotkey to delete stuff).
Also, because it's multiplayer, we need to decide who gets to pause, when and how often. That's from a game perspective.
From the technical perspective, it will require some changes to how client time synchronization works. Currently a message is sent from clients when they are finished processing messages for the current turn, the server won't proceed to the next turn until all clients report back. This is why in multiplayer, the other clients appear stuck with their animations continuing, they are still interpolating for the current turn. Worse, the current "hack" would break if we had timeouts to drop clients that stop responding.
(The obvious solution of sending a pause message to all clients won't work, because currently a pause means it stops processing messages, so it couldn't be unpaused. See IRC discussion from 23:45 on 2013-03-07)