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

Joystick hotplug support #4141

Merged
merged 16 commits into from Sep 8, 2018

Conversation

Projects
None yet
4 participants
@B3n30
Contributor

B3n30 commented Aug 26, 2018

This is my take on adding joystick hotpluging to citra.

I had to remove the SDL_JoystickUpdate calls and add the SDL_PollEvent to catch the reconnect events. The SDL_PollEvent needs to be called from the main thread so I had to add that to the qt-main-loop.

I added States for all Button/Axes/Hats to the InputCommon::SDL::SDLJoystick class. The PollEvent will update those states from the main-thread. The HID-Service well GetStatus() from the citra thread to get the state of the mapped buttons/...

I also added the GameController support of SDL2. This will enable nice naming of the joysticks for the config ui. And in a follow up PR it is planned to add auto config of joysticks which will also require the GameController support.

I hope this explains all change I made.

Disclaimer: I never wrote stuff with SDL and also the input stuff for citra was new to me. So there might be issues I didn't catch. Please consider that while reviewing.


This change is Reviewable

use SDL_PollEvent instead of SDL_JoystickUpdate
Register hot plugged controller by GUID if they were configured in a previous session

@B3n30 B3n30 force-pushed the B3n30:controller2 branch 3 times, most recently from 49f75b1 to 0566452 Aug 26, 2018

@B3n30 B3n30 force-pushed the B3n30:controller2 branch from 0566452 to aa60b23 Aug 26, 2018

@B3n30 B3n30 added canary-merge and removed canary-merge labels Aug 27, 2018

@B3n30 B3n30 added the canary-merge label Aug 27, 2018

@wwylele

The index / instance ID model seems broken and I can't do further review because I am puzzled lol.

