DolphinQt / InputCommon - Support multiple DSU servers

[iwubcode]

This review allows support for multiple DSU servers, needed if you want to use a motion controller and a separate head tracking source (#8747)

Also, master made some assumptions about the name of the controller. It basically copied what DS4Windows used, where the "Mode" dictated the controller name. However, this documentation is more flexible and is what is used in this PR. This means mode is now just how much gyro functionality the server supports. So where does the "name" of the device come from?

It is now based on the server's description for the server added in the "Add New DSU Server" configuration window.

[mbc07]

Testing this ASAP!

[mbc07]

So, I threw every compatible controller I have, simultaneously, and everything seems to work as expected! Good work! The controllers were a DualShock 4 (through DS4Windows), a DualShock 3 (through ScpToolkit), a Wii Remote Plus (through WiimoteHook) and an old Wii Remote with a separate Motion Plus attachment (also through WiimoteHook).

Every controller entry was correctly shown on Dolphin's device list after adding the DSU server addresses on the "Alternate Input Sources" window. Strangely enough, I couldn't reproduce issue 12052, even through this PR shouldn't address it (yes, I checked the PR build I used was compiled after you reverted the commit that was supposed to address this issue from this PR). Every controller reported its own motion data, that reflected the movement of the matching physical controller. Haven't tried on master, though. Perhaps it could be an OS-specific issue?

About the UI, I have some nitpicks. The actual design doesn't seem consistent with anything else we currently have on Dolphin and looks a little bit crowded, there's no padding between the elements too. Perhaps a better approach could be adopting a style similar to the existing USB Passthrough and Game Directory boxes from the Main Settings. I did a mockup of this suggestion below.

Starting from the current design on master, get rid of the current "Server Address" and "Server Port" text inputs but keep the "Enable" checkbox. Once the checkbox is marked, change the state from a list box that shows the currently added servers, with the "Add" and "Remove" buttons aligned on the right, from disabled to enabled and allow the user to interact with it:

Upon clicking on "Add...", open a new window, with the actual Name, IP and Port text fields, and the common Add and Cancel buttons (almost identical to the USB Passthrough whitelist window, minus the device list part):

---

---

These footnotes aren't directly related to this PR, but I think are worth mentioning anyway (I noticed them while testing this PR but they probably apply to master too):

• When connected to a controller that is reported as partial gyro (e.g. DualShock 3, Wiimote without Motion Plus), the Advanced Input Window glitches a little. The "Battery" reading which was supposed to be on the input list for DSU Clients since PR ControllerInterface: Exposse DSU client battery level as an input. #8474 seems gone and appears only for XInput devices on this case:

  

  Also noticed that some DSU Servers (like WiimoteHook) seems to change between reporting a Full Gyro device (Wiimote with Motion Plus) and a Partial Gyro device (Wiimote without Motion Plus) on the fly if you attach/detach a standalone Motion Plus accessory, but Dolphin's DSU implementation seems to check the controller type only once and assumes it will never change at runtime. Only relaunching Dolphin seems to correct this.

• Although this PR enables labeling different servers with different names, the actual button bindings will still be referenced with DualShock specific names, like "Cross", "Triangle", "L1", "L2", and so on. Given there's a wide variety of DSU Servers nowadays, perhaps we should switch to generic names, like Button 1, Button 2, Left Trigger, Right Trigger and so on? This will definitely break existing profiles, but I think it's more doable to do this now, while the feature isn't widely known/used than later, I think.

  Another possible approach I thought (but that I could see getting messy code-wise very quickly) would be to filter the label the user gave to that DSU Server and react accordingly. E.g. if the word "DualShock" was found on the server name the user entered, somehow display PlayStation button names on the GUI when interacting with that server but still treat them internally (on the INI and on the expression parser) as Button 1, Button 2, Left Trigger, Right Trigger and so on...

[iwubcode]

@mbc07 - thank you very much for your detailed feedback. For your UI comments, I have updated this PR to follow your mockups. Good ideas!

Good catch on the battery issue, it seems like something that needs to be fixed

I agree with your comment about the button names but not sure what the best solution would be. At the moment I'd probably lean toward generic button names. I'd almost imagine that the user could specify some translation when adding a server but that might be difficult because our configuration is rather limited, "structures" or even lists are not inherently supported by Dolphin. I used a string for the current implementation, which works but would get quickly out of hand if we wanted something more complex. We could potentially embed json but it feels a bit evil. And even if we had a perfect solution, not sure if that would be too confusing to users.

[mbc07]

Switching the bindings to generic button names (probably not in the scope of this PR), in conjunction with this PR, should be sufficient for now. We can think of solutions for having button labels without hardcoding PlayStation-specific names on the bindings later, I think.

Anyway, LGTM apart from the code comment I made...

(I should probably open an issue report to avoid burying these notes under the PR discussion, especially the Advanced Input Window glitches)

[lioncash]

QStringLiteral("%1:%2 - %3").arg(server_info[1], server_info[2], server_info[0])

Will allow this to be done without a need to convert from std::string.

[iwubcode]

Tried this but wasn't working. Seems like arg can't accept std::string arguments. I could make this work if I use fromStdString 3 times but seems like that'd be worse? Maybe I misunderstood you. I'm not very familiar with the Qt library.

[iwubcode]

Thank you for reviewing @lioncash ! I have made most of the changes you have requested and left a couple of comments. Please let me know if you see anything else

[jordan-woyak]

Entering a : into the description box breaks things of course.
Entering a : at the end of the "IP Address" box actually crashes dolphin in a stoi call.
Maybe you should remove any colons from the user input?

You make no attempt to load the old single-server configs?
I guess I don't really care, but someone might?

An "Edit" button would be nice, but I'm nitpicking.

Other than that, things look good.

[iwubcode]

Entering a : into the description box breaks things of course.
Entering a : at the end of the "IP Address" box actually crashes dolphin in a stoi call.
Maybe you should remove any colons from the user input?

Good points. Let me play with that idea

You make no attempt to load the old single-server configs?
I guess I don't really care, but someone might?

I don't. I'm really bad about this. Part of me wonders if we should have a global solution to handling this because of how often it comes up but I don't know what that would look like.

Let me see how hard it would be to support the old config. I could probably give it some dummy description.

An "Edit" button would be nice, but I'm nitpicking.

I did think of an edit button but just didn't get to it

[rlnilsen]

It could be handy to have enable/disable per DSU server.

[iwubcode]

@jordan-woyak - I added a change to address your concerns. More could be done to validate the ip address input but I couldn't find anything I was happy with. Decided to leave it with just the changes we discussed. Let me know what you think.

@rlnilsen - thought of that but didn't have a UI that I was very happy with and did not want to complicate the review. I was assuming add/remove was reasonable enough for most users.

[JMC47]

While this PR works for me, I'm not able to test it in a satisfactory manner that would let me merge it, nor do I understand the code well enough.