Opened 11 years ago

Closed 8 years ago

Last modified 8 years ago

#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)

t1950_show_who_paused_v1.patch (11.5 KB ) - added by elexis 9 years ago.
Displays a chat notification if a player pauses or unpauses the game.
pausing.patch (14.2 KB ) - added by echotangoecho 8 years ago.
pausing2.patch (16.0 KB ) - added by echotangoecho 8 years ago.
pausing_3_wip.patch (20.4 KB ) - added by elexis 8 years ago.
Rewrote all JS code to be less ugly. Needs the removal of XML duplicates and some fixing.
pausing4.patch (19.5 KB ) - added by echotangoecho 8 years ago.
pausing5.patch (20.0 KB ) - added by echotangoecho 8 years ago.
pausing6.patch (20.7 KB ) - added by echotangoecho 8 years ago.
pausing7.patch (23.0 KB ) - added by echotangoecho 8 years ago.

Download all attachments as: .zip

Change History (36)

comment:1 by historic_bruno, 11 years ago

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)

comment:2 by Kieran P, 11 years ago

Milestone: Alpha 14Backlog

comment:3 by Jia Henry, 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 elexis, 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 elexis, 9 years ago

Displays a chat notification if a player pauses or unpauses the game.

comment:5 by elexis, 9 years ago

Keywords: patch review added; multiplayer simulation pause freeze removed
Milestone: BacklogAlpha 19
Priority: Must HaveShould 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?

Question: How should we handle multiple players pausing simultaneously? We could just prohibit that to make things easy to implement. On the other hand it currently works if multiple players pause the game and the players can utilize that: If one player pauses the game for say 5 minutes, the other player can pause the game too, take a toilet break and make sure that the game won't continue before he unpauses it too.

Last edited 9 years ago by elexis (previous) (diff)

comment:6 by Itms, 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 elexis, 9 years ago

Milestone: Alpha 19Alpha 20

comment:8 by Itms, 8 years ago

Milestone: Alpha 20Backlog

comment:9 by echotangoecho, 8 years ago

Owner: set to echotangoecho

comment:10 by elexis, 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 echotangoecho, 8 years ago

Attachment: pausing.patch added

comment:11 by echotangoecho, 8 years ago

Keywords: review added
Milestone: BacklogAlpha 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.

Last edited 8 years ago by echotangoecho (previous) (diff)

comment:12 by Stan, 8 years ago

I need to ask, why did you only Doxygen some parts of your code ? :)

comment:13 by elexis, 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 existing SetPaused. Maybe you can add a function MultiplayerPause 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.

Last edited 8 years ago by elexis (previous) (diff)

in reply to:  13 comment:14 by echotangoecho, 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 echotangoecho, 8 years ago

Attachment: pausing2.patch added

comment:15 by echotangoecho, 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.

Last edited 8 years ago by echotangoecho (previous) (diff)

comment:16 by echotangoecho, 8 years ago

Keywords: review added

comment:17 by elexis, 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 reuses pauseGame.
  • Using pauseGame for singleplayer and togglePause for multiplayerpause seems terrible. The behavior of the code to not pause when some message boxes are opened was clarified by adding an explicit argument avoidMultiplayerPause, 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 introducing SetMultiplayerPaused means not having to do an if-else construct
  • updatePauseOverlayMultiplayer should be in menu.js near the pause functions, not in session.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 use remove_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 elexis, 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.

comment:18 by Stan, 8 years ago

Why is resumegame calling itself ?

by echotangoecho, 8 years ago

Attachment: pausing4.patch added

comment:19 by echotangoecho, 8 years ago

Keywords: review added

comment:20 by elexis, 8 years ago

  • The peculiarity of pauseGame to not pause in singleplayer if explicitPause 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 set onPress regardless, since it won't have an effect if its hidden anyway.
  • g_Paused ? togglePause : function() {}; -> maybe g_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 echotangoecho, 8 years ago

Attachment: pausing5.patch added

comment:21 by elexis, 8 years ago

  • translate("Paused by") + " " + playernames should use a sprintf 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)
Version 0, edited 8 years ago by elexis (next)

comment:22 by elexis, 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 echotangoecho, 8 years ago

Attachment: pausing6.patch added

comment:23 by echotangoecho, 8 years ago

New patch sends the rejoined message from C++ instead of sending it from JS.

by echotangoecho, 8 years ago

Attachment: pausing7.patch added

comment:24 by elexis, 8 years ago

In 18203:

Network cleanup.

Send the "client has rejoined" message after the synchronization finished instead of right after the loading screen.
Patch by echotangoecho, refs #1949, #1950.

comment:25 by elexis, 8 years ago

Resolution: fixed
Status: newclosed

In 18204:

Send a network message to update the GUI if another player pauses a multiplayer game. Patch by echotangoecho, fixes #1950.
Pause animations then and on disconnect.
Don't unpause unintentionally when closing a message box in singleplayer.

comment:26 by elexis, 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 convert null to false to pass it in non-explicit singleplayer pausing.
  • One updatePauseOverlay passed an unused g_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).
Last edited 8 years ago by elexis (previous) (diff)

comment:27 by elexis, 8 years ago

In 18208:

Fix whitespace and remove duplicate comment, refs #1950.

comment:28 by elexis, 8 years ago

Component: UI & SimulationNetwork

(set component to network)

Note: See TracTickets for help on using tickets.