Opened 3 years ago

Closed 16 months ago

Last modified 16 months ago

#3143 closed defect (fixed)

[PATCH] "Show Full Games" option hides games that are not full

Reported by: elexis Owned by: elexis
Priority: Must Have Milestone: Alpha 21
Component: Multiplayer lobby Keywords: patch
Cc: Patch:

Description (last modified by leper)

The "Show Full Games" button in the game lobby hides all games where the number of clients (not players) is greater or equal to the number of player slots. It is enabled by default.

This means if one player spectates (or is not assigned because of other reasons), the server will be hidden although there are open player slots (if observer-count + playercount = slot count).

As a result the server will usually never be filled.

The only workaround for a18 is then that the host adds one more player slot temporarily, so that the game will not be listed as full.

Quick Fix: Disable the option (the way it was on a17) - then full games are listed too by default.

Better Fix: List games only as full if the number of assigned players is equal to the number of player slots.

Attachments (17)

3143_lobby_data.patch (12.0 KB) - added by Imarok 18 months ago.
Show only full games as full. Display the connected players arranged according their team in the lobby window. Display observers in the lobby window. refs #3476
3143_lobby_data_v2.patch (12.1 KB) - added by Imarok 17 months ago.
Fixed comments from irc
3143_lobby_data_v2.1.patch (12.6 KB) - added by Imarok 17 months ago.
updated year in the source files
3143_lobby_data_v2.2.patch (12.6 KB) - added by Imarok 17 months ago.
removed the commented out code (thx leper)
3143_lobby_data_v2.3.patch (13.0 KB) - added by Imarok 17 months ago.
Use JSON.stringify and encodeURI/decodeURI (escape is deprecated) to pass the player and observer list
3143_lobby_data_v2.4.patch (13.6 KB) - added by Imarok 17 months ago.
use escapeText instead of encodeURI.
3143_lobby_data_v2.5.patch (13.5 KB) - added by Imarok 17 months ago.
Move duplicating formatting into common. Translate ,
3143_lobby_data_gameattribs.patch (3.9 KB) - added by Imarok 17 months ago.
preparation patch defining g_GameAttributes for session.js
3143_lobby_data_v2.6.03.patch (17.4 KB) - added by Imarok 17 months ago.
The patch should be finished. But when testing my game wasn't even displayed in the lobbys game list…
3143_lobby_data_v2.6_lobby_bot.patch (916 bytes) - added by Imarok 17 months ago.
Refreshes the "players" too when executing changeGameState
3143_lobby_data_v2.6_lobby_bot_v2.patch (997 bytes) - added by Imarok 17 months ago.
3143_lobby_data_v2.6.2.patch (15.8 KB) - added by Imarok 17 months ago.
Fixes #3143 and #3476 besides the gamestart time
3143_lobby_data_v2.6.3.patch (16.2 KB) - added by Imarok 17 months ago.
only remove the lengt limitation of escapeText for some parts
3143_lobby_data_v2.6.4.patch (16.5 KB) - added by Imarok 17 months ago.
Made the changes proposed by elexis
3143_lobby_data_v2.6.5.patch (16.6 KB) - added by Imarok 17 months ago.
Move the strinigfication and escapement into the new format changing functions
3143_lobby_data_v2.6.6.patch (16.6 KB) - added by Imarok 17 months ago.
use whitelist in sendLobbyStanzaUpdate and changed the comment of the format function
3143_lobby_data_v3.patch (20.4 KB) - added by elexis 17 months ago.
Fixes what was encountered and can be fixed without rewriting or extending a lot of c++ lobby / XML code. Didn't test yet and didn't review some parts of the patch yet.

Download all attachments as: .zip

Change History (35)

comment:1 Changed 3 years ago by leper

Description: modified (diff)

No to the quick fix, users joining full games is annoying.

comment:2 Changed 3 years ago by Ben Brian

Milestone: Alpha 19Backlog

comment:3 Changed 2 years ago by stanislas69

Looking at the lobby.js code, line 152 is responsible for it :

   if (!showFullFilter.checked && game.tnbp <= game.nbp)
       return true;

Just removing the number of observers should solve the problem.

