atar-axis / xpadneo

Advanced Linux Driver for Xbox One Wireless Controller (shipped with Xbox One S)
https://atar-axis.github.io/xpadneo/
GNU General Public License v3.0
1.88k stars 110 forks source link

"Replace combined_z_axis with additional axis" seems to break controller detection in (older?) Unity games (Escapists 2, Bloody Rally Show) #345

Closed moll closed 5 months ago

moll commented 2 years ago

Hey,

I'm not 100% sure what's going on here, but perhaps you can offer some troubleshooting tips to get to the bottom of this. Long story short is that I finally upgraded from Xpadneo (commit https://github.com/atar-axis/xpadneo/commit/706fca9) to the latest (https://github.com/atar-axis/xpadneo/commit/4fd620cd6cb80fb0c1490dc1c864108679d91ab1 as of now), and immediately saw Escapists 2 and Bloody Rally Show games act weird with my Xbox One S controller over bluetooth. The former seems to have swapped the X and B keys (possibly other keys, too). The latter couldn't even get the left stick right -- up became down and vice versa, and the left trigger was constantly down. I tracked this down to https://github.com/atar-axis/xpadneo/commit/3f17cbcd5638d393e1be547c0f93d9dbc0e6765e. The commit prior works as before with both (Bloody Rally Show even properly uses X-box button names) while 3f17cb breaks both.

Both games seem to be built with Unity, so I'm suspecting there could be a systemic issue with Unity's controller detection after the added axis. Have you heard of anything related to this? I ran Bloody Rally Show directly with Steam not running, so I presume that disqualifies Steam affecting anything.

This with Linux v5.17.1-arch1-1. If you think additional debugging details are helpful, let me know and I'll extract all that's necessary.

Thanks in advance!

kakra commented 2 years ago

This is something I'd like to address during the course of v0.11 development when I add customization profiles. The problem I'm currently seeing that xpadneo should stop reporting values for the triggers when the combined axis is used - this is currently not possible. If we implement that, I'd discard the additional axis and instead combine both triggers into the left (or right) trigger while muting the other when the feature is enabled.

I think we can come up with an intermediate solution by using the existing profile switcher that's already in the code to enable upcoming mouse emulation or switch (yet) imaginary profiles 1 to 4.

moll commented 2 years ago

Thanks. Umm, isn't it just the presence of the combined axis at the moment that's breaking these particular games with regards to identifying the controller and having sensible mappings for it though? Given combined_z_axis was off by default and I've never explicitly turned it on, wouldn't reverting https://github.com/atar-axis/xpadneo/commit/3f17cbcd5638d393e1be547c0f93d9dbc0e6765e and gatekeeping the combined axis behind an option again be the right course of action? Or am I reading the change wrong and it also affects how ABS_Z and ABS_RZ work? If I'm not misunderstanding you, your idea of exclusively reporting either both or the combined seems close to what it was before, no? Prior to https://github.com/atar-axis/xpadneo/commit/3f17cbcd5638d393e1be547c0f93d9dbc0e6765e the combined axis seems to have been zeroed.

kakra commented 2 years ago

I'd rather like to make this an option to switch at runtime per controller, and not re-introduce questionable module parameters again.

My proposed solution would be:

  1. You have two profiles to select from (via a Guide button key-combo on the controller)
  2. Profile 1 would be the default reporting the left trigger and the right trigger, and no additional/artificial axes
  3. Profile 2 would be the combined profile, reporting both triggers combined into one, leaving the other dead

For 3, I'll have to come up with an idea how to map the range correctly.

Maybe we drop the feature completely, I think it's really a special edge case for one or two games only, and probably should rather be solved in user-space.

moll commented 2 years ago

Maybe we drop the feature completely, I think it's really a special edge case for one or two games only, and probably should rather be solved in user-space.

Given that the old default of not having the combined axis and the new default of having one seems to break the two games I listed above, I'm all for removing that feature. :)

I'm curious why it hasn't come up before. Did I have bad luck and stumbled upon the only Unity games with poor controller detection or are most Xpadneo users still on an older version? I haven't done an exhaustive test with older controller-supporting games of mine though. Dead Cells, a non-Unity game, at least properly detected sticks and ABXY keys. I haven't tried the Overcooked series yet.

If the rarity of the problem is because some popular distributions are still using an older version of Xpadneo, I fear that once they upgrade, the new default may surface more breakage and leave a bad taste in their mouth. I suspect the average Linux user, albeit more technical than a Windows user, won't be able to easily revert patches and recompile kernel modules.

How far do you reckon the profile approach is that effectively backs out of the combined axis again?

Thanks!

kakra commented 2 years ago

Did I have bad luck and stumbled upon the only Unity games with poor controller detection

Many old Unity versions (probably the native Linux versions only) have a very very poor controller support implementation. It should just work when running the games through Proton or wine which is probably the future of Linux gaming anyways: It shifts the burden of maintaining an abstraction layer from the game devs to Valve, and essentially any game can benefit.

How far do you reckon the profile approach is that effectively backs out of the combined axis again?

Now that I got the new firmware, I'm finalizing the bugfix backports to v0.9, transform the v0.10 branch to the new source code layout, tag v0.10, then declare v0.9 EOL. The transformation will make most bugfix backports incompatible to v0.9. Essentially, the master branch will then work towards v0.11, and I backport simple feature fixes and bugfixes to the v0.10 branch.

That said, there is now time frame when the profile setup will appear in the v0.11 dev branch and when and how I backport it to v0.10.x. But it's probably one of the first things to work on because I have a few open features on queue I'd like to get into v0.11 dev branch fast.

It also means, v0.9 will not remove the additional axis but v0.10.x will.

moll commented 2 years ago

Many old Unity versions (probably the native Linux versions only) have a very very poor controller support implementation.

Yeah, that's probable, but shouldn't we follow the kernel's principle of not breaking user-space in this situation? Or at least until there's a solution that doesn't involve patching Xpadneo.

It should just work when running the games through Proton or wine which is probably the future of Linux gaming anyways: It shifts the burden of maintaining an abstraction layer from the game devs to Valve, and essentially any game can benefit.

I sure hope not. If the Steam Deck is an indicator, Linux-native builds are only getting started. I personally don't use Steam for the majority of my purchases (GOG and Itch prevail) and if I forced, try to limit purchases to Linux-games without DRM to run those games outside of Steam.

Thanks for the roadmap details and continued maintenance! I'm a little unclear though, what's preventing just reverting the patch on the v0.9 branch today, if we deem the by-default visible new axis a "regression"? Wouldn't keeping previously working-things working take precedence over a feature that required opt-in prior? Or are you trying to avoid further changes in minor 0.x versions, even if they could be classified as fixes?

Thanks!

kakra commented 2 years ago

I sure hope not. If the Steam Deck is an indicator, Linux-native builds are only getting started.

Off-topic but... click...

Linux-native games are not really a thing unless they are open source: The Linux runtime is shifting too fast without maintaining long-term compatibility - Windows (at least in the past) has actually been a good example of very stable APIs. Many native games just cannot load on current distributions because lib symbols or versions are missing (I'm watching this since 15+ years now, old games just cannot load or crash), not to mention the strange symbol patching practice Debian is doing to glibc and openssl (a result of their long-term support to overcome exactly that "unstable API" issue but it makes binaries compiled for Debian incompatible with other distros). Both glibc and openssl are mostly incompatible between updates, especially openssl (it breaks ABI compatibility with almost every update, and API compatibility with likely every other update, ffmpeg does similar dumb things, that's why libav exists). This is not something a game developer wants to maintain as they work profit-orientated. Also, many "native" ports aren't really native ports. They are the Windows source code (in the best case) or binary code (in most cases), wrapped in compatibility layers similar to wine and DXVK, and then just compiled as a native binary. That's not really a native port - it's more like statically linked wine/DXVK, inheriting all the API stability issues mentioned above. But basically it's what Proton and wine handle at **runtime** by providing all the "wrapper" functions and a runtime linker and loader to load PE executables, the same thing that glibc provides for ELF binaries, but without getting the benefits of improvements to DXVK or wine. Thus, Proton games actually execute natively, using a Windows-like runtime (clean-room implementation of the Windows API) instead of Linux solibs - but with the difference of providing an ultra stable API (and ABI, which is the real reason) that classical Linux just doesn't provide at the deep core of some essential packages (often mostly ABI issues because a re-compile usually fixes issues, but good luck asking a game dev to recompile a five year old commercial game for a current distribution, or maybe even different distributions). So, I'd rather ask game developers verifying their games to properly working with Proton/wine. Then Proton/wine can maintain ABI compatibility. Don't get me wrong, I'm all in for Linux (and I don't use Windows at all) but games aren't provided as a common good to improve life or humanity, they are provided to make money and get developers something to eat. And there's no point in maintaining it after the main selling phase, so no one would properly maintain a native port that you could still run in 10 years. But Proton can do that. And it can even improve performance due to optimizations even when the game is no longer maintained. Why does flatpak exist? Because the Linux runtime API/ABI is so effing unstable. Flatpak provides a stable runtime dynamically mounted per package version. Steam does something similar with the Steam Runtime and bwrap (the technology driving flatpak) - but that's hard to maintain and doesn't get all the improvements and love, and it's double the work when we already have Proton. And the Windows API/ABI is very stable, you can just throw new improvements and functions at it and old games would still work. Microsoft, like them or not, has actually done a good job here. And Proton is even better because it maintains support and fixes that MS starts to drop from Windows 10 - so Proton is even more stable than Windows itself. Seen this way, it's actually the future of gaming. It goes that far that people start using DXVK in Windows to run older games. The major remaining issue is game performance optimization which Windows has seen since 20+ years now and wine and open source driver stacks probably only have done since less than 5 years. I'm currently seeing more and more performance related analyses and fixes. It reached a point where we see games run much better on Proton than on Windows, e.g. Elden Ring.

but shouldn't we follow the kernel's principle of not breaking user-space in this situation?

xpadneo is not the kernel... And we are in the v0.x range of versions. While I try to not break user-space compliance, features have a higher priority - and removing module parameters was such a priority. The v0.9 branch is going to be declared EOL soonish, I won't break a feature there that is working for others. The choice is: We break either the one or the other.

That said, I may consider it for the v0.10 branch but actually I'm more inclined to remove that feature completely in favor of customization profiles. So whatever comes, you'll get your fix in v0.10. I don't want to break with the module parameters clean-up that was done in v0.9. I'd rather backport fixes to the v0.8 branch if that works for you - if you'd test that and let me know what is missing. But I think it makes more sense to wait for v0.10.

Reminder: The master branch isn't a stable release, v0.9.x is stable and I didn't break anything there - at least that I'd know of. ;-) Your breakage is between v0.8 and v0.9 - and that's expected and documented.

kakra commented 2 years ago

I tested Dreamfall Chapters as a native Linux game. It probably uses an older Unity version, and it worked fine there. Do you know how to find the Unity version the game uses?

moll commented 2 years ago

Hey, I'll look into that and what you wrote last time, too, over this weekend and get back to you!

moll commented 2 years ago

Sorry, I started testing a bunch of Unity games I had lying around with and without the combined axis and it took longer than expected. I'll try to write up what I discovered tomorrow, though just to quickly answer the version identification question:

I'm not sure what the correct way would be to identify the Unity version, but a rough method seems to be to run the game's executable or the associated UnityPlayer.so (or even any of the game's .assets files) through strings and look for the regular Unity version format.

For example, if I'm not off, Bloody Rally Show (v1.8.3), the first game that misidentifies the controller's keys if the combined axis is present and swaps the up-down of the left stick, seems to be based on 2020.3.7f1:

$ strings UnityPlayer.so | grep -E '[0-9]\.[0-9]+\.[0-9]+' 
2020.3.7f1 (dd97f2c94397)

The Escapists 2, which swaps X and B, seems to be based on 2017's Unity 5.5.0p4:

$ strings TheEscapists2.x86_64 | grep -E '[0-9]\.[0-9]+\.[0-9]+' 
UnityPlayer/5.5.0p4 (http://unity3d.com)

I possibly found other games that may not work with the latest master, but I'll carry on testing tomorrow to identify whether it's the combined axis or something else in the master or 0.9 branch that's causing this. I also found games that worked fine, with Unity versions around 2017 and 2019. I'll list the details once all are tested.

moll commented 2 years ago

Did one more quick reconfirmation before going to bed. :P

I also found that Absolute Drift, a racing game, is also broken with the combined axis. It doesn't seem to even register steering or the D-pad, but once you get in-game with the keyboard, it acts as if you simultaneously hold down break and gas. Much like Bloody Rally Show, though in this game you couldn't even turn and no other controller buttons worked. Its based off of Unity 2017.4.16f1:

$ strings absolutedrift.x64 | grep -E '[0-9]\.[0-9]+\.[0-9]+'
UnityPlayer/2017.4.16f1 (UnityWebRequest/1.0, libcurl/7.51.0-DEV)

This with the latest v0.9.1 (a0782869f24308fb10547e3b44f20a8189c70a48). Then I stripped the combined axis in the following way, and it worked again.

diff --git a/hid-xpadneo/src/hid-xpadneo.c b/hid-xpadneo/src/hid-xpadneo.c
index 59d3416..5b9522e 100644
--- a/hid-xpadneo/src/hid-xpadneo.c
+++ b/hid-xpadneo/src/hid-xpadneo.c
@@ -799,9 +799,6 @@ static int xpadneo_raw_event(struct hid_device *hdev, struct hid_report *report,
        memcpy(&xdata->input_report_0x01, data, size);
    }

-   /* reset the count at the beginning of the frame */
-   xdata->count_abs_z_rz = 0;
-
    /* we are taking care of the battery report ourselves */
    if (xdata->battery.report_id && report->id == xdata->battery.report_id && reportsize == 2) {
        xpadneo_update_psy(xdata, data[1]);
@@ -878,9 +875,6 @@ static int xpadneo_input_configured(struct hid_device *hdev, struct hid_input *h
    input_set_abs_params(xdata->idev, ABS_Z, 0, 1023, 4, 0);
    input_set_abs_params(xdata->idev, ABS_RZ, 0, 1023, 4, 0);

-   /* combine triggers to form a rudder, use ABS_MISC to order after dpad */
-   input_set_abs_params(xdata->idev, ABS_MISC, -1023, 1023, 3, 63);
-
    return 0;
 }

@@ -903,12 +897,14 @@ static int xpadneo_event(struct hid_device *hdev, struct hid_field *field,
                goto stop_processing;
            }
            break;
+
        case ABS_Z:
            xdata->last_abs_z = value;
-           goto combine_z_axes;
+           break;
+
        case ABS_RZ:
            xdata->last_abs_rz = value;
-           goto combine_z_axes;
+           break;
        }
    } else if ((usage->type == EV_KEY) && (usage->code == BTN_XBOX)) {
        /*
@@ -960,13 +956,6 @@ static int xpadneo_event(struct hid_device *hdev, struct hid_field *field,
    /* Let hid-core handle the event */
    return 0;

-combine_z_axes:
-   if (++xdata->count_abs_z_rz == 2) {
-       xdata->count_abs_z_rz = 0;
-       input_report_abs(idev, ABS_MISC, xdata->last_abs_rz - xdata->last_abs_z);
-   }
-   return 0;
-
 stop_processing:
    return 1;
 }
diff --git a/hid-xpadneo/src/xpadneo.h b/hid-xpadneo/src/xpadneo.h
index b8419f3..8870017 100644
--- a/hid-xpadneo/src/xpadneo.h
+++ b/hid-xpadneo/src/xpadneo.h
@@ -142,7 +142,6 @@ struct xpadneo_devdata {
    u8 input_report_0x01[XPADNEO_REPORT_0x01_LENGTH];

    /* axis states */
-   u8 count_abs_z_rz;
    s32 last_abs_z;
    s32 last_abs_rz;

Foreshadowing slightly and if I'm not mistaken, the master branch as of today doesn't work with Absolute Drift nor Bloody Rally Show even with the above patch applied.

I wonder how do these three games identify controllers that they're so fixated on having a specific layout present. Isn't it the case that the keys in some structs those games read are always there and Xpadneo just sets their values non-zero, like setting ABS_MISC? After all, my patch above just stopped setting/reporting ABS_MISC, yet for some reason that made controller detection work in the aforementioned games.

kakra commented 2 years ago

It may well be that old Unity detects layouts by looking at the amount of buttons and axes as a fallback or validation. That type of detection is just broken, we probably cannot do much about it. Could you check with lddtree -a <GAMEBIN> if the game uses SDL? If it does, does it ship with a controllerdb mapping file (which many older games did)? You could try deleting it or adding the controller.

It seems I have one game with UnityPlayer.so 2021.1.24f1: "Sir, You Are Being Hunted" - I'll try that later. It doesn't link to SDL, so Unity probably ships its own controller API or it links to SDL statically.

There's a PR which aims to provide 100% compatibility with the wired 360 controller: #283 but it's probably a little outdated. You could check it, and I'll try to rebase it. People reported success with that for some retro emulation software.

moll commented 2 years ago

Thanks, I'll investigate the SDL-ness of those games, though I thought Unity as a framework already does everything SDL does.

It may well be that old Unity detects layouts by looking at the amount of buttons and axes as a fallback or validation.

Ah, you're right, I thought input_set_abs_params was setting the value, but now I see that it just registers the presence of an axis/button (https://www.kernel.org/doc/Documentation/input/input-programming.txt). The proper future-safe way would be to identify controllers via some manufacturer id the game can query somehow, I presume?

There's a PR which aims to provide 100% compatibility with the wired 360 controller: https://github.com/atar-axis/xpadneo/pull/283 but it's probably a little outdated. You could check it, and I'll try to rebase it. People reported success with that for some retro emulation software.

I'll look into that. It seems to introduce a new module parameter, which I thought you wanted to get rid of? :P From the shown diff it also looks like the input_set_abs_params(xdata->gamepad, ABS_MISC... line is still present and from my testing above, its presence was all that was required to break those games, but perhaps if the controller is presented as an entirely different model, things are different. Then again, if we're already in module-parameter land, we might as well have an "original Xbox One S" compatibility mode that needs nothing other than removing ABS_MISC like my diff above (at least with the changes in v0.9). I don't remember having controller troubles in any game for the past 3.5 years since starting to use Xpadneo, so the pre-ABS-MISC and presumably pre-separate Xbox button (which is in master) layouts are supported well enough.

I'm also thinking, should we record our findings somewhere in a structured manner? As in, which Xpadneo versions work out of the box with which games and whether some games need workarounds?

kakra commented 2 years ago

which I thought you wanted to get rid of?

That's why it's not merge - see the todo list of that PR.

I'm also thinking, should we record our findings somewhere in a structured manner?

Well, we could create another markdown file in the docs folder with a table.

The next xpadneo release will refactor a lot of code and get rid of some old work-arounds. If possible, I want to aim for better compliance and compatibility without adding any new module load parameters, and instead work towards a sysfs interface for runtime configuration, ultimately adding a desktop application for configuring settings, mappings, and profiles.