Game listings fetched by IRC List #570

GrantBartlett · 2024-10-09T14:33:46Z

Changes

Request LIST of game lobby, should return somewhat instantly the list of games
Parse the topic of the channel to give the CnCNet lobby game information (as opposed to using ctcp)
Updates topic if game options have changed

github-actions · 2024-10-09T14:42:17Z

Nightly build for this pull request:

artifacts-TS.zip
artifacts-Ares.zip
artifacts-YR.zip
This comment is automatic and is meant to allow guests to get latest automatic builds without registering. It is updated on every successful build.

SadPencil · 2024-10-09T16:45:51Z

DXMainClient/DXGUI/Multiplayer/GameListBox.cs

- DrawTexture(hostedGame.Game.Texture,
- new Rectangle(x, height,
- hostedGame.Game.Texture.Width, hostedGame.Game.Texture.Height), Color.White);
+ var hostedGameTexture = hostedGame.HasSpecialGameMode == true && hostedGame.Game.TextureSpecialGameMode != null ? 


remove == true since HasSpecialGameMode is bool

SadPencil · 2024-10-09T16:47:36Z

DXMainClient/DXGUI/Multiplayer/GameLobby/CnCNetGameLobby.cs

+ connectionManager.SetChannelTopic(channel, sb.ToString());
+ }
+
+ private StringBuilder BuildGameBroadcastingString()


return string since the caller does not need to append more strings

SadPencil · 2024-10-09T16:50:29Z

DXMainClient/DXGUI/Multiplayer/GameLobby/CnCNetGameLobby.cs

@@ -1925,11 +1964,19 @@ private void BroadcastGame()
 sb.Append(";");
 sb.Append(GameMode?.UntranslatedUIName ?? string.Empty);
 sb.Append(";");
- sb.Append(tunnelHandler.CurrentTunnel.Address + ":" + tunnelHandler.CurrentTunnel.Port);
+ sb.Append(tunnelHandler?.CurrentTunnel?.Address + ":" + tunnelHandler?.CurrentTunnel?.Port);


Have you test the case where tunnelHandler?.CurrentTunnel?.Address is null, which string will be append?

Personally I prefer if tunnderHandler?.CurrentTunnel is null, append string.Empty, otherwise append {address}:{port}.

SadPencil · 2024-10-09T16:51:35Z

DXMainClient/DXGUI/Multiplayer/GameLobby/CnCNetGameLobby.cs


- broadcastChannel.SendCTCPMessage(sb.ToString(), QueuedMessageType.SYSTEM_MESSAGE, 20);
+ // Append additional game info (11 onwards)
+ sb.Append(hasSpecialGameMode ? "1" : "0");


Something like sb.Append(Convert.ToInt32(isCustomPassword));

SadPencil · 2024-10-09T16:53:59Z

DXMainClient/DXGUI/Multiplayer/CnCNet/CnCNetLobby.cs