According to elexis, the number of observers is not sent to the bot, that means we have to send the data via session.js in order to filter the games.

Last edited 2 years ago by stanislas69 (previous) (diff)

comment:4 Changed 19 months ago by Imarok

Should game.nbp just be number of players without observers or should there be some new data containing the number of observers transmitted?

comment:5 Changed 19 months ago by elexis

Since we want more information for lobby matches (#3476), better send two lists. One for the players and one for the observers. Instead of sending the number. If needed i can lookup my old broken WIP patch for that ticket.

Changed 18 months ago by Imarok

Attachment: 3143_lobby_data.patch added

Show only full games as full. Display the connected players arranged according their team in the lobby window. Display observers in the lobby window. refs #3476

comment:6 Changed 18 months ago by Imarok

Keywords: review patch added
Milestone: BacklogAlpha 21
Summary: "Show Full Games" option hides games that are not full[PATCH] "Show Full Games" option hides games that are not full

comment:7 Changed 18 months ago by elexis

Keywords: review removed

Changed 17 months ago by Imarok

Attachment: 3143_lobby_data_v2.patch added

Fixed comments from irc

comment:8 Changed 17 months ago by Imarok

Keywords: review added

Changed 17 months ago by Imarok

Attachment: 3143_lobby_data_v2.1.patch added

updated year in the source files

Changed 17 months ago by Imarok

Attachment: 3143_lobby_data_v2.2.patch added

removed the commented out code (thx leper)

comment:9 Changed 17 months ago by elexis

Keywords: review removed

Broken due to using CSV and assuming players not having commas in their names. That might be true for lobby registered nicknames, but not people who join by IP. The code doesn't throw errors, but it still displays broken stuff in the lobby status.

At least use JSON.stringify and escapeText / unescapeText if you don't have time to check how to add actual XML tags in the XmppClient.cpp and glooxwrapper.cpp. (It should still be considered an hack as there should be no technical need to mix JSON and XML.)

Changed 17 months ago by Imarok

Attachment: 3143_lobby_data_v2.3.patch added

Use JSON.stringify and encodeURI/decodeURI (escape is deprecated) to pass the player and observer list

comment:10 Changed 17 months ago by Imarok

Keywords: review added

Changed 17 months ago by Imarok

Attachment: 3143_lobby_data_v2.4.patch added

use escapeText instead of encodeURI.

Changed 17 months ago by Imarok

Attachment: 3143_lobby_data_v2.5.patch added

Move duplicating formatting into common. Translate ,

comment:11 Changed 17 months ago by elexis

Should use the strings from formatPlayerInfo introduced in r18440, or even better that function itself, especially for #3476.

Changed 17 months ago by Imarok

preparation patch defining g_GameAttributes for session.js

comment:12 Changed 17 months ago by elexis

In 18452:

Cache gameAttributes in the session. Patch by Imarok, refs #3143, #3263.

Changed 17 months ago by Imarok

The patch should be finished. But when testing my game wasn't even displayed in the lobbys game list...

Changed 17 months ago by Imarok

Refreshes the "players" too when executing changeGameState

Changed 17 months ago by Imarok

Changed 17 months ago by Imarok

Fixes #3143 and #3476 besides the gamestart time

Changed 17 months ago by Imarok

only remove the lengt limitation of escapeText for some parts

comment:13 Changed 17 months ago by elexis

  • Some things mentioned in IRC
  • formatClients is ambiguous, it should be clear that this is used for the lobby report
  • The JSON data sent via network would become smaller if it would use the format { "observers": ["name1", "name2", ...], "1": ["player1", ...] , ...}. The code might become ugly when still reusing that format function so prepare mentally ;)
  • //if client is no observer not needed as that fact emerges from the else part
  • JSON.parse(JSON.stringify really? Use clone or deepcopy if you have to, but it would be likely cleaner to create a new object / array, perhaps with map if it doesn't become too entangled
  • sendLobbyReportUpdate sounds like that game report for ratings that sends half the simulation state to the server, while the functions you change have something with "gamestate" or "stanza" in it. Perhaps sendChangeStateGame?

Changed 17 months ago by Imarok

Made the changes proposed by elexis

Changed 17 months ago by Imarok

Move the strinigfication and escapement into the new format changing functions

Changed 17 months ago by Imarok

use whitelist in sendLobbyStanzaUpdate and changed the comment of the format function

comment:14 Changed 17 months ago by elexis

In 18510:

Fix autostart replays following r18452 by grabbing g_GameAttributes from the simulation instead of the GUI. Patch by Imarok, refs #3143.
Don't remove attribs from the init attributes as that is still used by the loading screen to show the mapname.

comment:15 Changed 17 months ago by elexis

Component: Core engineMultiplayer lobby

Incomplete review:

Issues:

  • Typo: "AIDiff": pData.AI should be "AIDiff": pData.AIDiff
  • Lots of unneeded packet load. network data should be as small as possible, as great as needed.
  • nbp doesn't need to be sended anymore at all, since you already distinguish observers from players in the list maxnbp is only needed in the gamesetup now, but we likely won't get away with only optionally sending it

Code style fun:

  • Duplicate check if (g_IsController && Engine.HasXmppClient()) before every sendLobbyStanzaUpdate
  • The defeat/won case of g_NotificationsTypes should be used, rather than the formatDefeatMessage
  • The patch adds a call to the defeat case, but not the won case. So if the "won" message is sent last, it wouldn't show up in the lobby.
  • clone isn't needed at all since the calls to that function already deliver a copy
  • Adding Object.freeze() to ensure it can't be changed by stupid mistakes (also briefly discussed with sanderd17 on irc yesterday)
  • renaming player and playerData to pData, since they are an item of g_GameAttributes.settings.PlayerData
  • renaming originalplayerDataArray to playerData since it was copied from g_GameAttributes.settings.PlayerData
  • JS peculiarity: the for...in returns strings as keys, not numbers, so that one function doesn't invert the other (thereby also introduces 2 bytes per player of useless traffic ), therefore pData.Team = +team;
  • removing unneeded comment // Update lobby gamestatus

session/utility_functions.js:

  • one wrong space before function sendLobbyStanzaUpdate()
  • renaming sendLobbyStanzaUpdate to sendLobbyPlayerlistUpdate
  • numberOfPlayers and playerStates are declared long before its first usage
  • Adding a comment why that map is done in the first place
  • Why are there 3 loops over the player index? they can all be merged
  • Caching team in a variable as its reused often there
  • playerStates[player.name] = { "State": player.state -> unneeded package load
  • Offline -> unneeded package load if its false
  • State and Offline is looped twice`, the reader doesn't have time to read that code twice and find the diff
  • First you copy the Team, then you remove it in some circumstance. So why not just copy it if you need it only.

gamesetup.js

  • Renaming numberOfPlayers and nbp to connectedPlayers

Note for the commit message:

  • unescapeText by sanderd17.

Changed 17 months ago by elexis

Attachment: 3143_lobby_data_v3.patch added

Fixes what was encountered and can be fixed without rewriting or extending a lot of c++ lobby / XML code. Didn't test yet and didn't review some parts of the patch yet.

comment:16 Changed 16 months ago by elexis

Keywords: review removed
Milestone: Alpha 21
Resolution: duplicate
Status: newclosed

Closing as dupe of #3476 as there has to be a distinction between players and observers in the clientlist to fix this. Having two tickets in the review queue for one patch, nor uploading one patch here and some other patch there doesn't really help.

comment:17 Changed 16 months ago by elexis

Owner: set to elexis
Resolution: duplicatefixed

In 18534:

Submit and display more information about matches in the lobby. Patch by Imarok, refs #3476.

Includes team numbers, online/offline- and won/defeated state, AI type and difficulty for running games and
only the playernames with observer-player distinction in the gamesetup.
Use JSON format inside the XML stanza and minimize traffic by packing teams.

Use the observer distinction to correctly apply the "full games" trigger in the lobby, fixes #3143.

XPartaMupp patch applied by scythetwirler.
unescapeText function by sanderd17, refs #3409.

comment:18 Changed 16 months ago by elexis

Milestone: Alpha 21
Note: See TracTickets for help on using tickets.