tweak(gamemessage): Remove unused MSG_AREA_SELECTION message from game logic#2667
tweak(gamemessage): Remove unused MSG_AREA_SELECTION message from game logic#2667Caball009 wants to merge 2 commits intoTheSuperHackers:mainfrom
Conversation
|
| Filename | Overview |
|---|---|
| GeneralsMD/Code/GameEngine/Include/Common/MessageStream.h | Adds MSG_META_AREA_SELECTION to the META (client-local) section and renames MSG_AREA_SELECTION to MSG_AREA_SELECTION_DEPRECATED to preserve enum ordinals for network/replay compatibility. |
| GeneralsMD/Code/GameEngine/Source/Common/MessageStream.cpp | Updates getCommandTypeAsString to register the new MSG_META_AREA_SELECTION label and rename MSG_AREA_SELECTION to MSG_AREA_SELECTION_DEPRECATED. |
| GeneralsMD/Code/GameEngine/Source/GameClient/MessageStream/HintSpy.cpp | Updates case label from MSG_AREA_SELECTION to MSG_META_AREA_SELECTION; the adjacent comment still refers to the old name. |
| GeneralsMD/Code/GameEngine/Source/GameClient/MessageStream/SelectionXlat.cpp | Both appendMessage call sites updated from MSG_AREA_SELECTION to MSG_META_AREA_SELECTION, correctly preventing the selection message from being forwarded to other clients. |
Sequence Diagram
sequenceDiagram
participant User
participant SelectionXlat
participant MessageStream
participant HintSpy
participant Network
User->>SelectionXlat: drag-select / click-select
SelectionXlat->>MessageStream: appendMessage(MSG_AREA_SELECTION_HINT)
MessageStream->>HintSpy: MSG_AREA_SELECTION_HINT
HintSpy->>HintSpy: beginAreaSelectHint()
User->>SelectionXlat: release / confirm
SelectionXlat->>MessageStream: appendMessage(MSG_META_AREA_SELECTION)
note over MessageStream: META messages stay local
MessageStream->>HintSpy: MSG_META_AREA_SELECTION
HintSpy->>HintSpy: endAreaSelectHint()
MessageStream--xNetwork: NOT forwarded (was MSG_AREA_SELECTION before)
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/Source/GameClient/MessageStream/HintSpy.cpp:105
**Stale comment — enum name not updated**
The comment still says `AREA_SELECTION` but the case now matches `MSG_META_AREA_SELECTION`. It's a minor inaccuracy that could confuse future readers.
```suggestion
// An AREA_SELECTION_HINT is always followed by a MSG_META_AREA_SELECTION, so
```
Reviews (2): Last reviewed commit: "Addressed feedback." | Re-trigger Greptile
| MSG_COMBATDROP_AT_LOCATION, ///< dump out all rappellers | ||
| MSG_COMBATDROP_AT_OBJECT, ///< dump out all rappellers | ||
|
|
||
| // TheSuperHackers @todo MSG_AREA_SELECTION can be moved out of the network messages because it's currently unused by the game logic. |
There was a problem hiding this comment.
Can it already be moved out and then a MSG_AREA_SELECTION_DEPRECATED placed here to preserve the order?
There was a problem hiding this comment.
That sounds like a good idea. I'll check it out.
There was a problem hiding this comment.
Do you have a suggestion where the non-network version should go?
EDIT:
// "meta" messages should be thought of as "virtual keystrokes" -- they exist
// solely to provide an abstraction layer useful for keyboard/mouse remapping.
// they should NEVER be sent over the network.
I suppose it fits this description, because the client uses it for the selection of the drawables and then generates the necessary GameMessages to update the selection for the game logic.
MSG_META_SELECT_HERO, ///< selects player's hero character, if exists...
MSG_META_SELECT_ALL, ///< selects all units across screen
MSG_META_SELECT_ALL_AIRCRAFT, ///< selects all air units just like select all
MSG_META_SCATTER,
Perhaps between MSG_META_SELECT_ALL_AIRCRAFT and MSG_META_SCATTER?
There was a problem hiding this comment.
There is a MSG_AREA_SELECTION_HINT outside network messages.
There was a problem hiding this comment.
That has a distinct purpose, right?
There was a problem hiding this comment.
It's used when you start to create an area selection by dragging the mouse. When you release the mouse button a MSG_AREA_SELECTION message is created.
There was a problem hiding this comment.
Why can it not be grouped together with MSG_AREA_SELECTION_HINT ?
There was a problem hiding this comment.
Because it's not a hint? Unless you want me to change it to a hint? I'd be fine with that.
MSG_AREA_SELECTION_HINT->MSG_BEGIN_AREA_SELECTION_HINTMSG_AREA_SELECTION->MSG_END_AREA_SELECTION_HINT
This works well for consistency.
| case GameMessage::MSG_AREA_SELECTION: | ||
| case GameMessage::MSG_META_AREA_SELECTION: | ||
| TheInGameUI->endAreaSelectHint( msg ); | ||
| break; |
There was a problem hiding this comment.
FYI this message is now outside the network message range, so it'll get removed eventually, and won't be sent to other clients.
|
I could make the following changes if that's cleaner:
|
xezon
left a comment
There was a problem hiding this comment.
The disconnect in grouping for the 2 Area select messages is confusing.
|
|
||
| // TheSuperHackers @todo MSG_AREA_SELECTION can be moved out of the network messages because it's currently unused by the game logic. | ||
| MSG_AREA_SELECTION, ///< (pixelRegion) rectangular selection area | ||
| // TheSuperHackers @todo Remove MSG_AREA_SELECTION_DEPRECATED when possible. |
There was a problem hiding this comment.
Todo comment is not necessary.
Instead
MSG_AREA_SELECTION_DEPRECATED, ///< TheSuperHackers @tweak former MSG_AREA_SELECTION is deprecated as network message
| MSG_COMBATDROP_AT_LOCATION, ///< dump out all rappellers | ||
| MSG_COMBATDROP_AT_OBJECT, ///< dump out all rappellers | ||
|
|
||
| // TheSuperHackers @todo MSG_AREA_SELECTION can be moved out of the network messages because it's currently unused by the game logic. |
There was a problem hiding this comment.
Why can it not be grouped together with MSG_AREA_SELECTION_HINT ?
| @@ -104,12 +104,8 @@ GameMessageDisposition HintSpyTranslator::translateGameMessage(const GameMessage | |||
| //----------------------------------------------------------------------------- | |||
| // An AREA_SELECTION_HINT is always followed by an AREA_SELECTION, so | |||
GameMessage::MSG_AREA_SELECTIONis currently passed on to the game logic (and sent to other clients), but it isn't used for anything. This PR changes that so that it isn't sent to other clients anymore.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: