FosterFramework / Foster

A small C# game framework
MIT License
422 stars 37 forks source link

Nintendo and Sony Detection wrong (uses vendor and product instead of just vendor) #73

Closed theofficialgman closed 6 months ago

theofficialgman commented 6 months ago

Vendor detection is bugged due to combining vendor and product in order to determine the current vendor.

Only vendor should be used eg (official USB vendor id) Sony 054c Nintendo 057e Xbox 045e and 0738

those get translated in SDL GUID and the two characters sets get swapped

this bug causes many controllers to be detected as the wrong type (eg: Nintendo Switch Combined Joy-Cons with Joycond on Linux since they have a GUID of Joystick GUID: 06004cfc7e0500000820000000000000 end up returning Gamepads.Xbox ). https://github.com/FosterFramework/Foster/blob/72c45061702e9a2cf850edbb86312a4e618a8d2d/Framework/Input/Controller.cs#L121-L140

Please change this to only use the vendor and not the vendor and product id. also, the entire purpose of the SDL2 gamecontrollerdb mapping system is to abstract that away.

NoelFB commented 6 months ago

Yeah this makes sense to me. I use gamecontrollerdb in my other projects and have meant to pull it into this for forever, but have not, so that would be good to do.

Are we sure that the vendor is the only thing we want to check? I haven't really looked through them all but if it's fine to do that then it sounds good to me. Alternatively it might make more sense to just use SDL_GameControllerGetType but I haven't yet looked to see if that covers everything we'd want it to.

Regarding joycond, is it needed anymore for modern versions of the kernel now that hid_nintendo is included? I know Arch linux says to remove it now:

If you have joycond, then delete it, because it is useless for such Switch-like gamepads, moreover joycond has a udev rule that disallows Steam to provide its own user-space driver.

(This is a little outside my area of knowledge, I just know someone on Celeste64 had issues with their Switch Pro controller until they removed joycond)

theofficialgman commented 6 months ago

Yeah this makes sense to me. I use gamecontrollerdb in my other projects and have meant to pull it into this for forever, but have not, so that would be good to do.

SDL2 already uses its own (decently good) gamecontrollerdb internally, it syncs with the community gamecontrollerdb occasionally. there is no need to provide your own unless the database in use is lacking. https://github.com/libsdl-org/SDL/blob/SDL2/src/joystick/SDL_gamecontrollerdb.h

Are we sure that the vendor is the only thing we want to check? I haven't really looked through them all but if it's fine to do that then it sounds good to me.

Lets break down the problem and what is available. SDL2 provides to applications the status of gamecontroller buttons through a standard ABXY, the physical position of those buttons and their interpretation is set by the following hint. When SDL_HINT_GAMECONTROLLER_USE_BUTTON_LABELS (https://github.com/libsdl-org/SDL/blob/de0cb94e7225539fb283005fc4eebc66a96d3092/include/SDL_hints.h#L519-L540) is 0 (as you are defining here https://github.com/FosterFramework/Foster/blob/dc065e0bfe1ed73e7285701c7128f5c6d1443b4b/Platform/src/foster_platform.c#L89) All SDL2 mappings use SDL_CONTROLLER_BUTTON_A as South, SDL_CONTROLLER_BUTTON_B as East, SDL_CONTROLLER_BUTTON_Y as North, SDL_CONTROLLER_BUTTON_X as West regardless of WHAT the physical button is called. So for example, if the user presses the south button (A on Xbox, B on Nintendo, and X on Sony) SDL2 will return SDL_CONTROLLER_BUTTON_A. With the hint set to 0, it is now up to you the application or framework developer to determine what the button style is (which as we have already determined, is hard).

So now that is cleared up lets decide what you want the Gamepads type to define. In my opinion, this should define the "button naming scheme" used. So Xbox would correspond to the xbox style naming (A as south, B as East, Y as North, X as West), nintendo would correspond to the nintendo style naming (B as south, A as East, X as North, Y as West), and sony (X as south, O as East, triangle as North, square as West). Based on this it makes sense to only use the vendor ID as Xbox/Nintendo/Sony have used their naming scheme for all their controllers.

IMHO, rather than doing all that, you should simply NOT set that hint because the default (1) handles all of the POSITIONAL priority that we (generally) want already. With the hint unset, then you are free to report back North/South/East/West (which will become the default in SDL3 as well https://github.com/libsdl-org/SDL/commit/2991b9f6ac9b48cdb079ce36e62d1ccac935090f) and leave it up to the application to visualize buttons. Applications should visualize the buttons then by a icon such as this

image

If the application/game wants to swap out that asset for a vendor specific one then that can be done in the application settings. You made it demonstrably more difficult by setting the hint to 0 and took on the trouble of trying to determine what the style of controller is yourself rather than letting SDL handle that detection for you. In SDL3 this will become even easier, since GetGamepadButtonLabel was added and will give you the corresponding label (e.g. A/B/X/Y or Cross/Circle/Square/Triangle) for the position (north/south/west/east).

Alternatively it might make more sense to just use SDL_GameControllerGetType but I haven't yet looked to see if that covers everything we'd want it to.

There are quite a few types returned: https://github.com/libsdl-org/SDL/blob/de0cb94e7225539fb283005fc4eebc66a96d3092/include/SDL_gamecontroller.h#L61-L78 You would need a mapping table from gamecontrollertype to your xbox,nintendo,sony

Regarding joycond, is it needed anymore for modern versions of the kernel now that hid_nintendo is included? I know Arch linux says to remove it now:

If you have joycond, then delete it, because it is useless for such Switch-like gamepads, moreover joycond has a udev rule that disallows Steam to provide its own user-space driver.

(This is a little outside my area of knowledge, I just know someone on Celeste64 had issues with their Switch Pro controller until they removed joycond)

joycond is a userspace daemon for combining joycons into one controller based on the two individual controllers provided by hid-nintendo kernel driver. It always relied upon hid-nintendo, it was never a replacement. If you only use Steam games then you don't need it because steam internally combines the joycons into one controller with a custom HID driver. For literally anything else, joycond is beneficial and necessary unless you want to implement a custom combiner software into every individual game.

NoelFB commented 6 months ago

My desire is to by default use the physical locations (north/south/east/west), except where specifically desired (ex. for confirm/cancel prompts which should be swapped for Switch/sony). Things like a "Jump" action should always be the South button, regardless of whether that's A/B/X/Y/etc. If I leave SDL_HINT_GAMECONTROLLER_USE_BUTTON_LABELS to 1 then it will also swap around actions that I do not want to be moved around. Based on the documentation and my own work with Xbox/Switch/PS4 controllers I want this hint off.

It sounds like how SDL3 is implemented is ultimately what I want: north/south/east/west for inputs, and it's up to the game to swap confirm/cancel based on the controller, and the labels hint no longer does anything.

There are quite a few types returned: https://github.com/libsdl-org/SDL/blob/de0cb94e7225539fb283005fc4eebc66a96d3092/include/SDL_gamecontroller.h#L61-L78 You would need a mapping table from gamecontrollertype to your xbox,nintendo,sony

Yeah my thought is to maybe replace my enum with a 1-1 mapping with the SDL one, which might be easiest.

joycond is a userspace daemon for combining joycons into one controller

That makes sense! Thanks for the insight.

theofficialgman commented 6 months ago

My desire is to by default use the physical locations (north/south/east/west), except where specifically desired (ex. for confirm/cancel prompts which should be swapped for Switch/sony). Things like a "Jump" action should always be the South button, regardless of whether that's A/B/X/Y/etc. If I leave SDL_HINT_GAMECONTROLLER_USE_BUTTON_LABELS to 1 then it will also swap around actions that I do not want to be moved around. Based on the documentation and my own work with Xbox/Switch/PS4 controllers I want this hint off.

It sounds like how SDL3 is implemented is ultimately what I want: north/south/east/west for inputs, and it's up to the game to swap confirm/cancel based on the controller, and the labels hint no longer does anything.

SDL3 implementation is like the hint being ON. Hence why I suggest leaving it on and implementing everything based on that. It will be easier then when you transition to SDL3 as the only change you will need to do is change from interpreting gamecontrollertype to using GetGamepadButtonLabel to determine what label should be shown in GUI dialogs.

With the hint ON, controls in game are automatically correct (SDL_CONTROLLER_BUTTON_A is always guaranteed to be South). So all you need to do is check gamecontrollertype to switch the icons/functionality for menus. Much easier that way than the other way around, and like I said, it is the way SDL3 will force you to use.

NoelFB commented 6 months ago

SDL3 implementation is like the hint being ON.

I think you're mistaken/have this backwards. According to the documentation:

The variable can be set to the following values:
 *    "0"       - Report the face buttons by position, as though they were on an Xbox controller.
 *    "1"       - Report the face buttons by label instead of position

If this is enabled ("1"), Switch Pro controllers automatically swaps around A/B on me without me doing anything, which is what I do not want. With this off, they use the north/south/east/west positions, which is what I want.

Edit: I'll double check here by testing this on my end but I don't think I have this backwards.

Edit: Yeah based on the discussion here where they did these changes for SDL3, I am correct and want the hint off ("0"): https://github.com/libsdl-org/SDL/issues/6117

theofficialgman commented 6 months ago

yes, sorry, I have it backwards. but the rest of what I said still applies. I think the best way of moving forward (if you are to keep using SDL2 for the time being) would be to use gamecontrollertype to interpret the vendor information as a means of assigning on screen button prompts rather than the current https://github.com/FosterFramework/Foster/blob/72c45061702e9a2cf850edbb86312a4e618a8d2d/Framework/Input/Controller.cs#L121-L140

NoelFB commented 6 months ago

Yeah, I totally agree the current way it gets gamepad is weird and should be fixed. I think I'm in favor of using SDL_GameControllerGetType and either making the Foster enum 1-1 or map those values, and then we don't need to worry about parsing vendor information at all, if that makes sense?

Edit: Also yeah I want to maintain SDL2 until 3 is officially out, but I do have a semi-functional branch with SDL3 https://github.com/FosterFramework/Foster/tree/sdl3

theofficialgman commented 6 months ago

Yeah, I totally agree the current way it gets gamepad is weird and should be fixed. I think I'm in favor of using SDL_GameControllerGetType and either making the Foster enum 1-1 or map those values, and then we don't need to worry about parsing vendor information at all, if that makes sense?

making the foster enum 1-1/mapping makes sense. since a game might want to show different icons for sideways joycons (where ABXY doesn't make sense) or any of the other controller types more specific than just xbox/sony/nintendo. sideways joycons is not a case that could be reasonably used in Celeste64 (which kinda needs two analog sticks) but I can imagine other games wanting it.

So basically that then transfers the issue to Celeste64 where you will want to have SDL_CONTROLLER_TYPE_NINTENDO_SWITCH_PRO and SDL_CONTROLLER_TYPE_NINTENDO_SWITCH_JOYCON_PAIR use the nintendo on screen buttons and select/back and similarly for SDL_CONTROLLER_TYPE_PS3/4/5

theofficialgman commented 6 months ago

also as a sanity check, I modified https://github.com/Grumbel/sdl-jstest to print that info and can confirm that my switch pro controller and combined joycon return their expected values (5 and 13)

Found 2 joystick(s)

Joystick Name:     'Nintendo Switch Combined Joy-Cons'
Joystick GUID:     06004cfc7e0500000820000000000000
Joystick Number:    0
GameControllerTypeForIndex: 13
Number of Axes:     4
Number of Buttons: 22
Number of Hats:     0
Number of Balls:    0
GameControllerConfig:
  Name:    'Nintendo Switch Combined Joy-Cons'
  Mapping: '06004cfc7e0500000820000000000000,Nintendo Switch Combined Joy-Cons,a:b0,b:b1,back:b9,dpdown:b15,dpleft:b16,dpright:b17,dpup:b14,guide:b11,leftshoulder:b5,leftstick:b12,lefttrigger:b7,leftx:a0,lefty:a1,misc1:b4,rightshoulder:b6,rightstick:b13,righttrigger:b8,rightx:a2,righty:a3,start:b10,x:b3,y:b2,platform:Linux,'
  GameControllerType: 13

Joystick Name:     'Nintendo Switch Pro Controller'
Joystick GUID:     050056fb7e0500000920000001800000
Joystick Number:    1
GameControllerTypeForIndex:  5
Number of Axes:     4
Number of Buttons: 18
Number of Hats:     0
Number of Balls:    0
GameControllerConfig:
  Name:    'Nintendo Switch Pro Controller'
  Mapping: '050056fb7e0500000920000001800000,Nintendo Switch Pro Controller,a:b0,b:b1,back:b9,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,dpup:h0.1,guide:b11,leftshoulder:b5,leftstick:b12,lefttrigger:b7,leftx:a0,lefty:a1,misc1:b4,rightshoulder:b6,rightstick:b13,righttrigger:b8,rightx:a2,righty:a3,start:b10,x:b3,y:b2,platform:Linux,'
  GameControllerType:  5
NoelFB commented 6 months ago

OK awesome, I've refactored this now by using SDL_GameControllerGetType: https://github.com/FosterFramework/Foster/commit/fe672db5ad387d298c9b6d3b8c14018189688b63

I definitely think this is better overall.