Skip to content

feat(gui): Distinguish patch players from retail players by 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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
4 changes: 4 additions & 0 deletions GeneralsMD/Code/GameEngine/Include/GameNetwork/GameInfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,9 @@ class GameSlot

void mute( Bool isMuted ) { m_isMuted = isMuted; }
Bool isMuted( void ) const { return m_isMuted; }

void setPatchVersion(UnsignedInt patchVersion) { m_patchVersion = patchVersion; }
UnsignedInt getPatchVersion() const { return m_patchVersion; }
protected:
SlotState m_state;
Bool m_isAccepted;
Expand All @@ -146,6 +149,7 @@ class GameSlot
FirewallHelperClass::FirewallBehaviorType m_NATBehavior; ///< The NAT behavior for this slot's player.
UnsignedInt m_lastFrameInGame; // only valid for human players
Bool m_disconnected; // only valid for human players
UnsignedInt m_patchVersion; // TheSuperHackers patch version
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can label this as "Community made product version"

};

/**
Expand Down
7 changes: 7 additions & 0 deletions GeneralsMD/Code/GameEngine/Include/GameNetwork/LANAPI.h
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,7 @@ class LANAPI : public LANAPIInterface
void handleGameStartTimer( LANMessage *msg, UnsignedInt senderIP );
void handleGameOptions( LANMessage *msg, UnsignedInt senderIP );
void handleInActive( LANMessage *msg, UnsignedInt senderIP );
void handlePatchVersion( LANMessage *msg, UnsignedInt senderIP );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest use terminology

"ProductTitle"
"ProductVersion"
"ProductAuthor"


};

Expand Down Expand Up @@ -319,6 +320,7 @@ struct LANMessage
MSG_INACTIVE, ///< I've alt-tabbed out. Unaccept me cause I'm a poo-flinging monkey.

MSG_REQUEST_GAME_INFO, ///< For direct connect, get the game info from a specific IP Address
MSG_PATCH_VERSION, ///< TheSuperHackers patch version

MSG_MAX
} LANMessageType;
Expand Down Expand Up @@ -415,6 +417,11 @@ struct LANMessage
char options[m_lanMaxOptionsLength+1];
} GameOptions;

struct
{
UnsignedInt patchVersion;
} PatchInfo;

};
};
#pragma pack(pop)
Expand Down
17 changes: 10 additions & 7 deletions GeneralsMD/Code/GameEngine/Include/GameNetwork/LANPlayer.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
class LANPlayer
{
public:
LANPlayer() { m_name = m_login = m_host = L""; m_lastHeard = 0; m_next = NULL; m_IP = 0; }
LANPlayer() { m_name = m_login = m_host = L""; m_lastHeard = 0; m_next = NULL; m_IP = 0; m_patchVersion = 0; }

// Access functions
inline UnicodeString getName( void ) { return m_name; }
Expand All @@ -56,14 +56,17 @@ class LANPlayer
inline void setNext( LANPlayer *next ) { m_next = next; }
inline UnsignedInt getIP( void ) { return m_IP; }
inline void setIP( UnsignedInt IP ) { m_IP = IP; }
inline void setPatchVersion(UnsignedInt patchVersion) { m_patchVersion = patchVersion; }
inline UnsignedInt getPatchVersion() const { return m_patchVersion; }

protected:
UnicodeString m_name; ///< Player name
UnicodeString m_login; ///< login name
UnicodeString m_host; ///< machine name
UnsignedInt m_lastHeard; ///< The last time we heard from this player (for timeout purposes)
LANPlayer *m_next; ///< Linked list pointer
UnsignedInt m_IP; ///< Player's IP
UnicodeString m_name; ///< Player name
UnicodeString m_login; ///< login name
UnicodeString m_host; ///< machine name
UnsignedInt m_lastHeard; ///< The last time we heard from this player (for timeout purposes)
LANPlayer *m_next; ///< Linked list pointer
UnsignedInt m_IP; ///< Player's IP
UnsignedInt m_patchVersion; ///< TheSuperHackers patch version
};

#endif //_LANPLAYER_H_
Original file line number Diff line number Diff line change
Expand Up @@ -532,24 +532,28 @@ void PopulateInGameDiplomacyPopup( void )
if (staticTextStatus[rowNum])
{
staticTextStatus[rowNum]->winHide(FALSE);

Color frontColor = 0;
UnicodeString text;

if (isInGame)
{
if (isAlive)
{
staticTextStatus[rowNum]->winSetEnabledTextColors( aliveColor, backColor );
GadgetStaticTextSetText(staticTextStatus[rowNum], TheGameText->fetch("GUI:PlayerAlive"));
frontColor = aliveColor;
text = TheGameText->fetch("GUI:PlayerAlive");
}
else
{
if (isObserver)
{
staticTextStatus[rowNum]->winSetEnabledTextColors( observerInGameColor, backColor );
GadgetStaticTextSetText(staticTextStatus[rowNum], TheGameText->fetch("GUI:PlayerObserver"));
frontColor = observerInGameColor;
text = TheGameText->fetch("GUI:PlayerObserver");
}
else
{
staticTextStatus[rowNum]->winSetEnabledTextColors( deadColor, backColor );
GadgetStaticTextSetText(staticTextStatus[rowNum], TheGameText->fetch("GUI:PlayerDead"));
frontColor = deadColor;
text = TheGameText->fetch("GUI:PlayerDead");
}
}
}
Expand All @@ -558,15 +562,21 @@ void PopulateInGameDiplomacyPopup( void )
// not in game
if (isObserver)
{
staticTextStatus[rowNum]->winSetEnabledTextColors( observerGoneColor, backColor );
GadgetStaticTextSetText(staticTextStatus[rowNum], TheGameText->fetch("GUI:PlayerObserverGone"));
frontColor = observerGoneColor;
text = TheGameText->fetch("GUI:PlayerObserverGone");
}
else
{
staticTextStatus[rowNum]->winSetEnabledTextColors( goneColor, backColor );
GadgetStaticTextSetText(staticTextStatus[rowNum], TheGameText->fetch("GUI:PlayerGone"));
frontColor = goneColor;
text = TheGameText->fetch("GUI:PlayerGone");
}
}

if (slot->isHuman() && (TheGameInfo->getLocalSlotNum() == slotNum || slot->getPatchVersion() > 0))
text.concat(L"[SH]");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This string ideally is build from the product title and not hardcoded like this.


staticTextStatus[rowNum]->winSetEnabledTextColors(frontColor, backColor);
GadgetStaticTextSetText(staticTextStatus[rowNum], text);
}

slotNumInRow[rowNum++] = slotNum;
Expand Down
6 changes: 6 additions & 0 deletions GeneralsMD/Code/GameEngine/Source/GameNetwork/GUIUtil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,12 @@ void UpdateSlotList( GameInfo *myGame, GameWindow *comboPlayer[],
}
if(slot->isHuman())
{
if (i == myGame->getLocalSlotNum() || myGame->getSlot(i)->getPatchVersion() > 0)
{
GadgetComboBoxSetEnabledTextColors(comboPlayer[i], 0xFFFFFF00, 0xFF000000);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These colours should be put into variables, preferably configurable through INI.

GadgetComboBoxSetDisabledTextColors(comboPlayer[i], 0xFFC0C000, 0xFF000000);
}

UnicodeString newName = slot->getName();
UnicodeString oldName = GadgetComboBoxGetText(comboPlayer[i]);
if (comboPlayer[i] && newName.compare(oldName))
Expand Down
6 changes: 6 additions & 0 deletions GeneralsMD/Code/GameEngine/Source/GameNetwork/GameInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ void GameSlot::reset()
m_origPlayerTemplate = -1;
m_origStartPos = -1;
m_origColor = -1;
m_patchVersion = 0;
}

void GameSlot::saveOffOriginalInfo( void )
Expand Down Expand Up @@ -1479,7 +1480,12 @@ Bool ParseAsciiStringToGameInfo(GameInfo *game, AsciiString options)
//DEBUG_LOG(("ParseAsciiStringToGameInfo - game options all good, setting info"));

for(Int i = 0; i<MAX_SLOTS; i++)
{
if (game->getConstSlot(i)->getState() == SLOT_PLAYER && newSlot[i].getState() == SLOT_PLAYER)
newSlot[i].setPatchVersion(game->getConstSlot(i)->getPatchVersion());

game->setSlot(i,newSlot[i]);
}

game->setMap(mapName);
game->setMapCRC(mapCRC);
Expand Down
12 changes: 12 additions & 0 deletions GeneralsMD/Code/GameEngine/Source/GameNetwork/LANAPI.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "Common/crc.h"
#include "Common/GameState.h"
#include "Common/Registry.h"
#include "Common/version.h"
#include "GameNetwork/LANAPI.h"
#include "GameNetwork/networkutil.h"
#include "Common/GlobalData.h"
Expand Down Expand Up @@ -417,6 +418,9 @@ void LANAPI::update( void )
case LANMessage::MSG_INACTIVE: // someone is telling us that we're inactive.
handleInActive( msg, senderIP );
break;
case LANMessage::MSG_PATCH_VERSION: // someone is sharing their patch version with us
handlePatchVersion( msg, senderIP );
break;

default:
DEBUG_LOG(("Unknown LAN message type %d", msg->LANMessageType));
Expand Down Expand Up @@ -461,6 +465,14 @@ void LANAPI::update( void )
}
}

if (m_inLobby || (m_currentGame && !m_currentGame->isGameInProgress())) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is broadcast every 200 ms to all other clients. It works perfectly, but it's a bit 'spammy'. Should try to find a more elegant solution for this.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should only be broadcasted when a new player joins the lobby. It is a fixed value that doesn't change.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eliminating the broadcast spam would good.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, the game periodically broadcasts other data that may or may not have changed. This happens with 10 second intervals, which I found to be a bit too slow for this feature.

It should only be broadcasted when a new player joins the lobby.

It's a bit more complicated than that, though. I also need data about the hosts of each game and about other players in the same match.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be redesigned with new message versions.

On RETAIL_COMPATIBLE_NETWORK versions, send the original messages

MSG_GAME_ANNOUNCE
MSG_LOBBY_ANNOUNCE
MSG_REQUEST_JOIN

and on top send new messages, give them a generous offset to avoid colliding with other community products.

MSG_COMMUNITY_GAME_ANNOUNCE = 1000
MSG_COMMUNITY_LOBBY_ANNOUNCE
MSG_COMMUNITY_REQUEST_JOIN

Then try to design the packets so that they cover all sorts of use cases we will have.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume MSG_COMMUNITY_GAME_ANNOUNCE equals MSG_GAME_ANNOUNCE + some patch information. Wouldn't it be better to do this when we break compatibility? Otherwise we'll be sending this data in duplicate. Also, redesigning these packets is absolutely outside the scope of this PR as far as I'm concerned.

I do agree that if we keep MSG_PATCH_VERSION or similar, it should be given a decent offset.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well ideally we build this feature here without the packet spam every 200 ms. Instead send 2 packets on the announcements, which is better than the spamming. The 2 packets will also only be sent on RETAIL_COMPATIBLE_NETWORK clients, so it is temporary. I did not think this fully through but my instict tells me this is the way to go?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we understand well enough what our messages will look like after we break retail compatibility, so I propose we delay redesigning them until we have a better idea what that should look like.

I think it's possible to avoid spamming MSG_PATCH_VERSION by sending it whenever certain other messages are sent as well. This doesn't require newly designed messages.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you can also send the product version message side by side with other messages. Try to anticipate future requirements for communications so that we do not need to touch it often.

LANMessage msg;
fillInLANMessage(&msg);
msg.LANMessageType = LANMessage::MSG_PATCH_VERSION;
msg.PatchInfo.patchVersion = TheVersion->getVersionNumber();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only contain major 1 minor 4 for Zero Hour. This means the version will not change between builds.

I think this data can be extended with more version information.

The least we need is a product name and its version.

For reference, see

Version::getUnicodeProductTitle
Version::getUnicodeProductVersion
Version::getUnicodeProductAuthor

sendMessage(&msg);
}

Bool playerListChanged = false;
Bool gameListChanged = false;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,8 @@ void LANAPI::OnPlayerList( LANPlayer *playerList )
LANPlayer *player = m_lobbyPlayers;
while (player)
{
Int addedIndex = GadgetListBoxAddEntryText(listboxPlayers, player->getName(), playerColor, -1, -1);
const Color color = (player->getPatchVersion() > 0 || m_localIP == player->getIP()) ? 0xFFFFFF00 : playerColor;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic colour, create INI configurable variable

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put it by the others

const Color playerColor =  GameMakeColor(255,255,255,255);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic colour, create INI configurable variable

The current white and grey colors are not configurable either, and I don't see that as an issue.

Put it by the others

Will do.

Int addedIndex = GadgetListBoxAddEntryText(listboxPlayers, player->getName(), color, -1, -1);
GadgetListBoxSetItemData(listboxPlayers, (void *)player->getIP(),addedIndex, 0 );

if (selectedIP == player->getIP())
Expand Down
55 changes: 55 additions & 0 deletions GeneralsMD/Code/GameEngine/Source/GameNetwork/LANAPIhandlers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -682,3 +682,58 @@ void LANAPI::handleInActive(LANMessage *msg, UnsignedInt senderIP) {
RequestGameOptions(options, FALSE);
lanUpdateSlotList();
}

void LANAPI::handlePatchVersion(LANMessage* msg, UnsignedInt senderIP)
{
if (m_inLobby)
{
LANPlayer *player = m_lobbyPlayers;
while (player)
{
if (player->getIP() == senderIP)
{
if (!player->getPatchVersion() && msg->PatchInfo.patchVersion > 0)
{
player->setPatchVersion(msg->PatchInfo.patchVersion);
OnPlayerList(m_lobbyPlayers);
}
break;
}

player = player->getNext();
}

LANGameInfo* game = m_games;
while (game)
{
GameSlot *slot = game->getSlot(0);
if (slot->getIP() == senderIP)
{
DEBUG_ASSERTCRASH(game->getHostIP() == slot->getIP(), ("First game slot is not used by the host"));

if (!slot->getPatchVersion() && msg->PatchInfo.patchVersion > 0)
{
slot->setPatchVersion(msg->PatchInfo.patchVersion);
OnGameList(m_games);
}
break;
}

game = game->getNext();
}
}
else if (m_currentGame && !m_currentGame->isGameInProgress())
{
for (int player = 0; player < MAX_SLOTS; ++player)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can split this into 2 functions:

handlePatchVersionInLobby(LANMessage* msg, UnsignedInt senderIP)
handlePatchVersionInGameRoom(LANMessage* msg, UnsignedInt senderIP)

{
if (m_currentGame->getIP(player) == senderIP)
{
if (!m_currentGame->getSlot(player)->getPatchVersion() && msg->PatchInfo.patchVersion > 0)
{
m_currentGame->getSlot(player)->setPatchVersion(msg->PatchInfo.patchVersion);
}
break;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,10 @@ void LANDisplayGameList( GameWindow *gameListbox, LANGameInfo *gameList )
{
txtGName.concat(L"]");
}
Int addedIndex = GadgetListBoxAddEntryText(gameListbox, txtGName, (gameList->isGameInProgress())?gameInProgressColor:gameColor, -1, -1);
const Color color = (gameList->getSlot(0)->getPatchVersion() > 0)
? ((gameList->isGameInProgress()) ? 0xFF808000 : 0xFFFFFF00)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic colours

: ((gameList->isGameInProgress()) ? gameInProgressColor : gameColor);
Int addedIndex = GadgetListBoxAddEntryText(gameListbox, txtGName, color, -1, -1);
GadgetListBoxSetItemData(gameListbox, (void *)gameList, addedIndex, 0 );

if (selectedPtr == gameList)
Expand Down
Loading