PCSX2 / pcsx2

PCSX2 - The Playstation 2 Emulator
https://pcsx2.net
GNU General Public License v3.0
11.79k stars 1.63k forks source link

[Feature Request]: Persistent device IDs for controllers #11816

Open ColinM9991 opened 1 month ago

ColinM9991 commented 1 month ago

Description

Hi,

I'm creating this here to open a discussion around the pad binding logic to see what others think. I have searched first and found only 1 thread in the PCSX2 forums.

At the moment, controllers managed via SDL are assigned an ID that is based on an SDL "player index". This is stored in the ini config for each pad and the IDs are formatted as SDL-{}/<Binding> where {} is the ID. On startup, the config is read and controllers are bound based on the serialized ID and keybind.

The problem is that this player index isn't unique and is based on the order in which joysticks/controllers are connected to the host

For example, I bind Controller 1 (SDL-1) and Controller 2 (SDL-2) before turning both controllers off. If I then turn on Controller 2 first, that becomes SDL-1 as far as PCSX2 is concerned.

Similarly, if I bind Controller 1 before turning it off, I come back later and plug in a flight sim joystick or throttle - that then is assigned SDL-1 while Controller 1 is assigned SDL-2.

My personal view, which is limited due to my lack of knowledge around the PCSX2 codebase as well as device emulation, is that SDL_JoystickGetProduct may be a better candidate for mapping and storing/serializing bindings as that guarantees devices will be bound correctly.

C++ isn't my forte since I'm mainly C# based but I'm happy to spend some time on this if there are no concerns with this proposed approach.

Reason

This ensures that bindings are handled uniquely per controller

Examples

No examples at the moment. Happy to put one together by forking and contributing. I'll assess whether this is also possible for XInput and DInput but will prioritize SDL as a proof-of-concept.

ColinM9991 commented 1 month ago

I did look into this and I think it oversteps my knowledge of C++ and this repository as it seems a bigger change than I'd originally anticipated.

With my proposal being to update the INI string handling to use the product ID so it would be set as

LDown = SDL-{ProductId}/+LeftY
LLeft = SDL-{ProductId}/-LeftX
RUp = SDL-{ProductId}/-RightY
...etc...

My train of thought was to update the ControllerData struct to add a joystick_product unsigned int. Within the OpenDevice event handler for SDL I then fetched the product ID via SDL_GetJoystickProduct and assigned it to the cd variable that is pushed to the m_controllers vector.

const uint joystick_product = SDL_JoystickGetProduct(joystick);

ControllerData cd = {};
cd.player_id = player_id;
cd.joystick_id = joystick_id;
cd.haptic_left_right_effect = -1;
cd.game_controller = gcontroller;
cd.joystick = joystick;
cd.joystick_product = joystick_product;

At that point, the instance or player ID could be used as a lookup value for m_controllers that would then allow you to fetch the joystick_product value (that is unique to the device) to read the binding.

std::optional<InputBindingKey> SDLInputSource::ParseKeyString(const std::string_view device, const std::string_view binding)
{
    ...
    ...
    const auto controller_data = GetControllerDataForPlayerId(player_id.value())
    key.source_index = static_cast<u32>(controller_data->joystick_product);

As I've realized though, the bindings are parsed first and then the controllers registered afterwards via the SDL events API (asynchronously, since it's event driven). So m_controllers is empty when ParseKeyString is invoked. Thus the controller instance and the key bindings only work on the basis that the SDL-{PlayerID} part is equal to the Player ID returned by SDL based on the ordering of the joysticks/controllers.

RedPanda4552 commented 1 month ago

You will also still need to track insertion order, for identical controller models.

A major reason using insertion order is favorable is because it will inherently never collide - if controllers are mismatched, you can simply unplug and replug them and it will match up again.

Also, using hard-coded device IDs on the other hand means every time you change a controller, you now have to redo your mappings. Suppose someone has a Dualshock 4 and Xbox One controller they like to switch between. You can no longer unplug one and plug in another without also going back to controller settings and redoing your mappings.

TL;DR: Nay.

ColinM9991 commented 1 month ago

Thanks for your input on this. The above approach is certainly naive and there are edge-cases that I haven't yet identified, such as what you've described above.

Also, using hard-coded device IDs on the other hand means every time you change a controller, you now have to redo your mappings. Suppose someone has a Dualshock 4 and Xbox One controller they like to switch between. You can no longer unplug one and plug in another without also going back to controller settings and redoing your mappings.

This is a good point and not one I'd considered as intentional behaviour since it does also mean that bindings need to be reconfigured if you have another device plugged in, reserving the controllers' usual spot, that cannot be removed at that time. I imagined that controllers should fall back into their original 'slot' (so to speak) to pull bindings based on what has been configured for that controller.

Now with that said, and to play devil's advocate to my own request, I recognize that bindings are actually configured on a virtual Pad and not the physical controller. The bindings simply map to the controller based on the instance ID (to state the obvious, mainly to myself). Dolphin Emulator also behaves this way so it's certainly consistent with the broader emulation ecosystem.

Whether to use persistent device IDs or transient IDs for consistency is a trade-off and, in my specific case, this issue will naturally resolve itself as I plan to build a PC dedicated to emulation, whereas with my current PC I have several different devices that connect/disconnect throughout the day.