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

Onepad2 : Greg/sdl2 #1895

Merged
merged 21 commits into from Apr 28, 2017

Conversation

Projects
None yet
7 participants
@gregory38
Contributor

gregory38 commented Apr 13, 2017

Upgrade onepad to leverage new SDL2 capabilities

  • hot plug
  • automatic mapping a la xinput so plug and play
  • better support of not standard pad (dualshock....)

It should take care of the remaining onepad issue or at least greatly improve the user experience (#1418 #1385 #1291 #1235 #1096 #414 )

But it brings interesting discussion/limitation

  • Do we want to keep SDL1.2 support ?
  • Mapping is based on a database (SDL built-in, env variable, resources). What do we do if gamepad isn't supported ?
    • Use old joystick code path
    • Ask the user to use an external tool to configure their pad mapping. (Stream/antimicro/export SDL_GAMECONTROLLERCONFIG=). And drop tons of code.
    • Provide a GUI to generate the mapping. And drop some codes.

There are some limitations

  • Pressure emulation won't be possible
    • I think we can still get access to raw data. But it will only work if the "button" is mapped as an axis.
  • We might need a GUID <=> player number mapping to properly support multiplayer

The PR is already big enough so I think it is a good opportunity to discuss it now.

TODO

  • Remove DS3 hack in the GUI
  • Behavior of the GUI when joystick is auto-configured
  • player mapping is based on index whereas it ought to use guid

@gregory38 gregory38 requested a review from turtleli Apr 13, 2017

@gregory38 gregory38 changed the title from Greg/sdl2 to WIP : Greg/sdl2 Apr 13, 2017

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Apr 14, 2017

FWIW, I'm leading toward to drop completely the support of SDL1 joystick API and to rely only on automapping. It will remove a lots of cruft.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Apr 14, 2017

So I was in the mood to remove everything, 600 lines of code !

I took the opportunity to shuffle the code a bit. And I fixed a regression for the analog pad, in case someone did test the branch ;)
~~~Edit: and it seems some issue remains :(~~~

@gregory38 gregory38 force-pushed the greg/sdl2 branch Apr 14, 2017

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Apr 14, 2017

So I added a GUI combo box to select the pad player mapping. I need to test it further, an entry of the list used to disappear from time to time.

Now we have a huge mess of updates ...

@Hirato

This comment has been minimized.

Hirato commented Apr 15, 2017

I would definitely say drop SDL 1.2, there's no point keeping it.
I presume every linux user of PCSX2 already pulls SDL2 anyway because PortAudio requires it for PulseAudio output.

I think the SDL2 gamecontroller api also needs you to activate the joystick subsystem, so it's probably not that big a deal if you keep code around to leverage it as a fallback.

SDL_Joystick does contain enumerations for things like arcade sticks, which I'm sure a lot of people would like to use for fighting games, emulated or not - no idea if these things are supported by SDL_gamecontroller.
If it's easy to generate working mappings for any such arbitrary controller which makes them compatible with SDL_gamecontroller, then I say go ahead and dump the joystick path.
Otherwise you really should keep it, and obviously enhance it to listen for SDL_JOYDEVICEADDED and SDL_JOYDEVICEREMOVED in the event loop.

Also, I'm pretty sure SDL reports between -32767 and 32767 for the trigger buttons (whereas the actual hardware will report between 0-255), same goes for all 6 of the axis

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Apr 15, 2017

my concern is distribution that still link wxWidget against SDL1.2 but yeah it is is time to upgrade anyway.

PortAudio requires it for PulseAudio output.

Oh, I didn't know that PortAudio supports PulseAudio.

Actually the gamecontroller API is basically a wrapper around the joystick API. Gamecontroller is a virtual joystick + a virtual to physical mapping. On Linux various games require that you configure (v2p mapping) your PAD (if not detected) with Steam Big Picture. There are also 3rd party app such as AntiMicro.

Onepad currently got nearly 150 builtin profiles. We just need one guy to post the profile for a missing controller and it will work for all users of the same controller. Currently we have around 600 line of code and quirk to handle the mapping. It is half-broken for some pads... So it is quite appealing :)

Also, I'm pretty sure SDL reports between -32767 and 32767 for the trigger buttons (whereas the actual hardware will report between 0-255), same goes for all 6 of the axis

Hum, the wiki said 0-32k for triggers, I will check the code.

@Hirato

This comment has been minimized.

Hirato commented Apr 15, 2017

Yeah, when I played with SDL_gamecontroller back in 2013, I handled the triggers with (event.caxis.value + 32767) >> 1 to normalise it within the 0 - 32767 range.
Could legitimately just be a bug that was fixed in the interim.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Apr 15, 2017

