Updated SDL2 to use the game controller API #8993

neilmunday · 2016-09-18T22:29:20Z

Replaced existing SDL2 joystick code and with SDL2's game controller API. This removes the need for any hard coded mappings in the code and instead means SDL2's game controller database can be used instead.

The game controller API is much easier to user and also much cleaner than using the joystick API.

Pressing the Home/Guide button on a control pad will also now bring up the PPSSPP in game menu also.

At least SDL 2.0.4 is required.

Tested under Fedora 23 (x86_64) with GCC and SDL 2.0.4 using PS3 and XBOX 360 control pads.

Hopefully this pull request should address issue: #8986

unknownbrackets

Looks good overall, I don't see anything wrong really, but a few tweaks might be nice. I'm most concerned this might lose multiple controller support (which I don't know for sure we had before.)

-[Unknown]

unknownbrackets · 2016-09-19T01:30:14Z

SDL/SDLJoystick.cpp

+
+	if (SDL_GameControllerAddMappingsFromFile("assets/gamecontrollerdb.txt") == -1)
+	{
+			printf("Failed to load control pad mappings, please place gamecontrollerdb.txt in your assets directory\n");


Minor, but there's some wrong indentation here. Also, prefer consistent indentation. For new code we're using:

if (foo) { bar(); } else { baz(); }

With tabs, but it's fine to leave it - just best not to mix (else {...)

-[Unknown]

Understood. The previous code had a mix of curly bracket use so I decided to stick with one. I guess I chose the wrong one ;-) Easily fixed though.

Understood... I was just matching indentation used previously... it was mixed too! This of course can be easily fixed ;-)

unknownbrackets · 2016-09-19T01:31:43Z

SDL/SDLJoystick.cpp

+			break;
+		}
+	}
+	if (joyIndex != -1)


Hm. I wonder if we ought to fall back to the old handling if none of the joysticks are supported game controllers? Probably the large majority would be, though?

-[Unknown]

The old handling only had mappings for ~4 control pads. Also, it applied the same mapping to all control pads connected. This of course could produced odd behaviour if different types of control pads were hot plugged as the mappings were not re-calculated. The SDL2 controller database supports over 50 control pads for Linux, over 25 for Windows and over 10 for OSX.

It would be possible to assign a default mapping to unknown control pad, but this would be guess work as we would know I idea of how to map buttons and axis etc.

Well, the UI has a way to map buttons. It won't work inside the UI for navigating the UI (otherwise you could trap yourself), but if the game exposes a typical hat the old mapping can be usable. It might be better than not being able to use the controller at all.

-[Unknown]

I thought the UI relied upon SDL for event handling as well as in game (ext/native/base/PCMain.cpp)? Sorry if I have misunderstood.

No, in game uses the game mapping. The NK codes are mapped to PSP ctrl codes.

This is similar, conceptually, to how we have a "virtual touch pad" on screen during the game. It's also not used in the UI. We take all the inputs by device id and allow them to be mapped to PSP buttons, arrows, and analogs - as well as special modifier keys (like fast forward.)

-[Unknown]

unknownbrackets · 2016-09-19T01:33:04Z

SDL/SDLJoystick.cpp

+	}
+	if (joyIndex != -1)
+	{
+		printf("Player 1 will be using joystick %d, %s\n", joyIndex, SDL_JoystickNameForIndex(joyIndex));


Since the PSP only supports one player, this might be confusing. Maybe something like Using joystick %d, %s\n or maybe Pad 1 will be SDL joystick %d, %s\n (all multiple pads do is let you configure if you have multiple attached)? Sorry to be pedantic.

-[Unknown]

"the PSP only supports one player"

Correct. Here by "Player 1" I mean player 1 of the PSP, not control pad number 1.

When multiple control pads are connected, if the control pad assigned to player 1 is disconnected, then the new code loops through the remaining control pads connected looking for the first recogonised control pad. If one is found then this control pad then becomes assigned to "player 1" (see the SDL_CONTROLLERDEVICEADDED event handling on line 171).

I was assuming that only control pad should be used to control PPSSPP. If you prefer that PPSSP responds to input from any connected control pad then this can be done but I thought that would be confusing to have more than one control pad acting as "player 1"?

There's a gamepad mapping under the Settings which allows you to map controls from specific pads (that's why the deviceId) to certain game input.

For example, you could say that "A" on controller 1 is the PSP's "X", and "A" on controller 2 is the PSP's "O". Then you could play some weird game with two hands or a friend or whatever weird use case you have.

More practically, you can have multiple controllers simultaneously connected, and choose in the settings which ones map to PSP keys. People use this for multiplayer netplay AFAIK.

I also think it would be strange if my phone told me it was displaying a webpage on "Display 1". It only has one display. That's what I'm saying here: it's confusing for the printf (which many Linux users might see, especially when troubleshooting gamepad issues) to go out of its way to say "Player 1". Player 1 as opposed to... Player Zed? Player 9? There's only one player.

