fix(MouseHandler): route XButtons (Mouse4/5) as keybinds instead of mouse ownership

[color-dev]

Summary

Adjusts MouseHandler so that XButtons (Mouse4/5) are routed like keybinds instead of sharing mouse ownership.

Rationale

When bound through uikeys, Mouse4/5 attempt to assign ownership to GameInputReceiver. This causes misrouting when pressed in combination with other mouse inputs. By routing them through the keybind path and bypassing ownership, XButtons can be pressed/released without stealing or confusing the active receiver.

Scope

• Changes limited to MouseHandler.cpp
• No new binds or widget logic introduced
• Purely input routing fix

Testing

• Verified bound mouse4 through uikeys allowed simultaneous mouse presses without skipped inputs of mouse4 or LMB
• Existing mouse button behavior unchanged

Notes / Dependencies

• Mouse Buildspacing widget currently hardcodes MOUSE4/MOUSE5 for buildspacing changes; it should be disabled if testing MOUSE4/MOUSE5 with the uikeys system.
  gui_buildspacing.lua

[badosu]

Hi!

Would you be so kind as to answer a few questions?

1. Would you mind briefly describing how this ties in (or not) with the native Engine keybinding system?

• Does the keybinding system enabled by this change require some particular gameside changes? (e.g. widget/manager)
• Is this via native engine support. For example: bind <mousetrigger> say hi? What are the mousetrigger options?

2. Am I correct in assuming currently the MousePress is handled "correctly" and the change is motivated specifically to allow MouseRelease to be received by skipping the ownership mechanism for those specific buttons?

Thanks for contributing to Recoils input system!

[color-dev]

1. Really I discovered this incompatibility once I deactivated 'Mouse Buildspacing' widget–after that, I tested 'bind mouse4 boxbuild' and that the action is triggered by mouse4. It works fine but yeah, the gameside widget Mouse Buildspacing also causes mouse4 binding to misbehave. That is because the widget simply hardcodes the mouse4 and mouse5 buttons to change the buildspacing. That is a second blocker.

2. The mousepress in this file currently manages all mousepresses together, tying them to a receiver. This works correctly for the LMB, RMB, etc., but fails when using the Mouse4&5 as binds. This is where there is for example a chance for the LMB release to go to 'GameInputReceiver' (set in MousePress) because mouse4 had a keybind. If activeReceiver wasn't locked in during MousePress-with-xbutton, that would solve the LMB RMB etc side (the first changed section). But then the XButton release wouldn't know where to go. That is why there also is routing for them in receiver order as is done with keypresses (the second changed section).

Another possible bug this handles is if the player presses LMB down and the activeReceiver is set to anywhere, the release of keybind mouse4 will get sent there and not back to the keybind system

So in short, yes, the problem starts in MousePress and is noticed with missed input MouseReleases

[badosu]

Alright, tell me if I understood incorrectly:

• The actual change here is to allow skipping the mouse ownership mechanism to enable a gameside implementation (what you call "Mouse Buildspacing" widget). One issue I see with current code is that you aren't relaying MouseMove as well.
• This change has nothing to do with engine native mouse keybindings except some possible mechanism you're using to set some modifier.

While I have assessed in the past the current mouse ownership system is very restrictive and demands some rethinking I have a few points to address:

• Would you mind clarifying how the current mouse ownership system blocks your ability to change the game to support your feature?
• It seems to me your problem is a widget capturing the mousepress earlier and that this might be able to be addressed without any engine changes. If I am incorrect please share the pieces of code involved.

Remark

Similar to #2603, in your "Notes / Dependencies" section please provide all context necessary for engine devs to understand without assuming specific BAR knowledge.

[color-dev]

This is for allowing the mouse4/5 buttons to work well with the uikeys keybinding system, which currently results in missed inputs when the mouse4/5 buttons are bound, and the LMB RMB etc. are pressed at the same time (a common input)

Actually, it doesn't enable Mouse Buildspacing widget, Mouse Buildspacing is already loaded in BAR. That widget also ultimately causes problems when you want to bind mouse4/5 it is aka gui_buildspacing.lua.

This has to do with the uikeys.txt keybindings system, specifically for mouse4/5

Here for example LMB can set activeReceiver

RecoilEngine/rts/Game/UI/MouseHandler.cpp

Lines 377 to 381 in 3f0f87a

	if (guihandler != nullptr && guihandler->MousePress(x, y, button)) {
	if (activeReceiver == nullptr)
	activeReceiver = guihandler; // for default (rmb) commands
	}
	}

Or here for mouse4 when something like "bind mouse4 boxbuild" is in uikeys.txt

RecoilEngine/rts/Game/UI/MouseHandler.cpp

Lines 366 to 370 in 3f0f87a

	auto activeControllerReceiver = (activeController == nullptr) ? nullptr : activeController->GetInputReceiver();
	if (button >= ACTION_BUTTON_MIN && activeControllerReceiver && activeControllerReceiver->MousePress(x, y, button)) {
	activeReceiver = activeControllerReceiver;
	return;
	}

