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

Global Keyboard Navigation #816

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

jbienz
Copy link
Contributor

@jbienz jbienz commented Jul 3, 2024

I've implemented basic global keyboard navigation for the following keys:

Key.UpArrow = MenuAction.Up
Key.DownArrow = MenuAction.Down
Key.LeftArrow = MenuAction.Left
Key.RightArrow = MenuAction.Right
Key.Enter = MenuAction.Green
Key.Escape = MenuAction.Red

When pressed, these keys simulate the specified input action for the first player. It works on all screens and does not require binding.

I'm happy with this and would like to see it merged, but I'd also like to start a conversation about more enhanced input.

For example, I'd love to see Page Down go to Next Section and Page Up go to Previous Section in the Music list. Unfortunately, there is no MenuAction for Next and Previous Section. This is currently implemented as a combo of holding Orange plus Up or Down. I was able to simulate holding and releasing Orange, but because this happens so quickly the popup menu appears. This is due to using a timer on Orange.

Ideally, I would prefer to see MenuActions added for Next Section and Previous Section. This could work on many menus (including settings) but it would require an update to Yarg.Core. I'm seeking input on this change. I could include it as part of this PR or as a separate one.

@jbienz
Copy link
Contributor Author

jbienz commented Jul 3, 2024

This PR addresses #810.

@jbienz
Copy link
Contributor Author

jbienz commented Jul 3, 2024

I'm thinking that I should probably add bindings for the colors too. Like B = Blue and O = Orange.

I could also add a setting that would disable these built-in keyboard navigation "bindings" in case the user wants to customize bindings manually by adding the keyboard device themselves. Though I still think these default binds should be enabled by default.

@jbienz jbienz marked this pull request as draft July 3, 2024 07:58
@TheNathannator
Copy link
Collaborator

My thoughts on the comments here so far:

For example, I'd love to see Page Down go to Next Section and Page Up go to Previous Section in the Music list.

This would certainly be a tricky thing to implement support for currently, we need to rethink how section seeking is done I think. In Rock Band, holding Orange will open a navigable menu of sections, and just pressing Orange will seek to the next section. We already have a menu for this IIRC, but it only exists in the More Options menu currently.

We can also take a page out of the Rock Band 3 Deluxe mod's book, and bind the left/right menu actions to seek up/down a section (or make it jump, say, 5 songs at a time).

Ideally, I would prefer to see MenuActions added for Next Section and Previous Section.

This I'm a little hesitant on, as none of the instrument peripherals would be able to use them without sacrificing other menu actions. They would, however, be nice QoL actions to have for devices where it makes sense to implement (e.g. this could also be bound to the left/right triggers on gamepads, taking a page from the Xbox 360/One menus). Perhaps they can just be named PageUp/PageDown so we can be a little more flexible with how they function if needed?

I'm thinking that I should probably add bindings for the colors too. Like B = Blue and O = Orange.

For color bindings, I'd suggest using the 1-5 number row keys, I imagine that's the most intuitive bindings for those. These can exist in addition to the current bindings on Enter/Escape.

Copy link
Collaborator

@TheNathannator TheNathannator left a comment

Choose a reason for hiding this comment

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

And now the code side of things; looks good to me currently!

private void UpdateKeyboardInput()
{
// Get current keyboard
Keyboard keyboard = Keyboard.current;
Copy link
Collaborator

@TheNathannator TheNathannator Jul 7, 2024

Choose a reason for hiding this comment

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

I might suggest using the InputState.AddChangeMonitor API instead of doing manual frame-based polling, as these will allow multiple input events to be registered within a single frame (for cases like stuttering or lag spikes, our music library menu is rather inefficient currently lol).

I'm not adamant about this change though, as keyboard inputs don't seem to be threaded currently in the native input backend on Unity's side. What's implemented now will be plenty good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @TheNathannator. I was unaware of InputState.AddChangeMonitor but that looks like a cool API. I'm happy to incorporate it.

I would like the defaults in this "global" bind to reflect the defaults when a keyboard is added to a profile. Other than just using the same keys, do you recommend a different path to making sure the two would remain in sync over time?

Finally, I think I should add a setting that would allow this "global binding" to be turned off. That would allow users to still add a keyboard to a profile and customize it like before without any conflicts. Do you agree?

Keyboard keyboard = Keyboard.current;

// If there is no keyboard, bail
if (keyboard == null) { return; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Code style: don't put single statements on the same line as ifs (braces optional).

Suggested change
if (keyboard == null) { return; }
if (keyboard == null)
{
return;
}

Some of us used to do this, but we discourage it now since it mildly obscures the flow of code enough to make reading it more difficult (though I think the braces help with that a little); just a matter of keeping each line to only one action. (I think we still need to update the Rider config to change it to this.)

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

3 participants