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

joy2key: new version using PySDL2 #3356

Merged
merged 1 commit into from Jun 28, 2021
Merged

Conversation

cmitu
Copy link
Contributor

@cmitu cmitu commented Jun 12, 2021

Added a new joy2key implementation, using PySDL2 for joystick event handling.
PySDL2 is a python module that wraps SDL2 (and other SDL libraries) using the built-in ctypes module.

Pros:

  • event handling is simplified a bit, using SDL's event loop.
  • (subjective) the code is a bit more structured and easy to follow.
  • joystick handling is rewritten based on EmulationStation code, movement is smoother and scrolling is improved.
  • support for input repeat to improve scrolling, just keep the input pressed and scrolling continues

Cons:

  • module is a bit larger (296 LOC vs 224 in current joy2key.py)
  • needs PySDL2 (which might not be packaged, see the notes below) and SDL2
  • arguably, device conf. to keyboard event mapping is more complex (abstracted as InputDev)

Notes:

  • availability of PySDL2 as a system package is good, but it's not standard in Debian 10 (stable) at the moment.
    The module is present though (as a backport) in Ubuntu 18.04 (and later) and Raspberry Pi OS (Buster).
    If not found at runtime, then the current joy2key.py is used instead and for this the joy2keyStart helper was modified.

* in the current version of joy2keyStart, joy2key is started from $scriptdir, the changes will make it run from runcommand's installation folder. Not sure if this was an oversight or on purpose.

  • added PgUp/PgDown to the default parameters for joy2key, they are mapped in a similar fashion to EmulationStation to the shoulder buttons.

@joolswills
Copy link
Member

Thanks very much for this. Will test. Much appreciated!

@joolswills
Copy link
Member

joolswills commented Jun 13, 2021

It's started from scriptdir from RetroPie-Setup as it needs to work when nothing is installed. So runcommand should use the installed version, but RetroPie-Setup should use the local copy. But the installed version should always be in sync as it's installed after every update.

fi

local joy2key="$(_joy2key_runcommand)"
Copy link
Member

Choose a reason for hiding this comment

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

Although joy2key is included as part of runcommand, and maybe this should be changed
A core helper function shouldn't rely on the runcommand module.
So this function would be better integrated into the core imho

@joolswills
Copy link
Member

Might be more logical if the code was handled like inifuncs.sh and archivefuncs.sh and shared between runcommand and RetroPie (but different copies). But we can do that later if needed.

But it's useful to be able to run it on a start of RetroPie-Setup even if runcommand isn't installed.

@joolswills
Copy link
Member

Please disregard the function location moving. Only need it to be standalone so it works without install. I will rework it later as I want to move some of our existing libraries/scripts we install such as inifuncs.sh and archivefuncs.sh and it would be logical to reorganize this at the same time.

@cmitu cmitu marked this pull request as draft June 13, 2021 05:24
@cmitu cmitu force-pushed the joy2key-rework branch 2 times, most recently from 98f4a81 to 68b8139 Compare June 14, 2021 07:12
@cmitu
Copy link
Contributor Author

cmitu commented Jun 14, 2021

OK, I added a few changes:

  • modified joy2keyStart in helpers.sh so that it works without relying on the runcommand being installed.
  • added the option to switch between the joy2key implementations (udev/sdl). I know this would ideally be part of retropie.cfg, but currently there's no UI/module for the options there. I've added it to runcommand's config, since the option should also be available in the main runcommand script.
  • modified the main runcommand.sh to use the new version of joy2key, since it doesn't rely on the implementation in helpers.sh (might be moved later to a helper lib.sh, similar to inifuncs.sh).
  • in the new .py script, I added the handling of A/B being switched (via retroarch.cfg option of menu_swap_ok_cancel_buttons).

@cmitu cmitu marked this pull request as ready for review June 14, 2021 17:09
@joolswills
Copy link
Member

Thanks for this. Will give it a test.

@joolswills
Copy link
Member

The SDL python code fails on Raspberry Pi OS buster due to the python library lacking SDL_HINT_JOYSTICK_HIDAPI and SDL_HINT_NO_SIGNAL_HANDLERS

commenting the related code out makes it work. I guess it shouldn't be a big workaround if this is missing in the older module.

@cmitu
Copy link
Contributor Author

cmitu commented Jun 16, 2021

The SDL python code fails on Raspberry Pi OS buster due to the python library lacking SDL_HINT_JOYSTICK_HIDAPI and SDL_HINT_NO_SIGNAL_HANDLERS

Those can be guarded with a SDL version check - I assume this is with the default libSDL2 from the distro, before the RetroPie's SDL2 version is added.

EDIT: actually we'd have to guard it behind the Python wrapper library check, though checking the libSDL2 version shouldn't hurt.

@joolswills
Copy link
Member

This was with our RetroPie SDL 2.0.10.

