Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to use Mouse 4 and Mouse 5 as hotkeys #20811

Merged
merged 1 commit into from
Apr 28, 2023

Conversation

natelastname
Copy link
Contributor

This pull request adds the ability to use mouse 4 and mouse 5 as hotkeys.

Certain models of high-end gaming mice can map the side buttons to keyboard inputs which can be used as hotkeys (e.g., "#" or "%"), so this change makes the game more fair to players that use standard mice.

The reason why mouse4 and mouse5 were previously not usable as hotkeys is because a one-to-one hotkey to keycode mapping was enforced by the input handling code. This was an artificial restriction; there is no technical reason why the internal enum OpenRA.Keycode has to correspond bijectively to SDL keycodes.

Thank you and please let me know if you need any additional information

@natelastname
Copy link
Contributor Author

Changelog entry:

Added ability to use Mouse 4 and Mouse 5 as hotkeys

@PunkPun
Copy link
Member

PunkPun commented Apr 17, 2023

Interesting, wouldn't it make sense to add more mouse buttons? Surely we shouldn't be limiting ourselves to just 2

@natelastname
Copy link
Contributor Author

natelastname commented Apr 17, 2023

Interesting, wouldn't it make sense to add more mouse buttons? Surely we shouldn't be limiting ourselves to just 2

Mouse1(left), Mouse2 (middle), Mouse3 (right) are somewhat special in the code, and it's probably not necessary to mes with them. This PR handles Mouse4 and Mouse5 (which previously were ignored.) I don't think there are any other mouse buttons that SDL2 can return: https://wiki.libsdl.org/SDL2/SDL_MouseButtonEvent

@natelastname
Copy link
Contributor Author

Just fixed a minor bug. Now, mouse4 and mouse5 are theoretically handled exactly the same way as any other keyboard input events.

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some style issues, commented them below. To test style locally you can run the make check command.

Also could you please rebase on upstream/bleed? We don't do merge commits

OpenRA.Platforms.Default/Sdl2Input.cs Outdated Show resolved Hide resolved
OpenRA.Platforms.Default/Sdl2Input.cs Outdated Show resolved Hide resolved
OpenRA.Platforms.Default/Sdl2Input.cs Outdated Show resolved Hide resolved
OpenRA.Platforms.Default/Sdl2Input.cs Show resolved Hide resolved
OpenRA.Platforms.Default/Sdl2Input.cs Show resolved Hide resolved
@natelastname
Copy link
Contributor Author

Thank you for the feedback. I believe I have just fixed these problems (merge commit + linting issues.)

@TheChosenEvilOne
Copy link
Contributor

Interesting, wouldn't it make sense to add more mouse buttons? Surely we shouldn't be limiting ourselves to just 2

Mouse1(left), Mouse2 (middle), Mouse3 (right) are somewhat special in the code, and it's probably not necessary to mes with them. This PR handles Mouse4 and Mouse5 (which previously were ignored.) I don't think there are any other mouse buttons that SDL2 can return: https://wiki.libsdl.org/SDL2/SDL_MouseButtonEvent

I tested this in Linux, SDL does return mouse buttons up to 8 (tested using a device with buttons up to 10, it looks like the limitation is X rather than SDL though).

@natelastname
Copy link
Contributor Author

Interesting, wouldn't it make sense to add more mouse buttons? Surely we shouldn't be limiting ourselves to just 2

Mouse1(left), Mouse2 (middle), Mouse3 (right) are somewhat special in the code, and it's probably not necessary to mes with them. This PR handles Mouse4 and Mouse5 (which previously were ignored.) I don't think there are any other mouse buttons that SDL2 can return: https://wiki.libsdl.org/SDL2/SDL_MouseButtonEvent

I tested this in Linux, SDL does return mouse buttons up to 8 (tested using a device with buttons up to 10, it looks like the limitation is X rather than SDL though).

So, you saying it should also handle mouse 6-8? I could implement that.

@abcdefg30
Copy link
Member

Seems like our SDL wrapper just exposes X1 and X2, so I would limit it to that.

abcdefg30
abcdefg30 previously approved these changes Apr 24, 2023
Copy link
Member

@abcdefg30 abcdefg30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't test the two new buttons but the related code changes lgtm.

@TheChosenEvilOne
Copy link
Contributor

TheChosenEvilOne commented Apr 24, 2023

Seems like our SDL wrapper just exposes X1 and X2, so I would limit it to that.

I looked a bit more into it.
SDL only has defines for those (https://github.com/libsdl-org/SDL/blob/SDL2/include/SDL_mouse.h#L449), and it seems like it only supports mouse 6 and above under Linux with evdev (https://github.com/libsdl-org/SDL/blob/SDL2/src/core/linux/SDL_evdev.c#L137) or X11 (https://github.com/libsdl-org/SDL/blob/SDL2/src/video/x11/SDL_x11events.c#L1363).
It doesn't look like the the extra buttons work under Windows (https://github.com/libsdl-org/SDL/blob/SDL2/src/video/windows/SDL_windowsevents.c#L361) or Wayland (https://github.com/libsdl-org/SDL/blob/SDL2/src/video/wayland/SDL_waylandevents.c#L672) in Linux.

@PunkPun
Copy link
Member

PunkPun commented Apr 24, 2023

Could you squash the commits?

@natelastname
Copy link
Contributor Author

Commits are now squashed

@PunkPun PunkPun merged commit 8f511a3 into OpenRA:bleed Apr 28, 2023
@PunkPun
Copy link
Member

PunkPun commented Apr 28, 2023

Changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants