New gamepad types, button labels, conventional/default "confirm"/"cancel" buttons#15891
New gamepad types, button labels, conventional/default "confirm"/"cancel" buttons#15891nstbayless wants to merge 3 commits into
Conversation
|
I don't know that we need all the different controller layouts (but I'm not necessarily against it), but I do like the confirm/cancel idea. |
|
Okay, I can roll SNES/NES into one gamepad type; Saturn/Genesis into one gamepad type; and postpone Master System/TurboGrafx-16/Neo Geo/3DO to a later PR if we're not sure about those yet (they're definitely less common). But at least SNES, Sega, and N64 are important to codify IMO, they're very common, and Sega layout even has documentation on the db readme, and relevant to the confirm/cancel convention. |
|
Let's hold off until @slouken chimes in. I don't think having this list is useless, because (for example) a Genesis controller doesn't map well to modern gamepads and that's worth knowing if you hit this situation, but I also wonder if games will be interested in offering that level of customization when the gamepad API is meant to avoid those details. Also, I worry about the slippery slope of the whole thing. But that's not me saying it's a bad idea! But no matter what else, I definitely want the confirm/cancel functionality. This has come up for me multiple times on shipping console titles. |
|
We could also add |
| static bool SDL_GamepadLocaleIsJP(void) | ||
| { | ||
| int count = 0; | ||
| int i; | ||
| SDL_Locale **locales = SDL_GetPreferredLocales(&count); | ||
|
|
||
| face_style = gamepad->face_style; | ||
| // check if first locale which specifies a country is jp | ||
| if (locales) { | ||
| for (i = 0; i < count; ++i) | ||
| { | ||
| if (locales[i]->country) | ||
| { | ||
| if (SDL_strcasecmp(locales[i]->country, "jp") == 0) | ||
| { | ||
| SDL_free(locales); | ||
| return true; | ||
| } | ||
| else | ||
| { | ||
| SDL_free(locales); | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
| SDL_free(locales); | ||
| } | ||
| SDL_UnlockJoysticks(); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
| static bool SDL_GamepadLocaleIsJP(void) | |
| { | |
| int count = 0; | |
| int i; | |
| SDL_Locale **locales = SDL_GetPreferredLocales(&count); | |
| face_style = gamepad->face_style; | |
| // check if first locale which specifies a country is jp | |
| if (locales) { | |
| for (i = 0; i < count; ++i) | |
| { | |
| if (locales[i]->country) | |
| { | |
| if (SDL_strcasecmp(locales[i]->country, "jp") == 0) | |
| { | |
| SDL_free(locales); | |
| return true; | |
| } | |
| else | |
| { | |
| SDL_free(locales); | |
| return false; | |
| } | |
| } | |
| } | |
| SDL_free(locales); | |
| } | |
| SDL_UnlockJoysticks(); | |
| return false; | |
| } | |
| static bool SDL_GamepadLocaleIsJP(void) | |
| { | |
| bool japanese_locale = false; | |
| SDL_Locale **locales = SDL_GetPreferredLocales(NULL); | |
| // check if first locale which specifies a country is jp | |
| if (locales) { | |
| for (int i = 0; locales[i]; ++i) { | |
| if (locales[i]->country) { | |
| if (SDL_strcasecmp(locales[i]->country, "jp") == 0) { | |
| japanese_locale = true; | |
| } | |
| break; | |
| } | |
| } | |
| SDL_free(locales); | |
| } | |
| return japanese_locale; | |
| } |
|
My first impression is that this is a little much, but let me think on it. |
|
@cgutman re: touchpad; I'm not so sure... I think on PC, most people with a PS5 gamepad would likely expect the select/share button to act like a typical left-of-center button (and open a secondary menu). This would be my preference at least -- the touchpad thing always seemed to me like a hacky workaround for sony's commandeering of the select button for "share." However, I recognize this is a matter of opinion, and to be consistent with the other principles I laid out, it'd be sensible to have touchpad be the preferred secondary button on PS5. Let's defer it to a later PR at least. |
|
Since it seems like everyone is saying "this seems like too much," I've reduced the number of additions here. The only new gamepad types now are |
|
This is looking much better, thanks for trimming it down. I'll add specific feedback below. |
| * PSX and PS2 controllers should be classified as PS3. | ||
| * | ||
| * Sega Saturn controllers should be classified as Sega Genesis. | ||
| * |
| SDL_GAMEPAD_BUTTON_LABEL_SQUARE, | ||
| SDL_GAMEPAD_BUTTON_LABEL_TRIANGLE | ||
| SDL_GAMEPAD_BUTTON_LABEL_TRIANGLE, | ||
| SDL_GAMEPAD_BUTTON_LABEL_C, |
There was a problem hiding this comment.
Adding all the labels for all possible controllers is a monumental task and one that never ends, as controller vendors add new and unique buttons to their controllers. Rather than promising SDL can provide all possible labels, we should have a different system, possibly dynamic, or punt entirely.
For example, this is Microsoft's attempt, and you'll notice that it's even missing a bunch that you've included here:
https://github.com/microsoftconnect/GameInput/blob/6d1f7bfaa3361a7c0d1ba1879c37e02c5828f73a/include/GameInput.h#L367-L493
There was a problem hiding this comment.
Just as an example of this, you're missing the L2/L3 buttons, the DualSense Edge has Fn buttons, and the new Steam Controller has a QAM button that's labeled ". . ."
There was a problem hiding this comment.
L3 is present in the PR already. L2 omitted in this PR because it's an axis, not a button -- a future PR could address that. Steam Controller I haven't touched because I'm not sure what controller GAMEPAD_TYPE_STEAM refers to (I mentioned this in the PR). PS5 fn button is a fair point.
I only aim to support the labels of the gamepads in the gamepad type enum, which is why I removed some labels when I trimmed down the PR. Since there are a small number of these, the number of labels to add is small.
If we want to support every controller, not just the prototypes which have an enum, then unusual controllers in the database could be annotated with custom labels, perhaps.
There was a problem hiding this comment.
I only aim to support the labels of the gamepads in the gamepad type enum, which is why I removed some labels when I trimmed down the PR. Since there are a small number of these, the number of labels to add is small.
That's a good point, and with that scope, what you've done seems very reasonable.
| typedef enum SDL_GamepadConventionalAction | ||
| { | ||
| SDL_GAMEPAD_CONVENTION_CONFIRM, /* typically 'A' */ | ||
| SDL_GAMEPAD_CONVENTION_CANCEL, /* typically 'B' */ | ||
| /* TODO - onscreen keyboard actions */ | ||
| } SDL_GamepadConventionalAction; |
There was a problem hiding this comment.
| typedef enum SDL_GamepadConventionalAction | |
| { | |
| SDL_GAMEPAD_CONVENTION_CONFIRM, /* typically 'A' */ | |
| SDL_GAMEPAD_CONVENTION_CANCEL, /* typically 'B' */ | |
| /* TODO - onscreen keyboard actions */ | |
| } SDL_GamepadConventionalAction; | |
| typedef enum SDL_GamepadAction | |
| { | |
| SDL_GAMEPAD_ACTION_CONFIRM, /* typically 'A' */ | |
| SDL_GAMEPAD_ACTION_CANCEL, /* typically 'B' */ | |
| } SDL_GamepadAction; |
| extern SDL_DECLSPEC SDL_GamepadButtonLabel SDLCALL SDL_GetGamepadButtonLabel(SDL_Gamepad *gamepad, SDL_GamepadButton button); | ||
|
|
||
| /** | ||
| * Convert from an SDL_GamepadButtonLabel enum to a string. |
There was a problem hiding this comment.
I'm not sure this is useful, especially for things like "Triangle" and "Minus" which would need to be localized. Also see my note above about the proliferation of different button labels. What's the use case for this?
There was a problem hiding this comment.
Perhaps Triangle/Minus can be replaced with unicode representations of the glyphs -- I can update the PR to do this.
I view these as primarily being useful for e.g. looking up a filename to show the appropriate glyph/button image, but also usable as fallback to display literally if no image is present. The images can be further separated by controller type to exactly match the colour and style as needed, but this at least allows applications to fall back to another glyph with a matching label even if the exact controller type is not recognized (and then to a plain string if the label has no glyph either).
There was a problem hiding this comment.
Perhaps Triangle/Minus can be replaced with unicode representations of the glyphs -- I can update the PR to do this.
Minus and Plus can probably be the literal "-" and "+".
| * | ||
| * \sa SDL_GetGamepadConventionalActionButton | ||
| */ | ||
| extern SDL_DECLSPEC SDL_GamepadButton SDLCALL SDL_GetGamepadConventionalActionButtonForType(SDL_GamepadType type, SDL_GamepadConventionalAction action); |
There was a problem hiding this comment.
| extern SDL_DECLSPEC SDL_GamepadButton SDLCALL SDL_GetGamepadConventionalActionButtonForType(SDL_GamepadType type, SDL_GamepadConventionalAction action); | |
| extern SDL_DECLSPEC SDL_GamepadButton SDLCALL SDL_GetGamepadActionButtonForType(SDL_GamepadType type, SDL_GamepadAction action); |
| * | ||
| * \sa SDL_GetGamepadConventionalActionButtonForType | ||
| */ | ||
| extern SDL_DECLSPEC SDL_GamepadButton SDLCALL SDL_GetGamepadConventionalActionButton(SDL_Gamepad* gamepad, SDL_GamepadConventionalAction action); |
There was a problem hiding this comment.
| extern SDL_DECLSPEC SDL_GamepadButton SDLCALL SDL_GetGamepadConventionalActionButton(SDL_Gamepad* gamepad, SDL_GamepadConventionalAction action); | |
| extern SDL_DECLSPEC SDL_GamepadButton SDLCALL SDL_GetGamepadActionButton(SDL_Gamepad *gamepad, SDL_GamepadAction action); |
| _SDL_GetGamepadButtonFromString | ||
| _SDL_GetGamepadButtonLabel | ||
| _SDL_GetGamepadButtonLabelForType | ||
| _SDL_GetGamepadButtonLabelString |
There was a problem hiding this comment.
You can't add functions in the middle of the exports. You should revert all your dynapi changes, rebase against main, and then run gendynapi.py to automatically generate the exports.
| SDL_strlcat(mapping_string, "a:b1,b:b0,back:b4,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,dpup:h0.1,leftshoulder:b9,leftstick:b7,lefttrigger:a4,leftx:a0,lefty:a1,rightshoulder:b10,rightstick:b8,righttrigger:a5,rightx:a2,righty:a3,start:b6,x:b3,y:b2,hint:!SDL_GAMECONTROLLER_USE_BUTTON_LABELS:=1,", sizeof(mapping_string)); | ||
| } else if (SDL_IsJoystickSInputController(vendor, product)) { | ||
|
|
||
| Uint8 face_style = (guid.data[15] & 0xE0) >> 5; |
There was a problem hiding this comment.
You're changing the protocol of shipping controllers. You should check with the author to see if this is okay.
| } | ||
| else if (SDL_strstr(gamepad->mapping->mapping, "SDL_GAMECONTROLLER_USE_BUTTON_LABELS")) { |
There was a problem hiding this comment.
| } | |
| else if (SDL_strstr(gamepad->mapping->mapping, "SDL_GAMECONTROLLER_USE_BUTTON_LABELS")) { | |
| } else if (SDL_strstr(gamepad->mapping->mapping, "SDL_GAMECONTROLLER_USE_BUTTON_LABELS")) { |
| if (gamepad->type != SDL_GAMEPAD_TYPE_NINTENDO_SWITCH_PRO | ||
| && gamepad->type != SDL_GAMEPAD_TYPE_NINTENDO_SWITCH_JOYCON_LEFT |
There was a problem hiding this comment.
SDL style leaves the && at the end of the line:
| if (gamepad->type != SDL_GAMEPAD_TYPE_NINTENDO_SWITCH_PRO | |
| && gamepad->type != SDL_GAMEPAD_TYPE_NINTENDO_SWITCH_JOYCON_LEFT | |
| if (gamepad->type != SDL_GAMEPAD_TYPE_NINTENDO_SWITCH_PRO && | |
| gamepad->type != SDL_GAMEPAD_TYPE_NINTENDO_SWITCH_JOYCON_LEFT |
| switch (type) | ||
| { |
There was a problem hiding this comment.
| switch (type) | |
| { | |
| switch (type) { |
SDL style puts braces at the end of the line.
| if (type == SDL_GAMEPAD_TYPE_PS3) | ||
| return SDL_GAMEPAD_BUTTON_LABEL_SELECT; |
There was a problem hiding this comment.
SDL style always uses braces for conditionals:
| if (type == SDL_GAMEPAD_TYPE_PS3) | |
| return SDL_GAMEPAD_BUTTON_LABEL_SELECT; | |
| if (type == SDL_GAMEPAD_TYPE_PS3) { | |
| return SDL_GAMEPAD_BUTTON_LABEL_SELECT; | |
| } |
| SDL_LockJoysticks(); | ||
| { | ||
| CHECK_GAMEPAD_MAGIC(gamepad, SDL_GAMEPAD_BUTTON_LABEL_UNKNOWN); | ||
| static bool SDL_GamepadLocaleIsJP(void) |
There was a problem hiding this comment.
Any particular reason you didn't take my suggested edits here? Your code doesn't follow the SDL style and has the unnecessary count variable.
There was a problem hiding this comment.
Sorry -- the github interface for some reason 500-errored when I tried to accept your edits, and I wasn't sure how to copy the code without getting a + at the start of the line. So I just made the edits myself, seemed faster. My apologies, I should have paid closer attention.
| "050000006964726f69643a636f6e0000,idroid:con,a:b1,b:b2,back:b8,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,dpup:h0.1,leftshoulder:b4,leftstick:b10,lefttrigger:b6,leftx:a0,lefty:a1,rightshoulder:b5,rightstick:b11,righttrigger:b7,rightx:a2,righty:a3,start:b9,x:b0,y:b3,", | ||
| "03000000b50700001503000010010000,impact,a:b2,b:b3,back:b8,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,dpup:h0.1,leftshoulder:b4,leftstick:b10,lefttrigger:b5,leftx:a0,lefty:a1,rightshoulder:b6,rightstick:b11,righttrigger:b7,rightx:a3,righty:a2,start:b9,x:b0,y:b1,", | ||
| "030000009b2800008000000020020000,raphnet technologies 1-player WUSBMote v2.2,a:b1,b:b4,back:b2,dpdown:b13,dpleft:b14,dpright:b15,dpup:b12,leftshoulder:b6,rightshoulder:b7,start:b3,x:b0,y:b5,", | ||
| "030000009b2800006300000001010000,raphnet technologies Dual N64 to USB,a:b1,b:b0,dpdown:b11,dpleft:b12,dpright:b13,dpup:b10,leftshoulder:b4,leftx:a0,lefty:a1,rightshoulder:b5,righttrigger:b2,-rightx:b8,+rightx:b9,-righty:b6,+righty:b7,start:b3,type:n64,platform:Linux,", |
There was a problem hiding this comment.
Is there a better product string for this? Usually we put what's on the box instead of the USB descriptor string.
|
|
||
| bool SDL_IsJoystickGameCube(Uint16 vendor_id, Uint16 product_id) | ||
| { | ||
| if (vendor_id == USB_VENDOR_NINTENDO && product_id == USB_PRODUCT_NINTENDO_GAMECUBE_ADAPTER) |
There was a problem hiding this comment.
This is already in the gamecube_devices list. Is there a reason you added this?
There was a problem hiding this comment.
I started this PR two years ago before the gamecube controller had proper SDL support. This slipped my notice when rebasing.
| gamepad_button_small.png | ||
| gamepad_dual_touchpad.png | ||
| gamepad_face_1234.png | ||
| gamepad_face_abcd.png |
There was a problem hiding this comment.
It looks like these are no longer used.
| SDL_RenderTexture(ctx->renderer, ctx->face_abxy_texture, NULL, &dst); | ||
| break; | ||
| case SDL_GAMEPAD_BUTTON_LABEL_X: | ||
| switch (ctx->type) { |
There was a problem hiding this comment.
You're encouraging people to hard-code based on controller type rather than looking at what the label is for the face buttons. This means that this code will break when the PS6 controller comes out, where it wouldn't if you left it as-is.
|
I would suggest breaking this down into several PRs:
|
Ok, agreed. |
As noted above, with the scope that you've defined for the labels, that seems like a good idea. Please document that scope in the header so it's clear what users should expect? |
Description
It's a long-standing problem that games don't properly respect "confirm" and "cancel" buttons on some gamepad types (they'll either assume xbox convention of EAST=cancel, SOUTH=confirm; or, more rarely -- but I've seen this on some recent steam demos for example -- nintendo convention of EAST=confirm and SOUTH=cancel). Additionally, games have to roll their own detection of gamepad type to determine what kind of glyphs to show.
This is particularly bothersome when using, for example, a SNES controller with a usb adaptor, or other retro gamepads with USB adaptors. It's not common for games to detect these more rare kinds of gamepads. Sega layout in particular tends to confuse games.
I'd like to make progress toward the eventual goal of having SDL be able to identify exactly what glyphs should be shown (even if SDL doesn't itself ship with the actual glyphs), and make it easier for game devs to identify and handle different gamepad types more broadly.
New Gamepad Types
I've added a handful of gamepad type enums that I think are famous and interesting enough to warrant supporting their glyphs, and not similar enough to any existing gamepad to be able to be rolled into e.g. the standard or xbox 360 layout. This includes:
,NESSNES,N64,,SEGA Master SystemSEGA Genesis,.SEGA Saturn,TurboGrafx-16,Neo Geo, and3DOI've also clarified the comments here a bit to reflect what I've found in the code and the standard practices in SDL GamepadControllerDB.
New Gamepad Button Labels
There was already some work in this direction in SDL, but it was incomplete. I've fleshed it out now, adding all the most common button labels I've encountered, including all button labels used by the gamepad types above.
I've also added labels for numbers 1-9 even though only some of those are actually used by supported controllers right now, under the expectation that they'll eventually gain support in the future. (For example, atari/jaguar models with keypads 1-9? Or arcade gamepads?)"Conventional Actions"
For common UI actions that are platform-dependent. For now, I've just added "confirm" and "cancel" (since these are generally either SOUTH/EAST or EAST/SOUTH, though on gamecube it's SOUTH/WEST) but I could see onscreen keyboard controls being added to this too. For PS4 (and PS3?) it's region-dependent, so users in japan with a PS4 controller will get the expected CIRCLE=confirm, CROSS=cancel.
DB Changes
type:" field, a more general system to replace the rather deficient "USE_BUTTON_LABELS" scheme. (example:type:n64,type:switchpro,type:gamecube,type:ps5, etc.)Lingering Issues
type:snesand so on. This was suggested in an issue for the DB before, but it was suggested to be pushed upstream to SDL first I think?SDL_GAMEPAD_TYPE_STEAM? Is it any controller under steam input, or only steam controller? steam deck? SC1 and SC2 have different button labels... Not sure what to do here!SDL_GetGamepadButtonFromStringseems broken to me... it turns a button string into its enum equivalent, but for some reason also takes into account abxy/axby arrangement? and doesn't handle playstation button names? I'm not sure what it's even trying to achieve here. I think this is all caused by not using compass notation and somebody got confused.