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

Android input overhaul #11385

Merged
merged 38 commits into from
Mar 11, 2023
Merged

Conversation

JosJuice
Copy link
Member

@JosJuice JosJuice commented Dec 27, 2022

(The description of this pull request touches on events from before I joined the Dolphin project. I believe my descriptions of how the code worked back then are more or less accurate, but I may be misstating things when it comes to the motivations behind the code!)

In 2023, the Android port of Dolphin is going to hit an anniversary: 10 years since the start of its development. That long ago, there certainly weren't any phones that could run Dolphin fast, but the development of Dolphin's Android port never stopped (well, at least never fully), and by now it's become quite a capable emulator.

The person who laid the groundwork for the Android port all those years ago was Sonicadvance1. He's said that he enjoyed the time he spent adding an ARM JIT and OpenGL ES support, but tellingly, I can't remember him ever saying that he enjoyed working with Android.

Really, adding any major feature to Dolphin's Android GUI is not fun at all. Having to completely reimplement a GUI that we already have a fully working PC version of is a drag as it is, but what makes it much worse is JNI. Because Android's GUI libraries are written in Java and Dolphin is written in C++, you need some way to bridge the two languages. And the tool we have for that, JNI, really is a pain to use. My guess from reading the code left around from the early days of Dolphin's Android port is that Sonicadvance1 tried to avoid using JNI wherever possible, but still had to use it in some places because it was absolutely necessary.

This code from the early days worked, but in some cases, its avoidance of JNI led to features getting reimplemented in Java in subtly incompatible or buggy ways, more advanced features being hard to add, and other problems. The past few years, I've been going through Dolphin and removing reimplemented features in favor of calling existing C++ code where it would be beneficial. Yes, it meant I had to spend time writing lots of JNI code, but I think the result has been worth it. The game list, the settings... Those are both areas that improved a lot with the ability to call into our existing C++ code. (Remember when you couldn't change settings while a game was running?) But there's one area that I haven't touched until now because of how big and daunting it was: Input.

On PC, Dolphin has an incredibly sophisticated system for mapping inputs to emulated controllers. Because of how quirky Wii controllers are compared to normal gamepads, having a good system for this is almost a necessity. The Android version of Dolphin has until now been completely sidestepping this system. Let me try to explain what it has been doing instead.

Initially when Dolphin was being ported to Android, it only supported touchscreen input. Gamepad input didn't come until a bit later. For the initial touchscreen support, a slightly crude but perfectly functional approach was used: When you first launched Dolphin, an input mapping file would be created with pre-defined mappings that mapped the buttons and sticks of the emulated controller to "fake" inputs provided by the Android app. The app would then change the state of the "fake" inputs as you touched the virtual buttons and sticks on the touchscreen.

Why did there have to be a mapping file? Well, simply because Dolphin's existing C++ code expected there to be mappings. On PC, this file is where the user's choices for how to map their gamepad (or keyboard, or other input device) would be stored. But it's not like the code cares if the mappings were set up by the user or by an automated process, so the solution of pre-defined mappings worked fine.

Then it was time to add gamepad support, which required some way of storing the mappings set up by the user. Simply storing them in the mapping file would be a problem in that it would break the touchscreen controls. So, what the Android version of Dolphin did instead was to create its own input mapping system in Java, which stored the mappings in SharedPreferences... and also in Dolphin.ini at the same time. The inputs from the gamepad would be mapped by this second input mapping system to the "fake" inputs I mentioned before, and would then be mapped a second time by Dolphin's original input mapping system using the pre-defined mappings. Certainly not the prettiest solution, but it worked, and implementing a solution without this second input mapping system would be a lot of work.

The biggest problem of this second input mapping system is that it doesn't inherit support all the sophisticated features of Dolphin's usual input mapping system. Essentially, the developers had to start from scratch in terms of features. Various people have tried to add features since then, but it's always been hard to do, and some of the features that have been added have been pretty clunky. (Did you know that if you set a Wii Remote extension in the per-game settings, the Android app will actually copy the entirety of the pre-defined mappings, put them in an input profile, edit the game INI so that the new input profile will be loaded when launching the game, just to then be able to set the Wii Remote extension in the input profile? That's the extent of workarounds that had to be added because of the fundamentally different approaches of the two input mapping systems!)

What I've been wanting to do is to remove this second input mapping system and make the Android app use Dolphin's normal input mapping system. The first big blocker was figuring out some way for touch input to work without the pre-defined mappings. That was done in PR #9624. Then, after that, it was time for JNI work. Tons of JNI work. I've never done a JNI pull request as big as this one before, and maybe I will never need to again. But now, after one year of on-and-off work, here it is.

So with that history lesson out of the way, what new features do we get in this pull request?

  • You can change controller settings while emulation is running.
  • You can save and load input profiles. (The old feature of per-game mappings is replaced by per-game input profiles.)
  • Advanced input expressions.
  • You can map your device's accelerometer and gyroscope to whatever you want, not just Wii Remote motions.
  • Support for many new device sensors. (How about the ambient light level, or the hinge angle of a foldable phone?)
  • Support for sensors in gamepads. (Requires Android 12 or later.) Removed for the time being due to a bug.
  • If a device or gamepad has multiple motors, you can now choose between them for rumble. (Requires Android 12 or later.)
  • You can set many boolean and numeric controller settings that weren't available on Android before, like Relative Input.
  • The Sideways Wii Remote setting, the numeric settings for Wii Remote pointing, and the setting for disabling accelerometer/gyroscope pointing have been moved from the in-emulation settings to the regular settings.
  • The touchscreen controller type setting has been overhauled. You can now choose between all controllers 1-4, but selecting a Wii Remote extension is now done in the regular settings.
  • Separate gamepads are never treated as a single gamepad anymore. (Previously this would happen if they had the same descriptor.)
  • Dolphin no longer mixes up buttons and axes that have the same ID.
  • Possibly fixes for various issues with detecting axes. (Though I'm sure there are also new weird issues that will need to be hammered out!)

There are some heavily requested features that are not included here, and that is because I tried to keep this pull request as small as possible. You may not believe that at first with how huge it is, but more or less everything I've added in this pull request is something I had to add to not have any feature regressions. (Such is the nature of completely removing an old system and building something new in its place.) But this pull request does make those features much more feasible to add, so expect follow-up pull requests from me sometime after this pull request is merged.

I am now going to say something I very rarely say: I would like as many Android users as possible to test this pull request and provide feedback. This is a big pull request, and not only are there very many ways you might want to set up controller mappings, there are also very many different input devices out there that need to be tested for compatibility!

Things that need fixing:

  • Maybe make it clearer that there's a very important submenu hiding behind the controller type setting, now that a bunch of settings that are useful for touchscreen users have been moved there. (Fixed in Android: Add a button for accessing controller mappings #11605.)
  • Improve the discoverability of the advanced mapping dialog. (Dependent on Android: Add a button for accessing controller mappings #11605.)
  • For some reason MotionAlertDialog can detect the phone moving around even though I made IsDetectable return false for all sensor axes. Can't seem to reproduce this anymore.
  • MotionAlertDialog doesn't detect volume key presses if you press the key too fast (probably because the inputs are only polled every 10 ms). This doesn't tend to be a problem with gamepad buttons. Probably not worth holding up this PR over, but it would be nice to improve this at some point.
  • Unsuspending sensors with BLR optimization enabled usually causes a JNI error. This manifests as most Wii games crashing on boot. (Fixed in Jit: Don't use a second stack #11399.)
  • The profile dialog could use some design improvement.
  • The advanced mapping dialog could use some design improvement.
  • I couldn't get the profile dialog and advanced mapping dialog to work right without hardcoding dimensions (marked with TODO comments in the layout XML files).
  • The bottom insets of SettingsActivity are a little wonky due to the warning for old controller configs.
  • Motion Input > Point > Enabled resets to false when you launch a Wii game. (Fixed in InputCommon: Fix ControlGroup::SaveConfig with DefaultValue::Disabled #11606.)

@JosJuice JosJuice force-pushed the android-input-overhaul branch 4 times, most recently from c8fbf7e to 85c5596 Compare December 28, 2022 10:21
@mbc07
Copy link
Member

mbc07 commented Dec 31, 2022

@JosJuice I gave it a try and all Wii games I have thrown at this PR crashed shortly after booting (Wii Menu included). I grabbed a logcat of the crash but I don't know if it includes everything you need to debug the crash. I tested on a Samsung Galaxy S10e (Exynos variant), running One UI 4.1 (based on Android 12).

Despite that, the GUI seem to properly detect both my phone sensors, as well as the motion sensors from the DualShock 4. Mapping is very finicky, though, since the popup that asks for the button to be pressed also binds the motion axis (like `Button A`|`Full Accel Up`). The controller must be perfectly still when pressing the desired button for the GUI to map only the button.

Another side effect is that is very, very challenging to bind the motion axes in the motion input menu. More often than not, the wrong direction will get mapped, or both an accelerometer and a gyroscope axis ends being mapped to the same binding. I worked around that by manually editing the INI file, but I'm not sure if it worked or not because of the crash at boot (the manually edited bindings were shown correctly at the GUI, though)...

@JosJuice
Copy link
Member Author

JosJuice commented Dec 31, 2022

I gave it a try and all Wii games I have thrown at this PR crashed shortly after booting (Wii Menu included). I grabbed a logcat of the crash but I don't know if it includes everything you need to debug the crash. I tested on a Samsung Galaxy S10e (Exynos variant), running One UI 4.1 (based on Android 12).

The log contains some relevant detail, though I am still kind of confused about what's going on. Anyway, I've gotten more or less the same log myself, so this doesn't give me any new info.

Fixing this crash should probably be my highest priority for now. FWIW, Xenoblade Chronicles didn't crash for me, probably because it doesn't use motion controls.

Despite that, the GUI seem to properly detect both my phone sensors, as well as the motion sensors from the DualShock 4. Mapping is very finicky, though, since the popup that asks for the button to be pressed also binds the motion axis (like `Button A`|`Full Accel Up`). The controller must be perfectly still when pressing the desired button for the GUI to map only the button.

OK, good to hear. And yeah, that's a known problem.

Another side effect is that is very, very challenging to bind the motion axes in the motion input menu. More often than not, the wrong direction will get mapped, or both an accelerometer and a gyroscope axis ends being mapped to the same binding. I worked around that by manually editing the INI file, but I'm not sure if it worked or not because of the crash at boot (the manually edited bindings were shown correctly at the GUI, though)...

The intent is that you shouldn't be able to trigger the accelerometer and gyroscope through the input detection dialog, and instead should go through the advanced input mapping dialog. If you haven't tried it already, long press on a controller mapping setting and you'll get the advanced input mapping dialog. Kind of like on PC but with slightly less features.

@mbc07
Copy link
Member

mbc07 commented Dec 31, 2022

FWIW, Xenoblade Chronicles didn't crash for me, probably because it doesn't use motion controls.

I specifically tried only motion-enabled games, so that makes sense.

If you haven't tried it already, long press on a controller mapping setting and you'll get the advanced input mapping dialog.

Oh, didn't know that. Just gave it a try and it indeed shows everything from the DualShock 4...

@JosJuice JosJuice force-pushed the android-input-overhaul branch 2 times, most recently from 2650731 to a29121b Compare January 1, 2023 12:37
@JosJuice
Copy link
Member Author

JosJuice commented Jan 1, 2023

If you want a workaround for the motion controls crash for the time being, use Config > Debug > Disable Fastmem. The underlying problem seems tricky and I will probably not have a fix for it soon.

@mbc07
Copy link
Member

mbc07 commented Jan 2, 2023

Gave it another try with fastmem disabled and apart from the (expected) performance impact, motion data from the DualShock 4 seems to be delivered correctly to the game.

I couldn't get the IR cursor simulation to work, but I suspect I might have misconfigured something (I went with Full Accel/Full Gyro bindings, unsure if I should've used Accel/Gyro bindings instead). Once I managed to get in-game, though, it seemed to track motion just fine (tried mostly with Wii Sports).

The UI currently is a bit rough, but it's usable (no worries if this gets addressed in follow up PRs, as this one is already quite huge). We should definitely make the "long-press for advanced input" action more discoverable before merging this PR, though, since it's crucial for properly mapping the motion axes and as it stands now, it's completely hidden (I wouldn't know it existed if you hadn't mentioned)...

@JosJuice
Copy link
Member Author

JosJuice commented Jan 2, 2023

The long press is mentioned in the first screen of the settings. Maybe users will overlook it because they've already seen that screen so many times, though...

@JosJuice JosJuice force-pushed the android-input-overhaul branch from a29121b to e96b38c Compare January 2, 2023 19:05
@mbc07
Copy link
Member

mbc07 commented Jan 3, 2023

Haven't looked at the code so I don't know how feasible is that, but adding one more button to the Input Binding popup (next to the existing "clear" and "cancel" buttons) to open the advanced input popup should make it very clear that there's more.

(perhaps we could also show a toast message informing you can long press the binding if the user enters the advanced input popup by clicking on the button from the Input Binding popup, but that might be a bit too verbose?)

@JosJuice
Copy link
Member Author

JosJuice commented Jan 3, 2023

That's not a bad idea, but I'm a bit worried about users on devices that don't have a touchscreen or mouse. They might think that pressing that button is the only way to access the advanced mapping dialog, and that button is impossible for them to press since the input mapping dialog eats all controller inputs.

@blakbin
Copy link
Contributor

blakbin commented Jan 3, 2023

Ive test it to play with wii party, and mario galaxy seem look ok to me but, point not working at the menu or main or at the home window. Toggle Point at the motion input always revert to disable state and recenter not working

@JosJuice
Copy link
Member Author

JosJuice commented Jan 3, 2023

You're right, for some reason when starting a Wii game the Enabled setting for the Point group gets disabled. I'll have to investigate this.

}

if (positive && negative)
AddAnalogInputs(positive, negative);
Copy link
Member

Choose a reason for hiding this comment

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

I'm basing this comment on AddSensors being strictly for accel/gyro. I'm not sure if there are other "sensors"?

The purpose of AddAnalogInputs, which should probably be more clearly named, is to add the provided inputs and additionally FullAnalogSurface versions of the inputs, which deal with e.g. analog triggers which are sometimes, depending on the gamepad, in their neutral state at a normalized -1 or a 0. The android input code, which I'm guessing is removed now, had some hard-coded special cases for dealing with this on xbox controllers and whatnot. It's unknowable to us how a trigger might be exposed so inputs for each situation are created.

Anyways, this shouldn't be necessary for accel/gyro. We just need to AddInput both directions. Also, the whole if (positive && negative) check seems strange to me, particularly for the accel/gyro, but maybe I'm not digging deep enough. Shouldn't we always have accel neg and pos?

Copy link
Member Author

Choose a reason for hiding this comment

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

The purpose of AddAnalogInputs, which should probably be more clearly named, is to add the provided inputs and additionally FullAnalogSurface versions of the inputs, which deal with e.g. analog triggers which are sometimes, depending on the gamepad, in their neutral state at a normalized -1 or a 0.

I wasn't entirely sure what the actual purpose was, but that makes sense. Thanks for the information.

The android input code, which I'm guessing is removed now, had some hard-coded special cases for dealing with this on xbox controllers and whatnot.

Yes, I removed that code.

Anyways, this shouldn't be necessary for accel/gyro. We just need to AddInput both directions.

Yeah, with what you said in mind, I agree. I'll remove the use of AddAnalogInputs for sensors.

Also, the whole if (positive && negative) check seems strange to me, particularly for the accel/gyro, but maybe I'm not digging deep enough. Shouldn't we always have accel neg and pos?

There are other sensors that have just a positive axis. For instance, the proximity sensor (originally intended for turning off the display if you hold your phone to your head during a phone call), the ambient light sensor, and the hinge angle (on a foldable phone). There's a full list in the addSensors method in DolphinSensorEventListener.java if you're interested. But indeed, for accel/gyro specifically, there's always both neg and pos.

@JosJuice JosJuice force-pushed the android-input-overhaul branch from e96b38c to e59b503 Compare January 4, 2023 09:47
@Rhouelyn
Copy link

Rhouelyn commented Jan 7, 2023

pr-11385-dolphin-latest (5.0-18180-debug) new mapping problem

Hi Jos,

I tested this build pr-11385-dolphin-latest (5.0-18180-debug),

my controller is Gulikit Kingkong2 Pro NS09 (this has hall effect analog sticks)

it seems the Right Analog +14 -14 +11 -11 is now detected,

BUT,

theres a new problem, can't map any button on the Shake in nunchuk Section,

and 2nd Shake mappings is missing on Button Section,

i also can't open any games maybe it because it's a debug build i guess?

i also prefer the ui of the old mapping it seems more organize than this latest pr-11385-dolphin-latest (5.0-18180-debug)

i hope this shake gets fix soon i cant wait to play on my controller properly thanks so much in advance ^_^

@blakbin
Copy link
Contributor

blakbin commented Jan 7, 2023

@Rhouelyn check disable fastmem in config > debug it can play game

@JosJuice
Copy link
Member Author

JosJuice commented Jan 7, 2023

theres a new problem, can't map any button on the Shake in nunchuk Section,

So... When you press X or Y or Z, the dialog that tells you to press a button shows up, but then when you press a button nothing happens?

and 2nd Shake mappings is missing on Button Section,

It's not in the Buttons section, it's in the Motion Simulation section.

i also can't open any games maybe it because it's a debug build i guess?

It's because of a known bug. As a workaround for now, you can use Config > Debug > Disable Fastmem.

i also prefer the ui of the old mapping it seems more organize than this latest pr-11385-dolphin-latest (5.0-18180-debug)

The reason why I reorganized things is because I thought the Wii Remote options would get too long otherwise, with how many new options there are.

@Rhouelyn
Copy link

Rhouelyn commented Jan 7, 2023

theres a new problem, can't map any button on the Shake in nunchuk Section,

So... When you press X or Y or Z, the dialog that tells you to press a button shows up, but then when you press a button nothing happens?

and 2nd Shake mappings is missing on Button Section,

It's not in the Buttons section, it's in the Motion Simulation section.

i also can't open any games maybe it because it's a debug build i guess?

It's because of a known bug. As a workaround for now, you can use Config > Debug > Disable Fastmem.

i also prefer the ui of the old mapping it seems more organize than this latest pr-11385-dolphin-latest (5.0-18180-debug)

The reason why I reorganized things is because I thought the Wii Remote options would get too long otherwise, with how many new options there are.

Hi Jos,

As for the question in xyz shake, yes i cannot map any buttons on it when ask me to click a button to map to it. So yeah new problem is the shake cant map any buttons.

So i still cant play properly even if i play games because i need shake on my controller, i usually map it in my L2 and R2, but i tested to map other buttons to the shake and still nothing.

@Rhouelyn
Copy link

Rhouelyn commented Jan 8, 2023

So i tried mapping r2 and L2 on the xyz shake in the motion section, i can map there but not in the nunchuk section,

I tried testing all button on a game and all works shake and point works.
But running games on this debug build is slower than the developer build in case nobody knows.

Its just i cannot map like 3 shake button on xyz on motion section, just L2 on x or y or z not the 3 xyz, and r2 too.
And also i suggest maybe organize the ui a bit like the old one ( but this is just my preference ).

Anyway thanks so much as i can use all buttons now.

@blakbin
Copy link
Contributor

blakbin commented Jan 8, 2023

Yes button L2 R2 cannot detected in motion simulation but it can be manualy bind by long pressing the wanted input and select desired button

@mbc07
Copy link
Member

mbc07 commented Jan 8, 2023

But running games on this debug build is slower than the developer build in case nobody knows.

That's expected, current version of this PR needs fastmem disabled to avoid a crash and running without fastmem causes a big performance hit...

@Rhouelyn
Copy link

Rhouelyn commented Jan 8, 2023

Quick question,
when will this new controller mapping implemented in the dolphin development version?
(im just excited )

@JosJuice
Copy link
Member Author

JosJuice commented Jan 8, 2023

Its just i cannot map like 3 shake button on xyz on motion section, just L2 on x or y or z not the 3 xyz, and r2 too.

That's strange... It works fine for me. I do have a different controller, but I'm not sure why that would matter.

when will this new controller mapping implemented in the dolphin development version?

We don't know exactly when, but I think it will take a while still.

@JosJuice JosJuice force-pushed the android-input-overhaul branch from e59b503 to 3e2930c Compare January 9, 2023 21:13
@JosJuice JosJuice force-pushed the android-input-overhaul branch from 55d0c5c to 6b66953 Compare March 3, 2023 22:16
@JosJuice JosJuice marked this pull request as draft March 5, 2023 10:44
@JosJuice
Copy link
Member Author

JosJuice commented Mar 6, 2023

The feature added in "ControllerInterface/Android: Implement sensor input for InputDevices" is causing a strange crash for t895. Since this feature isn't actually needed for feature parity with master, I'm going to disable the feature and look into solving the crash later once this PR is merged. (I tried to remove the commit from this PR, but it caused annoyingly many conflicts with "ControllerInterface/Android: Automatically suspend sensors", so instead I submitted a new commit that disables the feature.)

@JosJuice JosJuice marked this pull request as ready for review March 6, 2023 18:04
JosJuice and others added 9 commits March 7, 2023 17:39
It's missing a lot of features from the PC version for now, like
buttons for inserting functions and the ability to see what the
expression evaluates to. I mostly just wanted to get something in
place so you can set up rumble.

Co-authored-by: Charles Lombardo <clombardo169@gmail.com>
The Android-specific controller mapping system is now gone,
so IsSettingSaveable can be greatly simplified.
You can set this in the normal controller settings now.
This too can be set in the normal controller settings now.
Up until now, there have been two settings on Android that stored the
selected Wii Remote extension: the normal one that's also used on PC,
and a SharedPreferences one that's used by the overlay controls to
determine what controls to show. It is possible for these two to end up
out of sync, and my input changes have made that more likely to happen.

To fix this, let's rework how the overlay controller setting works.
We don't want it to encode the currently selected Wii Remote extension.
However, we can't simply get rid of the setting, because for some Wii
games we need the ability to switch between a GameCube controller and a
Wii Remote. What this commit does is give the user the option to select
any of the 4 GameCube controllers and any of the 4 Wii Remotes. (Before,
controllers 2-4 weren't available in the overlay.) Could be useful for
things like the Psycho Mantis fight in Metal Gear Solid. I'm also
switching from SharedPreferences to Dolphin.ini while I'm at it.
The previous commit wasn't enough for getting inputs to work for
controllers 2-4.
Some people might wonder where the ability to select an extension
and the Sideways Wii Remote went. This button will take them to the
general settings, which is where those settings now live.

At some point in the future, we should probably move everything to the
general settings. But this pull request is already big enough as it is!
@JosJuice JosJuice force-pushed the android-input-overhaul branch from 6921d7d to 75fb1a7 Compare March 7, 2023 16:39
@t895
Copy link
Contributor

t895 commented Mar 7, 2023

This is fantastic. LGTM!

@mbc07
Copy link
Member

mbc07 commented Mar 7, 2023

@JosJuice I just tried the most recent build from this PR and I'm no longer able to pick any of the motion sensors from a DualShock 4 connected over Bluetooth (hadn't tried USB), not even from the advanced input dialog. Is that supposed to happen? Earlier versions of this PR used to be able to access them just fine...

@JosJuice
Copy link
Member Author

JosJuice commented Mar 7, 2023

As I mentioned in my comment from yesterday, I have removed that from this PR due to a crash and will work on it in a separate PR later.

Copy link
Member

@mbc07 mbc07 left a comment

Choose a reason for hiding this comment

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

Yeah, somehow I missed that (just saw the comment). Anyway, I played with it a little and didn't experience any crashes, it's also a huge improvement compared to what we currently have on master, so this LGTM too. Great work!

@JosJuice
Copy link
Member Author

JosJuice commented Mar 8, 2023

@jordan-woyak Any final comments?

@JosJuice JosJuice merged commit 62ff2f1 into dolphin-emu:master Mar 11, 2023
@JosJuice JosJuice deleted the android-input-overhaul branch March 11, 2023 11:38
@chrisleigh-stack
Copy link

Waiting for the Dualshock 4 to become a wii more sensors on android, Great Job Devs more power

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

9 participants