tweak(gamemessage): Reduce number of MSG_DESTROY_SELECTED_GROUP messages

[Caball009]

• Similar to PRs tweak(gamemessage): Remove unused argument of MSG_DESTROY_SELECTED_GROUP #2664 and tweak(gamemessage): Remove unused MSG_AREA_SELECTION message from game logic #2667

The client considers primary mouse clicks in the game world as a deselection, and will also update the game logic. This means that
GameMessage::MSG_DESTROY_SELECTED_GROUP messages are sent frequently, even if no objects are currently selected. There's actually an old EA comment on this issue. This PR changes that by checking first if the local player has objects selected, which reduces the number of messages.

I've tested with two local clients in multiplayer (VS22 debug builds); one had this feature and one didn't, and everything worked fine.

TODO:

• Replicate in Generals.

[greptile-apps]

Greptile Summary

This PR reduces unnecessary MSG_DESTROY_SELECTED_GROUP network messages by (1) skipping the message in deselectAllDrawables when no units are currently selected, and (2) deferring deselection in alternate-mouse mode from MSG_RAW_MOUSE_LEFT_BUTTON_UP to MSG_MOUSE_LEFT_CLICK via a new m_pendingDeselection flag to avoid double-deselecting when switching selection targets. All existing callers that immediately follow deselection with MSG_CREATE_SELECTED_GROUP now pass FALSE to suppress the redundant destroy message.

Confidence Score: 5/5

Safe to merge — no functional regressions found; all changed call sites follow a consistent and correct pattern.

All six files contain narrowly scoped, mechanical changes. The core optimisation (guard in deselectAllDrawables) is sound: the empty-list snapshot is taken before the loop so there is no TOCTOU issue. The deferred deselection via m_pendingDeselection correctly handles all MSG_MOUSE_LEFT_CLICK exit paths and is cleared before the function returns. Call sites that pass FALSE are each immediately followed by MSG_CREATE_SELECTED_GROUP, preserving game-logic consistency. No P1 or P0 issues found.

No files require special attention.

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Source/GameClient/InGameUI.cpp	Core optimization: deselectAllDrawables now captures selected->empty() before clearing, renames parameter to updateGameLogic, and guards MSG_DESTROY_SELECTED_GROUP behind both the new name and the non-empty check. One idle-worker call site updated to FALSE.
GeneralsMD/Code/GameEngine/Source/GameClient/MessageStream/SelectionXlat.cpp	Removes thin deselectAll() wrapper, introduces m_pendingDeselection to defer deselect in alternate-mouse mode from RAW_MOUSE_LEFT_BUTTON_UP to MSG_MOUSE_LEFT_CLICK, preventing double destroy-group messages when clicking a new unit while one is already selected.
GeneralsMD/Code/GameEngine/Source/GameClient/MessageStream/CommandXlat.cpp	Five call sites updated to deselectAllDrawables(FALSE) — each is immediately followed by MSG_CREATE_SELECTED_GROUP, so skipping the destroy message is safe.
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/ControlBar/ControlBarCommandProcessing.cpp	Single call site updated to deselectAllDrawables(FALSE) before a MSG_CREATE_SELECTED_GROUP; change is consistent with the pattern across the PR.
GeneralsMD/Code/GameEngine/Include/GameClient/SelectionXlat.h	Adds Bool m_pendingDeselection private member to SelectionTranslator, correctly initialised in the constructor.
GeneralsMD/Code/GameEngine/Include/GameClient/InGameUI.h	Parameter rename postMsg → updateGameLogic in the virtual declaration; no interface change.

Sequence Diagram

sequenceDiagram
    participant Mouse
    participant SelectionXlat
    participant InGameUI
    participant MsgStream as MessageStream

    Note over Mouse,MsgStream: Alternate mouse mode — click on new unit while one is selected (NEW behaviour)

    Mouse->>SelectionXlat: MSG_RAW_MOUSE_LEFT_BUTTON_UP
    SelectionXlat->>SelectionXlat: m_pendingDeselection = TRUE

    Mouse->>SelectionXlat: MSG_MOUSE_LEFT_CLICK (drawable found)
    SelectionXlat->>SelectionXlat: m_pendingDeselection = FALSE
    SelectionXlat->>InGameUI: deselectAllDrawables(FALSE)
    Note right of InGameUI: No MSG_DESTROY_SELECTED_GROUP
    SelectionXlat->>MsgStream: MSG_CREATE_SELECTED_GROUP

    Note over Mouse,MsgStream: Click on empty space (nothing selected) — optimisation
    Mouse->>SelectionXlat: MSG_RAW_MOUSE_LEFT_BUTTON_UP
    SelectionXlat->>SelectionXlat: m_pendingDeselection = TRUE
    Mouse->>SelectionXlat: MSG_MOUSE_LEFT_CLICK (no drawables)
    Note right of SelectionXlat: break early
    SelectionXlat->>InGameUI: deselectAllDrawables() post-switch
    Note right of InGameUI: selected list empty → MSG_DESTROY_SELECTED_GROUP skipped

Loading

_{Reviews (4): Last reviewed commit: "Added logic to avoid double deselection ..." | Re-trigger Greptile}

[xezon]

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]

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]

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

[Caball009]

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);
	}
}

[Caball009]

!ThePlayerList->getLocalPlayer()->isCurrentlySelectedGroupEmpty()

With the addition of the tweak to avoid double deselection on MSG_RAW_MOUSE_LEFT_BUTTON_UP and MSG_MOUSE_LEFT_CLICK, I don't think the logic selection check is even still necessary.