@Hirato
Here an extract of SDL2.0.2 code

Sint16
SDL_GameControllerGetAxis(SDL_GameController * gamecontroller, SDL_GameControllerAxis axis)
{
   ...

    if (gamecontroller->mapping.axes[axis] >= 0 )
    {
        Sint16 value = ( SDL_JoystickGetAxis( gamecontroller->joystick, gamecontroller->mapping.axes[axis]) );
        switch (axis)
        {
            case SDL_CONTROLLER_AXIS_TRIGGERLEFT:
            case SDL_CONTROLLER_AXIS_TRIGGERRIGHT:
                /* Shift it to be 0 - 32767. */
                value = value / 2 + 16384;
            default:
                break;
        }
        return value;

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Apr 15, 2017

Hum, latest HG is more complicated. I can see this kind of code so I think it is still valid.

   468         if (axis == SDL_CONTROLLER_AXIS_TRIGGERLEFT || axis == SDL_CONTROLLER_AXIS_TRIGGERRIGHT) {
   469             bind.output.axis.axis_min = 0;
   470             bind.output.axis.axis_max = SDL_JOYSTICK_AXIS_MAX;

Edit it was changed in 2013 ;) https://hg.libsdl.org/SDL/diff/ad08f0d710f3/src/joystick/SDL_gamecontroller.c

@turtleli

This comment has been minimized.

Member

turtleli commented Apr 15, 2017

my concern is distribution that still link wxWidget against SDL1.2

I think Fedora and OpenSUSE do this. For Fedora I think frantisekz has a COPR repo with an alternate wx package, but I'm not sure it's well known. I don't know if OpenSUSE has an alternate wx package that can be used with SDL2.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Apr 15, 2017

@frantisekz
Is there any reason why Fedora still use SDL 1.2 for wxWidget ?

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Apr 15, 2017

@jobermayr I think you know OpenSUSE. Do you know if wxWidget could be linked againt SDL2 (or no SDL) instead of SDL1.2 ?

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Apr 15, 2017

@turtleli
Otherwise, we can duplicate onepad-1.3 to onepad-legacy-1.3 and create a SDL2 only onepad-1.4
This way it gives people a smoother transition path.

On the PR itself, I think we have all the features (except pressure emulation). I might need to squash some commits.

@frantisekz

This comment has been minimized.

Contributor

frantisekz commented Apr 15, 2017

@gregory38 I've contacted Fedora package maintainer of wxGTK3. It's probably just leftover IMO, but we'll see. I'll post post here when I get response.

@frantisekz

This comment has been minimized.

Contributor

frantisekz commented Apr 15, 2017

@gregory38 I've received reply from package maintainer. It looks like that there is not any SDL2 support in wxGTK3. My repository apparently just disabled SDL support completely in wxGTK3 (it doesn't know --with-sdl2 nor it needs SDL2-devel to compile).

So, I tried to hack around PCSX2 (cmake/ApiValidation.cmake) and lower the FATAL_ERROR to WARNING on line 21. Compilation went well, but

ldd /home/fanys/pcsx2/bin/PCSX2 | grep "SDL"
	libSDL-1.2.so.0 => /lib/libSDL-1.2.so.0 (0xf58df000)

it looks like that it switched to SDL1.2 somewhere else. Do you have any idea how to try to "force" SDL2?

(I am compiling it with build.sh --gtk3 --release)

EDIT:
If I build PCSX2 against wxGTK3 without SDL support, it doesn't load SDL so file at all(but reports SDL2 as found during build). It looks like that my repository just disabled SDL in wxGTK3 completely and worked around PCSX2 SDL2 detection.

Can anybody on Ubuntu/Debian open PCSX2 executable in ldd?

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Apr 15, 2017

you mean for wx? Did you try to set the variable SDL_CONFIG to /usr/bin/sdl2-config (not sure of the path neither name). Wx code has a SDL_MAJOR_VERSION define so I think the code was ported (tbh I'm not even sure that someone really use the sdl audio plugin of wxwidget)

@frantisekz

This comment has been minimized.

Contributor

frantisekz commented Apr 15, 2017

(Check my updated post, idk if github updates it automatically)

Doing export SDL_CONFIG=/usr/bin/sdl2-config doesn't change anything. How can you tell if it's compiled against SDL2? Cause if I have wxGTK3 without SDL support (which PCSX2 detects as SDL2), it doesn't load any SDL so file.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Apr 15, 2017

Doing export SDL_CONFIG=/usr/bin/sdl2-config doesn't change anything. How can you tell if it's compiled against SDL2? 

Who ? WxWidget ? You can do a ldd on the wx lib (I don't know which one). Just to be sure, you need to export the variable before the configure step of wx. Otherwise you got a free ride to debug the configure script to understand how to set properly the lib flags (i.e. -lsdl2)

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Apr 15, 2017

fwiw,

export SDL_CONFIG=/usr/bin/sdl2-config
./configure --with-sdl

Based on Makefile it seems work.

% g sdl2 Makefile
CFLAGS = -pthread -Wall -Wundef -O2 -fno-strict-aliasing -pthread ..... -I/usr/include/SDL2 -D_REENTRANT -D_GNU_SOURCE -fvisibility=hidden
CXXFLAGS = -DWX_PRECOMP -pthread -O2 -fno-strict-aliasing -pthread ..... -I/usr/include/SDL2 -D_REENTRANT -fvisibility=hidden -fvisibility-inlines-hidden
EXTRALIBS_SDL = -L/usr/lib/i386-linux-gnu -lSDL2
@frantisekz

This comment has been minimized.

Contributor

frantisekz commented Apr 15, 2017

Who ? WxWidget ?

No, PCSX2. According to wxGTK3 package maintainer in Fedora, wxGTK currently does not support SDL2 at all, so it can be either SDL1 or noSDL.

So, if I use wxGTK3 compiled with SDL(1.2) support, PCSX2 binary is linked to SDL(1.2), but plugins (SPU2, onepad,...) get linked to both SDL1.2 and SDL2. IDK why both versions, it doesn't make sense.

I tried to compile your (Greg/sdl2) branch with default wxGTK3 in Fedora. It crashes on opening onepad config:
Thread 1 "PCSX2" received signal SIGSEGV, Segmentation fault.
0xf614c5ed in mem_write () from /lib/libSDL-1.2.so.0

I'll try to compile wxGTK3 (and not PCSX2) with export SDL_CONFIG=/usr/bin/sdl2-config and let you know.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Apr 15, 2017

Yes SDL_CONFIG is for wx autotool build.

PCSX2 binary is linked with (from build_prof/build.ninja)

LINK_LIBRARIES = common/src/Utilities/libUtilities.a common/src/x86emitter/libx86emitter.a -L/usr/lib/i386-linux-gnu -pthread -lwx_baseu-3.0 -lwx_gtk2u_core-3.0 -lwx_gtk2u_adv-3.0 -lglib-2.0 -lgobject-2.0 -latk-1.0 -lgio-2.0 -lgthread-2.0 -lgmodule-2.0 -lgdk_pixbuf-2.0 -lcairo -lpango-1.0 -lpangocairo-1.0 -lpangoft2-1.0 -lpangoxft-1.0 -lgdk-x11-2.0 -lgtk-x11-2.0 -lz -laio -lrt -ldl -lm -lwx_baseu-3.0 -lwx_gtk2u_core-3.0 -lwx_gtk2u_adv-3.0

In my opinion the usual suspects are

  • wx_baseu-3.0
  • wx_gtk2u_core-3.0
  • wx_gtk2u_adv-3.0

However spu2x and onepad will also got SDL from our build system. SDL2 by default and 1.2 if you force it with -DSDL2_API=FALSE.
So you have this kind of link hierarchy.

onepad <= wxwdiget <= SDL1.2
onepad <= SDL2
@frantisekz

This comment has been minimized.

Contributor

frantisekz commented Apr 15, 2017

Ok, I compiled wxGTK3 with export SDL_CONFIG=/usr/bin/sdl2-config . I checked few lines of compilation log and it actualy included SDL2. I am compiling PCSX2 with --gtk3, so I don't think wxGTK2 will affect it in any way.

Configuring PCSX2 went fine (no warning about wxGTK3 against SDL1.2) but your Greg/SDL2 branch is crashing on same place (after opening settings of onepad plugin).

0xf614c5ed in mem_write () from /lib/libSDL-1.2.so.0

EDIT: well, I was compiling and replacing the x86_64 wxGTK3.... :(

@frantisekz

This comment has been minimized.

Contributor

frantisekz commented Apr 15, 2017

So, it's working all fine, PCSX2 and all plugins linked only to SDL2 when wxGTK3 compiled with export SDL_CONFIG=/usr/bin/sdl2-config.

I'll ask package maintainer if it's acceptable solution. Thanks!

@turtleli

This comment has been minimized.

Member

turtleli commented Apr 15, 2017

Otherwise, we can duplicate onepad-1.3 to onepad-legacy-1.3 and create a SDL2 only onepad-1.4
This way it gives people a smoother transition path.

Well, question - is this PR for 1.6 or not? If it is, then I guess it'd be an okay short term solution.

If it's not, then another solution would be to add wxGTK to 3rdparty - that'll also solve the problem, and we won't have to wait for wxWidgets upstream to release a new version to get GUI fixes on Linux (3.0.2 was released about 2.5 years ago and newer GTK versions don't work too well with it, especially GTK3).

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Apr 16, 2017

Well, question - is this PR for 1.6 or not? If it is, then I guess it'd be an okay short term solution.

Tbh, I didn't have any plan. Now with the possibility of a smooth upgrade path, I think we can do it for 1.6. This way people can still fallback on current well tested version.

The biggest limitation of this new onepad version are

  • no pressure emulation
  • Only SDL2
  • Can only add a single unsupported controller (based on env variable so you're screw if you need 2+ pads)
  • need tests on various pads (typically all DS models) on various systems (USB, bluetooth...). At least we don't have to test SDL1.2 with the old js device interface.

And we win

  • plug and play
  • hotplug
  • better support of fancy pads
  • smaller code base
If it's not, then another solution would be to add wxGTK to 3rdparty - that'll also solve the problem, and we won't have to wait for wxWidgets upstream to release a new version to get GUI fixes on Linux (3.0.2 was released about 2.5 years ago and newer GTK versions don't work too well with it, especially GTK3).

I'm afraid that all distributions will remove the 3rd party version to use an older version. Is there any distribution that provide wx 3.1 ? I've the feeling that migration will be painful if they don't release the 3.2 soon.

gregory38 added some commits Apr 11, 2017

onepad: properly indent comments
Clang-format doesn't like the double *
onepad: add a game controller db resource
It will be used later by the SDL2 API
onepad: add hot-plugging support
Note: pad to player mapping is done later

v2: remove the useless print
onepad: Move init/destroy code to constructor/destructor
* prefix remaining member with m_
* Use array for m_effect_id
* Properly Destroy/Close Haptic/Joystick/Game Controller
 (except on older SDL versions which are buggy)
onepad: drop the pad if an error was detected
v2:
init m_no_error in gamepad constructor
onepad: move enum first in .h file
It would avoid future compilation issue
onepad: add a GUI list box to select joypad based on UID
Note: remove the cancel management of the small modal
It is easier, it doesn't always work anyway
onepad: replace pthred/queue with std::mutex/mt_queue
v2: based on turtleli feedback
rename m_q into m_queue
add includes
onepad: remove autorepeat deadcode
a9af374 onepad: don't touch autorepeat setup
onepad: clean the gamepad/joystick interface
Remove return of empty function
Use final/override qualifier
Remove useless virtual

Thanks turtleli for the advices

@gregory38 gregory38 force-pushed the greg/sdl2 branch to 389ad0d Apr 26, 2017

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Apr 26, 2017

Note: first commit is pending work on my master branch.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Apr 28, 2017

@nE0sIghT Hello, I think you still use SDL1 on Gentoo

New onepad version requires SDL2. But it would also requires to either drop SDL from WX (nobody use it, for example, Debian/Ubuntu doesn't enable it and nobody complains) or upgrade it to SDL2.

@turtleli I pushed all your change. Tell me if it is ok for you.

@nE0sIghT

This comment has been minimized.

Contributor

nE0sIghT commented Apr 28, 2017

@gregory38
Thanks for notice. I will look into it, but after 15 May only.

@turtleli

This comment has been minimized.

Member

turtleli commented Apr 28, 2017

I pushed all your change. Tell me if it is ok for you.

Yeah, I think it's fine.

Side note: wx3.0.3 might be released soon - wxWidgets/wxWidgets@b6ad01d

@gregory38 gregory38 merged commit 5b4c948 into master Apr 28, 2017

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@gregory38

This comment has been minimized.

Contributor

gregory38 commented Apr 28, 2017

Ok. Code merged. Thanks for the review.

Do we need something from Wx3.0.3 ?

@turtleli

This comment has been minimized.

Member

turtleli commented Apr 28, 2017

Not really, but it should fix some of the GTK2/GTK3 issues that wx3.0.2 had.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Apr 28, 2017

Oh. It won't hurt :) Hopefully distribution will pick this new version soon.

@gregory38 gregory38 deleted the greg/sdl2 branch Apr 30, 2017

@danilaml

This comment has been minimized.

danilaml commented May 2, 2017

Btw, wxWidgets 3.0.3 is now out.

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