Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Qt: Native DualShock 3 support in Windows using official Sony driver #9138

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

David-CPP
Copy link

@David-CPP David-CPP commented Jul 1, 2023

Description of Changes

Adds native DualShock 3 support in Windows to Qt PCSX2 using official Sony driver. An advantage this PR has over SDL is that rumble is supported.

I have created a new InputSource for DualShock 3 analogous to SDL/DirectInput/XInput.

I have implemented Automatic Mapping for convenience.

Credit to @rewasdadmin for implementing DualShock 3 native support in the old wxWidgets PCSX2 which I ported (#7099).

Rationale behind Changes

DualShock 3 rumble support is not provided by SDL.

Suggested Testing Steps

Test with multiple DualShock 3 controllers on a range of games. I have tested with the two DualShock 3 controllers I have and all seems well.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for submitting a contribution to PCSX2

As this is your first pull request, please be aware of the contributing guidelines.

Additionally, as per recent changes in GitHub Actions, your pull request will need to be approved by a maintainer before GitHub Actions can run against it. You can find more information about this change here.

Please be patient until this happens. In the meantime if you'd like to confirm the builds are passing, you have the option of opening a PR on your own fork, just make sure your fork's master branch is up to date!

@stenzek
Copy link
Member

stenzek commented Jul 1, 2023

Bit of a shame this can't be added to SDL instead, to avoid duplication.

AFAIK DS3 with pressure is supported under SDL on Linux.

@seta-san
Copy link
Contributor

seta-san commented Jul 1, 2023

Bit of a shame this can't be added to SDL instead, to avoid duplication.

AFAIK DS3 with pressure is supported under SDL on Linux.

Is it a bug in sdl?

@Bitwolfies
Copy link

Bit of a shame this can't be added to SDL instead, to avoid duplication.

AFAIK DS3 with pressure is supported under SDL on Linux.

I assume the standard Linux DS3 driver supports rumble but not pressure? Any reason rumble cant be added to SDL's HIDAPI driver? Seems very odd.

@David-CPP
Copy link
Author

Bit of a shame this can't be added to SDL instead, to avoid duplication.

I believe there is a limitation with the official Sony driver where you can't initialize HID Reports and this is why SDL do not have HID support for DualShock 3. See libsdl-org/SDL@1fc7f68

@mirh
Copy link

mirh commented Jul 3, 2023

I'm sorry, what are we even talking about here?
There is *no* driver out of the box in Windows, that's what the unassuming note in the sources is saying. It's not just that HIDAPI doesn't work, everything is bust until you install sixaxis.sys, or dshidmini, or fireshock or SCP.
And as I moaned in libsdl-org/SDL#5148 (comment), SDL didn't care yet to support any of the specific windows solutions.

So.. uhm, whatever you are eventually using in your "vanilla SDL" scenario seems just directinput or xinput.

@refractionpcsx2
Copy link
Member

I'm sorry, what are we even talking about here? There is no driver out of the box in Windows, that's what the unassuming note in the sources is saying. It's not just that HIDAPI doesn't work, everything is bust until you install sixaxis.sys, or dshidmini, or fireshock or SCP. And as I moaned in libsdl-org/SDL#5148 (comment), SDL didn't care yet to support any of the specific windows solutions.

So.. uhm, whatever you are eventually using in your "vanilla SDL" scenario seems just directinput or xinput.

Nobody is saying it's out of the box, there is an official Sony driver for DS3. https://www.pcgamingwiki.com/wiki/Controller:DualShock_3#Wired_connection_-_official_drivers

@mirh
Copy link

mirh commented Jul 3, 2023

Yes, I know.. I probably wrote a part of that page..
That's what shipped first in the ps3 sdk for ages, then came to notice of the wider world with its inclusion in the ps now client, and it's also known as sixaxis.sys.
Believing "the official Sony driver [...] can't initialize HID Reports" doesn't seem to have clear this detail though.

An advantage the native Sony driver has over SDL is that rumble is supported.

It's just not clear what OP is thinking here.
Unless they are comparing another device driver to it (which of course would be pretty nonsensical and confusing) of course "SDL" is inferior when they aren't supporting anything DS3-specific on windows.
Then, this is the first time I hear about rumble somehow being special (and it could be a bug on its own right?), but analog buttons are definitively not working too.

All this premised, good that the old wx code was ported here.. but as stenzek noted it would be even nicer if this could be addressed upstream.

@David-CPP
Copy link
Author

David-CPP commented Jul 3, 2023

An advantage the native Sony driver has over SDL is that rumble is supported.

It's just not clear what OP is thinking here.

The point I was making is that if you use a DS3 controller with Qt PCSX and select SDL input, you don't get rumble support. With my PR, which uses Windows HID API, if you have the Sony sixaxis.sys driver installed then rumble is supported. Edited to make it clearer.

@mirh
Copy link

mirh commented Jul 3, 2023

Yes, but it just wasn't clear what kind of driver you were using with SDL.
Anyway, assuming it's still sixaxis.sys.. and assuming this isn't different for DS4, I seem to understand then force feedback just isn't exposed over DirectInput (I also just checked with rpcs3 and x360ce and the same happens).

So.. if nobody is adding this to SDL, I suppose this PR is the second best thing.

@eVenent
Copy link

eVenent commented Jul 3, 2023

Great job David! I was waiting for this since Qt has become official PCSX2 version. Finally I will be able to leave old wxWidgets build. I'm using official Sony's driver with RPCS3, DS4Windows and reWASD, so changing driver to unofficial one was not comfortable for me. This one I will be able to use with official DS3, virtual DS3 by reWASD and with adapter DS2 to DS3 and make my pressure-sensitive controllers work again with the newest version of PCSX2. I hoped that SDL will add this feature, but they skipped Windows version, so official driver is the most comfortable way to play with this controller. Thank you very much!

Copy link
Member

@stenzek stenzek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CMakeLists also needs to be updated.

std::array<bool, 17> physicalButtonState = {};
std::array<bool, 17> lastPhysicalButtonState = {};

//If we wever want to support pressure sensitive buttons
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a bit silly to go to all the effort of adding a DS3-specific backend but not support pressure-sensitive buttons? Since that would be the main reason for using a DS3.

May as well just use DInput in that case.


bool active = false;

u8 SmallMotorOn = 0; // 0 or 1 (off/on)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent variable naming. Try to use snake_case for all fields/local variables.

static constexpr u16 DS3_VID = 0x054c; // Sony Corp.
static constexpr u16 DS3_PID = 0x0268; // PlayStation 3 Controller

static constexpr const char* DualShock3AxisNames[] = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these declarations should be in the .cpp file, not the .h.


namespace WindowsHIDUtility
{
std::vector<HidDeviceInfo> FindHids(u16 vid, u16 pid)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code isn't used anywhere else, and is probably unlikely to be, so it's best to make it local to the DS3 input source, rather than adding additional files.

if (GetLastError() != ERROR_INSUFFICIENT_BUFFER || !size)
continue;

SP_DEVICE_INTERFACE_DETAIL_DATA_W* devInterfaceDetails = (SP_DEVICE_INTERFACE_DETAIL_DATA_W*)malloc(size);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a unique_ptr, not C malloc/free. You're leaking memory with the continues otherwise.

{
HIDD_ATTRIBUTES attributes;
attributes.Size = sizeof(attributes);
if (HidD_GetAttributes(hfile, &attributes))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use continue here rather than a staircase of ifs.

{
std::vector<HidDeviceInfo> FindHids(u16 vid, u16 pid)
{
std::vector<HidDeviceInfo> foundDevs;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See variable naming note above.


public:
bool Activate();
void Deactivate();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A destructor is better than an explicit "Deactivate()" call.

@stenzek
Copy link
Member

stenzek commented Jul 4, 2023

As I noted in the review above, this doesn't seem to actually report pressure sensitivity, so I'm questioning what the point is in its current state.

@mirh
Copy link

mirh commented Jul 4, 2023

As noted above, adding rumble is already a pretty important addition.
On the other hand it seems odd to claim to be porting something, while missing some of its features.

And if you are still going to have a subpar experience, than at this point you could as well just recommend dshidmini isn't it?

@David-CPP
Copy link
Author

Thanks for the feedback. I'll look into implementing pressure-sensitive buttons.

@diego-rbb-93
Copy link

Thanks for the feedback. I'll look into implementing pressure-sensitive buttons.

Sorry to bump this up, but I've been recently catching some new DS3 controllers to test this out in the future but, are you managing to finish the PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants