Page MenuHomeWildfire Games

Delete Lobby Presence Update messages altogether
ClosedPublic

Authored by elexis on Sep 5 2019, 7:49 PM.

Details

Summary

In the original introduction of the lobby, there was a GUI message sent for every player status change (such as busy -> online).
Each time such a message was received, a JS cache of the lobby status was incrementally updated, and the lobby playerlist was rebuilt on that.

This turned out to be a big problem (#3386), because if one plays a game for 1 or 2 hours, there would be several hundreds of these messages.
It was also bugged because first the playerlist was updated and then the incremental patches applied to the too recent playerlist (not the playerlist at the time the message was sent).
This was changed to updating the entire list upon receiving such a message.

So returning from the match then meant having to wait for the playerlist to rebuilt several hundred times, which caused serious (10s+) freezes.

Then there was rP16997 that added the erasure function, so that the list was rebuilt only once. (2015)

But better than deleting the message when its not needed is not creating the message to begin with, and only store a single dumb boolean to indicate whether the playerlist needs rebuild or not.
This patch implements it, and it could have been like that in 2015 already.

In rP20040 the fix rP16997 was updated to have one helper function removed, but it didn't go all the way to remove as much as possible and instead added the lobby.js return values.

In rP20070 (2017) the lobby dialog was added, and "historic" GUI messages were added so that one can retrieve chat messages when reopening the lobby page.
This meant that the code complexity of treating these messages was increased.

To solve #4877 / D2264, there will even be more code complexity needed, so better reduce it beforehand, and improve performance also.

Test Plan

Notice that the same issue conceptually applies to for instance the GameList or the profile updates.
Only some of the GUI message actually have historic relevance, for example chat messages.
For others (gamelist, playerlist, ...), we only need the most recent state.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Sep 5 2019, 7:49 PM
Vulcan added a comment.Sep 5 2019, 7:55 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/82/display/redirect

Vulcan added a comment.Sep 5 2019, 8:01 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/lobby/IXmppClient.h
|  24| namespace·StunClient·{
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language.

source/lobby/XmppClient.h
|  24| 
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language.

source/lobby/scripting/JSInterface_Lobby.h
|  26| namespace·JSI_Lobby
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceJSI_Lobby{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  47|  47|  * The playerlist will be assembled using these values.
|  48|  48|  */
|  49|  49| var g_PlayerStatuses = {
|  50|    |-	"available": { "color": "0 219 0",     "status": translate("Online") },
|    |  50|+	"available": { "color": "0 219 0", "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'away'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  48|  48|  */
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|    |-	"away":      { "color": "229 76 13",   "status": translate("Away") },
|    |  51|+	"away": { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  48|  48|  */
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|    |-	"away":      { "color": "229 76 13",   "status": translate("Away") },
|    |  51|+	"away":      { "color": "229 76 13", "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'playing'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|    |-	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|    |  52|+	"playing": { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|    |-	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|    |  52|+	"playing":   { "color": "200 0 0", "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'offline'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|    |-	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    |  53|+	"offline": { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|  56|  56| 
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|    |-	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    |  53|+	"offline":   { "color": "0 0 0", "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|  56|  56| 
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'unknown'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|    |-	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    |  54|+	"unknown": { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|  56|  56| 
|  57|  57| var g_RoleNames = {
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 211| 211| 					me ?
| 212| 212| 						translate("You have been muted.") :
| 213| 213| 						translate("%(nick)s has been muted.") :
| 214|    |-				newrole == "moderator" ?
|    | 214|+					newrole == "moderator" ?
| 215| 215| 					me ?
| 216| 216| 						translate("You are now a moderator.") :
| 217| 217| 						translate("%(nick)s is now a moderator.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 212| 212| 						translate("You have been muted.") :
| 213| 213| 						translate("%(nick)s has been muted.") :
| 214| 214| 				newrole == "moderator" ?
| 215|    |-					me ?
|    | 215|+						me ?
| 216| 216| 						translate("You are now a moderator.") :
| 217| 217| 						translate("%(nick)s is now a moderator.") :
| 218| 218| 				msg.oldrole == "visitor" ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 213| 213| 						translate("%(nick)s has been muted.") :
| 214| 214| 				newrole == "moderator" ?
| 215| 215| 					me ?
| 216|    |-						translate("You are now a moderator.") :
|    | 216|+							translate("You are now a moderator.") :
| 217| 217| 						translate("%(nick)s is now a moderator.") :
| 218| 218| 				msg.oldrole == "visitor" ?
| 219| 219| 					me ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 214| 214| 				newrole == "moderator" ?
| 215| 215| 					me ?
| 216| 216| 						translate("You are now a moderator.") :
| 217|    |-						translate("%(nick)s is now a moderator.") :
|    | 217|+							translate("%(nick)s is now a moderator.") :
| 218| 218| 				msg.oldrole == "visitor" ?
| 219| 219| 					me ?
| 220| 220| 						translate("You have been unmuted.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 215| 215| 					me ?
| 216| 216| 						translate("You are now a moderator.") :
| 217| 217| 						translate("%(nick)s is now a moderator.") :
| 218|    |-				msg.oldrole == "visitor" ?
|    | 218|+						msg.oldrole == "visitor" ?
| 219| 219| 					me ?
| 220| 220| 						translate("You have been unmuted.") :
| 221| 221| 						translate("%(nick)s has been unmuted.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 216| 216| 						translate("You are now a moderator.") :
| 217| 217| 						translate("%(nick)s is now a moderator.") :
| 218| 218| 				msg.oldrole == "visitor" ?
| 219|    |-					me ?
|    | 219|+							me ?
| 220| 220| 						translate("You have been unmuted.") :
| 221| 221| 						translate("%(nick)s has been unmuted.") :
| 222| 222| 					me ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 217| 217| 						translate("%(nick)s is now a moderator.") :
| 218| 218| 				msg.oldrole == "visitor" ?
| 219| 219| 					me ?
| 220|    |-						translate("You have been unmuted.") :
|    | 220|+								translate("You have been unmuted.") :
| 221| 221| 						translate("%(nick)s has been unmuted.") :
| 222| 222| 					me ?
| 223| 223| 						translate("You are not a moderator anymore.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 218| 218| 				msg.oldrole == "visitor" ?
| 219| 219| 					me ?
| 220| 220| 						translate("You have been unmuted.") :
| 221|    |-						translate("%(nick)s has been unmuted.") :
|    | 221|+								translate("%(nick)s has been unmuted.") :
| 222| 222| 					me ?
| 223| 223| 						translate("You are not a moderator anymore.") :
| 224| 224| 						translate("%(nick)s is not a moderator anymore.");
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 219| 219| 					me ?
| 220| 220| 						translate("You have been unmuted.") :
| 221| 221| 						translate("%(nick)s has been unmuted.") :
| 222|    |-					me ?
|    | 222|+							me ?
| 223| 223| 						translate("You are not a moderator anymore.") :
| 224| 224| 						translate("%(nick)s is not a moderator anymore.");
| 225| 225| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 220| 220| 						translate("You have been unmuted.") :
| 221| 221| 						translate("%(nick)s has been unmuted.") :
| 222| 222| 					me ?
| 223|    |-						translate("You are not a moderator anymore.") :
|    | 223|+								translate("You are not a moderator anymore.") :
| 224| 224| 						translate("%(nick)s is not a moderator anymore.");
| 225| 225| 
| 226| 226| 			addChatMessage({
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 221| 221| 						translate("%(nick)s has been unmuted.") :
| 222| 222| 					me ?
| 223| 223| 						translate("You are not a moderator anymore.") :
| 224|    |-						translate("%(nick)s is not a moderator anymore.");
|    | 224|+								translate("%(nick)s is not a moderator anymore.");
| 225| 225| 
| 226| 226| 			addChatMessage({
| 227| 227| 				"text": "/special " + sprintf(txt, { "nick": msg.nick }),

binaries/data/mods/public/gui/lobby/lobby.js
|1014| »   »   switch·(sortBy)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/591/display/redirect

elexis edited the summary of this revision. (Show Details)Sep 5 2019, 8:18 PM
elexis added a subscriber: JoshuaJB.
elexis added a subscriber: ffffffff.
Stan added a subscriber: Stan.Sep 5 2019, 9:06 PM
Stan added inline comments.
source/lobby/XmppClient.h
185 ↗(On Diff #9640)

Updated/Changed ?

elexis updated the Trac tickets for this revision.Sep 6 2019, 4:31 AM
elexis added a comment.Sep 6 2019, 4:46 AM

One could say the presence message could be useful in case we have a selected player and that needs to be updated.
But the message only indicates the online / away status, and that can safely be rebuilt every update (like the entire playerlist is updated every update following https://trac.wildfiregames.com/ticket/3386#comment:3) if one wants to.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 6 2019, 4:53 AM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Sep 6 2019, 4:53 AM

Woah, give this one a minute for review. I think this is really the wrong approach and needs major rework before landing. I haven't had a chance to write up all my comments yet, but I wanted to quickly jump in and say that I think this should be rolled back until the review process can run its course.

elexis added a comment.Sep 6 2019, 7:35 PM

Woah, give this one a minute for review. I think this is really the wrong approach and needs major rework before landing. I haven't had a chance to write up all my comments yet, but I wanted to quickly jump in and say that I think this should be rolled back until the review process can run its course.

Thank you for your feedback Joshua!

I will revert it immediately, please tell me what is the wrong approach!

elexis added a comment.Sep 6 2019, 7:59 PM

In particular I would need to know whether you liked rP20040.

With a much more thorough review, I believe my earlier concerns were completely misplaced. While I think we need to sit down and think about a cleaner way to handle all the possible lobby states, this does move things in the right direction. I only have one minor comment about side-effects.

JoshuaJB added inline comments.Sep 6 2019, 11:53 PM
ps/trunk/source/lobby/XmppClient.cpp
658

Forgot to save this comment earlier...

It seems unintuitive to clear this flag as a side-effect. While expected in the message-queue functions like GUIPollNewMessage, it seems out of place for a single flag value. Making this explicit seems like a better approach to me, e.g. GuiCheckPresenceStatusUpdate and GuiClearPresenceStatusUpdate.