-
Notifications
You must be signed in to change notification settings - Fork 151
feat(network): Send product information to distinguish clients (with a colored name and different status) #1404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(network): Send product information to distinguish clients (with a colored name and different status) #1404
Conversation
GeneralsMD/Code/GameEngine/Source/GameNetwork/LANAPICallbacks.cpp
Outdated
Show resolved
Hide resolved
xezon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GeneralsMD/Code/GameEngine/Source/GameNetwork/LANAPICallbacks.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameNetwork/LANAPIhandlers.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Diplomacy.cpp
Outdated
Show resolved
Hide resolved
|
What is the current status of this change? I think this would be a nice change for the first release. |
The current implementation ought to be changed to make it more efficient / optimal, but that requires much more work. I did some of the work locally, but it didn't really work out iirc, so the PR is kind of stuck for now. |
9fb6a01 to
c4e3f1d
Compare
|
I've revisited this PR and was able to move away from the broadcast implementation. It feels better now. I put networking related changes in one commit and the color / status stuff in another. What sort of information do we want to exchange just to establish which clients are on a patched version? Maybe it's a good idea to send the commit hash as well. |
|
Short overview of the new implementation. Patch information is not broadcast and only sent when needed. Interacting with players in the lobby:
Interacting with game hosts in the lobby:
Interacting with players in the pre-match game:
Currently, players in the lobby do not have a way to know which players in a match are on a 'patched version' except the host. It doesn't seem necessary to me to have the host communicate this information, and it also breaks the current pattern where each client only sends data about itself. |
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Diplomacy.cpp
Outdated
Show resolved
Hide resolved
xezon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is going into the right direction but needs more polishing and wider scope.
Are the pictures of the first post still relevant?
The terminology of the network data needs changing from "Patch" to "Product", because "Patch" is a much more limited term than "Product" is.
The ProductInfo message needs to contain all relevant information to describe the Product that the remote player is using. A version number alone will not be enough to properly describe a Product. Think of 100 different products that the user could interact with in the lobby.
Renamed 'patch' to 'product' and added more information to the product information that's exchanged between clients.
Yes, the colors are still like that; the status text will be slightly different. My current idea is that it shows the Git short hash string there (there isn't much room for additional text, unfortunately).
Fair enough. Everything should say 'product' instead of 'patch' now.
I added a couple of stuff (e.g. exe and ini crc values), but I don't feel like this is final version yet. |
Fixed Zero Hour version.
|
I could use some feedback on this PR. |
xezon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current thoughts on this. No exhaustive review.
| } | ||
| if(slot->isHuman()) | ||
| { | ||
| if (myGame->getSlot(i)->getProductInfo().productVersion > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only useful for a destinction between Retail and any Community Product version. It does not work well as soon as there are 2 Community Products.
It perhaps would be better to show Tooltip with Product Information on Player Hover.
Other than that, you could implement an auto color coding that is generated from a Product Hash (and blend that new color with the default color to retain overall appearance (bright, dark)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I currently have no grand designs for any color coding, other than to provide the option to see at a glance who's on retail and who's not.
| UnicodeString gitShortHash; | ||
| gitShortHash.translate(slot->getProductInfo().gitShortHash); | ||
|
|
||
| text.format(L"%s [%s]", text.str(), gitShortHash.str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if a product has no git hash? It was not created from a git repository. I think this needs to be more general. Git Hash is also user unfriendly/unreadable.
Maybe use first 3 letters of Product Title and last 5 letters of Product Version or similar. Or build a new hash from Product Title, Version, Author. That is needed anyway if you do the color coding.
| &productInfo.productAuthor, | ||
| &productInfo.gitShortHash | ||
| }; | ||
| getProductInfoStrings(msg->ProductInfo.data, strings); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xezon The getProductInfoStrings and setProductInfoStrings logic, is this ok with you? It's basically a way to not waste data on separate arrays for 4 or more strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function interface is oddly specific expecting these array sizes. Perhaps use template instead, although VC6 might have some trouble with [Size]
| * LAN message class | ||
| */ | ||
| #pragma pack(push, 1) | ||
| struct LANMessage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You moved this whole struct up so you can use it further down below, which is fine. Can you make a separate Pull Request moving it, so that the diff of this change is cleaner?
| const Color chatSystemColor = GameMakeColor(255,255,255,255); | ||
| const Color acceptTrueColor = GameMakeColor(0,255,0,255); | ||
| const Color acceptFalseColor = GameMakeColor(255,0,0,255); | ||
| const Color playerColor = GameMakeColor(255,255,255,255); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This refactor can also be in separate change.
| UnsignedInt flags; | ||
| UnsignedInt exeCRC; | ||
| UnsignedInt iniCRC; | ||
| WideChar data[201]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the absolute max possible length for this? I suspect MAX_PACKET_SIZE-12 ?
| { | ||
| enum CPP_11(: UnsignedInt) | ||
| { | ||
| COMMUNITY_PATCH = 1 << 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this flag? It seems oddly specific. Now every product that derives from this will deliver this flag. If all this is needed for is seeing if the remote player has any non retail patch, then simply check if productTitle (or exeCRC?) is set, because the retail game will not serve this information.
bool isRetailGame() const
{
return productTitle.empty();
}| char options[m_lanMaxOptionsLength+1]; | ||
| } GameOptions; | ||
|
|
||
| // ProductInfo is sent with REQUEST_PRODUCT_INFO and RESPONSE_PRODUCT_INFO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps also explain how this is different from the original GameToJoin struct, which also contains exeCRC and iniCRC.
On that note, does ProductInfo also implicitly cover Product information for Game Rooms?
| //DEBUG_ASSERTCRASH(false, ("setting host to %ls@%ls", m_currentGame->getLANSlot(0)->getUser()->getLogin().str(), | ||
| // m_currentGame->getLANSlot(0)->getUser()->getHost().str())); | ||
|
|
||
| // TheSuperHackers @feature Caball009 06/11/2025 Request players in the match to send product information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You added feature comments at many different places for the same feature. Prefer to document the feature at a single central place, for example before MSG_GAME_REQUEST_PRODUCT_INFO = 1000, or somewhere else suitable.
Update: Scope of this PR has expanded from patch information to product information.
With this PR it'll become clear which clients are on a patched version with this feature and which are not:
LAN Lobby:

Pre-match / Game Options:

Status in match:

TODO: