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

Refactor input emulation & add SDL gamepad support #2497

Merged
merged 11 commits into from Mar 17, 2017

Conversation

@wwylele
Member

wwylele commented Jan 30, 2017

First of all, sorry to @makotech222 for interrupting your attempt. Also thanks a lot for the inspiration.

Recommend reviewing commit-by-commit


What is changed in this PR

  • Remake the button and analog input using factory pattern
  • ini configuration files are now using a new format to store input configuration
  • SDL gamepad supported. But only configurable through ini files
  • Stop supporting ZL, ZR, Home button and C-Stick for now. They were in the wrong places.

What is not done in this PR, which can be done in the followup

  • Remake the touch and motion input as well. Completely decouple input from EmuWindow
  • Gamepad configuration through GUI
  • Handle ZL, ZR, Home button and C-Stick in correct services

Concept

An InputDevice object is an atomic unit of one input function in 3DS. InputDevice exposes one function GetStatus to 3DS system side, and expose several functions to the user side for changing status. The return type of GetStatus depends on the category. A 3DS consists of these InputDevice in 4 categories:

  • ButtonDevice (GetStatus returns bool):
    • handled by HID: face buttons A/B/X/Y, direction buttons, shoulder buttons L/R, Start, Select
    • handled by IR/IRRST: extra shoulder buttons ZL/ZR (not implemented in this PR)
    • handled by NS?: Home button (not implemented in this PR)
  • AnalogDevice (GetStatus returns a 2D vector)
    • handled by HID: circle pad
    • handled by IR/IRRST: circle pad pro, c-stick (not implemented)
  • Touch screen device (GetStatus returns a 2D vector and a bool): the touch screen handled by HID (not refactored in this PR)
  • Motion sensor device (GetStatus returns two 3D vector for accelerometer and gyroscope): one motion sensor handled by HID (not refactored in this PR)

A Factory produces InputDevices in the same category and using the same input emulation engine. It can also help frontend to communicate with produced InputDevices.

ParamPackage

This is a small implementation of string-based key-value container, which supports serializing to and deserializing from a single string for storing in ini file. This class will be used as parameter for creating input devices. Apart from a mandatory parameter "engine" for the name of the factory, the factory implementation will decide the name (key) of parameters and their meanings. This class will serialize the data in the following format: "key0:value0,key1:value1,...,keyN:valueN", and it support escape charactor: "$0", "$1" and "$2" stand for ":", "," and "$" that appear in keys/values, respectively.

I am not sure if I should implement this by hand, or should use some existing structure like INI, JSON or XML.

input_common

This is a collection of implemented input devices/factories. As a module, Its dependency level is between core and citra/citra-qt. In other words, input_common depends on core (specifically core/frontend/input.h), and is depended by the frontend.

Implemented input device/factory

  • Keyboard (provides ButtonDevice): a reimplementation of current keyboard input. Replaces the "direct keys" part of key_map.cpp. Parameters required in ParamPackage are "engine:keyboard,code:[key_code]", where [key_code] is a integer for key code
  • AnalogFromButton (provides AnalogDevice): a reimplementation of current circle pad emulation. Replaces the "indirect keys" part of key_map.cpp. Parameters required in ParamPackage are "engine:analog_from_button,up:[button],down:[button],left:[button],right:[button],modifier:[button],modifier_scale:[scale]", where [button]s are second level ParamPackages to create ButtonDevices, and [scale] is a float number;
  • SDL gamepad input (provides both ButtonDevice and AnalogDevice): an implementation of SDL input. Parameters required in ParamPackage are "engine:sdl,joystick:[gamepad_id],button:[button_id](for ButtonDevice),hat:[hat_id],direction=[direction_name](for ButtonDevice bound with hat as direction button),axis_x=[axis_id],axis_y=[axis_id](for Analog Device)"

Example configuration string in ini file:

The default configuration

button_a="code:65,engine:keyboard"
button_b="code:83,engine:keyboard"
circle_pad="engine:analog_from_button,modifier_scale:0.500000,up:code$016777235$1engine$0keyboard,down:code$016777237$1engine$0keyboard,left:code$016777234$1engine$0keyboard,right:code$016777236$1engine$0keyboard,modifier:code$068$1engine$0keyboard"

Example configuration to a gamepad

button_a="engine:sdl,joystick:0,button:0"
button_b="engine:sdl,joystick:0,button:1"
button_up="engine:sdl,joystick:0,hat:0,direction:up"
circle_pad="engine:sdl,joystick:0,axis_x:0,axis_y:1"

Note: input using different engine can mix together.

Reworking Qt configuration window

In this PR, I reworked Qt input configuration code, but kept the its function and appearance as is (i.e. can only configure input from keyboard). Enabling it to configure gamepad is another big topic and I don't want to introduce it here. For any one want to test gamepad, please manually edit the ini file following the format above.

Note: When reworking input configuration window, I replaced all Qt::Key by int. This is due to a Qt convention that Qt::Key is just a constant group, and use int to store key code (take QKeyEvent::key() for example, which returns int for code). This also reduces a lot of type casting


This change is Reviewable

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Jan 30, 2017

Member

Should fix the input part of #1883. I tried making it thread-safe in each input devices.

This PR conflict with #2479 and doesn't cover the issue there... I can follow the idea and do a similar fix. Should I include it in this PR?

Member

wwylele commented Jan 30, 2017

Should fix the input part of #1883. I tried making it thread-safe in each input devices.

This PR conflict with #2479 and doesn't cover the issue there... I can follow the idea and do a similar fix. Should I include it in this PR?

@j-selby

Just picking at spelling etc.

Awesome PR!

