dolphin-emu
Xinput2: use raw events and queries
This series reworks XInput2 to fix https://bugs.dolphin-emu.org/issues/10668 (and two duplicates). The first commit in this series is the (only) commit in #11758. GitHub won't let me make a pull request based off another pull request (it redirects me to my fork). I thought I was helping by fixing one bug per PR, but actually I just created noise. Sorry about that!
Bug 10668 is a complaint that while Dolphin is open, Openbox does not respond to clicks on the root window, and Dolphin only sees clicks on the root window or window decorations, not inside Dolphin's own windows. The root cause of this conflict is special X server behavior that causes button press events to grant the receiving client an implicit passive grab on the pointer. Because only one client can get the grab, the X server has special rules about button press event delivery across the core protocol and both XInput versions. This series switches to using raw events, which are always delivered to all clients listening for them. As an Openbox user, I can confirm this resolves the conflict between Dolphin and Openbox. I also tested using Mutter running under X, where it continues to work as it did before.
- c6f9e61 fixes using two or more master keyboards at the same time, as you might do if you and a friend are playing a two-player game, each with your own mouse and keyboard.
- 7aeae81 replaces
unsigned intwithu32in the definition of a flag field. No functional change. Could be part of 323d08f, which cares that it's specifically 32 bits, but split here for ease of review. - c5391c9 requests XInput 2.1, up from 2.0, to ensure we get raw events all the time.
- 323d09f switches from normal events to raw events for button presses and key movements. Then to avoid manually applying mappings (e.g., swapping left and right buttons for a left-handed user), we ignore the event payload and just query the current state after the event loop if events were delivered. Actually we were already querying the state, just not using it as the single source of truth. Regarding performance, we were unconditionally querying the keyboard before, so we actually do fewer queries now.
- 2000f0b stops listening to slave devices to avoid processing each raw event twice (and also to delete some code).
This is my first nontrivial pull request, so I would appreciate general feedback or style nits. I also have these specific questions for reviewers:
- 2000f0b is a user-visible change, because we were processing each raw motion event twice and now we process it only once. I don't know what relative mouse input is generally used for, so I'm not sure if this is fixing a bug, or if we should multiply by 2 to restore the previous behavior, or tweak the smoothing constants, or...?
- In the controller mapping dialog, the (GUI) buttons don't update their boldness when other applications are focused. If I start a GameCube game with background input enabled, then open the GameCube controller mapping dialog with GC buttons bound to clicks, then repeatedly click on another application's window, the game gets the GC button presses but the GUI buttons don't update. Also, if stick inputs are bound to Cursor X/Y+/-, the stick calibration widget updates even when the pointer is over another application's focused window. It's just the GUI buttons that don't update.
- Old commit 2b640a4f suggested that queries, being synchronous, are a performance problem, but bbb12a75 added an unconditional keyboard query. We now query the pointer/keyboard only if a pointer/keyboard event arrived. In limited testing, at native resolution, I saw no difference in frame times between no input and continual mouse movement and clicking. Cranking the resolution up to drop to ~55 FPS, mouse input does increase frametime variance slightly, but also brings the framerate back up to 60 FPS, which I can't explain. Any ideas on quantifying the performance impact of this change?
- Is the detail in the commit message for 323d09f appropriate? Having lost important information in personal projects behind messages like "see discussion in # 40", I want commit messages to include all the information necessary to understand the commit, and whoever looks at this code next is unlikely to be an X expert (I sure wasn't). On the other hand, it is a very long message.
Would be interesting to know if this adds a proper fix for https://bugs.dolphin-emu.org/issues/12913. We added a workaround for this issue with https://github.com/dolphin-emu/dolphin/commit/d40dbe467040668a296ff488b47d99ccadae7dc5. (Unfortunately, I don't have a Linux machine on hand that I can easily test on.)
I tested the current changes, and they do not fix the Qt 6.3+ bug on Linux when the workaround is removed. That's too bad.
I tested this branch with the setenv("QT_XCB_NO_XI2", "1", true); commented out and a logging statement added to check that the environment variable isn't getting set some other way. I don't see any difference in behavior with the workaround removed. Mouse clicks are recognized in the controller mapping dialog and while playing games in both Openbox and Mutter. So at least for me, this branch does remove the need for the workaround in d40dbe4.
For comparison, I tested master in Mutter with the workaround (clicks are recognized) and without (clicks are not recognized), so without this branch's changes, the workaround is necessary in my environment.
I tested with: Arch Linux kernel 6.2.11.arch1-1 xorg-server 21.1.8-1 xf86-input-libinput 1.3.0-1 openbox 3.6.1-10 mutter-43.4-1 gnome-shell-1:43.4-1 qt6-5compat 6.5.0-1 qt6-base 6.5.0-3 qt6-declarative 6.5.0-2 qt6-multimedia 6.5.0-1 qt6-multimedia-ffmpeg 6.5.0-1 qt6-svg 6.5.0-1 qt6-tools 6.5.0-4 qt6-translations 6.5.0-1 qt6-wayland 6.5.0-2 qt6-websockets 6.5.0-1
I run Openbox with exec openbox-session at the end of my ~/.xinitrc.
I run Mutter with export XDG_SESSION_TYPE=x11; export GDK_BACKEND=x11; exec gnome-session at the end of my ~/.xinitrc (as instructed by https://wiki.archlinux.org/title/GNOME#Xorg_sessions).
I'm not particularly familiar with XInput2, but this code seems generally sane and apparently fixes a long-standing bug, so if no one has any complaints I'll merge this in the next few days.