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

gamepad: native standard layout for Linux #2587

Merged
merged 1 commit into from
Mar 3, 2023

Conversation

divVerent
Copy link
Contributor

@divVerent divVerent commented Mar 3, 2023

What issue is this addressing?

Closes #2052

What type of issue is this addressing?

feature

What this PR does | solves

Implements native standard layout for Linux gamepads by using the kernel-provided button IDs, thereby expanding support to gamepads not listed in gamecontrollerdb.txt.

Requires #2588

Linux's docs: https://www.kernel.org/doc/Documentation/input/gamepad.txt

SDL2's source: https://fossies.org/linux/SDL2/src/joystick/linux/SDL_sysjoystick.c#l_1740

Note that I am NOT 100% convinced about the X/Y swap between Xbox and PlayStation controllers - the Xbox compatible pad I have however does have BTN_NORTH and BTN_WEST swapped (and thus BTN_X and BTN_Y assigned right), which confirms SDL's logic and opposes the kernel docs.

Tested with this gamepad: "20d6:2802 BDA Xbox ONE Core controller", label says "PowerA Model 1508491-02" - even after clearing out gamecontrollerdb.txt, examples/gamepad shows a 100% correct mapping.

@divVerent divVerent changed the title Linux stdlayout gamepad: native standard layout for Linux Mar 3, 2023
@divVerent divVerent force-pushed the linux_stdlayout branch 4 times, most recently from 46a20c7 to 9f8cdcb Compare March 3, 2023 02:58
internal/gamepad/gamepad.go Outdated Show resolved Hide resolved
internal/gamepad/gamepad.go Outdated Show resolved Hide resolved
internal/gamepad/gamepad_xbox_windows.go Outdated Show resolved Hide resolved
@hajimehoshi
Copy link
Owner

hajimehoshi commented Mar 3, 2023

The first commit is a refactor of the existing native standard layout system to provide the ability to implement buttons or axes using any of buttons, axes or hats. The second commit then implements the same mapping for Linux as SDL2.

In Ebitengine, we squash all the commits and rebase it onto the latest commit. So, if you don't want them to be squashed, please send a separate PR. (I recommend separating it for easy reviews)

@divVerent
Copy link
Contributor Author

Done that; note that I did not push to this branch, but rather pushed HEAD^ to the other PR, so if that PR is merged first, this one will no longer contain its first commit.

@divVerent
Copy link
Contributor Author

Damn, looks like all this work was for nothing - I had hoped you'd review this BEFORE splitting up into separate PRs so I don't get into merge conflict hell all the time. Now I will have to do lots of duplicate work :(

@hajimehoshi
Copy link
Owner

Damn, looks like all this work was for nothing - I had hoped you'd review this BEFORE splitting up into separate PRs so I don't get into merge conflict hell all the time. Now I will have to do lots of duplicate work :(

Sorry I don't understand the current situation. I want to review each PR one by one.

@divVerent
Copy link
Contributor Author

Yeah, it was a big mistake to try to already get this to work for Linux.

@divVerent divVerent force-pushed the linux_stdlayout branch 9 times, most recently from fccd6a8 to 75f4867 Compare March 3, 2023 13:20
hajimehoshi pushed a commit that referenced this pull request Mar 3, 2023
… in the per-platform implementations. (#2588)

Refactors native standard layout so standard axes and buttons can be implemented using any of axes, buttons or hats.

This is required to be able to implement a native standard layout mapping for Linux, as e.g. shoulder buttons can be
backed either by an analog or digital input.

Precedes #2587

Updates #2052
Copy link
Owner

@hajimehoshi hajimehoshi left a comment

Choose a reason for hiding this comment

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

Seems good.

internal/gamepad/gamepad_linux.go Show resolved Hide resolved
Copy link
Owner

@hajimehoshi hajimehoshi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@divVerent
Copy link
Contributor Author

One thing actually - if you want, I can change the standard mapping function to use ABS_HAT0X and ABS_HAT1X even when referring to the Y axis. That would allow removing some of the constants, and would allow to still skip the hat indexes.

Do you want me to do that? I find the current version cleaner as it's easier to compare to SDL's implementation.

@hajimehoshi
Copy link
Owner

One thing actually - if you want, I can change the standard mapping function to use ABS_HAT0X and ABS_HAT1X even when referring to the Y axis. That would allow removing some of the constants, and would allow to still skip the hat indexes.

Wouldn't this be a breaking change?

@divVerent
Copy link
Contributor Author

One thing actually - if you want, I can change the standard mapping function to use ABS_HAT0X and ABS_HAT1X even when referring to the Y axis. That would allow removing some of the constants, and would allow to still skip the hat indexes.

Wouldn't this be a breaking change?

It would not break as nothing would be accessing the fields for the second hat axis anymore (like before), but it'd be more confusing (but shorter code).

For that reason I'd prefer keeping it the way it is now in this change.

Note that gamepaddb entries still have priority.
@divVerent
Copy link
Contributor Author

divVerent commented Mar 3, 2023 via email

@hajimehoshi
Copy link
Owner

It would not break as nothing would be accessing the fields for the second hat axis anymore (like before), but it'd be more confusing (but shorter code).

Hm I see. So skipping the values might be very slightly efficent but the code would be hacky, right? I prefer the current style then.

Copy link
Owner

@hajimehoshi hajimehoshi left a comment

Choose a reason for hiding this comment

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

Still LGTM

@hajimehoshi
Copy link
Owner

Done that now (replying here as I can't find this comment on github).

Sorry I removed the comment. Thank you for adding the comment!

@hajimehoshi hajimehoshi merged commit 06c1414 into hajimehoshi:main Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[linux] support gamepad standard layout without mapping when mapping is provided by kernel
2 participants