HelpComponent: Display different help icons depending on controller

[sylt]

Before this commit, all buttons shown in the help component would use the SNES layout, which wouldn't match when having plugged in an Xbox or Sony Playstation controller.

If there are different kinds of controllers plugged in, the input mapping shown will be of the last used or inserted.

The intention here is to help solving #442, or at least most of it.

[pjft]

Thanks for sending this over, and apologies for the delay in getting to this.

From reading the code, I am comfortable with the change. Two questions, though:
1 - Is the intended behavior that, if one has a PS3 and an XBox controller plugged in, that it will change the icons depending on the controller that was last used? I see that that's what the code does, but I want to confirm that that is what you're aiming for.
2 - If the override icon can't be found, could we not just return the default icon? You could test if the path exists with the overrideItvariable, and only assign it when you're sure you'll proceed, otherwise just use the default one.

Thoughts on that?

[sylt]

No worries about the delay! Just as the second revision of the patch, the first version wasn't perfect either :) A big thank you for having a look!

1. Yes, that is exactly the behavior I intended for. I hope this makes sense!
2. Yes, you are absolutely right! I will upload a new patch-set as soon as I've done some sanity testing.

For the sake of transparency, I can just mention briefly what I have been testing with: I have a keyboard, an Xbox One controller, and a 15+ year old PSX -> USB converter. So even though I have a few devices to play around with, I have not tested this with a real PlayStation controller. However, based on my Google results, PlayStation controllers seem to identify as "Sony DualShock" or similar, so simply searching (case-insensitive) for "sony" should work.

Conclusively, if you have any alternative strategy, please let me know. Otherwise, just hang tight for the new patch set :)

[pjft]

Got it. Sounds good. I don't have many controllers to test it either, as I only have a PS2 controller around, and then it's the arcade cabinet joysticks. If you'd want to send this over in the forums, maybe some people will be happy to test with other devices :)

Let me know when it's ready for review then - I appreciate you taking the time here.

Thank you!

[sylt]

Alright, here we go again! I've followed your advice and posted this request for test help to the forums! Let's see then :)

From a code standpoint, I think I'm ready to have it reviewed (whenever it suits you). In summary, the changes to the latest patch-set set are:

• Incorporated your suggestion regarding checking the overrides first.
• Consistent use of .cend().
• Changed IconMapPath key type from "const char*" to std::string. This to avoid implicit char* -> std::string conversions.

[cmitu]

I've tested this a bit and seems to work fine (P3 controller OK, DualShock 4 not ok, but see below).

My only observation would be:

• the help display style for the buttons shouldn't be queries on each display of the help component. I'd rather have a compromise between the implemented solution and the one suggested in Feature Request: Need help implementing button labels #442 - assign a 'style' for each input config when the input is added and use it directly from the help component (no need to re-calculate on display the display style).

Just testing this, I noticed that the button_x icon is not suited for the PS style controllers, the X is too 'slim', while on the Playstation controller button is more of a rotated cross.

NB: you can take a look at some common controller names in the RetroArch joypad configurations. For instance, a Dualshock 4 will identify itself as a Wireless Controller when connected via Bluetooth. I think for now the name based detection is ok, if we're going to add Prod/Vendor IDs to the configuration (as proposed in #797), then we can filter by that also, allowing us to match some other controller models with similar Xbox or PS style (i.e. like HORI PS4 Mini Wired Gamepad or DUALSHOCK®4 USB Wireless Adaptor).

[cmitu]

For controller filtering, I forgot about the GUIDs calculated by SDL - they should work better for matching a certain controller (not just by name). The list included in libSDL for Linux starts here and covers quite a few controllers (and variations).

[sylt]

Thank you @cmitu for the feedback and testing! I will rework the patch to address the points you have raised. And I will definitely check out that controller list you linked to! Just for my own sake, I will write down the plan then:

• When a controller is detected/added to EmulationStation, we'll try to categorize it for later usage in HelpComponent.
• The categorization will be done using the libSDL controller database which was linked to. At least, that will be what I'll attempt first.
• While being no artist, I can see if anything can be done as regards to the X button for PlayStation.

I'll let you guys know when I have my next attempt ready :) And thanks again!

[cmitu]

• While being no artist, I can see if anything can be done as regards to the X button for PlayStation.

That's optional, was just my observation from testing the help icons.

[sylt]

Got some time over, so I had another attempt at this. Now in the latest change-set, the identification of the controller is made in InputManager at creation time by using the SDL_GameController API. Let's hope it works better/more robust this time!

Also, regarding the X button, it was simply me who had used the wrong icon :) Now I think it should look better?

[cmitu]

Alright, this looks better and still works.
While we don't use the gamecontroller system in EmulationStation, it doesn't seem to get in the way (my idea was to get the device's GUID and compare it a list of known controllers, based on SDL's gamecontrollerdb).
Could be simplified by using initializing SDL_INIT_GAMECONTROLLER only, since it implies SDL_INIT_JOYSTICK [1].

I'm not sure why the other changes in removeJoystickByJoystickID were necessary (deleting the configs unconditionally) ?

The only thing I'm not sure is shipping gamecontrollerdb.txt, I think we can rely on the data that SDL has built-in. Note that the check you added is evaluated at compile time, but at runtime there might be a different SDL version (i.e. RetroPie ships on the Pi with SDL 2.0.10).

Otherwise, looks good.

[1] https://wiki.libsdl.org/SDL2/SDL_Init

[sylt]

While we don't use the gamecontroller system in EmulationStation, it doesn't seem to get in the way (my idea was to get the device's GUID and compare it a list of known controllers, based on SDL's gamecontrollerdb).

I think if we can get away using the SDL provided API, we should. Otherwise, we risk to (poorly) reimplement what SDL has already done.

Could be simplified by using initializing SDL_INIT_GAMECONTROLLER only, since it implies SDL_INIT_JOYSTICK [1].

Done.

I'm not sure why the other changes in removeJoystickByJoystickID were necessary (deleting the configs unconditionally) ?

Not necessary, just one of those common anti-patterns you see (same is true for C++ delete). To keep the patch more on-point however, I have removed it.

The only thing I'm not sure is shipping gamecontrollerdb.txt, I think we can rely on the data that SDL has built-in. Note that the check you added is evaluated at compile time, but at runtime there might be a different SDL version (i.e. RetroPie ships on the Pi with SDL 2.0.10).

Alright, thanks for the info, I wasn't aware of what SDL version that RetroPie bundled with!

With that in mind, I have removed the gamecontrollerdb.txt for now. It should be trivial to reintroduce once we start shipping with a newer SDL version.

Otherwise, looks good.

Nice! I think I have addressed all comments so far, but please let me know though if there are any adjustments to the latest patch set you'd like to make.

[cmitu]

Looks fine, re-tested again with a DS4, a DS3 and another (non Xbox/PS) controller and everything seems to be working fine.

[cmitu]

I've tried to compile this on an older (than current) SDL2 release and it fails with missing SDL_GameControllerType and associated defines. Seems this function has been added in 2.0.22 (2.0.20 maybe?), so the current patch will not work with older SDL versions.
@sylt what's your testing environment (SDL2 version, OS) ? 2.0.22 is quite 'new' for current stable releases (Debian/Ubuntu 22.04 LTS)

[sylt]

My testing environment is actually Debian Unstable, so no wonder if I have a very new version :) However, I interpret the 2.0.12 changelog that game controller support/identification appeared then? If so, then it shouldn't be that new (released originally 2020-03-11). Do you agree with that interpretation of the change log?

Anyway, it's a bit unfortunate, if now RetroPie ships with libSDL version 2.0.12... how often is libSDL upgraded? I guess it has to be upgraded at some point? The reason I am asking is if it's worth doing the controller identification ourselves, or if we should just wait out a newer SDL version, where it's more or less done for us. Like I said in my earlier comment, I think there are clear benefits simply relying on what SDL provides.

[cmitu]

My testing environment is actually Debian Unstable, so no wonder if I have a very new version :) However, I interpret the 2.0.12 changelog that game controller support/identification appeared then? If so, then it shouldn't be that new (released originally 2020-03-11). Do you agree with that interpretation of the change log?

Yes, you're right, the function appeared earlier than 2.0.22. What was later (2.0.22) is the identifiers for SDL_CONTROLLER_TYPE_GOOGLE_STADIA, SDL_CONTROLLER_TYPE_AMAZON_LUNA. It's possible that - similarly - other types of controllers may have been added in between 2.0.12 (when the function was introduced) and 2.26.0 (which is the sid version right now).

My suggestion would be:

• let's use just the XBOX/XBOX360 and PS3/PS4 for testing the controller type. There are supported by the 2.0.12 commit. I think PS5 was added a bit later on.
• 'hide' the switch that calculates the layour behind an #ifdef checking for the SDL at least 2.0.12 (compile time is ok). This way the improvements will still apply for users that have a newer SDL version, while not failing compilation for older/ancient SDL versions.

Anyway, it's a bit unfortunate, if now RetroPie ships with libSDL version 2.0.12... how often is libSDL upgraded? I guess it has to be upgraded at some point? The reason I am asking is if it's worth doing the controller identification ourselves, or if we should just wait out a newer SDL version, where it's more or less done for us.

We tried one upgrade and had some regressions and some bugs and unfortunately that has stalled our uprading process. We may bump the version sooner than later.
Do note that "PC" users will use the default SDL2 shipped by the distro, not all RetroPie supported platforms use the SDL2 version we ship.

Like I said in my earlier comment, I think there are clear benefits simply relying on what SDL provides.

Yes, I agree - I don't want to change from using SDL_GameControllerTypeForIndex.