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

Add new "input override" system for TAS input and Android touch controls #9624

Merged
merged 4 commits into from
Oct 3, 2022

Conversation

JosJuice
Copy link
Member

@JosJuice JosJuice commented Apr 5, 2021

Dolphin currently has two parts of the codebase that directly map UI elements to emulated controls: TAS input in DolphinQt, and the touch overlay on Android. The two accomplish this using entirely different methods, both of which have their own drawbacks. TAS input manually pokes around inside Wii Remote input reports instead of letting this be handled by the normal input report creation code, and does so in a way that isn't always reliable. The Android touch overlay requires a fixed mapping to be present in GCPadNew.ini and WiimoteNew.ini, meaning that these two files can't be used for storing the user's actual controller mappings, which has led to the creation of a second controller mapping system implemented in Java which lacks many of the features present on PC.

This pull request tries to improve the situation by replacing both of these two systems with a new "input override" framework. The basis of it is that each EmulatedController lets you set a callback function which can inspect and replace the input coming from InputCommon. TAS input and Android both register such callback functions.

Getting rid of the Android touch overlay's dependency on ButtonManager like this PR does is a prerequisite for getting rid of the controller indirection on Android. I'm aiming to work on fully getting rid of the controller indirection around July if this gets merged.

One important note: This pull request breaks a certain hack that some Android users have been using in which they intentionally desync the controller used by the emulation core and the controller used by the touch overlay by first configuring one Wii Remote extension in a game INI and then configuring a different Wii Remote extension in the touch overlay settings. Normally this would result in the touch overlay not working, but what these Android users then do is to edit the fixed mappings in WiimoteNew.ini (typically by using a profile instead of by editing the file directly) to let them play games that use a Wii Remote or Wii Remote + Nunchuk using an on-screen Classic Controller, which has more buttons. (This is especially popular for Skyward Sword, because of its complex motion controls.) With this PR, the contents of WiimoteNew.ini no longer affect the touch controller, so this "trick" no longer works. Since editing the fixed mappings in WiimoteNew.ini isn't something you're intended to do, and since this PR is necessary for future improvements of the controller code on Android, I am not going to try to support this hack. The proper way to go would be to add a way for users to add custom touch overlay buttons with custom mappings, but that would have to be a future project.

@jordan-woyak I would appreciate if you could review this. Not sure if this approach is the best one possible, but I would really like to be able to get rid of the controller indirection on Android.

@JosJuice JosJuice force-pushed the input-override branch 3 times, most recently from 2563bc3 to 8fc82f0 Compare April 5, 2021 11:31
@JosJuice JosJuice mentioned this pull request Apr 22, 2021
@jordan-woyak jordan-woyak self-requested a review April 25, 2021 17:27
@JosJuice JosJuice force-pushed the input-override branch 2 times, most recently from a593ecc to 5c4cb92 Compare April 25, 2021 19:19
@Rumi-Larry
Copy link

Why the terms "Main Stick" and "C-stick" not translatable?

@JosJuice
Copy link
Member Author

The translatable terms are "Control Stick" and "C Stick". See PR #3033.

@Rumi-Larry
Copy link

Rumi-Larry commented Apr 25, 2021

The translatable terms are "Control Stick" and "C Stick". See PR #3033.

Interesting, thank you!

@AdmiralCurtiss
Copy link
Contributor

The rest of the non-Android code looks fine from what I can tell, though I'm not super familiar with the input code.

@JosJuice
Copy link
Member Author

@AdmiralCurtiss
Copy link
Contributor

Can we get a review from @jordan-woyak on this? I'd like to see this merged so we can inch closer to fixing that ridiculous Android input system.

I suspect this can also be leveraged to improve (non-GC) controller behavior on netplay...

Copy link

@saminamo saminamo left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Member

@jordan-woyak jordan-woyak left a comment

Choose a reason for hiding this comment

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

Code looks good. Untested, but LGTM.

Like 3bc4968 but for Wii Remote extensions. I'm doing this to ensure
that TAS input values will still roundtrip after the next commit.
This lets the TAS input code use a higher-level interface for
overriding inputs instead of having to fiddle with raw bits.
WiiTASInputWindow in particular was messy with how much
controller code it had to re-implement.
This is the first step of getting rid of the controller indirection
on Android. (Needing a way for touch controls to provide input
to the emulator core is the reason why the controller indirection
exists to begin with as far as I understand it.)
@JMC47
Copy link
Contributor

JMC47 commented Oct 3, 2022

whelp, I see approval, I see green button.

@JMC47 JMC47 merged commit 052c739 into dolphin-emu:master Oct 3, 2022
@JosJuice JosJuice deleted the input-override branch October 3, 2022 20:27
@@ -151,7 +216,6 @@ QSpinBox* TASInputWindow::CreateSliderValuePair(QBoxLayout* layout, int default_
slider->setRange(0, max);
slider->setValue(default_);
slider->setFocusPolicy(Qt::ClickFocus);
slider->setInvertedAppearance(invert);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this (and the invert parameter in general) removed? The IR widget used them. (However, it seems like things still behave correctly - the sliders for the IR widget still match the position and aren't reversed, at least on my machine, so I'm not sure what it does; the docs for setInvertedAppearance don't really clarify either.)

Copy link
Member Author

@JosJuice JosJuice Oct 4, 2022

Choose a reason for hiding this comment

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

Previously, the IR sliders were more or less directly mapped to the positions of the sensor bar lights from the perspective of the IR camera. When you move a Wii Remote right, the lights appear to move to the left from the perspective of the camera – so TAS input was designed so that when you move the slider right, we invert the input so that the light points actually move to the left. With the new input override API, the sliders are mapped to the Point action, and WiimoteEmu takes care of translating from "location you're pointing at" to "light locations".

Comment on lines 36 to 40
// Should be part of class but fails to compile on mac os
static const u16 ir_min_x = 0;
static const u16 ir_min_y = 0;
static const u16 ir_max_x = 1023;
static const u16 ir_max_y = 767;
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that these can be in the class (at least if they're changed to constexpr). This is a change I make in the balance board PR (#8268).

@JosJuice JosJuice mentioned this pull request Dec 27, 2022
10 tasks
Felk added a commit to Felk/dolphin that referenced this pull request Sep 12, 2023
see dolphin-emu#9624 for details on the rewrite.
We now basically get per-button control for all supported input methods for free.
Fixes for documentation and python stubs pending. Breaking changes!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
7 participants