@@ -34,6 +35,12 @@ void Init() {
udp = CemuhookUDP::Init();
}
void StartJoystickEventHandler() {
#ifdef HAVE_SDL2
static std::future<void> future = std::async(std::launch::async, SDL::PollLoop);

This comment has been minimized.

@wwylele

wwylele Aug 30, 2018

Member

std::thead(...) might be better semantically and probably has less overhead.

std::tuple<float, float> GetStatus() const override {
return joystick->GetAnalog(axis_x, axis_y);
}
private:
std::shared_ptr<SDLJoystick> joystick;
std::shared_ptr<VirtualJoystick> joystick;
std::string guid;

This comment has been minimized.

@wwylele

wwylele Aug 30, 2018

Member

unused member

SDLAnalog(std::shared_ptr<SDLJoystick> joystick_, int axis_x_, int axis_y_)
: joystick(std::move(joystick_)), axis_x(axis_x_), axis_y(axis_y_) {}
SDLAnalog(std::shared_ptr<VirtualJoystick> joystick_, int axis_x_, int axis_y_)
: joystick(joystick_), axis_x(axis_x_), axis_y(axis_y_) {}

This comment has been minimized.

@wwylele

wwylele Aug 30, 2018

Member

what is the reason of removing std::move?

const int port = params.Get("port", 0);
auto joystick = GetJoystickByGUID(guid, port);
if (!joystick) {

This comment has been minimized.

@wwylele

wwylele Aug 30, 2018

Member

This repeating pattern of "if not exist them make one" can be moved into GetJoystickByGUID

#include <cmath>
#include <future>

This comment has been minimized.

@wwylele

wwylele Aug 30, 2018

Member

unused <future>

std::string guid;
int port;
int init_id;
int sdl_id;

This comment has been minimized.

@wwylele

wwylele Aug 30, 2018

Member

both init_id and sdl_id should have type SDL_JoystickID according to the return type of SDL_JoystickInstanceID

return nullptr;
}
void InitJoystick(int joystick_id) {

This comment has been minimized.

@wwylele

wwylele Aug 30, 2018

Member

rename the parameter to joystick_index to avoid confusion between joystick instance ID (deduced by that you call SDL_JoystickOpen with it)

Show resolved Hide resolved src/input_common/sdl/sdl.cpp Outdated
* The Init_ID is used by SDL_JOYDEVICEADDED
* It will always stay the same for each joystick for the runtime of Citra
*/
int GetInitID() const {

This comment has been minimized.

@wwylele

wwylele Aug 30, 2018

Member

Assuming init_id should be SDL_JoystickInstanceID, the return value here should be so as well

static std::shared_ptr<VirtualJoystick> GetJoystickByInitID(int id) {
std::lock_guard<std::mutex> lock(joystick_list_mutex);
for (auto joystick : joystick_list) {
if (joystick->GetInitID() == id) {

This comment has been minimized.

@wwylele

wwylele Aug 30, 2018

Member

The comparison doesn't make sense according to my "type deduction" made in other places... the left hand side is a instance ID and the right hand side is an index.

@CodingKoopa

This comment has been minimized.

Member

CodingKoopa commented Aug 30, 2018

There has been a report of Xbox controllers no longer working, starting with Canary build 798. The merge log (compare to this) seems to indicate that this may be a regression in this PR.

@B3n30

This comment has been minimized.

Contributor

B3n30 commented Aug 30, 2018

@wwylele So the first time you connect a joystick SDL will give the joystick a SDL_JoystickID. When you disconnect and reconnect the joystick the event.jdevice.which will use that same SDL_JoystickID again. But SDL_JoystickInstanceID() will return an updated number and all further events will use that updated number. The only event that will always use the inital SDL_JoystickID is the SDL_JOYDEVICEADDED event. Thus I store the init_id and the changing sdl_id in the VirtualJoystick class. The first one to tie it to the SDL_JOYDEVICEADDED events and the later to tie it to all other events. I hope that cleared it up a bit.

@B3n30

This comment has been minimized.

Contributor

B3n30 commented Aug 30, 2018

@CodingKoopa Yes. This Pr makes it necessary to configure the input again to work. I also changed what gets displayed in the input config tab. Instead of Joystick #num - Button #num it will now display #JoystickName (#num) - Button #num.

So the only real issue I see is that SetAnalog seems to be broken. I will look into that.

@B3n30

This comment has been minimized.

Contributor

B3n30 commented Aug 30, 2018

So I just tested it again. But SetAnalog is working without any issue...

@B3n30 B3n30 force-pushed the B3n30:controller2 branch 2 times, most recently from e60cc9d to 78b786d Aug 31, 2018

@B3n30 B3n30 force-pushed the B3n30:controller2 branch from 78b786d to 7c009a0 Sep 1, 2018

if (param.Has("hat")) {
text += QString(QObject::tr(" Hat %1 %2"))
.arg(param.Get("hat", "").c_str(), param.Get("direction", "").c_str());
return QString(QObject::tr(" Hat %1 %2"))

This comment has been minimized.

@wwylele

wwylele Sep 2, 2018

Member

If you don't prefix "Joystick %1", should you remove the prefix space as well?

}
if (param.Has("axis")) {
text += QString(QObject::tr(" Axis %1%2"))
.arg(param.Get("axis", "").c_str(), param.Get("direction", "").c_str());
return QString(QObject::tr(" Axis %1%2"))

This comment has been minimized.

@wwylele

wwylele Sep 2, 2018

Member

ditto

}
if (param.Has("button")) {
text += QString(QObject::tr(" Button %1")).arg(param.Get("button", "").c_str());

This comment has been minimized.

@wwylele

wwylele Sep 2, 2018

Member

ditto

if (dir == "left" || dir == "right") {
text += QString(QObject::tr(" Axis %1")).arg(param.Get("axis_x", "").c_str());
return QString(QObject::tr(" Axis %1")).arg(param.Get("axis_x", "").c_str());

This comment has been minimized.

@wwylele

wwylele Sep 2, 2018

Member

ditto

} else if (dir == "up" || dir == "down") {
text += QString(QObject::tr(" Axis %1")).arg(param.Get("axis_y", "").c_str());
return QString(QObject::tr(" Axis %1")).arg(param.Get("axis_y", "").c_str());

This comment has been minimized.

@wwylele

wwylele Sep 2, 2018

Member

ditto

* The number of joystick from the same type that were connected before this joystick
*/
int GetPort() const {
std::lock_guard<std::mutex> lock(mutex);

This comment has been minimized.

@wwylele

wwylele Sep 2, 2018

Member

ditto

* Get the nth joystick with the corresponding GUID
*/
static std::shared_ptr<VirtualJoystick> GetVirtualJoystickByGUID(const std::string& guid,
const int port) {

This comment has been minimized.

@wwylele

wwylele Sep 2, 2018

Member

remove const for port as it is a value type

/// Vector of all used VirtualJoystick instances
/// Every access needs to be locked by the joystick_list_mutex
static std::mutex joystick_list_mutex;
static std::vector<std::shared_ptr<VirtualJoystick>> joystick_list;

This comment has been minimized.

@wwylele

wwylele Sep 2, 2018

Member

Given that joystick_list is only accessed by traversal and check its guid and port, it makes more sense to change this to a std::unordered_map<std::tuple<guid, port>, VirtualJoystick> or std::unordered_map<guid, std::vector<VirtualJoystick, port as index>>

typedef std::unique_ptr<SDL_Joystick, decltype(&SDL_JoystickClose)> SDLJoystick;
static std::string GetGUID(SDL_Joystick* joystick) {

This comment has been minimized.

@wwylele

wwylele Sep 2, 2018

Member

Move function below variable definitions.

sdl_joystick_map[guid].begin(), sdl_joystick_map[guid].end(),
[sdl_joystick](const SDLJoystick& joystick) { return joystick.get() == sdl_joystick; });
int port = it - sdl_joystick_map[guid].begin();
return GetVirtualJoystickByGUID(guid, port);

This comment has been minimized.

@wwylele

wwylele Sep 2, 2018

Member

A follow up of previous comment. If you change joystick_list to std::unordered_map<guid, std::vector<VirtualJoystick, port as index>>, and also noticing that how port is the index of both the vector joystick_list[guid] and the vector sdl_joystick_map[guid], indicated by the code logic here, joystick_list and sdl_joystick_map are essentially isomorphic => considering merge them.

This comment has been minimized.

@B3n30

B3n30 Sep 2, 2018

Contributor

the index of sdl_joystick_map isn't guaranteed to be the port of the corresponding virtual joystick

@wwylele

wwylele approved these changes Sep 3, 2018

Mostly LGTM

return joystick->GetSDLJoystick() == sdl_joystick;
});
joystick_guid_list[std::distance(joystick_guid_list.begin(), joystick_it)]->SetSDLJoystick(
nullptr, [](SDL_Joystick*) {});

This comment has been minimized.

@wwylele

wwylele Sep 3, 2018

Member

Could just do *joystick_it->SetSDLJoystick(nullptr, [](SDL_Joystick*) {});. My idea of merging structures is for you to simplify your code.

});
if (it != joystick_guid_list.end()) {
joystick_guid_list[std::distance(joystick_guid_list.begin(), it)]->SetSDLJoystick(
sdl_joystick);

This comment has been minimized.

@wwylele

wwylele Sep 3, 2018

Member

ditto, *it->SetSDLJoystick(SetSDLJoystick)

return {};
SDL_JoystickUpdate();
return SDL_JoystickGetButton(joystick.get(), button) == 1;
bool GetButton(int button) {

This comment has been minimized.

@wwylele

wwylele Sep 3, 2018

Member

You can still mark all getters as const, as you marked mutex as mutable.

class SDLButton final : public Input::ButtonDevice {
public:
explicit SDLButton(std::shared_ptr<SDLJoystick> joystick_, int button_)
explicit SDLButton(std::shared_ptr<VirtualJoystick> joystick_, int button_)

This comment has been minimized.

@wwylele

wwylele Sep 3, 2018

Member

If you rename VirtualJoystick back to SDLJoystick you could just avoid these diffs but whatever.

RegisterFactory<ButtonDevice>("sdl", std::make_shared<SDLButtonFactory>());
RegisterFactory<AnalogDevice>("sdl", std::make_shared<SDLAnalogFactory>());
polling = false;
initialized = true;

This comment has been minimized.

@wwylele

wwylele Sep 3, 2018

Member

Should you call InitJoystick on all existing joystick here? I remember you had these before.

This comment has been minimized.

@B3n30

B3n30 Sep 3, 2018

Contributor

No. Each Joystick will trigger SDL_JOYDEVICEADDED when citra starts. And with the implementation we have now, that event will get handled properly. With the prior implementation that wasn't the case

SDL_JoystickUpdate();
return SDL_JoystickGetButton(joystick.get(), button) == 1;
std::lock_guard<std::mutex> lock(mutex);
return state.buttons.at(button);

This comment has been minimized.

@wwylele

wwylele Sep 3, 2018

Member

oops this is my bad. I didn't think about this. at is not good as
- it will crash if the user gives an invalid config
- it will crash if HID ask for a state of a button before any event of that button arrives

@wwylele

wwylele approved these changes Sep 3, 2018

MarkCangila added a commit to MarkCangila/citra that referenced this pull request Sep 5, 2018

MarkCangila added a commit to MarkCangila/citra that referenced this pull request Sep 5, 2018

MarkCangila added a commit to MarkCangila/citra that referenced this pull request Sep 5, 2018

MarkCangila added a commit to MarkCangila/citra that referenced this pull request Sep 5, 2018

MarkCangila added a commit to MarkCangila/citra that referenced this pull request Sep 5, 2018

MarkCangila added a commit to MarkCangila/citra that referenced this pull request Sep 5, 2018

MarkCangila added a commit to MarkCangila/citra that referenced this pull request Sep 5, 2018

@B3n30 B3n30 merged commit 17978cf into citra-emu:master Sep 8, 2018

2 checks passed

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

This comment has been minimized.

Nerrel commented Sep 19, 2018

I'm having an issue that's apparently related to this feature. Canary builds after 793 will not open, but 793 and earlier will work normally. There are no error messages after running the citra-qt.exe at all, the program window just never appears and the log remains empty. If I continue clicking or hitting enter on the qt.exe rapidly for an extended period, the program will occasionally open and the log will be filled in with a few details (which I've attached).

While I can eventually open Citra in this way, I use Steam to enable support for the Switch Pro controller, and Steam does not seem to be able to open Citra at all. The issue opening Citra directly by clicking the exe persists whether Steam is running or not.

I've tried disabling Windows Defender, the only antivirus I have running, with no effect. I also tried removing the qt-config.ini file to see if Citra would open and generate a new one, but it continues to fail and does not generate a new config file when I attempt to open it.

I'm using-
Windows 10x64
Intel i5 6600k
Nvidia GTX 970

qt-config.txt

citra_log.txt

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