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 redundant MSG_DESTROY_SELECTED_GROUP network messages by (1) guarding the message behind isCurrentlySelectedGroupEmpty() in deselectAllDrawables() so it is only sent when the game logic actually has a group to destroy, and (2) passing FALSE to deselectAllDrawables() at all call sites that immediately follow with a MSG_CREATE_SELECTED_GROUP, avoiding the now-unnecessary destroy message in those flows. The old EA @todo comment that identified this exact problem is correctly resolved and removed.

Confidence Score: 5/5

Safe to merge; optimization is well-scoped and author-tested in multiplayer

All changes follow a consistent pattern: skip MSG_DESTROY_SELECTED_GROUP when nothing is selected, or when a MSG_CREATE_SELECTED_GROUP immediately replaces the group. Only one trivial P2 comment nit found. Multiplayer testing was done.

No files require special attention

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Include/Common/Player.h	Adds isCurrentlySelectedGroupEmpty() declaration; comment slightly misleading (only describes false return value)
GeneralsMD/Code/GameEngine/Source/Common/RTS/Player.cpp	Implements isCurrentlySelectedGroupEmpty() cleanly by delegating to m_currentSelection->getSizeOfGroup()
GeneralsMD/Code/GameEngine/Include/GameClient/InGameUI.h	Renames deselectAllDrawables parameter from postMsg to updateGameLogic for clarity; no functional change
GeneralsMD/Code/GameEngine/Source/GameClient/InGameUI.cpp	Core optimization: guards MSG_DESTROY_SELECTED_GROUP behind isCurrentlySelectedGroupEmpty() check and removes stale TODO comments; also passes FALSE in selectNextIdleWorker (followed immediately by MSG_CREATE_SELECTED_GROUP)
GeneralsMD/Code/GameEngine/Source/GameClient/MessageStream/SelectionXlat.cpp	Removes deselectAll() wrapper, inlines calls to deselectAllDrawables(); correctly uses FALSE where MSG_CREATE_SELECTED_GROUP follows immediately, and true (default) for pure-deselection paths
GeneralsMD/Code/GameEngine/Source/GameClient/MessageStream/CommandXlat.cpp	Five call sites updated to deselectAllDrawables(FALSE), all immediately followed by MSG_CREATE_SELECTED_GROUP, consistent with the new pattern
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/ControlBar/ControlBarCommandProcessing.cpp	Single call site updated to deselectAllDrawables(FALSE) before MSG_CREATE_SELECTED_GROUP; consistent with new pattern

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[deselectAllDrawables called] --> B{updateGameLogic?}
    B -- FALSE --> C[Visual deselect only\nno network message]
    B -- TRUE --> D{isCurrentlySelectedGroupEmpty?}
    D -- Yes, empty --> E[Skip MSG_DESTROY_SELECTED_GROUP\nno unnecessary message]
    D -- No, has units --> F[Send MSG_DESTROY_SELECTED_GROUP]

    C --> G{Caller follows with\nMSG_CREATE_SELECTED_GROUP?}
    G -- Yes --> H[Game logic replaces group\nvia CREATE message]
    G -- No --> I[Pure visual deselect\nno game-logic change]

    F --> J[Game logic destroys current group]

Loading

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
GeneralsMD/Code/GameEngine/Include/Common/Player.h:648-649
**Incomplete comment — true case not described**

The comment only documents the `false` return value ("returns false if player has object(s) currently selected") but says nothing about when the function returns `true`. Since the function name already implies the semantics, consider inverting the comment to make the "empty" / `true` case the primary description, which is more natural for an `isEmpty`-style predicate.

```suggestion
	// returns true if the player has no objects currently selected
	Bool isCurrentlySelectedGroupEmpty() const;
```

_{Reviews (2): Last reviewed commit: "Removed TSH comment." | Re-trigger Greptile}

[xezon]

Where do you see this override?

[Caball009]

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp

Line 1682 in b97d954

selectObject(obj, createNewGroup && firstObject, msgPlayer->getPlayerMask());

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp

Lines 2733 to 2752 in b97d954

	AIGroupPtr group = TheAI->createGroup();
	group->add(obj);
	// add all selected agents to the AI group
	if (createNewSelection)
	{
	#if RETAIL_COMPATIBLE_AIGROUP
	player->setCurrentlySelectedAIGroup(group);
	#else
	player->setCurrentlySelectedAIGroup(group.Peek());
	#endif
	}
	else
	{
	#if RETAIL_COMPATIBLE_AIGROUP
	player->addAIGroupToCurrentSelection(group);
	#else
	player->addAIGroupToCurrentSelection(group.Peek());
	#endif
	}

[xezon]

Hmm ok. There are more MSG_CREATE_SELECTED_GROUP messages sent with TheInGameUI->deselectAllDrawables() called before.

[Caball009]

Yes, good point. I'll update those as well.

Is there a way to make clear we're changing all of those cases without a comment for each one?

[Caball009]

Should this TheSuperHackers comment be removed instead?

[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