fix(metaevent): Ignore order in which modifier keys are released to trigger meta events

[xezon]

• Merge after unify(gamemessage): Merge MessageStream, MetaEvent code #2756

This change ignores the order in which keys are released to trigger meta events.

This is useful because players do not necessarily and intuitively release the keys in the expected strict order. For example CTRL + A press, A + CTRL release. Trouble can arise when releasing CTRL before A, because then the mapped meta event would not trigger.

This is no issue in the original game because it has no meta event mapping on Release and Modifier, but it can be on Mods or Debug builds, for example with #2546 adding a Comma + CTRL release event.

TODO

• Replicate in Generals
• Test key mappings and key actions in game

[greptile-apps]

Greptile Summary

This PR fixes the order-dependent release bug in meta event triggering by replacing the old m_lastKeyDown/m_lastModState pair with a KeyDownInfo[KEY_COUNT] bitfield array that remembers which modifier combinations were active when each key was pressed. When a modifier key is released, the new modStateRemoved path scans all held keys and fires UP events for any combination whose modifiers are no longer fully active — regardless of which key was released first.

• KeyDownInfo struct (both headers): encodes up to 7 modifier-state combinations (CTRL, ALT, SHIFT, and all 3 pairs/triple) in a single UnsignedByte; index↔modState mapping is symmetric and safe from overflow in the loop (getMaxKeyModStateCount() = 7, bit 7 is never touched).
• translateGameMessage (both .cpp files): splits handling into a modStateRemoved path (modifier key UP → scan m_keyDownInfos) and a normal path (any other key event → fire event, then store/clear entry in m_keyDownInfos); the clearKeyModState call in the normal key-up path prevents the double-fire that would otherwise occur on the standard key-before-modifier release order.

Confidence Score: 5/5

Safe to merge; the core logic correctly handles all release orderings and the double-fire edge case is properly prevented by clearing the entry in the normal key-up path.

The new modStateRemoved scan combined with the clearKeyModState call in the normal UP path ensures each mapped UP event fires exactly once regardless of release order. The BitsAreSet check correctly uses the same bitmask values used to build newModState, so multi-modifier comparisons are sound. Only minor style issues were found.

Both .cpp files have a cosmetic indentation regression in the else block that is worth fixing before merge, but neither affects correctness.

Important Files Changed

Filename	Overview
Core/Libraries/Include/Lib/BaseType.h	Adds BitsAreSet macro — simple, correct, and complementary to existing BitIsSet.
Generals/Code/GameEngine/Include/GameClient/MetaEvent.h	Replaces m_lastKeyDown/m_lastModState with KeyDownInfo[KEY_COUNT], encoding which modifier combinations were active when each key was pressed. Index mapping and bitfield fit 7 combinations cleanly in a single byte.
Generals/Code/GameEngine/Source/GameClient/MessageStream/MetaEvent.cpp	Core logic rewrite: splits key-up handling into a modStateRemoved path (modifier released, scans all held keys) and a normal path. Logic is functionally sound, but the else block body has an indentation regression — the for-loop and inner if/else sit at the same indent level as else itself, making the block structure visually misleading.
GeneralsMD/Code/GameEngine/Include/GameClient/MetaEvent.h	Exact mirror of Generals/Code version; same KeyDownInfo struct added, same analysis applies.
GeneralsMD/Code/GameEngine/Source/GameClient/MessageStream/MetaEvent.cpp	Mirror of Generals .cpp; carries the same indentation inconsistency in the else block.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[MSG_RAW_KEY_DOWN / MSG_RAW_KEY_UP] --> B{Is systemKey a\nmodifier key?}
    B -- Yes --> C[key = MK_NONE]
    B -- No --> D[key = systemKey]
    C --> E[Compute newModState\nfrom keyState flags]
    D --> E
    E --> F{modStateRemoved?\nkey==MK_NONE &&\nt==KEY_UP}
    F -- Yes --> G[Scan ALL m_keyDownInfos]
    G --> H{entry.isKeyDown?}
    H -- No --> I[next key]
    H -- Yes --> J{BitsAreSet\nnewModState,\nkeyDownModState?}
    J -- Yes, mod still active --> K[skip — combination still held]
    J -- No, mod removed --> L[clearKeyModState\nfire UP meta-event]
    L --> I
    F -- No --> M[Iterate MetaMapRec list]
    M --> N{map matches\nkey+modState+transition?}
    N -- No --> O[next map]
    N -- Yes --> P[Fire meta-event\nbreak]
    P --> Q{t == KEY_DOWN?}
    Q -- Yes, newModState!=NONE --> R[m_keyDownInfos key\n.setKeyModState]
    Q -- No KEY_UP, newModState!=NONE --> S[m_keyDownInfos key\n.clearKeyModState]

Loading

Prompt To Fix All With AI

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

---

### Issue 1 of 2
Generals/Code/GameEngine/Source/GameClient/MessageStream/MetaEvent.cpp:518-524
The body of the `else` block is indented at the same level as the `else` keyword itself rather than one level deeper. The comment at the top of the block is correctly indented to 3 tabs, but the `for` loop and the `if (t == GameMessage::MSG_RAW_KEY_DOWN)` block that follow are both at 2 tabs — making the structure visually misleading. The same pattern repeats in `GeneralsMD/Code/GameEngine/Source/GameClient/MessageStream/MetaEvent.cpp`.

```suggestion
		else
		{
			// TheSuperHackers @info The regular key handler only triggers events when the mapped key is pressed,
			// not when the modifier (CTRL, ALT, SHIFT) is pressed, unless the key is MK_NONE.

			for (const MetaMapRec *map = TheMetaMap->getFirstMetaMapRec(); map; map = map->m_next)
			{
```

### Issue 2 of 2
Generals/Code/GameEngine/Include/GameClient/MetaEvent.h:415-418
`hasKeyModState` is declared but never called anywhere in the translation units. If it is not needed for the current implementation, removing it avoids dead-code accumulation. The same method exists in `GeneralsMD/Code/GameEngine/Include/GameClient/MetaEvent.h`.

_{Reviews (5): Last reviewed commit: "Replicate in Generals" | Re-trigger Greptile}

[xezon]

greptile claims "stale-bit bug in the normal KEY_UP path" but there is no bug and it works as intended.

[Mauller]

Do you think we can extract some of the inner loop handling?
The levels of indentation can make it hard to follow.

[xezon]

Yes I wanted to make a follow up change that refactors this function because it is quite a bit long.

[Skyaero42]

This looks good to go

[xezon]

Greptile claims "Missing break after dispatch in modStateRemoved inner loop" but this is as designed, because multiple events could trigger if a key modifier is released.

[xezon]

DrGoldfish:

first sight it looks normal will do some more testing
looks all normal tbh

[xezon]

MetaEvent replicate is conflicting. Looks like Generals needs merging first.

[xezon]

Replicated in Generals without conflicts