Gets set to GameInputReceiver

Then on mouserelease (any release, not linked to which button set the receiver)

RecoilEngine/rts/Game/UI/MouseHandler.cpp

Lines 506 to 513 in 3f0f87a

	if (activeReceiver != nullptr) {
	activeReceiver->MouseRelease(x, y, button);
	if (!buttons[SDL_BUTTON_LEFT].pressed && !buttons[SDL_BUTTON_MIDDLE].pressed && !buttons[SDL_BUTTON_RIGHT].pressed)
	activeReceiver = nullptr;
	return;
	}

Since widgets would be working with actions, they would still be affected by the misrouting that causes the bound mouse4 mouserelease to not get sent to GameInputReceiver (as if it's not released), or the LMB to be sent to GameInputReceiver instead of the correct location when pressed in combination

[badosu]

This is for allowing the mouse4/5 buttons to work well with the uikeys keybinding system, which currently results in missed inputs when the mouse4/5 buttons are bound, and the LMB RMB etc. are pressed at the same time (a common input)
...
This has to do with the uikeys.txt keybindings system, specifically for mouse4/5

How specifically? I still don't know how this change interacts with engine and gameside defined actions and bindings. I need further clarification to understand the use case.

Or here for mouse4 when something like "bind mouse4 boxbuild" is in uikeys.txt

Is the mouse4 keyset handling shown here performed by the game or is it using the native mouse keybinding system? Are you able to provide this information?

Since widgets would be working with actions, they would still be affected by the misrouting that causes the bound mouse4 mouserelease to not get sent to GameInputReceiver (as if it's not released), or the LMB to be sent to GameInputReceiver instead of the correct location when pressed in combination

This makes sense. I still require some clarification on the points above but I can anticipate some issues with the ownership model.

In particular, assuming we are using engine defined actions and the engine supports the mouse keysets natively, we should send the list of actions on mousepress/mouserelease. This way the game itself can skip the ownership model.

With the paragraph above in mind, it's likely some current issue with the ownership model would indeed have to be addressed on the engine. Before reaching that I'd like to understand what and how is the current gameside implementation attempt better as I asked in this message above and in other messages.

[color-dev]

Once the mouse4 press gets sent to GameInputReceiver,

RecoilEngine/rts/Game/GameInputReceiver.cpp

Lines 89 to 105 in 3f0f87a

	bool CGameInputReceiver::MousePress(int x, int y, int button)
	{
	int keyCode = CKeyCodes::GetMouseButtonSymbol(button);
	int scanCode = CScanCodes::GetMouseButtonSymbol(button);
	const CKeySet kc(keyCode, CKeySet::KSKeyCode);
	const CKeySet ks(scanCode, CKeySet::KSScanCode);
	bool isRepeat = false;
	const auto now = spring_gettime();
	curKeyCodeChain.push_back(kc, now, isRepeat);
	curScanCodeChain.push_back(ks, now, isRepeat);
	lastActionList = keyBindings.GetActionList(curKeyCodeChain, curScanCodeChain);
	return TryOnPressActions(isRepeat);
	}

It consumes the press

RecoilEngine/rts/Game/GameInputReceiver.cpp

Lines 123 to 134 in 3f0f87a

	bool CGameInputReceiver::TryOnPressActions(bool isRepeat)
	{
	// try our list of actions
	for (const Action& action: lastActionList) {
	if (game->ActionPressed(action, isRepeat)) {
	return true;
	}
	}
	// maybe a widget is interested?
	// allowing all listeners to process for backwards compatibility.
	bool handled = false;

Resolving the bound action via TryOnPressActions, and because the event was handled, the engine designates GameInputReceiver as the activeReceiver. These are on the engine side.

It's not really about the fact that the press is or is not consumed when bound using uikeys (though it would have to be consumed for the release event to reach the action system currently)
It's that the ownership/routing of the mouse release of all mouse buttons is set as soon as one press event is consumed, in the contexts mentioned (and it stays until LMB, RMB, and MMB are released)

By routing them in a focused, isolated path, mouse4/5 behave like keybinds in that they are sent to receivers in order and do not interact with other key press and release events, other than optionally stopping further propagation of just their own event, down the route, by consuming the press. The receivers don't "subscribe" to the release event of the key; it's a broadcast model
The benefit being that, like a keybind, mouse4/5 presses do not interfere with or are interfered by LMB MMB RMB

[sprunk]

Looks good. Some remarks:

• existing clients might assume mouse ownership anyway, in particular Lua. This would only affect people trying to use mouse4/5 though. So I think it's fine.
• maybe using MousePress at all for something meant to be used like a keyboard key is a bad idea in the first place, but again we probably have to keep as-is anyway since KeyPress doesn't sound like a great choice either and it would break back-compat even more. So this is also fine.
• would probably be a bit better to do != LMB MMB RMB, rather than == X1 X2, since that would handle any other extra keys in the future, but this is a nit and is trivial to change if needed. Also fine.

Provisional +1 from me but I would still like @badosu's feedback.