-[Unknown]

unknownbrackets · 2016-09-19T01:37:03Z

SDL/SDLJoystick.cpp

 void SDLJoystick::ProcessInput(SDL_Event &event){
 	switch (event.type) {
-	case SDL_JOYAXISMOTION:
+		case SDL_CONTROLLERBUTTONDOWN:


nit: We prefer not indenting cases, as labels, i.e.:

switch (event.type) { case SDL_CONTROLLERBUTTONDOWN: foo(); break; case SDL_JOYAXISMOTION: bar(); break; ... }

-[Unknown]

Understood. I'll sort this out together with the indentation as mentioned in previous comment.

unknownbrackets · 2016-09-19T01:38:58Z

SDL/SDLJoystick.cpp

-				key.keyCode = i->second;
-				key.deviceId = DEVICE_ID_PAD_0 + deviceIndex;
-				NativeKey(key);
+			if (event.cbutton.which == player1JoyInstanceID && event.cbutton.state == SDL_PRESSED)


Do we need to check the player1JoyInstanceID here? It seems like we're already using getDeviceIndex(). If I happen to have two controllers plugged in, ideally both should be mappable (i.e. in case I want to use what SDL considers my second controller.)

-[Unknown]

Yes, player1JoyInstanceID does need to be checked. This check means that PPSSPP only responds to input for the control pad being used by "player 1". As mentioned above, I thought it would be confusing if input from multiple control pads acted as player 1? Removing this check would mean that any control pad connected could control player 1.

It should also be noted that event.cbutton.which is the joystick instance ID and not the device ID. For example, if one control pad is connected its device and instance ID will be 0. If the control pad is disconnected and then plugged in again, the device ID will still be 0, but the instance ID will be 1.

Ideally, ALL connected gamepads should report to the native keybinding system. It should sort out what routes to which game presses, as per the user's configuration. That's why it has a deviceId field - this is used to differentiate the different gamepads, mouse, keyboard, and other devices. There are multiple gamepad device ids on purpose.

Ideally the deviceId should be as stable as possible. If another device is connected, it shouldn't change existing deviceIds. In a perfect world, restarting the program should also not alter the deviceIds (since they are saved with the user's configured mapping.)

This is just the cross platform feed to the central keybinding system.

-[Unknown]

On Linux at least, the device ID is not unique to the control pad. For example, if an XBOX control pad is plugged in, this will be assigned device ID 0. Then if this control pad is disconnected and another say PS3 control pad is connected its device ID will be 0.

SDL2 assigns its own GUID to each model of control pad which is how control pad mappings are handled. Of course, this is SDL's own system and may not translate well with other input systems used by PPSSPP.

unknownbrackets · 2016-09-19T01:40:02Z

SDL/SDLJoystick.cpp

+			// for removal events, "which" is the instance ID for SDL_CONTROLLERDEVICEREMOVED
+			if (event.cdevice.which == player1JoyInstanceID)
+			{
+				printf("control pad removed for player 1\n");


Maybe "pad 1"?

-[Unknown]

Again, here by player 1 I do not mean control pad number 1... I mean the control pad being used by player 1 of the PSP. Is this ok?

unknownbrackets · 2016-09-19T01:40:26Z

SDL/SDLJoystick.cpp

+					if (player1Controller && SDL_GameControllerGetAttached(player1Controller))
+					{
+						player1JoyInstanceID = SDL_JoystickInstanceID(SDL_GameControllerGetJoystick(player1Controller));
+						printf("Player 1 will be using control pad: %s, instance ID: %d\n", SDL_GameControllerName(player1Controller), player1JoyInstanceID);


Here too. Hmm, can we reuse the other code in a function?

-[Unknown]

Yes, I think you are right. I'll see what I can do later.

unknownbrackets · 2016-09-19T01:44:31Z

SDL/SDLJoystick.cpp

+	SDL_Joystick *joy;
+	for (int i = 0; i < numjoys; i++)
+	{
+		joy = SDL_JoystickOpen(i);


Hmm, is this fast? It might be nice if we could keep around the instance ids for activated controllers in an array since we handle add/remove anyway. I guess this probably isn't so slow as to add any significant latency to pad events...

-[Unknown]

It is fast, but I agree storing the instance IDs would be better. I'll see what I can do later.

neilmunday · 2016-09-19T07:05:40Z

I'm most concerned this might lose multiple controller support (which I don't know for sure we had before.)

Previously, the same control pad mapping was applied to all control pads so unless you had the same model of control pad connected you would get odd behaviour.

I thought it made more sense that only one control pad acted as player 1 of the PSP? If not, this can be changed pretty easily. What do the authors of PPSSPP prefer?

Also, the hot plugging code I have added assigns the next available control pad to player 1 if the control pad acting as player 1 is disconnected.

hrydgard · 2016-09-19T07:24:32Z

Well, there's ever only a Player 1 of the PSP, as it's a portable device there can't be a Player 2. But maybe someone would play using some bizarre combination of controllers, who knows :) I'm not really sure how I prefer this to work..

unknownbrackets · 2016-09-19T07:42:49Z

Well, we already have an issue open where users are complaining that the Windows code for XInput - #8250 (DirectInput works fine) only supports one controller. So I prefer not introducing that same issue on SDL, because then we'll just have another open issue.

-[Unknown]

hrydgard · 2016-09-19T08:18:29Z

That's fair. I should really go and fix XInput, it's not like it's hard...

…ors.

neilmunday · 2016-09-19T23:33:05Z

I've made the modifications as suggested. Let me know what you think.

neilmunday · 2016-09-20T19:25:20Z

Turns out that a number of mappings for well known control pads are also contained within the SDL2 library.

neilmunday · 2016-09-20T20:54:48Z

Thanks to @dajamu for reviewing the latest code also.

unknownbrackets · 2016-09-23T03:54:06Z

SDL/SDLJoystick.cpp

+		SDL_Init(SDL_INIT_JOYSTICK | SDL_INIT_VIDEO | SDL_INIT_GAMECONTROLLER);
+	}
+
+	auto dbPath = File::GetExeDirectory() + "assets/gamecontrollerdb.txt";


@xsacha will this work on Qt? I think it bundles the assets into the executable?

-[Unknown]

xsacha · 2016-09-23T03:55:47Z

It was designed to work alongside Qt, completely independently, so I don't see why not.

unknownbrackets · 2016-09-23T03:58:24Z

Well, I just meant since Qt uses SDL for controller support. Okay, cool.

LGTM.

-[Unknown]

xsacha · 2016-09-23T04:00:51Z

Qt does bundle the assets and you can set an ifdef to check the inbuilt assets. That's designed so you don't need the additional assets directory but it is entirely optional whether you use that feature.

neilmunday · 2016-09-23T19:53:19Z

For QT, is it just a case of updating Qt/assets.qrc to include an entry for the gamecontrollerdb.txt file?

xsacha · 2016-09-23T20:11:34Z

Since this isn't using the assetreader class to fetch the file, you would either need to use that or use an #ifdef USING_QT_UI

And yeah probably just desktop_assets.qrc because I don't think the SDL code runs on mobile version.

neilmunday · 2016-09-23T21:00:44Z

@xsacha I tried adding the gamecontrollerdb.txt to desktop_assets.qrc but I fear I am missing something else as after building the QT version I do not see any evidence that the SDLJoystick class is being used at all? Apologies if I have missed something.

xsacha · 2016-09-23T21:03:17Z

SDL in Qt only compiles in on linux and Mac when sdl2 package is found.

See: https://github.com/hrydgard/ppsspp/blob/master/Qt/PPSSPP.pro

neilmunday · 2016-09-23T21:22:47Z

My mistake, I thought my CMAKE variables were enough to pick up my install of SDL2 for the QT build. After setting the PKG_CONFIG environment variable I now have a QT build of PPSSPP linked against SDL 2.0.4, however, it seg faults at start-up. Note: I have added an IFDEF to avoid calling the
GetExeDirectory() function for now. Instead I just call setUpControllers() for the time being as SDL2 has built in defaults for the PS3 control pad which I am using for testing.

Here is the stack trace from GDB, using QT4:

#0  0x00007f67710f812c in QGLExtensionMatcher::QGLExtensionMatcher() () at /lib64/libQtOpenGL.so.4
#1  0x00007f67710f81f5 in QGLExtensions::currentContextExtensions() () at /lib64/libQtOpenGL.so.4
#2  0x00007f67710f897c in QGLExtensions::glExtensions() () at /lib64/libQtOpenGL.so.4
#3  0x00007f67710f9560 in qt_qgl_paint_engine() () at /lib64/libQtOpenGL.so.4
#4  0x00007f67707aecf1 in QWidgetPrivate::repaint_sys(QRegion const&) () at /lib64/libQtGui.so.4
#5  0x00007f67705d3417 in QWidgetPrivate::syncBackingStore() () at /lib64/libQtGui.so.4
#6  0x00007f67705e61f8 in QWidget::event(QEvent*) () at /lib64/libQtGui.so.4
#7  0x0000556b3598607b in MainUI::event(QEvent*) (this=0x556b387ff880, e=0x556b38ad6cc0) at ../ext/native/base/QtMain.cpp:344
#8  0x00007f677058f93c in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /lib64/libQtGui.so.4
#9  0x00007f6770596796 in QApplication::notify(QObject*, QEvent*) () at /lib64/libQtGui.so.4
#10 0x00007f67700217fd in QCoreApplication::notifyInternal(QObject*, QEvent*) () at /lib64/libQtCore.so.4
#11 0x00007f6770024e16 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () at /lib64/libQtCore.so.4
#12 0x00007f6770051c0e in postEventSourceDispatch(_GSource*, int (*)(void*), void*) () at /lib64/libQtCore.so.4
#13 0x00007f676d208e5a in g_main_context_dispatch () at /lib64/libglib-2.0.so.0
#14 0x00007f676d2091f0 in g_main_context_iterate.isra () at /lib64/libglib-2.0.so.0
#15 0x00007f676d20929c in g_main_context_iteration () at /lib64/libglib-2.0.so.0
#16 0x00007f6770051d7e in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /lib64/libQtCore.so.4
#17 0x00007f6770638416 in QGuiEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /lib64/libQtGui.so.4
#18 0x00007f6770020071 in QEventLoop::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /lib64/libQtCore.so.4
#19 0x00007f67700203e5 in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /lib64/libQtCore.so.4
#20 0x00007f6770025f79 in QCoreApplication::exec() () at /lib64/libQtCore.so.4
#21 0x0000556b3596a2e1 in main(int, char**) (a=...) at ../ext/native/base/QtMain.cpp:202
#22 0x0000556b3596a2e1 in main(int, char**) (argc=1, argv=<optimized out>) at ../ext/native/base/QtMain.cpp:501

xsacha · 2016-09-23T21:38:08Z

Looks like it is broken. Not sure where cause I haven't tested that code in a couple of years.
Qt works without SDL? For some reason crashing when checking opengl extensions.

neilmunday · 2016-09-23T21:48:18Z

@xsacha Without SDL it runs fine. I should also add that when using the code from the main PPSSPP repository that it also crashes at the same point so I don't think my mods are to blame.

neilmunday added 2 commits September 18, 2016 00:33

Initial attempt at adding SDL's game controller database to PPSSPP.

67425d7

Modified SDL2 code to make full use of SDL2's Game Controller API.

db2cc49

neilmunday mentioned this pull request Sep 18, 2016

SDL2 Controller Database Support #8986

Closed

hrydgard added this to the v1.3.1 milestone Sep 18, 2016

unknownbrackets reviewed Sep 19, 2016

View reviewed changes

Updated SDL2 game controller mods following feedback from PPSSPP auth…

e4a0a3c

…ors.

Various improvements including finding the assets directory properly.

182b9e0

Added installation of the SDL game controller database to CMakeLists.txt

7d9eed9

unknownbrackets reviewed Sep 23, 2016

View reviewed changes

unknownbrackets merged commit 246f3aa into hrydgard:master Sep 24, 2016

substring mentioned this pull request Nov 12, 2016

Commandline --escape-exit doesn't work anymore #9119

Closed

unknownbrackets mentioned this pull request Mar 21, 2017

Update README.md for 1.4 #9469

Merged

jbeich mentioned this pull request Apr 1, 2017

Unable to use/map controllers in 1.4 source build on Ubuntu 14.04 Linux #9519

Closed

unknownbrackets mentioned this pull request Jan 15, 2018

Ubuntu 16.04 xbox wireless controller can't map rtrigger #10181

Closed

unknownbrackets mentioned this pull request Nov 20, 2022

I have an issue with mapping controls. #16400

Open

5 tasks

unknownbrackets mentioned this pull request May 26, 2023

filter the sdl joystick number via command line #17503

Open

Updated SDL2 to use the game controller API #8993

Updated SDL2 to use the game controller API #8993

Conversation

neilmunday commented Sep 18, 2016

unknownbrackets left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neilmunday Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unknownbrackets Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neilmunday Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unknownbrackets Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neilmunday commented Sep 19, 2016

hrydgard commented Sep 19, 2016

unknownbrackets commented Sep 19, 2016 • edited Loading

hrydgard commented Sep 19, 2016

neilmunday commented Sep 19, 2016

neilmunday commented Sep 20, 2016

neilmunday commented Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

xsacha commented Sep 23, 2016

unknownbrackets commented Sep 23, 2016

xsacha commented Sep 23, 2016

neilmunday commented Sep 23, 2016

xsacha commented Sep 23, 2016

neilmunday commented Sep 23, 2016

xsacha commented Sep 23, 2016

neilmunday commented Sep 23, 2016

xsacha commented Sep 23, 2016

neilmunday commented Sep 23, 2016

neilmunday Sep 19, 2016 •

edited

Loading

unknownbrackets Sep 19, 2016 •

edited

Loading

neilmunday Sep 19, 2016 •

edited

Loading

unknownbrackets Sep 19, 2016 •

edited

Loading

unknownbrackets commented Sep 19, 2016 •

edited

Loading

neilmunday commented Sep 20, 2016 •

edited

Loading