+
+ // Ensure the topic starts with "GAME " as expected
+ if (!e.Topic.StartsWith("GAME "))
+ {


In this project, it is preferred not using braces for a single line. Same below.

SadPencil · 2024-10-09T16:56:28Z

DXMainClient/Domain/Multiplayer/GenericHostedGame.cs

@@ -20,6 +20,9 @@ public abstract class GenericHostedGame: IEquatable<GenericHostedGame>
 public string GameVersion { get; set; }
 public string HostName { get; set; }
 public string[] Players { get; set; }
+ public bool HasSpecialGameMode { get; set; }
+ public bool HasCrates { get; set; }
+ public bool HasSupers { get; set; }


I prefer HasSuperWeapons

SadPencil · 2024-10-09T16:57:16Z

DXMainClient/DXGUI/Multiplayer/GameInformationPanel.cs

+ lblCrates.Text = "Crates:".L10N("Client:Main:GameInfoCrates") + " " + Renderer.GetSafeString(game.HasCrates ? "Yes" : "No", lblCrates.FontIndex);
+ lblCrates.Visible = true;
+
+ lblSupers.Text = "Superweapons:".L10N("Client:Main:GameInfoSupers") + " " + Renderer.GetSafeString(game.HasSupers ? "Yes" : "No", lblSupers.FontIndex);


I prefer HasSuperWeapons

SadPencil · 2024-10-09T16:59:10Z

DXMainClient/Online/CnCNetManager.cs

@@ -28,11 +28,13 @@ public class CnCNetManager : IConnectionManager
 public delegate void UserListDelegate(string channelName, string[] userNames);

 public event EventHandler<ServerMessageEventArgs> WelcomeMessageReceived;
+ public event EventHandler ChannelMOTDComplete;


What is "MOTD" short for?

What is "MOTD" short for?

Message of the day

Use "MessageOfTheDay"/"MsgOfTheDay" to replace "MOTD" in the method name or at least leave comments on this abbreviation

…-game3289552

GrantBartlett · 2024-10-09T19:39:56Z

@SadPencil thanks for reviewing - have pushed up the changes

… the game quickly

SadPencil · 2024-10-09T20:21:51Z

DXMainClient/DXGUI/Multiplayer/GameLobby/CnCNetGameLobby.cs

+ sb.Append(";");
+
+ // Append additional game info (11 onwards)
+ sb.Append(hasSpecialGameMode ? "1" : "0");


duplicated line 1973 and 1974?

SadPencil

Sorry, but I have another comment and it should be the last, which was not proposed in my initial review.

SadPencil · 2024-10-10T10:41:25Z

DXMainClient/Online/CnCNetManager.cs

+
+ foreach (var nameTopic in channelList)
+ {
+ string channelName = nameTopic.Item1; 


C# has builtin support to deconstruct a tuple, which gets rid of meaningless name like "Item1".

(string name1, string name2) = tuple;

https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/functional/deconstruct

Good to know cheers. Have updated

SadPencil

You may also deconstruct a tuple in foreach loop: foreach ((string name1, string name2) in someCollection)

LGTM

Metadorius

The idea is good apart from the hacked in special icon, good as a first iteration / PoC, but we would need a bit more work on that so it fits the standards. I've written the suggestions on how to improve further.

Also I would recommend to split the PR if possible/not hard to address and merge the parts individually, but that's up to you, it's kinda in the gray area of features being intertwined enough or not enough.

Metadorius · 2024-10-11T10:18:03Z

DXMainClient/DXGUI/Multiplayer/CnCNet/CnCNetLobby.cs

+
+ // Extract the game information from the topic string
+ string msg = e.Topic.Substring(5); // Cut out "GAME " part
+ string[] splitMessage = msg.Split(new char[] { ';' });


It would be great if you added a comment with an example valid topic for reference.

Metadorius · 2024-10-11T10:22:27Z

DXMainClient/DXGUI/Multiplayer/CnCNet/CnCNetLobby.cs

+ if (UserINISettings.Instance.PlaySoundOnGameHosted &&
+ cncnetGame.InternalName == localGameID.ToLower() &&
+ !ProgramConstants.IsInGame && !game.Locked)
+ {
+ SoundPlayer.Play(sndGameCreated);
+ }


I am pretty sure the initial fetch with 100 games is gonna destroy your eardrums if the new game sound is on. Also since you're touching this code anyways - for the sake of better readability we now place operators at the beginning of a split line, not at the end.

Metadorius · 2024-10-11T10:34:00Z

DXMainClient/DXGUI/Multiplayer/CnCNet/CnCNetLobby.cs

- string gameRoomChannelName = splitMessage[3];
- string gameRoomDisplayName = splitMessage[4];
- bool locked = Conversions.BooleanFromString(splitMessage[5].Substring(0, 1), true);
- bool isCustomPassword = Conversions.BooleanFromString(splitMessage[5].Substring(1, 1), false);
 bool isClosed = Conversions.BooleanFromString(splitMessage[5].Substring(2, 1), true);


You're currently producing a ton of waste IRC traffic and only read 1% of that waste traffic. This won't do.

Either don't remove the game broadcast messages at all (then the routine of parsing the game info need to be split into a new function that would parse both CTCP and channel topics) or change what is broadcasted and how it is broadcasted. What one could do is to broadcast a message "hey, my options changed, please refetch the game info for channel #123456789" and make the client fetch that channel's topic, though it needs evaluation whether that is better than just broadcasting CTCPs.

@Rampastring wdyt?

Metadorius · 2024-10-11T10:41:24Z

DXMainClient/DXGUI/Multiplayer/GameLobby/CnCNetGameLobby.cs

+ private string BuildGameBroadcastingString()
+ {
+ // @TODO: Do we need a way to allow games/mods to specify their additional game options they want to broadcast
+ // For now hardcoded bits


Yeah this is a hack, I am quite negative about such hacks. We are trying to walk away from hardcoded per game hacks, this goes against that path.

The client already has options broadcasting for all players inside the lobby, it already builds the string for all the checkboxes, dropdowns etc., so you don't need to invent much, rather reuse. Could add a new INI tag for a game option that would indicate that it is broadcasted to the outside lobby and split options building from that function, and/or reorganize it's a bit since outside/inside options communication seems to be more and more similar with this kind of changes.

Metadorius · 2024-10-11T10:44:04Z

DXMainClient/DXGUI/Multiplayer/GameInformationPanel.cs

+ lblCrates.Text = "Crates:".L10N("Client:Main:GameInfoCrates") + " " + Renderer.GetSafeString(game.HasCrates ? "Yes" : "No", lblCrates.FontIndex);
+ lblCrates.Visible = true;
+
+ lblSuperWeapons.Text = "Superweapons:".L10N("Client:Main:GameInfoSuperWeapons") + " " + Renderer.GetSafeString(game.HasSuperWeapons ? "Yes" : "No", lblSuperWeapons.FontIndex);
+ lblSuperWeapons.Visible = true;


Also no, those things are already defined in INI.

Metadorius · 2024-10-11T10:53:24Z

ClientCore/CnCNet5/CnCNetGame.cs

+ public Texture2D TextureSpecialGameMode { get; set; }
+


Same, the solution is hacky, which in this case also goes against the design (this is a distribution/game/mod icon, it can't change depending on a game mode). We would need to come up with a proper way to achieve what you want.

An option I see is:
0. Use both RA2/YR icon as Texture for CnCNet YR,

Make an option in ClientDefintions to hide the main game icon in game list,

Make an option to add enabled/disabled icons to the game settings, something like EnabledIcon and DisabledIcon for INI defined game options, and Icon for dropdown items,

When some option is broadcasted to the outside world - prepend an icon to the game name, same for all of the options.

This way you could define ra2mode as broadcasted to cncnet lobby in INI, define enabled / disabled texture for it and it would change the "icon" of the game.

Metadorius · 2024-10-11T10:55:49Z

DXMainClient/DXGUI/Multiplayer/GameLobby/CnCNetGameLobby.cs

+ // @TODO: Sending ctcp for broadcasting may not be needed now, if its only informing cncnet lobby
+ broadcastChannel.SendCTCPMessage(gameBroadcastingString, QueuedMessageType.SYSTEM_MESSAGE, 20);
+
+ // @TODO: Perhaps we should only update the topic when something has changed?
+ connectionManager.SetChannelTopic(channel, gameBroadcastingString);


See the comment above about generating a ton of waste traffic. We would need to rethink when to send that too. TODOs are correct, they just need to be done. 😛

…ntil ctcp arrives

- Upon receiving user list, send a WHO to the chat channel - Upon succesfully receiving the WHO, request the channel list for games - Better handling of hosts closing the game - Broadcast in topic is by ident not usernames to save space

GrantBartlett · 2024-10-22T10:04:45Z

I'm going to stop working on this. The benefits aren't enough. Thank you all for your reviews/comments.
I've learnt a lot about the networking of the client so its not been a full waste of time.

GrantBartlett added 3 commits October 9, 2024 15:32

Add initial pass for fetching game listings with list & topic

aaa74bf

Consistency on naming

031ae8d

Tidy up of checkbox checks

8780cf6

GrantBartlett added 2 commits October 9, 2024 16:02

Upon MOTD received, request channel list

f39bcef

Add game list pattern to game collections

4dc36b0

GrantBartlett changed the title ~~WIP: Game listings fetched by IRC List~~ Game listings fetched by IRC List Oct 9, 2024

GrantBartlett marked this pull request as ready for review October 9, 2024 16:30

Doesn't seem needed

2617a2d

GrantBartlett requested a review from SadPencil October 9, 2024 16:32

SadPencil requested changes Oct 9, 2024

View reviewed changes

GrantBartlett added 3 commits October 9, 2024 20:35

Add way to find gamecollection by hosted channelname, e.g. #cncnet-yr…

c558641

…-game3289552

PR feedback fixes

9c2e6f4

lblSupers to lblSuperWeapons

a6e79aa

When the host closes a game, ensure we keep the ctcp capture to close…

82ad89c

… the game quickly

SadPencil requested changes Oct 9, 2024

View reviewed changes

Remove duplicate

4e06747

SadPencil requested changes Oct 10, 2024

View reviewed changes

Update tuple deconstructing

13e884e

GrantBartlett mentioned this pull request Oct 10, 2024

WIP: Feature/map peview game info #571

Draft

SadPencil approved these changes Oct 11, 2024

View reviewed changes

Metadorius requested changes Oct 11, 2024

View reviewed changes

GrantBartlett added 5 commits October 11, 2024 18:12

Remove special hacks, LIST updates and topic setting

3e6ce20

Tidy

4ac8f24

Further tidy

15b0cb4

Add example topic

b319952

Operators at start of lines

95cecfa

GrantBartlett requested a review from Metadorius October 13, 2024 10:05

Have a clear GAME and DETAIL section in the broadcast topic

1011c7d

GrantBartlett added 20 commits October 13, 2024 18:42

Hybrid ctcp/topic

ef5c6ea

Special topic irc message type

d103b50

wip

90c8c84

wip

da7888e

hybrid ctcp/topic handling

aed6566

Make sure we show the players in the room upon joining and not wait u…

8ce1a68

…ntil ctcp arrives

Remove mapname from broadcasting topic

3981ea1

Wip updates to ui processing

0e9acfa

Hosted game changes & user list updates

c08e22a

- Upon receiving user list, send a WHO to the chat channel - Upon succesfully receiving the WHO, request the channel list for games - Better handling of hosts closing the game - Broadcast in topic is by ident not usernames to save space

Puts back other bits

3cc60cc

Fix pipeline error

2aaa180

Few ui fixes

a8913b2

Fixes to ui updating

0627385

Update channel topic when player extra options updated

e73fb15

Remove our name if its in already in the topic

e21bb2f

No idea where this came from

b2e2882

Pipeline build error fix

a5b2369

Tidy up of logs

4eaa3a2

Tidy up

61d915c

Fix on topic change broadcasting player options

fe09dad

GrantBartlett closed this Oct 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Game listings fetched by IRC List #570

Game listings fetched by IRC List #570

GrantBartlett commented Oct 9, 2024 •

edited

Loading

github-actions bot commented Oct 9, 2024 •

edited

Loading

SadPencil Oct 9, 2024

SadPencil Oct 9, 2024

SadPencil Oct 9, 2024

SadPencil Oct 9, 2024 •

edited

Loading

SadPencil Oct 9, 2024

SadPencil Oct 9, 2024

SadPencil Oct 9, 2024

SadPencil Oct 9, 2024 •

edited

Loading

GrantBartlett Oct 9, 2024

SadPencil Oct 9, 2024

GrantBartlett commented Oct 9, 2024

SadPencil Oct 9, 2024

SadPencil left a comment

SadPencil Oct 10, 2024

GrantBartlett Oct 10, 2024

SadPencil left a comment

Metadorius left a comment •

edited

Loading

Metadorius Oct 11, 2024

Metadorius Oct 11, 2024

Metadorius Oct 11, 2024 •

edited

Loading

Metadorius Oct 11, 2024

Metadorius Oct 11, 2024

Metadorius Oct 11, 2024

Metadorius Oct 11, 2024

GrantBartlett commented Oct 22, 2024

Game listings fetched by IRC List #570

Game listings fetched by IRC List #570

Conversation

GrantBartlett commented Oct 9, 2024 • edited Loading

Changes

github-actions bot commented Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SadPencil Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SadPencil Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrantBartlett commented Oct 9, 2024

Choose a reason for hiding this comment

SadPencil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SadPencil left a comment

Choose a reason for hiding this comment

Metadorius left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Metadorius Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrantBartlett commented Oct 22, 2024

GrantBartlett commented Oct 9, 2024 •

edited

Loading

github-actions bot commented Oct 9, 2024 •

edited

Loading

SadPencil Oct 9, 2024 •

edited

Loading

SadPencil Oct 9, 2024 •

edited

Loading

Metadorius left a comment •

edited

Loading

Metadorius Oct 11, 2024 •

edited

Loading