search

RetroPie / RetroPie-Setup

Shell script to set up a Raspberry Pi/Odroid/PC with RetroArch emulator and various cores
Other
10.07k stars 1.39k forks source link

xpad triggers_to_buttons=1 patch breaks SDL gamepad mappings #3379

Closed cgutman closed 8 months ago

cgutman commented 3 years ago

Hello, I am the upstream maintainer of Moonlight Embedded. I am working on tracking down a user-reported issue with Xbox 360/One gamepads being mapped incorrectly while running inside RetroPie (and working perfectly fine when launched outside of RetroPie).

I tracked down the cause to the custom xpad which unconditionally maps triggers to button rather than analog axes: https://github.com/RetroPie/RetroPie-Setup/blob/6d396a3d5b70417869eddcb6d3d74324d5fce689/scriptmodules/supplementary/xpad.sh#L49-L51

https://github.com/RetroPie/RetroPie-Setup/blob/6d396a3d5b70417869eddcb6d3d74324d5fce689/scriptmodules/supplementary/xpad/01_enable_leds_and_trigmapping.diff#L12-L27

The triggers_to_buttons option was never meant to be used in this way, exactly because it can break unaware userspace applications. Upstream xpad only allows it to be used for unknown gamepads (and even for that it is not recommended, because many applications and libraries like SDL have heuristic mappings for things resembling Xbox controllers that triggers_to_buttons=1 will break).

This screws up any SDL gamepad mappings used by applications (like Moonlight) running inside RetroPie, because the SDL gamepad mapping GUID is identical to the version where triggers are mapped as axes. As a result of the change in number of axes and buttons, the mapping becomes completely wrong (not just the triggers).

Additionally, it prevents the use of the triggers as analog inputs for things like racing games where that functionality is important.

Analog triggers are the norm for all modern gamepads (Xbox 360/One/Series X, DualShock 4, DualSense), and hid-sony/hid-playstation don't allow you to opt out of it.

Can we nuke this triggers_to_buttons=1 change?

cmitu commented 3 years ago

Thank you for the comprehensive write-up and the debugging done to diagnose the problem.

Looking at the history of the RetroPie's included xpad driver, looks like the reason for the patch was EmulationStation (the gaming front-end used by RetroPie) didn't properly support the trigger axis on the Xbox gamepads ([1]).

In order for the patch to be dropped - or just not install the modified xpad driver it by default - we'll have to make sure first that EmulationStation will support the trigger axis properly on Xbox (xpad supported) gamepads.

Note that the user can always uninstall the driver - the system will revert to the in-kernel xpad driver. Further updates will not re-install the driver automatically.

Removing the patch altogether without proper support would probably break upgrading users, which have a configuration based on the previous xpad driver behavior, so most likely once we have proper support in EmulationStation, we can just skip installing the modified driver on new installations and update our docs.

[1] xpad patch - https://github.com/RetroPie/RetroPie-Setup/commit/ab7871d155c8fe393b50dbc58ef10de65bf20e5a#diff-d3abe0095936112ef6abe7c5dce7d57b236a7a1222f877b9539cca0b5468f03a

joolswills commented 3 years ago

Sorry I didn't get back on this issue sooner but @cmitu covered everything. Thanks also for the detailed report.

cgutman commented 3 years ago

Thank you for the comprehensive write-up and the debugging done to diagnose the problem.

Looking at the history of the RetroPie's included xpad driver, looks like the reason for the patch was EmulationStation (the gaming front-end used by RetroPie) didn't properly support the trigger axis on the Xbox gamepads ([1]).

In order for the patch to be dropped - or just not install the modified xpad driver it by default - we'll have to make sure first that EmulationStation will support the trigger axis properly on Xbox (xpad supported) gamepads.

Yep, I did check that and it does work today. I believe it was fixed back in 2019 by https://github.com/RetroPie/EmulationStation/commit/93fdfaa9c2815edd34ed79883d5a983eefde17d1

This image was captured while mapping my Xbox One S controller without the modified xpad driver. The analog trigger axes from the stock xpad driver can be mapped successfully in EmulationStation: IMG_0861

Removing the patch altogether without proper support would probably break upgrading users, which have a configuration based on the previous xpad driver behavior, so most likely once we have proper support in EmulationStation, we can just skip installing the modified driver on new installations and update our docs.

Yeah I noticed that all the button mappings for EmulationStation were wrong after going back to the stock xpad. It made it tricky to get back into the input mapping menu to be able to fix it.

Without some special upgrade handling for old mappings, it definitely will break users if the default is changed in an update. It could probably be done by deleting any gamepad mappings where the name is in the list in xpad, but it certainly wouldn't be pretty.

Unless there are any better ideas, skipping the modified driver on new installs seems like a good compromise. It will at least stop new users from being affected.

cmitu commented 3 years ago

This image was captured while mapping my Xbox One S controller without the modified xpad driver. The analog trigger axes from the stock xpad driver can be mapped successfully in EmulationStation:

Thanks for testing - it looks promising. I'll have to find an Xbox controller to re-test this and check the input mapping scripts that RetroPie uses to configure RetroArch and a few other emulators based on the EmulationStation's config. If the code is there, then it will be easier to implement a resolution.

I'm more worried about clones and various 'smart' controllers (8Bitdo) with have multiple connection modes which may not function correctly, I know that we received a few complaints in the forums when the RetroPie xpad driver was not installed and the users complained about not being able to map the trigger axis.

HerbFargus commented 3 years ago

Historically there were trigger issues with some Logitech controllers as well which may also be impacted

cgutman commented 3 years ago

The logic introduced for mapping axis Z and axis RZ (2 and 5) in https://github.com/RetroPie/EmulationStation/commit/93fdfaa9c2815edd34ed79883d5a983eefde17d1 is controller-agnostic. It's not looking at the controller name or axis count, so third party xpad gamepads should not cause problems (even those with special mapping flags like MAP_DPAD_TO_BUTTONS or MAP_TRIGGERS_TO_BUTTONS).

xpad exposes the set of axes and buttons for all gamepads of each Xbox generation that it supports (subject to a few exceptions like fightsticks and dancepads with special MAP_* options in xpad's list).

I have previously worked on both paroj/xpad and the upstream Linux xpad, so if you find some specific issues, we can run them down together.

cmitu commented 3 years ago

So I tested an old Xbox One controller and @HerbFargus tested his old Xbox360 & friend controllers and we can confirm that EmulationStation behaves correctly and detects/maps the Z/RZ axis. Clearly things have improved since the xpad patch that forced the triggers_to_buttons module parameter as a workaround.

Now, I don't have other Xbox controllers, but I've looked at other similar/compatible controllers in the RetroArch's udev configuration profiles and they are quite a few of them (42 if I counted correctly). I could probably get a similar list from SDL's gamescontrollerdb, but I'm not so familiar with the syntax.

Some of the clones/fightsticks that are Xbox compatible don't correctly expose the full axis range as a genuine controller does (i.e. arcade sticks like MadCatz FightStick TE or Mad Catz BrawlStick). Some of them are handled by xpad, as referenced by @cgutman here, but exceptions can show up with clones or fightsticks (like the recently released 8BitDo arcade stick reported here in RetroPie's forums).

So, as far as EmulationStation is concerned, we could forego without installing the patched xpad by default.

As a deprecation plan, we could: