Skip to content

tweak(gamemessage): Reduce number of MSG_DESTROY_SELECTED_GROUP messages #2668

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.

Sign up for GitHub

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
Caball009 wants to merge 7 commits into
base: main
Choose a base branch
from
Open

tweak(gamemessage): Reduce number of MSG_DESTROY_SELECTED_GROUP messages #2668

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6c8d336
tweak(msg): Reduce number of MSG_DESTROY_SELECTED_GROUP messages.
Caball009 Apr 30, 2026
db6a8e6
Removed function 'deselectAll'.
Caball009 Apr 30, 2026
4c917cd
Updated other callsites of `InGameUI::deselectAllDrawables` when used…
Caball009 Apr 30, 2026
4453191
Removed TSH comment.
Caball009 Apr 30, 2026
8c156f6
Added additional check for selected drawables.
Caball009 May 3, 2026
fcb6ef1
Added logic to avoid double deselection on left mouse click.
Caball009 May 3, 2026
65a34a7
Addressed feedback.
Caball009 May 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions GeneralsMD/Code/GameEngine/Include/Common/Player.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,6 +645,9 @@ class Player : public Snapshot
// adds the given AIGroup to the current selection of this player.
void addAIGroupToCurrentSelection(AIGroup *group);

// returns false if player has object(s) currently selected
Bool isCurrentlySelectedGroupEmpty() const;

// return the requested hotkey squad
Squad *getHotkeySquad(Int squadNumber);

Expand Down
2 changes: 1 addition & 1 deletion GeneralsMD/Code/GameEngine/Include/GameClient/InGameUI.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@ friend class Drawable; // for selection/deselection transactions
// Drawable selection mechanisms
virtual void selectDrawable( Drawable *draw ); ///< Mark given Drawable as "selected"
virtual void deselectDrawable( Drawable *draw ); ///< Clear "selected" status from Drawable
virtual void deselectAllDrawables( Bool postMsg = true ); ///< Clear the "select" flag from all drawables
virtual void deselectAllDrawables( Bool updateGameLogic = true ); ///< Clear the "select" flag from all drawables
virtual Int getSelectCount() { return m_selectCount; } ///< Get count of currently selected drawables
virtual Int getMaxSelectCount() { return m_maxSelectCount; } ///< Get the max number of selected drawables
virtual UnsignedInt getFrameSelectionChanged() { return m_frameSelectionChanged; } ///< Get the max number of selected drawables
Expand Down
5 changes: 5 additions & 0 deletions GeneralsMD/Code/GameEngine/Source/Common/RTS/Player.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3819,6 +3819,11 @@ void Player::addAIGroupToCurrentSelection(AIGroup *group) {
}
}

Bool Player::isCurrentlySelectedGroupEmpty() const
{
return m_currentSelection->getSizeOfGroup() == 0;
}

//-------------------------------------------------------------------------------------------------
/** addTypeOfProductionCostChange adds a production change to the typeof list */
//-------------------------------------------------------------------------------------------------
Expand Down
18 changes: 8 additions & 10 deletions GeneralsMD/Code/GameEngine/Source/GameClient/InGameUI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3553,7 +3553,7 @@ void InGameUI::deselectDrawable( Drawable *draw )
//-------------------------------------------------------------------------------------------------
/** Clear all drawables' "select" status */
//-------------------------------------------------------------------------------------------------
void InGameUI::deselectAllDrawables( Bool postMsg )
void InGameUI::deselectAllDrawables( Bool updateGameLogic )
Comment thread
xezon marked this conversation as resolved.
{
const DrawableList *selected = getAllSelectedDrawables();

Expand All @@ -3576,16 +3576,14 @@ void InGameUI::deselectAllDrawables( Bool postMsg )
// our selection can no longer consist of exactly one angry mob
m_soloNexusSelectedDrawableID = INVALID_DRAWABLE_ID;


///@todo don't we want to not emit this message if there wasn't a group at all? (CBD)
/** @todo also, we probably are sending this message too much, we should come up with
some kind of "selections are dirty" status that we can check once per frame and send
the correct group info over the network ... could be tricky tho (or impossible) given
the order of operations of things happening in the code (CBD) */
if( postMsg )
if (updateGameLogic)
{
// TheSuperHackers @tweak Originally this message had one boolean argument, but it wasn't used for anything.
TheMessageStream->appendMessage( GameMessage::MSG_DESTROY_SELECTED_GROUP );
// TheSuperHackers @tweak Avoid sending this message when no objects are currently selected.
if (!ThePlayerList->getLocalPlayer()->isCurrentlySelectedGroupEmpty())

@xezon xezon Apr 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks suspicious. Let's say we select the drawable and then deselect it real fast before the AIGroup is created in the logic (network delay). Wouldn't that then mean the deselect is missed entirely in the logic, while it is deselected in client?

Maybe empty list test needs to come with above drawable list instead?

@Caball009 Caball009 Apr 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Wouldn't that then mean the deselect is missed entirely in the logic, while it is deselected in client?

Yes, that's right, good find. I had overlooked that. I think it won't cause a mismatch because it's purely a client issue, but it needs changing.

Maybe empty list test needs to come with above drawable list instead?

As an early return if current selection is empty?

EDIT1: That needs some tweaking, otherwise you may not be able to deselect units while the game is paused.

EDIT2: Perhaps this is even less desirable. If the logic is slow to update, client deselection attempts may fail because the logic group selection is still empty, making it harder to deselect objects.

Let's say we select the drawable and then deselect it real fast before the AIGroup is created in the logic (network delay).

EDIT3: The reproduction is slightly more complex than described and not likely to happen imo:

  1. no selection
  2. select unit(s)
  3. deselect unit(s)
  4. hold SHIFT and select other unit(s)
  5. issue order
  6. if done fast enough, depending on the current runahead, the units selected with (2) and (4) will carry out the order

@xezon xezon May 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes so if there is an issue, even if difficult to reproduce for a human, we cannot do this.

@Caball009 Caball009 May 3, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That's fair. What about this instead? It has a check for client and logic:

const DrawableList *selected = getAllSelectedDrawables();
const Bool emptyDrawableSelection = selected->empty();

...

if (updateGameLogic)
{
	if (!emptyDrawableSelection || !ThePlayerList->getLocalPlayer()->isCurrentlySelectedGroupEmpty())
	{
		TheMessageStream->appendMessage(GameMessage::MSG_DESTROY_SELECTED_GROUP);
	}
}

{
// TheSuperHackers @tweak Originally this message had one boolean argument, but it wasn't used for anything.
TheMessageStream->appendMessage(GameMessage::MSG_DESTROY_SELECTED_GROUP);
}
}
}

Expand Down
4 changes: 3 additions & 1 deletion GeneralsMD/Code/GameEngine/Source/GameClient/MessageStream/SelectionXlat.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -750,7 +750,9 @@ GameMessageDisposition SelectionTranslator::translateGameMessage(const GameMessa
{
if (!addToGroup)
{
deselectAll();
Comment thread
Caball009 marked this conversation as resolved.
// TheSuperHackers @tweak Update the game client but not the game logic.
// It isn't required for the latter, as a new group will override the current selection.
Comment thread
Caball009 marked this conversation as resolved.
Outdated
TheInGameUI->deselectAllDrawables(FALSE);
}

GameMessage *newMsg = TheMessageStream->appendMessage(GameMessage::MSG_CREATE_SELECTED_GROUP);
Expand Down
Loading