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

[xezon]

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 replaces the simple m_lastKeyDown/m_lastModState pair with a per-key KeyDownInfo bitfield that tracks every (key, modState) combination that was pressed. When a modifier key is released, the modStateRemoved path iterates all tracked entries and fires the appropriate UP meta events, so players can release keys in any order without losing a mapped action.

• P1 — stale bit when modifier added mid-hold: clearKeyModState(newModState) in the normal KEY_UP path clears the bit for the current modState (e.g. CTRL_ALT bit 3), but the stored bit was written at press time with the original modState (e.g. CTRL bit 0). If a new modifier was added while the key was still held, the stored bit is never cleared, causing a spurious (key, original_modState, UP) event to fire from modStateRemoved after the key has already been physically released. Resetting the entire KeyDownInfo entry on key-up is the simplest correct fix.

Confidence Score: 4/5

Safe to merge after addressing the stale-bit bug in the normal KEY_UP path; the core modifier-release-order logic is correct for the primary use cases.

One P1 logic bug found: the normal KEY_UP path clears the wrong bit when a modifier is added after a key is pressed, leaving a stale entry that later fires a spurious UP event. The fix is a one-liner. All other findings are P2 or style-only.

GeneralsMD/Code/GameEngine/Source/GameClient/MessageStream/MetaEvent.cpp — specifically the normal KEY_UP path's clearKeyModState call.

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Source/GameClient/MessageStream/MetaEvent.cpp	Core change: replaces last-key/last-modState tracking with a per-key bitfield to fire UP events regardless of modifier-release order. The reverse-order case is correctly handled, but the normal UP path clears only the current-modState bit rather than all recorded bits, leaving a stale bit when a modifier is added after the key is pressed.
GeneralsMD/Code/GameEngine/Include/GameClient/MetaEvent.h	Introduces the KeyDownInfo struct with a compact UnsignedByte bitfield covering all 7 CTRL/ALT/SHIFT combinations; index mapping and bit operations are consistent and correct.
Core/Libraries/Include/Lib/BaseType.h	Adds BitsAreSet macro (all-bits-set check) adjacent to the existing bit-twiddling macros; semantics are correct and the expansion is safe.

Sequence Diagram

sequenceDiagram
    participant KB as Keyboard
    participant MET as MetaEventTranslator
    participant MS as MessageStream

    Note over KB,MS: Reverse-order release (modifier first) - now handled correctly
    KB->>MET: KEY_DOWN A with CTRL
    MET->>MET: setKeyModState(A, CTRL)
    KB->>MET: KEY_UP CTRL - modStateRemoved=true
    MET->>MET: BitsAreSet(0,CTRL)=false - fire and clear CTRL bit
    MET->>MS: Append meta event (A, CTRL, UP)
    KB->>MET: KEY_UP A - newModState=NONE, no-op

    Note over KB,MS: Normal-order release (key first) - double-fire prevented
    KB->>MET: KEY_DOWN A with CTRL
    MET->>MET: setKeyModState(A, CTRL)
    KB->>MET: KEY_UP A while CTRL held
    MET->>MS: Append meta event (A, CTRL, UP)
    MET->>MET: clearKeyModState(A, CTRL) - entry now empty
    KB->>MET: KEY_UP CTRL - modStateRemoved=true
    MET->>MET: isKeyDown(A)=false - skip, no duplicate

    Note over KB,MS: Bug - modifier added between press and release
    KB->>MET: KEY_DOWN A with CTRL
    MET->>MET: setKeyModState(A, CTRL) - bit0 set
    KB->>MET: KEY_DOWN ALT while A held
    KB->>MET: KEY_UP A with CTRL+ALT held
    MET->>MET: clearKeyModState(A, CTRL_ALT) - bit3 no-op, bit0 still set
    KB->>MET: KEY_UP CTRL - modStateRemoved=true, newModState=ALT
    MET->>MET: BitsAreSet(ALT,CTRL)=false - stale bit0 triggers
    MET->>MS: Spurious meta event (A, CTRL, UP) after A already released

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameClient/MessageStream/MetaEvent.cpp
Line: 598-603

Comment:
**Stale mod-state bit when modifier is added between key-press and key-release**

The stored bit for `key` was written with the modState at *press* time (e.g. `CTRL`, bit 0), but the clear here uses `newModState` at *release* time (e.g. `CTRL_ALT`, bit 3). If a modifier was added while the key was still held, `toIndex(CTRL_ALT) = 3 ≠ 0`, so the clear is a no-op and the `CTRL` bit stays set. When `CTRL` is subsequently released, `modStateRemoved` finds that stale bit and fires a `(KEY_A, CTRL, UP)` event even though `A` was already physically released.

Reproduce: hold `CTRL`, press `A`, press `ALT`, release `A`, release `CTRL` — the `(KEY_A, CTRL, UP)` event fires spuriously in the last step.

Fix: clear **all** recorded bits for the released key rather than just the current-modState bit, since the physical key is no longer pressed:
```suggestion
		else
		{
			DEBUG_ASSERTCRASH(key != MK_NONE, ("Key is expected to be not MK_NONE"));

			// Forget all recorded mod states for this released key.
			m_keyDownInfos[key] = KeyDownInfo();
		}
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameClient/MessageStream/MetaEvent.cpp
Line: 502-512

Comment:
**Missing `break` after dispatch in `modStateRemoved` inner loop**

The normal key-handling path always `break`s after the first matching meta-map entry (line 575). The `modStateRemoved` inner loop has no such break, so if the meta map ever contains more than one entry with the same `(key, modState, UP)` tuple but different `usableIn` values that are both currently usable, all matching events are fired. Consider adding a `break` after `disp = DESTROY_MESSAGE;` to match the normal-path contract.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (4): Last reviewed commit: "Fix clearKeyModState and remove m_lastMo..." | Re-trigger Greptile}

[Mauller]

some indentation has gone wrong around here

[xezon]

Yes this was intentional to reduce the complexity of the diff. If I change the indentation it will probably look complicated in the diff. I was thinking to make a follow up refactor and cleaning the function a bit.

[xezon]

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