Show outdated Hide outdated src/citra_qt/configure_input.cpp
}
updateButtonLabels();
applyConfiguration();
}
void ConfigureInput::updateButtonLabels() {
for (const auto& input_id : Settings::NativeInput::All) {
button_map[input_id]->setText(getKeyName(key_map[input_id]));
QString non_keyboard(tr("(non-keybaord)"));

This comment has been minimized.

@j-selby

j-selby Jan 30, 2017

Contributor

Typo (non-keyboard)?

@j-selby

j-selby Jan 30, 2017

Contributor

Typo (non-keyboard)?

Show outdated Hide outdated src/citra_qt/configure_input.cpp
grabKeyboard();
grabMouse();
timer->start(5000); // Cancel after 5 seconds
timer->start(5000); // Cancel after 5 seconds*/

This comment has been minimized.

@j-selby

j-selby Jan 30, 2017

Contributor

Am I missing the opening /* for this?

@j-selby

j-selby Jan 30, 2017

Contributor

Am I missing the opening /* for this?

This comment has been minimized.

@wwylele

wwylele Jan 30, 2017

Member

This */ is a leftover. will remove

@wwylele

wwylele Jan 30, 2017

Member

This */ is a leftover. will remove

Show outdated Hide outdated src/common/param_package.cpp
namespace Common {
constexpr char KEY_VALUE_SEPARATER = ':';

This comment has been minimized.

@j-selby

j-selby Jan 30, 2017

Contributor

SEPARATOR

@j-selby

j-selby Jan 30, 2017

Contributor

SEPARATOR

Show outdated Hide outdated src/common/param_package.cpp
namespace Common {
constexpr char KEY_VALUE_SEPARATER = ':';
constexpr char PARAM_SEPARATER = ',';

This comment has been minimized.

@j-selby

j-selby Jan 30, 2017

Contributor

Ditto, and all below

@j-selby

j-selby Jan 30, 2017

Contributor

Ditto, and all below

}
}
void ConfigureInput::handleClick(Settings::NativeInput::Values input_id) {
QPushButton* button = button_map[input_id];
void ConfigureInput::handleClick(QPushButton* button, std::function<void(int)> new_key_setter) {
button->setText(tr("[press key]"));

This comment has been minimized.

@j-selby

j-selby Jan 30, 2017

Contributor

Is there a reason why we are using two different brackets here (( above, versus [ here)?

@j-selby

j-selby Jan 30, 2017

Contributor

Is there a reason why we are using two different brackets here (( above, versus [ here)?

This comment has been minimized.

@wwylele

wwylele Jan 30, 2017

Member

no reason. Will fix it :p

@wwylele

wwylele Jan 30, 2017

Member

no reason. Will fix it :p

This comment has been minimized.

@TAD3V

TAD3V Aug 2, 2018

missing a third bracket at the end

@TAD3V

TAD3V Aug 2, 2018

missing a third bracket at the end

@Shinmegumi

This comment has been minimized.

Show comment
Hide comment
@Shinmegumi

Shinmegumi Jan 30, 2017

Great work. Will this end up in bleeding edge sooner or later?

Shinmegumi commented Jan 30, 2017

Great work. Will this end up in bleeding edge sooner or later?

static std::shared_ptr<Keyboard> keyboard;
void Init() {

This comment has been minimized.

@lioncash

lioncash Jan 30, 2017

Member

This is kind of the same criticism that I brought up in the original PR, but all interfacing with the input backend should be done through the System class. There should ideally be no need to explicitly require initialization or shutdown. These are what constructors and destructors are for (at least as far as initial initialization/teardown is concerned in class-based interfaces).

This kind of limiting to one input subsystem at a time should not be necessary. i.e. it should be possible to start many system instances using different backends if available or a user chooses without anything preventing that. This also makes using citra as a lib, or moving core libraries towards being used as a library less cumbersome (the only reason System is a singleton at the moment is because namespaced-based interfaces like these still exist). If retrieving the emulated CPU instance is situated within the System class, then it also follows that getting the current input backend instance should be possible through that as well (and ideally the same with the video backend and audio backend, but those aren't relevant right now).

Also how well will this handle several different input interfaces on a system? Say someone made an XInput backend and also had the SDL one around - how well would this be able to handle that? What if a user wanted XInput over SDL, or SDL over XInput, how well would it handle that scenario?

@lioncash

lioncash Jan 30, 2017

Member

This is kind of the same criticism that I brought up in the original PR, but all interfacing with the input backend should be done through the System class. There should ideally be no need to explicitly require initialization or shutdown. These are what constructors and destructors are for (at least as far as initial initialization/teardown is concerned in class-based interfaces).

This kind of limiting to one input subsystem at a time should not be necessary. i.e. it should be possible to start many system instances using different backends if available or a user chooses without anything preventing that. This also makes using citra as a lib, or moving core libraries towards being used as a library less cumbersome (the only reason System is a singleton at the moment is because namespaced-based interfaces like these still exist). If retrieving the emulated CPU instance is situated within the System class, then it also follows that getting the current input backend instance should be possible through that as well (and ideally the same with the video backend and audio backend, but those aren't relevant right now).

Also how well will this handle several different input interfaces on a system? Say someone made an XInput backend and also had the SDL one around - how well would this be able to handle that? What if a user wanted XInput over SDL, or SDL over XInput, how well would it handle that scenario?

This comment has been minimized.

@wwylele

wwylele Jan 30, 2017

Member

The point is input_common is not an input subsystem. ( If you like, the core/frontend/input.h is the actual input subsystem). core can run perfectly without input_common (just no input method). The original idea is, the frontend provides the input method and plugs in the factory via (core)Input::RegisterFactory. The input_common is here only because, our two frontends have the same factory to register (keyboard, analog_from_button and sdl). To avoid the repeating code, I move these three engines into a common module and provide a shortcut to register all of them.

If one want to make a new interface, they can just make a subclass of Factory, put the code anywhere they want, and call RegisterFactory in frontend booting.

@wwylele

wwylele Jan 30, 2017

Member

The point is input_common is not an input subsystem. ( If you like, the core/frontend/input.h is the actual input subsystem). core can run perfectly without input_common (just no input method). The original idea is, the frontend provides the input method and plugs in the factory via (core)Input::RegisterFactory. The input_common is here only because, our two frontends have the same factory to register (keyboard, analog_from_button and sdl). To avoid the repeating code, I move these three engines into a common module and provide a shortcut to register all of them.

If one want to make a new interface, they can just make a subclass of Factory, put the code anywhere they want, and call RegisterFactory in frontend booting.

This comment has been minimized.

@lioncash

lioncash Jan 30, 2017

Member

OK, that makes sense.

However, it looks like there's some backends that would be initialized but not shut down (e.g. SDL_Init is called via SDL::Init, but SDL_Quit would never be called by anything). SDL would also be initialized, even if it hypothetically wasn't an active input backend, but still built into citra.

@lioncash

lioncash Jan 30, 2017

Member

OK, that makes sense.

However, it looks like there's some backends that would be initialized but not shut down (e.g. SDL_Init is called via SDL::Init, but SDL_Quit would never be called by anything). SDL would also be initialized, even if it hypothetically wasn't an active input backend, but still built into citra.

Show outdated Hide outdated src/input_common/sdl.cpp
class SDLJoystick {
public:
SDLJoystick(int joystick_index) {

This comment has been minimized.

@lioncash

lioncash Jan 30, 2017

Member

explicit SDLJoystick

@lioncash

lioncash Jan 30, 2017

Member

explicit SDLJoystick

Show outdated Hide outdated src/input_common/keyboard.cpp
class KeyButton : public Input::ButtonDevice {
public:
KeyButton(std::shared_ptr<KeyButtonList> key_button_list_)

This comment has been minimized.

@lioncash

lioncash Jan 30, 2017

Member

explicit KeyButton

@lioncash

lioncash Jan 30, 2017

Member

explicit KeyButton

Show outdated Hide outdated src/common/param_package.h
ParamPackage(ParamPackage&& other);
ParamPackage& operator=(const ParamPackage& other) = default;
ParamPackage& operator=(ParamPackage&& other);

This comment has been minimized.

@lioncash

lioncash Jan 30, 2017

Member

You can default this for the same behavior.

@lioncash

lioncash Jan 30, 2017

Member

You can default this for the same behavior.

Show outdated Hide outdated src/common/param_package.cpp
} else {
try {
return std::stoi(pair->second);
} catch (...) {

This comment has been minimized.

@lioncash

lioncash Jan 30, 2017

Member

This should catch const std::logic_error& rather than sinking all exception types. Ditto for below in the other Get()

@lioncash

lioncash Jan 30, 2017

Member

This should catch const std::logic_error& rather than sinking all exception types. Ditto for below in the other Get()

Show outdated Hide outdated src/common/param_package.cpp
if (pair == data.end()) {
LOG_DEBUG(Common, "key %s not found", key.c_str());
return default_value;
} else {

This comment has been minimized.

@lioncash

lioncash Jan 30, 2017

Member

else isn't necessary since the if terminates with a return.

@lioncash

lioncash Jan 30, 2017

Member

else isn't necessary since the if terminates with a return.

Show outdated Hide outdated src/common/param_package.cpp
if (data.empty())
return "";
for (auto pair : data) {

This comment has been minimized.

@lioncash

lioncash Jan 30, 2017

Member

const auto&

@lioncash

lioncash Jan 30, 2017

Member

const auto&

Show outdated Hide outdated src/common/param_package.cpp
return "";
for (auto pair : data) {
std::string key_value[2]{pair.first, pair.second};

This comment has been minimized.

@lioncash

lioncash Jan 30, 2017

Member

std::array

@lioncash

lioncash Jan 30, 2017

Member

std::array

Show outdated Hide outdated src/common/param_package.h
using DataType = std::unordered_map<std::string, std::string>;
ParamPackage() = default;
ParamPackage(const std::string& serialized);

This comment has been minimized.

@lioncash

lioncash Jan 30, 2017

Member

This should likely be an explicit constructor.

@lioncash

lioncash Jan 30, 2017

Member

This should likely be an explicit constructor.

@lioncash

This comment has been minimized.

Show comment
Hide comment
@lioncash

lioncash Jan 30, 2017

Member

Also with regards to:

In other words, input_common depends on core

This should be the other way around. Core should depend on InputCommon. It's point of existence should be to bridge inputs so they can be used in conjunction with core. It's somewhat nonsensical to have a common lib depending on the core -- in the same way that it would be odd for the regular common to depend on core.

Member

lioncash commented Jan 30, 2017

Also with regards to:

In other words, input_common depends on core

This should be the other way around. Core should depend on InputCommon. It's point of existence should be to bridge inputs so they can be used in conjunction with core. It's somewhat nonsensical to have a common lib depending on the core -- in the same way that it would be odd for the regular common to depend on core.

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Jan 30, 2017

Member

This should be the other way around. Core should depend on InputCommon. It's point of existence should be to bridge inputs so they can be used in conjunction with core. It's somewhat nonsensical to have a common lib depending on the core -- in the same way that it would be odd for the regular common to depend on core.

Probably because I made a bad name, but input_common mean to be a common library for frontend (not for core!). The existence of this is to collect the code that both two frontends will use.

Also, say one want to port citra to Android, they will completely remove input_common and provide the input method that Android uses. In this sense core shouldn't depend on that

Member

wwylele commented Jan 30, 2017

This should be the other way around. Core should depend on InputCommon. It's point of existence should be to bridge inputs so they can be used in conjunction with core. It's somewhat nonsensical to have a common lib depending on the core -- in the same way that it would be odd for the regular common to depend on core.

Probably because I made a bad name, but input_common mean to be a common library for frontend (not for core!). The existence of this is to collect the code that both two frontends will use.

Also, say one want to port citra to Android, they will completely remove input_common and provide the input method that Android uses. In this sense core shouldn't depend on that

@lioncash

This comment has been minimized.

Show comment
Hide comment
@lioncash

lioncash Jan 30, 2017

Member

It's still a design issue though. System should be what houses or provides the reference to the input backend. (via Input(), an analog to the CPU backend's CPU(), therefore input_common cannot depend on core, otherwise you get a circular dependency.

Member

lioncash commented Jan 30, 2017

It's still a design issue though. System should be what houses or provides the reference to the input backend. (via Input(), an analog to the CPU backend's CPU(), therefore input_common cannot depend on core, otherwise you get a circular dependency.

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Jan 30, 2017

Member

First of all, there is no circular dependency as I already pointed out core doesn't depend on input_common. Second, core is housing the input backend. The role of input_core is just providing the implementation and call the core to receive them.

I probably still don't fully under stand what you says. So let's try this:
If I remove input_common, move everything in input_common to citra_qt (ignore citra to simplify the problem), and in citra_qt's booting it calls four Input::RegisterFactories. Is there any design issue in this case?

Member

wwylele commented Jan 30, 2017

First of all, there is no circular dependency as I already pointed out core doesn't depend on input_common. Second, core is housing the input backend. The role of input_core is just providing the implementation and call the core to receive them.

I probably still don't fully under stand what you says. So let's try this:
If I remove input_common, move everything in input_common to citra_qt (ignore citra to simplify the problem), and in citra_qt's booting it calls four Input::RegisterFactories. Is there any design issue in this case?

@lioncash

This comment has been minimized.

Show comment
Hide comment
@lioncash

lioncash Jan 30, 2017

Member

Is there any design issue in this case?

Excluding the cases that it's still:

  1. A namespace based "interface" (something that should gradually be gotten rid of in the codebase, at least for the 'main 4' - audio, input, graphics, cpu)
  2. Input not being a fixture in System

There would be a new third issue that it's now dependent on a specific UI frontend.

Excluding the above three, then no, there is no design issue.

Member

lioncash commented Jan 30, 2017

Is there any design issue in this case?

Excluding the cases that it's still:

  1. A namespace based "interface" (something that should gradually be gotten rid of in the codebase, at least for the 'main 4' - audio, input, graphics, cpu)
  2. Input not being a fixture in System

There would be a new third issue that it's now dependent on a specific UI frontend.

Excluding the above three, then no, there is no design issue.

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Jan 30, 2017

Member
  1. A namespace based "interface" (something that should gradually be gotten rid of in the codebase, at least for the 'main 4' - audio, input, graphics, cpu)
  2. Input not being a fixture in System

If this is an issue, I can wrap core/front_end/input.h to a class and move it into System. I didn't do this because core/front_end/input.h is so simple that it doesn't need an initialize function. I will do this later

There would be a new third issue that it's now dependent on a specific UI frontend.

Yes, I noticed this too. the SDL frontend (and any other PC frontend) should share these factories. So I move them out of citra_qt, to a common frontend library called input_common, so that all frontend can use it. In this step what I did is just to extract the common part of frontend. Does this generate new design issue?

Member

wwylele commented Jan 30, 2017

  1. A namespace based "interface" (something that should gradually be gotten rid of in the codebase, at least for the 'main 4' - audio, input, graphics, cpu)
  2. Input not being a fixture in System

If this is an issue, I can wrap core/front_end/input.h to a class and move it into System. I didn't do this because core/front_end/input.h is so simple that it doesn't need an initialize function. I will do this later

There would be a new third issue that it's now dependent on a specific UI frontend.

Yes, I noticed this too. the SDL frontend (and any other PC frontend) should share these factories. So I move them out of citra_qt, to a common frontend library called input_common, so that all frontend can use it. In this step what I did is just to extract the common part of frontend. Does this generate new design issue?

@lioncash

This comment has been minimized.

Show comment
Hide comment
@lioncash

lioncash Jan 30, 2017

Member

If this is an issue

Well, yeah, it kind of is. The entire point of System is to encapsulate all system state. Input is one of the things that comprise said state. You should ideally be able to just create a new instance of System in order to instantiate most of the necessary scaffolding to kick up a new core instance—that's the end goal. That's why it was created in the first place.

Does this generate new design issue?

No, in that case, it's fine.

Member

lioncash commented Jan 30, 2017

If this is an issue

Well, yeah, it kind of is. The entire point of System is to encapsulate all system state. Input is one of the things that comprise said state. You should ideally be able to just create a new instance of System in order to instantiate most of the necessary scaffolding to kick up a new core instance—that's the end goal. That's why it was created in the first place.

Does this generate new design issue?

No, in that case, it's fine.

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Jan 30, 2017

Member

No, in that case, it's fine.

So, that case is what it is now.

Member

wwylele commented Jan 30, 2017

No, in that case, it's fine.

So, that case is what it is now.

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Jan 30, 2017

Member

Also about making Input into a class and as a component, I still find it wierd, because:
The state Input contains is just some device factory list (currently two). Factories shouldn't be a state of 3DS system (in the same way that real 3DS factory doesn't package itself into a 3DS). Actually, two System object can share one factory without any problem (just remember to do synchronization for thread safety). What needs to be a state of System is the devices that factory created - yes, they are not System members now, but this involves that we should move all system module variables into the System object, which is another big topic that I don't want to talk about here

Member

wwylele commented Jan 30, 2017

Also about making Input into a class and as a component, I still find it wierd, because:
The state Input contains is just some device factory list (currently two). Factories shouldn't be a state of 3DS system (in the same way that real 3DS factory doesn't package itself into a 3DS). Actually, two System object can share one factory without any problem (just remember to do synchronization for thread safety). What needs to be a state of System is the devices that factory created - yes, they are not System members now, but this involves that we should move all system module variables into the System object, which is another big topic that I don't want to talk about here

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Jan 30, 2017

Member

Addressed comments above except for the SDL init/quit one. Give me a while to think

Member

wwylele commented Jan 30, 2017

Addressed comments above except for the SDL init/quit one. Give me a while to think

@archshift

This comment has been minimized.

Show comment
Hide comment
@archshift

archshift Jan 30, 2017

Contributor

Does this address threading at all? (wrt #1883)

Contributor

archshift commented Jan 30, 2017

Does this address threading at all? (wrt #1883)

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Jan 30, 2017

Member

@archshift tbh I don't really understand the thread issue of input. I think as long as the input status is atomic-read/write, it has no problems. Probably I missed something. Anyway, I am not confident enough so I erased that sentence.

Member

wwylele commented Jan 30, 2017

@archshift tbh I don't really understand the thread issue of input. I think as long as the input status is atomic-read/write, it has no problems. Probably I missed something. Anyway, I am not confident enough so I erased that sentence.

@makotech222

This comment has been minimized.

Show comment
Hide comment
@makotech222

makotech222 Jan 30, 2017

Contributor

@wwylele Keyboard inputs are gathered from the frontend on a separate thread than what citra is running on.

Contributor

makotech222 commented Jan 30, 2017

@wwylele Keyboard inputs are gathered from the frontend on a separate thread than what citra is running on.

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Jan 31, 2017

Member

@makotech222 sure I knew that. What I don't know is what else I should do, after I made the status that two threads read/write atomic.

Member

wwylele commented Jan 31, 2017

@makotech222 sure I knew that. What I don't know is what else I should do, after I made the status that two threads read/write atomic.

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Jan 31, 2017

Member

Update:

  • Add Shutdown function in input_common/main.cpp. Do quit SDL joystick subsystem when shutting down.
  • Change HID::ReloadInputDevices to just make a notification. The actual reloading will happen on the next UpdatePadCallback cycle. This avoid a potential race condition if the device gets reload when UpdatePadCallback is reading the status.

Also about what @lioncash said:

SDL would also be initialized, even if it hypothetically wasn't an active input backend, but still built into citra.

I do recognize this issue. However, I want to put this to another PR, where the SDL engine can be initialized conditionally depending on user settings. It should be as trivial as just break up InputCommon::Init() into small pieces. Currently leaving SDL initialized doesn't hurt anything.

Member

wwylele commented Jan 31, 2017

Update:

  • Add Shutdown function in input_common/main.cpp. Do quit SDL joystick subsystem when shutting down.
  • Change HID::ReloadInputDevices to just make a notification. The actual reloading will happen on the next UpdatePadCallback cycle. This avoid a potential race condition if the device gets reload when UpdatePadCallback is reading the status.

Also about what @lioncash said:

SDL would also be initialized, even if it hypothetically wasn't an active input backend, but still built into citra.

I do recognize this issue. However, I want to put this to another PR, where the SDL engine can be initialized conditionally depending on user settings. It should be as trivial as just break up InputCommon::Init() into small pieces. Currently leaving SDL initialized doesn't hurt anything.

@phire

This comment has been minimized.

Show comment
Hide comment
@phire

phire Feb 3, 2017

I tested this branch for a few hours; Configuring through ini files is a bit of a pain.

One minor flaw I found is that SDL doesn't expose the d-pad buttons over it's button API, instead it converts them to a hat input (which this PR doesn't support) and exposes that.

Fixing this would require adding a second ButtonDevice class to SDL which binds to hat inputs. Could be accessed with a engine:sdl,joystick:0,hat:up parameter pack.

phire commented Feb 3, 2017

I tested this branch for a few hours; Configuring through ini files is a bit of a pain.

One minor flaw I found is that SDL doesn't expose the d-pad buttons over it's button API, instead it converts them to a hat input (which this PR doesn't support) and exposes that.

Fixing this would require adding a second ButtonDevice class to SDL which binds to hat inputs. Could be accessed with a engine:sdl,joystick:0,hat:up parameter pack.

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Feb 3, 2017

Member

Update:

  • added support for hat in SDL. usage example: button_up="engine:sdl,joystick:0,hat:0,direction:up" (not tested yet. @phire could you help me test it?)
  • fixed variable shadowing and missing override in the SDL part
Member

wwylele commented Feb 3, 2017

Update:

  • added support for hat in SDL. usage example: button_up="engine:sdl,joystick:0,hat:0,direction:up" (not tested yet. @phire could you help me test it?)
  • fixed variable shadowing and missing override in the SDL part
@phire

This comment has been minimized.

Show comment
Hide comment
@phire

phire Feb 3, 2017

Tested the dpad mapping, it works.

Here is my config for an xinput controller, if anyone wants to test:

button_a="engine:sdl,joystick:0,button:1"
button_b="engine:sdl,joystick:0,button:0"
button_x="engine:sdl,joystick:0,button:3"
button_y="engine:sdl,joystick:0,button:2"
button_up="engine:sdl,joystick:0,hat:0,direction:up"
button_down="engine:sdl,joystick:0,hat:0,direction:down"
button_left="engine:sdl,joystick:0,hat:0,direction:left"
button_right="engine:sdl,joystick:0,hat:0,direction:right"
button_l="engine:sdl,joystick:0,button:4"
button_r="engine:sdl,joystick:0,button:5"
button_start="engine:sdl,joystick:0,button:7"
button_select="engine:sdl,joystick:0,button:6"
button_zl="engine:keyboard,code:49"
button_zr="engine:keyboard,code:50"
button_home="engine:keyboard,code:66"
circle_pad="axis_x:0,engine:sdl,joystick:0,axis_y:1"
c_stick="axis_x:2,engine:sdl,joystick:0,axis_y:3"

The mapping works with any xinput compatible controller, I personally tested xbox 360, xbox one and the dual shock 4 through a xinput wrapper. Note, X Y A B buttons match the 3DS's face layout, not the labels on the 360 controller. L and R are mapped to the bumpers.

One extra deficiency I've just noticed. You are going to need the reverse button_from_analog mapping at some point, otherwise you can't bind ZL and ZR to the triggers. I guess it's out of the scope of this PR.

phire commented Feb 3, 2017

Tested the dpad mapping, it works.

Here is my config for an xinput controller, if anyone wants to test:

button_a="engine:sdl,joystick:0,button:1"
button_b="engine:sdl,joystick:0,button:0"
button_x="engine:sdl,joystick:0,button:3"
button_y="engine:sdl,joystick:0,button:2"
button_up="engine:sdl,joystick:0,hat:0,direction:up"
button_down="engine:sdl,joystick:0,hat:0,direction:down"
button_left="engine:sdl,joystick:0,hat:0,direction:left"
button_right="engine:sdl,joystick:0,hat:0,direction:right"
button_l="engine:sdl,joystick:0,button:4"
button_r="engine:sdl,joystick:0,button:5"
button_start="engine:sdl,joystick:0,button:7"
button_select="engine:sdl,joystick:0,button:6"
button_zl="engine:keyboard,code:49"
button_zr="engine:keyboard,code:50"
button_home="engine:keyboard,code:66"
circle_pad="axis_x:0,engine:sdl,joystick:0,axis_y:1"
c_stick="axis_x:2,engine:sdl,joystick:0,axis_y:3"

The mapping works with any xinput compatible controller, I personally tested xbox 360, xbox one and the dual shock 4 through a xinput wrapper. Note, X Y A B buttons match the 3DS's face layout, not the labels on the 360 controller. L and R are mapped to the bumpers.

One extra deficiency I've just noticed. You are going to need the reverse button_from_analog mapping at some point, otherwise you can't bind ZL and ZR to the triggers. I guess it's out of the scope of this PR.

Show outdated Hide outdated src/input_common/sdl.cpp
explicit SDLDirectionButton(std::shared_ptr<SDLJoystick> joystick_, int hat_, Uint8 direction_)
: joystick(joystick_), hat(hat_), direction(direction_) {}
bool GetStatus() const {

This comment has been minimized.

@phire

phire Feb 4, 2017

Needs to be marked override

@phire

phire Feb 4, 2017

Needs to be marked override

Show outdated Hide outdated src/input_common/sdl.cpp
// Make sure the coordinates are in the unit circle,
// otherwise normalize it.
float r = x * x + y * y;
if (r > 1) {

This comment has been minimized.

@phire

phire Feb 4, 2017

Really should be r > 1.0f

@phire

phire Feb 4, 2017

Really should be r > 1.0f

This comment has been minimized.

@mailwl

mailwl Feb 4, 2017

Contributor

interesting... compiler casts r to int, or 1 to float?

@mailwl

mailwl Feb 4, 2017

Contributor

interesting... compiler casts r to int, or 1 to float?

This comment has been minimized.

@phire

phire Feb 4, 2017

Compiler casts 1 to float.

https://godbolt.org/g/E6ZycE

@phire

This comment has been minimized.

@mailwl

mailwl Feb 4, 2017

Contributor

ah, i see. so casting int(1) to (float)1 just for clarity or unusual compiler in the future?
make sense

@mailwl

mailwl Feb 4, 2017

Contributor

ah, i see. so casting int(1) to (float)1 just for clarity or unusual compiler in the future?
make sense

This comment has been minimized.

@phire

phire Feb 4, 2017

Type promotion behaviour is defined by the c++ standard, so all compilers should do the same thing.

It's mainly for readability. Some people do have the type promotion rules memorized and will instantly know it will do the correct thing. But for the rest of us, we just waste time wondering what will actually happen.

Also, I have been burned by the type promotion behaviour in the past.

@phire

phire Feb 4, 2017

Type promotion behaviour is defined by the c++ standard, so all compilers should do the same thing.

It's mainly for readability. Some people do have the type promotion rules memorized and will instantly know it will do the correct thing. But for the rest of us, we just waste time wondering what will actually happen.

Also, I have been burned by the type promotion behaviour in the past.

@mailwl

This comment has been minimized.

Show comment
Hide comment
@mailwl

mailwl Feb 4, 2017

Contributor

just a question. ZL/ZR+circle pad is really IR responsibility?
i mean new 3ds still doubles on IR keypressed events?

Contributor

mailwl commented Feb 4, 2017

just a question. ZL/ZR+circle pad is really IR responsibility?
i mean new 3ds still doubles on IR keypressed events?

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Feb 4, 2017

Member

@mailwl yes if 3dbrew documents it correctly: https://www.3dbrew.org/wiki/IRRST_Shared_Memory

Member

wwylele commented Feb 4, 2017

@mailwl yes if 3dbrew documents it correctly: https://www.3dbrew.org/wiki/IRRST_Shared_Memory

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Feb 4, 2017

Member

Update:

  • addressed comments above
  • InputCommon: add AnalogFromButton
- Input::UnregisterFactory<Input::ButtonDevice>("analog_from_button");
+ Input::UnregisterFactory<Input::AnalogDevice>("analog_from_button");
Member

wwylele commented Feb 4, 2017

Update:

  • addressed comments above
  • InputCommon: add AnalogFromButton
- Input::UnregisterFactory<Input::ButtonDevice>("analog_from_button");
+ Input::UnregisterFactory<Input::AnalogDevice>("analog_from_button");
@lioncash

Reviewed general things this time instead of the interface.

It might also be preferable to move the sdl code into its own subdirectory. That way, if any other backends are added, they're kept separate (and the directories can hold any extra files they may use, instead of putting them in the root directory).

Show outdated Hide outdated src/common/param_package.cpp
ParamPackage::ParamPackage(std::initializer_list<DataType::value_type> list) : data(list) {}
std::string ParamPackage::Serialize() const {
std::string result;

This comment has been minimized.

@lioncash

lioncash Feb 5, 2017

Member

This should likely be after the if statement below.

@lioncash

lioncash Feb 5, 2017

Member

This should likely be after the if statement below.

Show outdated Hide outdated src/common/param_package.cpp
result += key_value[0] + KEY_VALUE_SEPARATOR + key_value[1] + PARAM_SEPARATOR;
}
result.resize(result.size() - 1); // discard the trailing PARAM_SEPARATOR

This comment has been minimized.

@lioncash

lioncash Feb 5, 2017

Member

result.pop_back()

@lioncash

lioncash Feb 5, 2017

Member

result.pop_back()

Show outdated Hide outdated src/common/param_package.cpp
try {
return std::stoi(pair->second);
} catch (std::logic_error) {

This comment has been minimized.

@lioncash

lioncash Feb 5, 2017

Member

This should catch by const reference. (const std::logic_error&). Ditto below.

@lioncash

lioncash Feb 5, 2017

Member

This should catch by const reference. (const std::logic_error&). Ditto below.

Show outdated Hide outdated src/input_common/analog_from_button.h
/**
* Creates an analog device from direction button devices
* @param params contains parameters for creating the device:
* "up": a serialized ParamPackage for creating a button device for up direction

This comment has been minimized.

@lioncash

lioncash Feb 5, 2017

Member

These should have hyphens before them (e.g. - "up" ...) so that doxygen would generate a bulleted list.

@lioncash

lioncash Feb 5, 2017

Member

These should have hyphens before them (e.g. - "up" ...) so that doxygen would generate a bulleted list.

Show outdated Hide outdated src/input_common/sdl.cpp
return std::make_tuple(x, y);
}
bool GetHatDirection(int hat, Uint8 direction) {

This comment has been minimized.

@lioncash

lioncash Feb 5, 2017

Member

This can be a const member function

@lioncash

lioncash Feb 5, 2017

Member

This can be a const member function

Show outdated Hide outdated src/input_common/sdl.cpp
// Copyright 2017 Citra Emulator Project
// Licensed under GPLv2 or any later version
// Refer to the license.txt file included.

This comment has been minimized.

@lioncash

lioncash Feb 5, 2017

Member

<cmath>, <memory>, <string>, and <tuple> need to be included.

@lioncash

lioncash Feb 5, 2017

Member

<cmath>, <memory>, <string>, and <tuple> need to be included.

Show outdated Hide outdated src/input_common/main.h
#pragma once
#include <string>
#include "input_common/keyboard.h"

This comment has been minimized.

@lioncash

lioncash Feb 5, 2017

Member

this can be a forward declaration.

@lioncash

lioncash Feb 5, 2017

Member

this can be a forward declaration.

This comment has been minimized.

@wwylele

wwylele Feb 5, 2017

Member

Keyboard needs to expose its KeyPressed/Released interface to frontend

@wwylele

wwylele Feb 5, 2017

Member

Keyboard needs to expose its KeyPressed/Released interface to frontend

This comment has been minimized.

@lioncash

lioncash Feb 5, 2017

Member

You can just include this where it's needed then. The only thing this immediate header needs defined is Keyboard* which a forward declaration will suffice in doing.

@lioncash

lioncash Feb 5, 2017

Member

You can just include this where it's needed then. The only thing this immediate header needs defined is Keyboard* which a forward declaration will suffice in doing.

Show outdated Hide outdated src/input_common/main.cpp
// Licensed under GPLv2 or any later version
// Refer to the license.txt file included.
#include "common/param_package.h"

This comment has been minimized.

@lioncash

lioncash Feb 5, 2017

Member

<memory> needs to be included.

@lioncash

lioncash Feb 5, 2017

Member

<memory> needs to be included.

Show outdated Hide outdated src/input_common/keyboard.h
#pragma once
#include <list>
#include <mutex>

This comment has been minimized.

@lioncash

lioncash Feb 5, 2017

Member

<list> and <mutex> should be in the cpp file, as they're not used anywhere in the header.

@lioncash

lioncash Feb 5, 2017

Member

<list> and <mutex> should be in the cpp file, as they're not used anywhere in the header.

Show outdated Hide outdated src/input_common/analog_from_button.h
#pragma once
#include <list>

This comment has been minimized.

@lioncash

lioncash Feb 5, 2017

Member

<list> and <mutex> aren't used anywhere in the header.

@lioncash

lioncash Feb 5, 2017

Member

<list> and <mutex> aren't used anywhere in the header.

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Feb 5, 2017

Member

Addressed comments above and rebased. I will split out the sdl code on another day.

Member

wwylele commented Feb 5, 2017

Addressed comments above and rebased. I will split out the sdl code on another day.

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Feb 5, 2017

Member

Update: I added back the std::move in Keyboard::Create. Removing it causes error on clang:

/Users/travis/build/citra-emu/citra/src/input_common/keyboard.cpp:71:12: error: no viable conversion from returned value of type 'unique_ptr<InputCommon::KeyButton>' to function return type 'unique_ptr<Input::ButtonDevice>'

This is due to the return type is not the same of the returned object. It need to do derived->base conversion and an implicit move is not allowed.

Member

wwylele commented Feb 5, 2017

Update: I added back the std::move in Keyboard::Create. Removing it causes error on clang:

/Users/travis/build/citra-emu/citra/src/input_common/keyboard.cpp:71:12: error: no viable conversion from returned value of type 'unique_ptr<InputCommon::KeyButton>' to function return type 'unique_ptr<Input::ButtonDevice>'

This is due to the return type is not the same of the returned object. It need to do derived->base conversion and an implicit move is not allowed.

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Feb 9, 2017

Member

moved sdl.cpp/sdl.h into input_common/sdl folder

Member

wwylele commented Feb 9, 2017

moved sdl.cpp/sdl.h into input_common/sdl folder

@bunnei

This comment has been minimized.

Show comment
Hide comment
@bunnei

bunnei Feb 11, 2017

Member

Fixes #965

Member

bunnei commented Feb 11, 2017

Fixes #965

@bunnei

bunnei approved these changes Mar 2, 2017

Did a high-level review and I'm comfortable with this change, although I didn't go through this with a fine comb and unfortunately won't have the time to for the next few weeks. Nonetheless, signing off because I think this is a good improvement over what we already have, and I have no problems merging this.

That being said, reminder to folks who have strong opinions one way or another to chime in here, or review if you have anything to add. Thanks.

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Mar 2, 2017

Member

Signing off from bunnei means this is out of RFC state.
more reviews are welcome.

Member

wwylele commented Mar 2, 2017

Signing off from bunnei means this is out of RFC state.
more reviews are welcome.

@wwylele wwylele changed the title from [RFC] Refactor input emulation & add SDL gamepad support to Refactor input emulation & add SDL gamepad support Mar 2, 2017

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Mar 2, 2017

Member

Update: fixed connecting to nullptr in configure_input.

Member

wwylele commented Mar 2, 2017

Update: fixed connecting to nullptr in configure_input.

@disk64

This comment has been minimized.

Show comment
Hide comment
@disk64

disk64 Mar 2, 2017

Hey all,
While trying out this pull request, I've found that in games such as Majora's Mask and Ocarina of Time, the character often cannot run at full speed with the joystick fully pressed in a direction (depending on the direction pressed).
Looking at jstest-gtk, the axis values I'm getting from my joystick range from -32767 to 32124 for the X axis and -32458 to 32451 for Y axis, which are very close to the actual min/max range of these axes, so I suspect something in citra's code is the cause.

Digging into the code, the variable MAX_CIRCLEPAD_POS looked suspicious (defined here )

I tried changing the value of this variable from 0x9C to 0xFF for kicks to see what would happen.
Sure enough, I was then able to run at full speed in OoT3D/MM3D!

I'm not sure why a max analog cap was made for the circlepad, but I would recommend it be removed or revised, as best as I can tell it is resulting in inaccurate input emulation.
As a sidenote, this should also translate over when working on re-implementing C-stick support.

disk64 commented Mar 2, 2017

Hey all,
While trying out this pull request, I've found that in games such as Majora's Mask and Ocarina of Time, the character often cannot run at full speed with the joystick fully pressed in a direction (depending on the direction pressed).
Looking at jstest-gtk, the axis values I'm getting from my joystick range from -32767 to 32124 for the X axis and -32458 to 32451 for Y axis, which are very close to the actual min/max range of these axes, so I suspect something in citra's code is the cause.

Digging into the code, the variable MAX_CIRCLEPAD_POS looked suspicious (defined here )

I tried changing the value of this variable from 0x9C to 0xFF for kicks to see what would happen.
Sure enough, I was then able to run at full speed in OoT3D/MM3D!

I'm not sure why a max analog cap was made for the circlepad, but I would recommend it be removed or revised, as best as I can tell it is resulting in inaccurate input emulation.
As a sidenote, this should also translate over when working on re-implementing C-stick support.

@wwylele

This comment has been minimized.

Show comment
Hide comment
@wwylele

wwylele Mar 2, 2017

Member

@disk64 the value 0x9C is inherited from the old code (= not something I made up in this PR), and was originally observed as a maximum value that a real 3DS can get (= we were trying to get accurate input emulation).

Could you try if keyboard input shares the same issue (Try both master branch and this PR)? Note that keyboard input also uses this maximum value. If issue also presents in keyboard input, we need to revise the value; otherwise, it is more like the games are very sensitive to the little difference between actual value and maximum one. Then we need joystick calibration feature in citra (will be in another PR)

Member

wwylele commented Mar 2, 2017

@disk64 the value 0x9C is inherited from the old code (= not something I made up in this PR), and was originally observed as a maximum value that a real 3DS can get (= we were trying to get accurate input emulation).

Could you try if keyboard input shares the same issue (Try both master branch and this PR)? Note that keyboard input also uses this maximum value. If issue also presents in keyboard input, we need to revise the value; otherwise, it is more like the games are very sensitive to the little difference between actual value and maximum one. Then we need joystick calibration feature in citra (will be in another PR)

@disk64

This comment has been minimized.

Show comment
Hide comment
@disk64

disk64 Mar 3, 2017

I initially didn't think this was an issue joystick calibration will solve- looking at the min/max values for my joystick that would be used for calibration, my joystick is hitting very close to the limits of joystick input values (-32768 to 32767), so calibration would do practically nothing for the controller I'm using compared to just using the raw input values as-is.

I tested keyboard input and it does not have this issue- OK, so the value 0x9C should stay.
Given 0x9C is the correct max value for the circle pad, and that calibration would not significantly change the input coming from my controller (as I've described), I figured there must be a bug hiding somewhere else, so I took some time to dig deeper.
Eventually I found that the joystick values coming from SDL_JoystickGetAxis() were not matching the values I was seeing in jstest-gtk, instead ranging from -27628 to 25571 on the X-axis and -26857 to 26856 on the Y-axis.
jstest-gtk was adjusting the joystick values based on calibration settings, which I didn't think it was doing, my apologies.

On Linux SDL2 uses the newer evdev, which has a separate set of calibration settings from the older joydev (which jstest-gtk uses). Like joydev, evdev's calibration settings are applied system-wide (so SDL2's joystick values will be affected by this evdev joystick calibration).
Unfortunately there isn't a GUI program for working with evdev joystick calibration settings like jstest-gtk for joydev, but there is a command-line tool, evdev-joystick, so I used this to configure my joystick properly, after which SDL2 returned proper, calibrated values when SDL_JoystickGetAxis() was called.

At least in the case of Linux, this is less an issue of citra needing a calibration feature and more an issue of Linux needing a GUI tool for system-wide joystick calibration (evdev-joystick is a relatively new program, and at one point some websites were mistakenly reporting that evdev had no calibration feature at all because there was no tool that could modify the calibration settings that were there yet).

Well, this was a fun rabbit-hole to poke my head in, I learned a lot of things about my operating system I'm surprised I didn't know already ;)

disk64 commented Mar 3, 2017

I initially didn't think this was an issue joystick calibration will solve- looking at the min/max values for my joystick that would be used for calibration, my joystick is hitting very close to the limits of joystick input values (-32768 to 32767), so calibration would do practically nothing for the controller I'm using compared to just using the raw input values as-is.

I tested keyboard input and it does not have this issue- OK, so the value 0x9C should stay.
Given 0x9C is the correct max value for the circle pad, and that calibration would not significantly change the input coming from my controller (as I've described), I figured there must be a bug hiding somewhere else, so I took some time to dig deeper.
Eventually I found that the joystick values coming from SDL_JoystickGetAxis() were not matching the values I was seeing in jstest-gtk, instead ranging from -27628 to 25571 on the X-axis and -26857 to 26856 on the Y-axis.
jstest-gtk was adjusting the joystick values based on calibration settings, which I didn't think it was doing, my apologies.

On Linux SDL2 uses the newer evdev, which has a separate set of calibration settings from the older joydev (which jstest-gtk uses). Like joydev, evdev's calibration settings are applied system-wide (so SDL2's joystick values will be affected by this evdev joystick calibration).
Unfortunately there isn't a GUI program for working with evdev joystick calibration settings like jstest-gtk for joydev, but there is a command-line tool, evdev-joystick, so I used this to configure my joystick properly, after which SDL2 returned proper, calibrated values when SDL_JoystickGetAxis() was called.

At least in the case of Linux, this is less an issue of citra needing a calibration feature and more an issue of Linux needing a GUI tool for system-wide joystick calibration (evdev-joystick is a relatively new program, and at one point some websites were mistakenly reporting that evdev had no calibration feature at all because there was no tool that could modify the calibration settings that were there yet).

Well, this was a fun rabbit-hole to poke my head in, I learned a lot of things about my operating system I'm surprised I didn't know already ;)

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Mar 3, 2017

Maybe someone could write that "error" in an FAQ or something. People may encounter it in the future.

ghost commented Mar 3, 2017

Maybe someone could write that "error" in an FAQ or something. People may encounter it in the future.

@disk64

This comment has been minimized.

Show comment
Hide comment
@disk64

disk64 Mar 4, 2017

It's also worth noting that currently evdev lacks the same toolset that joydev has, which includes a graphical joystick calibration tool (jstest-gtk), a tool for storing and restoring calibration data (jscal-store, jscal-restore) your device), and a udev rule installed with the "joystick" debian package that calls jscal-restore when your device shows up.

Without these tools, users must re-calibrate their joystick using the command-line tool evdev-joystick every time they connect their controller if they want to use programs utilizing evdev for input. (How on earth has no one developed equivalent tools for evdev yet? I guess most people get by with uncalibrated joysticks and never notice anything.)

disk64 commented Mar 4, 2017

It's also worth noting that currently evdev lacks the same toolset that joydev has, which includes a graphical joystick calibration tool (jstest-gtk), a tool for storing and restoring calibration data (jscal-store, jscal-restore) your device), and a udev rule installed with the "joystick" debian package that calls jscal-restore when your device shows up.

Without these tools, users must re-calibrate their joystick using the command-line tool evdev-joystick every time they connect their controller if they want to use programs utilizing evdev for input. (How on earth has no one developed equivalent tools for evdev yet? I guess most people get by with uncalibrated joysticks and never notice anything.)

return result;
}
std::string ParamPackage::Get(const std::string& key, const std::string& default_value) const {

This comment has been minimized.

@jroweboy

jroweboy Mar 10, 2017

Member

Should this be made more generic? You can template this method using stringstream so we don't have to have 3 different copy pasted setters/getters This method should automatically infer the type from the default value provided.

#include <sstream>
...

template <typename T>
T ParamPackage::Get(const std::string& key, const T default_value) const {
    auto pair = data.find(key);
    if (pair == data.end()) {
        LOG_DEBUG(Common, "key %s not found", key.c_str());
        return default_value;
    }

    T value;
    std::stringstream stream(pair->second);
    stream >> value;
    if (stream.fail()) {
        LOG_ERROR(Common, "failed to convert %s to the expected type", pair->second.c_str());
        return default_value;
    }
    return value;
}

and I'm guessing the setter doesn't need to do anything special since to_string has overloads for the most common types

template <typename T>
void ParamPackage::Set(const std::string& key, const T value) {
    data[key] = std::to_string(value);
}
@jroweboy

jroweboy Mar 10, 2017

Member

Should this be made more generic? You can template this method using stringstream so we don't have to have 3 different copy pasted setters/getters This method should automatically infer the type from the default value provided.

#include <sstream>
...

template <typename T>
T ParamPackage::Get(const std::string& key, const T default_value) const {
    auto pair = data.find(key);
    if (pair == data.end()) {
        LOG_DEBUG(Common, "key %s not found", key.c_str());
        return default_value;
    }

    T value;
    std::stringstream stream(pair->second);
    stream >> value;
    if (stream.fail()) {
        LOG_ERROR(Common, "failed to convert %s to the expected type", pair->second.c_str());
        return default_value;
    }
    return value;
}

and I'm guessing the setter doesn't need to do anything special since to_string has overloads for the most common types

template <typename T>
void ParamPackage::Set(const std::string& key, const T value) {
    data[key] = std::to_string(value);
}

This comment has been minimized.

@wwylele

wwylele Mar 11, 2017

Member

This can lead to misuse for strings. Users of ParamPackage tends to write code like param_package.Get("some_key", "default_string"), but with templates this will be deduced as T = const char* and instantiate an ill-formed function. So I think I had better not make templates until we introduce c++17 deduction guides.

@wwylele

wwylele Mar 11, 2017

Member

This can lead to misuse for strings. Users of ParamPackage tends to write code like param_package.Get("some_key", "default_string"), but with templates this will be deduced as T = const char* and instantiate an ill-formed function. So I think I had better not make templates until we introduce c++17 deduction guides.

@Subv

Subv approved these changes Mar 17, 2017

@bunnei bunnei merged commit 423ab5e into citra-emu:master Mar 17, 2017

2 checks passed

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

@wwylele wwylele deleted the wwylele:input-2 branch Mar 17, 2017

jfmherokiller added a commit to jfmherokiller/citra that referenced this pull request Mar 29, 2017

Merge pull request #2497 from wwylele/input-2
Refactor input emulation & add SDL gamepad support
@epigramx

This comment has been minimized.

Show comment
Hide comment
@epigramx

epigramx May 30, 2017

People redirected here might be interested in this method I wrote about for Citra: https://community.citra-emu.org/t/how-to-use-a-mouse-as-a-virtual-joystick-in-citra-in-order-to-control-a-camera-view-or-for-custom-mouse-input/1885

It practically allows full capturing of the mouse for camera viewing (depended on game), and even makes it possible to bind the main mouse buttons on demand.

The main technology is Virtual Joystick via vJoy -> SDL -> Citra. An autohotkey script talks to vJoy. The use of Autohotkey can have very powerful effects.

epigramx commented May 30, 2017

People redirected here might be interested in this method I wrote about for Citra: https://community.citra-emu.org/t/how-to-use-a-mouse-as-a-virtual-joystick-in-citra-in-order-to-control-a-camera-view-or-for-custom-mouse-input/1885

It practically allows full capturing of the mouse for camera viewing (depended on game), and even makes it possible to bind the main mouse buttons on demand.

The main technology is Virtual Joystick via vJoy -> SDL -> Citra. An autohotkey script talks to vJoy. The use of Autohotkey can have very powerful effects.

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