libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
9.79k stars 1.82k forks source link

WGI and Rawinput joystick backend racing each other #4914

Closed cgutman closed 2 years ago

cgutman commented 2 years ago

With latest SDL main (1d40c69a1b21d0b168be5cd6eb959139ac37b331) compiled with CMake (which now properly enabled WGI), it looks like the WGI and RawInput backends are racing each other to discover XInput joysticks first. This can lead to duplicate SDL_CONTROLLERDEVICEADDED and SDL_JOYDEVICEADDED events depending on which backend wins the race. I can reproduce this by repeatedly replugging an Xbox One controller.

Usually, RAWINPUT_AddDevice() wins the race and WGI will ignore the joystick due to the RAWINPUT_IsDevicePresent() check in IEventHandler_CRawGameControllerVtbl_InvokeAdded().

However, sometimes WGI will win the race and IEventHandler_CRawGameControllerVtbl_InvokeAdded() runs first. If that happens, we'll get 2 joysticks created for a single device because RAWINPUT_AddDevice() doesn't check whether WGI is already handling that device. See callstacks.txt for the stacks of the racing callbacks and the duplicate events.

I don't think the solution as simple as making a WGI_IsDevicePresent(). Having the race condition is fundamentally problematic, because it means joystick behavior is non-deterministic. You will randomly either get WGI or Rawinput each time you plug in your gamead.

We could change the WGI backend to do polling-style detection in WGI_JoystickDetect() rather than relying on callbacks, but I'm not sure that would necessarily work either. It seems like WGI is fundamentally asynchronous (it spawns a worker thread in your process to handle events), so enumeration may still be non-deterministic even if we run the JoystickDetect() callbacks in a deterministic order.

What does the WGI backend get us anyway that we can't do via RawInput, DInput, and XInput?

slouken commented 2 years ago

What does the WGI backend get us anyway that we can't do via RawInput, DInput, and XInput?

Xbox trigger rumble, and racing wheels, which no longer show up under DirectInput.

cgutman commented 2 years ago

The RawInput backend uses WGI APIs for trigger rumble even without the WGI joystick backend, right?

If it's just racing wheels that we can't pick up with other APIs, maybe it makes sense to ignore regular controllers in the WGI backend? That would at least reduce the impact of this bug.

slouken commented 2 years ago

Possibly. I'd want to check the build flags, as someone might build without the other joystick subsystems and the WGI subsystem should pick up more of the slack.

cgutman commented 2 years ago

I dug a little deeper and it looks like the problem is even more complex than I originally thought.

The WGI backend has a function SDL_IsXInputDevice() which enumerates the raw input device list trying to determine if the device we're enumerating is an XInput device by matching VID+PID and checking for IG_ in the device path. If it matches, then we don't use WGI (which allows XInput or RawInput backends to handle it).

The problem is that the USB VID/PID of the game controller itself is not propagated to the HID device that ends up being included on the raw input device list. The "XINPUT compatible HID device" in the raw input device list will always use 0x45E/0x2FF and thus will not match the VID/PID we get from WGI. AFAICT, this function will never match an Xbox One-compatible controller, though it does work properly for older Xbox 360-compatible controllers.

If I disable the RawInput backend, we always get duplicate controllers because the WGI backend picks them up first and SDL_IsXInputDevice() return false, then the XInput backend picks them up again. It looks like we will need a different approach to ignore XInput-compatible controllers in WGI.

Here is a screenshot of the device hierarchy with a third-party Xbox One controller: image

and it's still broken even with a first-party official Xbox Series X controller: image

and the working case with an Xbox 360 controller: image

slouken commented 2 years ago

We have some other hacks around 0x45E/0x2FF, it might be appropriate to hard-code handling for this here. Take a look around for USB_PRODUCT_XBOX_ONE_XBOXGIP_CONTROLLER,

cgutman commented 2 years ago

I think this is fixed by b3e909dc4106d62e087136bd1ba0eb8e783a8ed3 and f6dc47caefa8ef83bd5d718aa0f399fd47ca7d6b.

slouken commented 2 years ago

Great!