I like the changes. Works well so far. I think I'd prefer a longer delay when holding before repeating and faster repeat speed if that's doable. I've not really looked at the code yet. Basically so it better matches how keyboard input works.

I will have a look at the code when I have more time but thanks again for all your work on this. It's definitely a big improvement.

@cmitu
Copy link
Contributor Author

cmitu commented Jun 16, 2021

I like the changes. Works well so far. I think I'd prefer a longer delay when holding before repeating and faster repeat speed if that's doable. I've not really looked at the code yet. Basically so it better matches how keyboard input works.

This can be tweaked at the top of the script by modifying JS_POLL_DELAY_DEFAULT and JS_POLL_DELAY_DEBOUNCE . The latter is used between input repeats, the former is the normal poll cycle.

@joolswills
Copy link
Member

Thanks. I'll have a play.

@cmitu
Copy link
Contributor Author

cmitu commented Jun 16, 2021

The SDL python code fails on Raspberry Pi OS buster due to the python library lacking SDL_HINT_JOYSTICK_HIDAPI and SDL_HINT_NO_SIGNAL_HANDLERS

This should be fixed now. I've opted for using env variables, since the SDL library might support the hint, but the wrapper might not:

  • SDL_HINT_JOYSTICK_HIDAPI was added in SDL 2.0.9 and supported by PySDL2 0.9.7
  • SDL_HINT_NO_SIGNAL_HANDLERS was added in SDL 2.0.4 and supported by PySDL2 0.9.5

@joolswills
Copy link
Member

The poll_delay setting to use JS_POLL_DELAY_DEBOUNCE is missing for SDL_JOYAXISMOTION - I added it to the PR but please rebase or adjust as needed.

I think I prefer DEBOUNCE set to 250 btw.

@joolswills
Copy link
Member

Sorry I rebased my change as had the wrong wording in comment but then included the 250 increase change by accident. I'll leave it as it will be squashed but adjust the code as needed

@joolswills
Copy link
Member

I wonder if the if mapped_events is not None: blocks could be de-deplicated maybe also. Minor things but it's a large loop with lots of nesting so maybe could be split out more. The old joy2key was also guilty.

@joolswills
Copy link
Member

Also could avoid the and event.jdevice.which in active_devices checks for each event type by putting them all under an elif event.jdevice.which in active_devices: in the loop etc.

@cmitu
Copy link
Contributor Author

cmitu commented Jun 25, 2021

I added a couple of modifications, based on the comments and testing performed

  • simplified event handling with a helper method
  • moved the event repeat handling in a separate filtering method based on the event's last fire time and the # of times it was repeated. This allows the event loop to have a constant and higher poll cycle - instead of using SDL_Delay globally - and detect 'immediately' when an input is added or removed (i.e. fast button press which would be masked by a larger SDL_Delay )

@joolswills
Copy link
Member

Looks good to me. I'm going to merge if you're happy? Thanks for this.

@cmitu
Copy link
Contributor Author

cmitu commented Jun 27, 2021

I will do a test with a new install just to check the configuration is ok.

Added a new `joy2key` implementation, using PySDL2 for joystick event handling.
PySDL2 is a python module that wraps SDL2 (and other SDL libraries) using the built-in `ctypes` module.

Pros:
 * event handling is simplified a bit, using SDL's event loop.
 * (subjective) the code is a bit more structured and easy to follow.
 * joystick handling is rewritten based on EmulationStation code, movement is smoother and scrolling is improved.
 * support for input repeat to improve scrolling, just keep the input pressed and scrolling continues

Cons:
 * module is a bit larger (296 LOC vs 224 in current joy2key.py)
 * needs PySDL2 (which might not be packaged, see the notes below) and SDL2
 * arguably, device config to keyboard event mapping is more complex (abstracted as InputDev)

Notes:
 * availability of PySDL2 as a system package is good, but it's not standard in Debian 10 (stable) at the moment.
   The module is present though (as a backport) in Ubuntu 18.04 (and later) and Raspberry Pi OS (Buster).
   When it will be universally available, we should probably revisit the code that checks for the version and make it a hard requirement in `get_retropie_depends`, similar to `python3-pyudev`.

 * added PgUp/PgDown to the default parameters for `joy2key`, they are mapped in a similar fashion to EmulationStation to the shoulder buttons. This doesn't apply to Runcommand's invocation, since it uses a different set of parameters.

 * added a configuration option in `runcommand` for selecting the joy2key version used. Default is the SDL version (when PySDL2 is installed), with the ability to fallback to the previous version (udev based).
@cmitu
Copy link
Contributor Author

cmitu commented Jun 28, 2021

Looks ok on a new installation (tested on a Pi4 RPI OS Lite).
I squashed the branch commits, should be ready to merge.

@joolswills joolswills merged commit 86a1acc into RetroPie:master Jun 28, 2021
@joolswills
Copy link
Member

Awesome work. Thanks.

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

Successfully merging this pull request may close these issues.

None yet

2 participants