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

New feature: Use gamepad combo to quit Retroarch #13017

Merged
merged 3 commits into from
Sep 24, 2021

Conversation

bhamiltoncx
Copy link
Contributor

Description

This is my first Retroarch PR, so please be kind and thorough in code review!

I built a home arcade with Retroarch. It has no keyboard, just arcade-style joysticks and buttons. I found my young kids couldn't quite understand how to quit Retroarch with the buttons.

I found the "toggle menu with gamepad combo" feature, and tried to teach my kids how to navigate to the quit option from there, but they couldn't figure it out.

I tried setting up AutoHotKey to map various joystick buttons to the ESC key on the keyboard, but it's fairly flaky and doesn't handle joysticks being plugged/unplugged (we have Bluetooth gamepads that we use from time to time).

So, this PR builds on the existing "toggle menu with gamepad combo" feature and:

  1. Refactors the existing "toggle menu" logic and gamepad combo-related enums so they can be reused for non-menu-toggling features
  2. Moves the static timers for the "hold start" / "hold select" combos out into input_driver_state_t so they can be used separately for the "toggle menu" feature and other features which use a gamepad combo
  3. Adds a new config option and menu entry to quit Retroarch with a gamepad combo

I tested this on macOS 11.5.2 and confirmed both the "hold start" and "hold select" options worked for quit and for toggling the menu.

Related Issues

This is another way to address #2670.

Reviewers

@jdgleaver was kind enough to inspire me in #12534 to contribute, so thanks to them, and maybe they can take a look as well.

@andres-asm
Copy link
Contributor

While this is cool and all... I never understood the need for toggle menu combo while RA has hotkey enable and hotkeys, the only reason is that "that combo" is limited to two buttons.

@bhamiltoncx
Copy link
Contributor Author

bhamiltoncx commented Sep 19, 2021

I never understood the need for toggle menu combo while RA has hotkey enable and hotkeys, the only reason is that "that combo" is limited to two buttons.

On a home arcade, the number of buttons is very limited, so it's typical to need 3 buttons (or a "hold button" action) to quit. Games tend to use all the rest of the 2-button combinations.

For my use-case, it's more the "hold start" timer feature that I plan to use.

@jdgleaver
Copy link
Contributor

@bhamiltoncx Thank you for the contribution! I like the refactor here, and this is useful functionality that is cleanly implemented.

One slight foible is the way that combo_timers contains a number of unnecessary entries - but I can't think of a way to rework this elegantly without introducing some performance overheads. We're only wasting ~260 bytes, though - so I believe we don't need to worry about it :)

Other than that, there are only a couple of incredibly trivial things that I have corrected in this patch:
quit_combo.patch.zip
(the main thing is adding some safety checks to input_driver_button_combo(), since this is more general purpose now and could potentially be used elsewhere)

If you could add this patch and fix the conflict in input/input_driver.c, we can go ahead and merge :)

@bhamiltoncx
Copy link
Contributor Author

@bhamiltoncx Thank you for the contribution! I like the refactor here, and this is useful functionality that is cleanly implemented.

Thanks! I'm glad the patch makes sense.

One slight foible is the way that combo_timers contains a number of unnecessary entries - but I can't think of a way to rework this elegantly without introducing some performance overheads. We're only wasting ~260 bytes, though - so I believe we don't need to worry about it :)

Yeah, I had thought about the same thing. One nice thing about the "unnecessary" entries is it lets us enable the same "hold combo" feature for any combo, not just start/select.

Other than that, there are only a couple of incredibly trivial things that I have corrected in this patch:
quit_combo.patch.zip
(the main thing is adding some safety checks to input_driver_button_combo(), since this is more general purpose now and could potentially be used elsewhere)

If you could add this patch and fix the conflict in input/input_driver.c, we can go ahead and merge :)

Thanks! I've applied the patch and fixed the merge conflict.

I see the safety checks you added — I tweaked them slightly to be debug asserts instead of runtime checks, since the values are not currently optional. If we want the function to allow input_driver_state or p_input to be optional in the future, we can probably come up with a better way to reflect that (maybe two APIs instead of one).

@inactive123
Copy link
Contributor

OK, @jdgleaver told me this is good to go for now.

@inactive123 inactive123 merged commit 1970786 into libretro:master Sep 24, 2021
@bhamiltoncx
Copy link
Contributor Author

Thanks, all!

@bhamiltoncx bhamiltoncx deleted the quit-gamepad-combo branch September 24, 2021 17:04
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.

4 participants