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

Allow input configuration with SDL joysticks #3116

Merged
merged 13 commits into from Dec 6, 2017

Conversation

Projects
None yet
8 participants
@muemart
Copy link
Contributor

muemart commented Nov 15, 2017

This PR adds the ability to configure controls using SDL gamepads (or joysticks, I guess). I created some scaffolding so that the settings dialog can ask for inputs from input backends. Basically, there is now an abstract DevicePoller class (if anyone has a better name, let me know) which can be implemented by different backends to return inputs.
There are a few ugly corners, which might or might not be a problem:

  • The poller for SDL devices drops all SDL events it doesn't need. The Qt frontend doesn't use them, so it should be fine, but there might be other problems I'm not aware of.
  • Configuring the analog sticks might be a bit confusing, because now you can configure them with 5 buttons, or an actual analog stick. Might need a better GUI for this.
  • The buttons still have placeholder labels for SDL mappings, but that's easy to do once everything else is done.

There are two new buttons under the input tab in the configuration window to set analog stick controls (labeled "Set Analog Stick"):


This change is Reviewable

@jroweboy jroweboy requested review from wwylele and jroweboy Nov 15, 2017

@jroweboy

This comment has been minimized.

Copy link
Member

jroweboy commented Nov 15, 2017

Thanks for the contribution; its something we definitely would like to see it get merged! I'll be doing a full review later, but in the meantime, I'm going to test it locally, and if its all fine, I'll add it to canary.

Also, I see that you added some new buttons https://github.com/citra-emu/citra/pull/3116/files#diff-282077f5f81f30cefcf9b34f15400054R292 Please include a screenshot for any UI changes that you make so people don't have to build just to give UI feedback. (Bonus points if you have before and after, and include a screenshot for each of the themes)

@@ -7,6 +7,7 @@
#include <string>
#include <tuple>
#include <unordered_map>
#include <utility>

This comment has been minimized.

@jroweboy

jroweboy Nov 15, 2017

Member

Need to also include vector (which is keeping it from being in canary for now)

muemart added some commits Nov 15, 2017

@muemart

This comment has been minimized.

Copy link
Contributor

muemart commented Nov 15, 2017

Added picture and hopefully made CI happy.

@BreadFish64

This comment has been minimized.

Copy link
Contributor

BreadFish64 commented Nov 18, 2017

My tests with emulated dInput and xInput controllers went swimmingly, however the physical XBone controller was trigger-happy about mapping everything to an analog stick, suggesting there needs to be a bigger dead-zone.

@wwylele
Copy link
Member

wwylele left a comment

Code looks fine. One concern I have is that the axis poller doesn't have any tolerance ("dead-zone") for input, so any unintentional small movement on stick can trigger it (Probably this is what BreadFish64 described).


/// This will be the the setting function when an input is awaiting configuration.
boost::optional<std::function<void(int)>> key_setter;
boost::optional<std::function<void(Common::ParamPackage)>> input_setter;

This comment has been minimized.

@wwylele

wwylele Nov 18, 2017

Member

consider passing by reference const Common::ParamPackage& if applicable.

}

private:
std::pair<int, int> analog_axes;

This comment has been minimized.

@wwylele

wwylele Nov 18, 2017

Member

IMO just using plain int analog_axis_x, analog_axis_y; is cleaner and more readable.

@muemart

This comment has been minimized.

Copy link
Contributor

muemart commented Nov 19, 2017

I added a deadzone of 50% now, that should be plenty to ignore unintentional movement.

@wwylele
Copy link
Member

wwylele left a comment

Code LGTM. I can't test due to lack of a proper gamepad.


// Keyboard keys can only be used as button devices
if (type == InputCommon::Polling::DeviceType::Button) {
want_keyboard_keys = true;

This comment has been minimized.

@lioncash

lioncash Nov 21, 2017

Member
want_keyboard_keys = type == InputCommon::Polling::DeviceType::Button;
// Empty event queue to get rid of old events. citra-qt doesn't use the queue
SDL_Event dummy;
while (SDL_PollEvent(&dummy)) {
};

This comment has been minimized.

@lioncash

lioncash Nov 21, 2017

Member

Unnecessary semicolon

#include "core/frontend/input.h"
#include "input_common/main.h"

This comment has been minimized.

@lioncash

lioncash Nov 21, 2017

Member

I'm fairly sure this can be replaced with forward declarations

@@ -4,7 +4,13 @@

#pragma once

#include <memory>
#include <vector>
#include "common/param_package.h"

This comment has been minimized.

@lioncash

lioncash Nov 21, 2017

Member

This can be replaced with a forward declaration

@@ -5,6 +5,8 @@
#pragma once

#include <string>
#include <vector>
#include "common/param_package.h"

This comment has been minimized.

@lioncash

lioncash Nov 21, 2017

Member

This can be replaced with a forward declaration.

};

