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

Sdl motion #10561

Merged
merged 7 commits into from
Jul 11, 2022
Merged

Sdl motion #10561

merged 7 commits into from
Jul 11, 2022

Conversation

shuffle2
Copy link
Contributor

@shuffle2 shuffle2 commented Apr 5, 2022

this is just #10000 , but rebased on master, msbuild stuff fixed, and some tweaks:

  1. static link against sdl
  2. use current latest sdl (i was going to use latest release, but noticed there have been some xbox controller related fixes since then)
  3. based on top of a libusb update

i tested that ps4 zct1, zct2 controllers and ps5 zct1 controller work as expected over usb (haven't tried bt).

CMakeLists.txt Outdated Show resolved Hide resolved
@AdmiralCurtiss
Copy link
Contributor

You think we should update libusb instead of modifying the code in externals?

@shuffle2
Copy link
Contributor Author

shuffle2 commented Apr 5, 2022

yes, we should update libusb. however, I have some pending PRs on libusb's github repo, so I'm going to give them some time and see if they merge some of them. or we can just update, i guess there's technically no reason to wait :P

@shuffle2
Copy link
Contributor Author

shuffle2 commented Apr 5, 2022

On the topic of SDL: I'm going to try to only build what we need for the wanted motion input support, since the rest is completely unused by dolphin..

update: I was only able to find the HAVE_LIBC SDL flag, which helps, but there's still a lot of cruft :/

@Rumi-Larry
Copy link

The title of the pr should also include the "rumble" part

@ds22x
Copy link

ds22x commented Apr 15, 2022

Works fine except for one issue, which is whenever the controller (in my case a DualShock 4) gets disconnected and reconnected, instead of being readded as "SDL/PS4 Controller", it gets added as "SDL/Wireless Controller", forcing me to restart Dolphin to get the correct device back.

@shuffle2
Copy link
Contributor Author

@ds22x was that with bluetooth disconnection or usb? was it zct1 or zct2 controller model?

@ds22x
Copy link

ds22x commented Apr 15, 2022

USB, and zct2.

@shuffle2
Copy link
Contributor Author

shuffle2 commented Apr 16, 2022

@ds22x ... actually, it's just that it switches between HIDAPI/WINDOWS driver every other time, not timing related.

@ds22x
Copy link

ds22x commented Apr 16, 2022

I see.
Would there be a way to disable/ignore the Windows driver in those cases?

@shuffle2
Copy link
Contributor Author

shuffle2 commented Apr 16, 2022

i opened an issue about it here libsdl-org/SDL#5543
I figured out the problem and suggested a solution to SDL. hopefully we can just update the SDL submodule in the future once they merge a fix and it won't require any dolphin code changes.

@ds22x that issue should be fixed now

@MitchRazga
Copy link

Working great with DS4 and Xbox Series X Controllers over bluetooth.
Able to use rumble on Apple Silicon now! 👍

@AdmiralCurtiss
Copy link
Contributor

This needs a rebase.

@OatmealDome
Copy link
Member

It would be great if this can be merged or the submodule stuff split off into its own PR and that gets merged, so we can move away from using brew-installed SDL on the macOS build machine.

Copy link
Member

@OatmealDome OatmealDome left a comment

Choose a reason for hiding this comment

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

CMake fails with the current SDL2 commit used by this PR on macOS. There might be some issues with some recent CMake changes on macOS (libsdl-org/SDL#5506), so perhaps it would be best to use the latest release version instead.

Source/Core/InputCommon/CMakeLists.txt Show resolved Hide resolved
@Rumi-Larry
Copy link

Recommended PR title: "Input: Support Controller Rumble and Motion Input through SDL"

@iwubcode
Copy link
Contributor

@shuffle2 - I'm assuming the graphical issue just requires a rebase. Is this ready to merge otherwise? Just needing a review?

@shuffle2
Copy link
Contributor Author

shuffle2 commented Jun 3, 2022

i think it's ready to merge, i haven't been pushing for it because i'm not actually that enthusiastic about adding sdl to windows. I'd much rather add the ps4/ps5 specific functionality in dolphin directly. Is this PR really just for ps4/ps5 controllers, or is there some other benefit which makes sdl worth it?

@iwubcode
Copy link
Contributor

iwubcode commented Jun 3, 2022

@shuffle2 - I assumed it also support joycons and switch pro controllers? But not 100% sure. Not sure what else it supports..

@OatmealDome
Copy link
Member

@shuffle2 in addition to what iwubcode mentioned, this can bring support for motion on those controllers to macOS too (the alternative is #9745 which I'm hesitant to approve because it introduces more macOS specific code)

@shuffle2
Copy link
Contributor Author

shuffle2 commented Jun 3, 2022

the non-windows parts of this PR are fine (non-windows is already using sdl). i'd just be surprised if there isn't a simpler way on windows.

@Rumi-Larry
Copy link

Rumi-Larry commented Jun 4, 2022

@shuffle2 in addition to what iwubcode mentioned, this can bring support for motion on those controllers to macOS too (the alternative is #9745 which I'm hesitant to approve because it introduces more macOS specific code)

IMO, the amount of code present in the pr you just mentioned is not THAT bad, as compared to adding a new submodule (and if you already eaten the poison, you might as well lick the plate). Although this pr may be more elegant, the other uses the "Apple approved" method. But Apple being Apple, maybe it's best to support both methods, just in case SDL breaks or the other way around.

@OatmealDome
Copy link
Member

@Rumi-Larry macOS technically does not need to use the submodule, though I will submit a separate PR later to use it if this PR is merged. It would greatly simplify the build environment on the build machine, since it needs to have both Intel and arm64 installed at the moment (+ there is a hack on it right now to get the Intel version to be compatible with Mojave, Catalina).

The only new code that really applies here is what's in 12131403c3c33c10950162e9824c633ef2a60864, which isn't much. Plus, others can help fix it if it breaks since SDL is multi-platform, whereas I'd probably be on the hook to fix 9745's code if something is wrong.

Internally, SDL also uses the "Apple approved" framework that 9745 does for some controllers (MFi). The framework doesn't support a bunch of controllers like Switch Pro and Joy-Cons, so SDL uses another method to get those to work, which I think is a big benefit for this PR.

@CrusadingNinja
Copy link

rebase?

@OatmealDome
Copy link
Member

OatmealDome commented Jul 10, 2022

Can this be rebased? Would be nice to have.

@shuffle2
Copy link
Contributor Author

i will rebase it and you can merge it; i still think sdl is absolutely abysmal code and it's a shame to use it

@JMC47 JMC47 merged commit b2be9b4 into dolphin-emu:master Jul 11, 2022
@shuffle2 shuffle2 deleted the sdl-motion branch July 31, 2022 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
10 participants