// Get all DevicePoller from all backends for a specific device type
std::vector<std::unique_ptr<DevicePoller>> getPollers(DeviceType type);

This comment has been minimized.

@lioncash

lioncash Nov 21, 2017

Member

This should be cased as GetPollers

namespace Polling {

/// Get all DevicePoller that use the SDL backend for a specific device type
std::vector<std::unique_ptr<InputCommon::Polling::DevicePoller>> getPollers(

This comment has been minimized.

@lioncash

lioncash Nov 21, 2017

Member

This should be cased as GetPollers

}

ConfigureInput::ConfigureInput(QWidget* parent)
: QWidget(parent), ui(std::make_unique<Ui::ConfigureInput>()),
timer(std::make_unique<QTimer>()) {
timeout_timer(std::make_unique<QTimer>()), poll_timer(std::make_unique<QTimer>()),
want_keyboard_keys(false) {

This comment has been minimized.

@lioncash

lioncash Nov 21, 2017

Member

want_keyboard_keys can be initialized directly in the class definition.

e.g.

class Thing {
...
private:
    bool want_keyboard_keys = false;
};

class SDLAnalogPoller final : public SDLPoller {
public:
SDLAnalogPoller() : analog_xaxis(-1), analog_yaxis(-1), analog_axes_joystick(-1) {}

This comment has been minimized.

@lioncash

lioncash Nov 21, 2017

Member

These can all be initialized directly at their definition site:

class SDLAnalogPoller final : public SDLPoller {
...
private:
    int analog_xaxis = -1;
    int analog_yaxis = -1;
    SDL_JoystickID analog_axes_joystick = -1;
};
@Subv

This comment has been minimized.

Copy link
Member

Subv commented Nov 21, 2017

I'll be testing this locally either tonight or tomorrow morning (GMT-5) with a few of my controllers

@Subv

This comment has been minimized.

Copy link
Member

Subv commented Nov 28, 2017

Sorry for the delay, I didn't review the code but performed a few tests, here's my thoughts on the matter:

  • Xbox One Controller wasn't detected when I plugged it in while Citra was already started. This only happened with the normal buttons, the Analog bindings did detect the controller even if it was plugged in after the fact. After binding an analog stick I was then able to bind the normal buttons.
  • Binding a gamepad button turns the label into "[non-keyboard]". I think this can be improved, at the very least it should include the gamepad id that the key was bound to. As a first implementation we could just show numbers like in those cheap controllers that have keys labelled 1, 2, 3, etc.

I consider the first point to be very important for this PR. The second observation can be left to a separate PR.

@muemart

This comment has been minimized.

Copy link
Contributor

muemart commented Nov 28, 2017

Thanks for testing. I didn't do anything with the labels because that's sort of independent of mapping inputs, and as you said this can be done in a separate PR which I'll probably do after this.
I couldn't reproduce your issue, though I only have a Logitech XInput controller, not an Xbox One controller. I did notice however that it takes a few seconds after plugging in until the controller works (I guess this depends on SDL/the OS?). The SDL joysticks get opened every time you press a button, so plugging in a controller while it's waiting for input won't work either, and you have to press the button again.

EDIT: After playing around a little, I noticed that the first time you pressed a button after plugging in a gamepad, no input got registered, but it worked after pressing a second time. That should be fixed now.

@Subv

This comment has been minimized.

Copy link
Member

Subv commented Nov 28, 2017

I tested again and the problem is indeed fixed, thanks!

@LogPod LogPod referenced this pull request Dec 1, 2017

Closed

Progress Report 2017 November #66

@nicoboss

This comment has been minimized.

Copy link
Contributor

nicoboss commented Dec 2, 2017

I’ve tested it using my N64 controller. Unfortunately, it always uses the wrong joystick number and so when configuring joystick 0 it configures it as joystick:1 in the config file but Citra expects it to be joystick:0 and when configuring joystick 1 it put joystick:0 in the config file but Citra expect to be joystick:1.
Configuring joystick 0:
button_a="engine:sdl,joystick:1,button:1" but Citra expect it to be button_a="engine:sdl,joystick:0,button:1"
For connecting real N64 controllers to my PC I’m using the N64 Dual Controller Gamepad to USB Adapter which works perfectly fine.

Furthermore, there seems to be a problem with the analog stick. The game always thinks that it has only moved a little when pushing it to the maximum and so at example in Ocarina of Time link is unable to walk in normal speed.

I’ve tested it using the latest version from your repository and in the Canary build but had the same problems on booth of them.

@muemart

This comment has been minimized.

Copy link
Contributor

muemart commented Dec 3, 2017

I just fixed a bug with the joystick numbers, but I'm not sure it's the problem you had. What do you mean by "Citra expects it to be joystick:0"? Do the buttons not work after mapping? How do you know what Citra expects?
And for the analog stick, make sure you use the "Set Analog Stick" button and don't map up/down/left/right separately.

@nicoboss

This comment has been minimized.

Copy link
Contributor

nicoboss commented Dec 3, 2017

Thank you very much the joystick numbers now working fine!
I knew what Citra expects by manually fixing the numbers in the configuration file.

For the analog stick the "Set Analog Stick" button is not configure the sensitivity properly (It's far to insensitive so that you can't roll in Ocarina of Time and walking is way too slow and it's the same for all other games I have). To configure I just pressed the “Set Analog Stick” button and then moved the joystick horizontally to the right until it reached the maximum, let it go back to the center and then moved it vertically to the bottom and let it go back to the center and it got accepted. Configure each direction separately would work but it's not how it should be done.
It saves the following into the configuration file:
circle_pad="engine:sdl,axis_x:0,joystick:0,axis_y:1"
I haven’t found any way to manually change the sensitivity in the configuration file.

@muemart

This comment has been minimized.

Copy link
Contributor

muemart commented Dec 3, 2017

Hm, the configuration looks correct to me. There's no sensitivity or deadzone in the emulator, so if anything it should be too sensitive. I assume your controller works fine in other software? Do you usually have to change sensitivity or similar things there?
There might be problems in Citra's input handling for your controller, but as far as configuration is concerned, it looks like it works as it should.

@nicoboss

This comment has been minimized.

Copy link
Contributor

nicoboss commented Dec 3, 2017

After doing some research I found a solution how to fix the sensitivity issue: Open Systems Control Center => Hardware and Sound => Right click on “USB Gamepad” => Gamecontrollersettings => double-click on the joystick you use => Go to Settings tap => Press “Calibrate…” => follow the calibration assistant and now the sensitivity should work properly in Citra. This is because unlike most other software (at example the Dolphin Emulator) Citra uses the calibration from the OS/Driver instead of an own one. It’s no problem to configure it properly but users must know how so maybe we should add this to the FAQ. It’s funny I haven’t noticed this misconfiguration for like two years and haven’t even known that you should calibrate them before first use because every software supported my N64 controller just handled this by his own… Sorry for wasting your time with something haven’t much to do with this PR. At least we know now how to help users having the same issue.
Everything in this PR works perfectly fine for me now. Thanks a lot for your contribution.

@muemart

This comment has been minimized.

Copy link
Contributor

muemart commented Dec 3, 2017

No problem. Cool to see that the Windows calibration thing can actually be useful!

@bunnei

bunnei approved these changes Dec 6, 2017

@bunnei

This comment has been minimized.

Copy link
Member

bunnei commented Dec 6, 2017

@muemart Thanks for your contribution! Based on this, I look forward to seeing more submissions from you.

Getting this feature right is pretty difficult - but it seems like this is a great start and things are pretty solid. I'm going to do something somewhat unconventional with this project and merge as-is. I'm sure there are a few bugs here and there, but they will be reported and we will work them out - this is a hugely requested feature, and there have been some tragically unsuccessful attempts getting it done, so I think we'll do things a little differently this time 😄

@bunnei bunnei merged commit e784434 into citra-emu:master Dec 6, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@FearlessTobi FearlessTobi referenced this pull request Jan 16, 2018

Closed

Things that should be ported over from Citra #40

34 of 35 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment