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

feat: Added HardwareKeyboardDetector #2257

Merged
merged 27 commits into from
Jan 27, 2023
Merged

feat: Added HardwareKeyboardDetector #2257

merged 27 commits into from
Jan 27, 2023

Conversation

st-pasha
Copy link
Contributor

@st-pasha st-pasha commented Jan 7, 2023

Description

This creates a new component HardwareKeyboardDetector, which is a more advanced version of the KeyboardEvents mixin:

  • HardwareKeyboardDetector is a component instead of a mixin, which means it can be added/removed by the user at any point;
  • multiple such detectors can be attached to a game - for example, in a 2-player game one component may be paying attention to arrow keys, while another to WASD keys;
  • the new component uses Flutter's HardwareKeyboard interface, bypassing the need for a Focus widget;
  • the component keeps the ordered list of keys that are currently being pressed, which is helpful for games where this order is important;
  • there is the ability to temporarily pause the reception of key events using keyEventsPaused property.

Checklist

  • I have followed the Contributor Guide when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • No, this PR is not a breaking change.

Related Issues

Closes #2245

@bernaferrari
Copy link

bernaferrari commented Jan 7, 2023

the new mixin uses Flutter's RawKeyboard interface, bypassing the need for a Focus widget;

That's fine, but I think it works both ways and maaaaybe the Focus could be better. You can use the keys from onKeyEvent, they are reliable, you just need to keep repeating them because onKeyEvent only transmits once duplicated keys. This would avoid the user inputting text and moving the object because RawEvent listened something. But for most people and most use cases, RawEvent would work fine anyway (and worst case scenario, just check if there is not a pop-up, another focus or something else).

@st-pasha
Copy link
Contributor Author

This is still a draft, meaning that there are still things to iron out API-wise. Comments/suggestions welcome.

Focus or not?

The RawKeyboardDetector mixin subscribes to the RawKeyboard events directly, bypassing the need for the Focus or FocusScope widgets. I'm basing this on the following comment found in the RawKeyboard class:

A [RawKeyboard] is useful for listening to raw key events and hardware buttons that are represented as keys. Typically used by games and other apps that use keyboards for purposes other than text entry.

So, it appears that using this approach is actually recommended by the Flutter team. Of course, it might be useful to think carefully about the pros/cons here:

  • the Focus widget adds a bit of complexity to the game's design. This mixing avoids that complexity (assuming we can remove the Focus widget eventually). This kinda makes sense, since Focus was designed for the situations where you might have multiple widgets such as text fields in a form, and you need to control which one of those widgets receives text input. A game is not like that: the key events are typically handled by the game as a whole.
  • by listening to raw keyboard events, we do not interfere with any other possible key event handlers. It now becomes trivial to add an overlay which would contain a text input widget.
  • a disadvantage may be that we now need to be really careful with respect to when the key events are handled or not. For example, the user may add a special flag to check whether the key events need to be handled.

KeyboardHandler components

Currently the keyboard-handler mixin propagates the key events to all components in the tree. This is probably suboptimal: normally there would be only one component listening to key events. So, we could introduce the concept of "focus component" which would be the receiver of all key events from the mixin. A KeyboardHandler component would then add itself into the focusComponents list when it mounts, and remove itself from the list when dismounts.

This would allow, for example, an opaque route to prevent the components underneath from receiving key events -- by putting itself on top of the focusComponents list.

onKeysPressed event

Honestly, I'm not very sure whether we even need this one. It just seems too trivial -- a user can always override the update() method themselves and check whether there are any currently pressed keys.

@bernaferrari
Copy link

Is update guaranteed to work "as fast" as something like Timer(16ms)? I'm afraid possible lags in update might affect it.

@st-pasha
Copy link
Contributor Author

Is update guaranteed to work "as fast" as something like Timer(16ms)? I'm afraid possible lags in update might affect it.

The update is called on every game tick. Which means it is called as often as the game itself is rendered. Even if you manage to set up a Timer which would call something every 1ms, those updates wouldn't be visible to the user because the game only re-renders on every game tick. Game ticks normally occur at 60fps, that is every 16.6ms.

Now, it's possible that this may not be "fast enough" for some games. The user may press and release a key within say 20ms, and they may want to get exactly 20ms worth of movement -- not 16ms or 32ms. The solution to that (assuming RawKeyboard delivers events without a delay) is to perform all character movement in the onKeyEvent handler (possibly checking the timer to see how much time has passed since the end of previous game tick).

Copy link
Member

@erickzanardo erickzanardo left a comment

Choose a reason for hiding this comment

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

We had a similar approach in the past, we used RawKeyboard to provide the events instead of the Focus stuff.

But we moved out of that approach because by using the RawKeyboard there was no way of preventing that annoying bell sound that MacOS makes.

So, I do like this approach and the proposed API, but we need to make sure that we won't be bringing back that issue

@renancaraujo
Copy link
Member

renancaraujo commented Jan 11, 2023

I think it is a great idea and I love the implementation. One only point: I would make lots of noise in the user console if they add this AND any kind of mixing/class/component in the game that relies on focus. For starters, I would add a check on every callback, if "this is KeyboardDetector" and yell in the console if true.

Edit: made it less harsh

@renancaraujo
Copy link
Member

I would also name every method and field in this mixing with "raw" to be explicit that the person using this knows the rabbit hole there are going into.

Comment on lines 16 to 25
/// The list of keys that are currently being pressed on the keyboard (or a
/// keyboard-like device). The keys are listed in the order in which they
/// were pressed, except for the modifier keys which may be listed
/// out-of-order on some systems.
List<PhysicalKeyboardKey> physicalKeysPressed = [];

/// The set of logical keys that are currently being pressed on the keyboard.
/// This set corresponds to the [physicalKeysPressed] list, and can be used
/// to search for keys in a keyboard-layout-independent way.
Set<LogicalKeyboardKey> logicalKeysPressed = {};
Copy link
Member

Choose a reason for hiding this comment

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

Given this issue flutter/flutter#99330 I would not duplicate this info that is already present and accessible on RawKeyboard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This field is copied from the RawKeyboard on every key event:

logicalKeysPressed = RawKeyboard.instance.keysPressed;

Thus, it is always kept in sync with the information available in RawKeyboard. The reason I decided to have this copy is because in RawKeyboard this field is defined as follows:

Set<LogicalKeyboardKey> get keysPressed => _keysPressed.values.toSet();

That is, they return a copy of the internal state on every access. This is good for safety, but not so much for efficiency: if your code needs to check whether a number of certain keys are pressed, there will be a new set created on every check. Even such simple methods as isShiftPressed/isControlPressed/isAltPressed each create two new copies of this set.

So I thought that a good solution to this is to simply keep a local copy of the set of logical keys, and make sure it is always kept up-to-date with RawKeyboard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As for physicalKeysPressed -- I believe it's important to know not only which keys are currently being pressed, but also in what order they were pressed. This information is currently missing in RawKeyboard, they return physical keys as an orderless set, and keep it internally as a dictionary.

The reason why the order may be important is a scenario like this: suppose the user presses [Left Arrow], holds it, and then presses [Right Arrow]. Now two arrows are pressed simultaneously, and the game needs to decide how to respond to this. One possible choice is to make the player character stand still; another choice is to make the player character go right, since the right arrow was pressed last. By keeping the physical keys in a list instead of a set, we give the game an opportunity to choose which behavior to implement (my personal preference is for the latter).

@st-pasha
Copy link
Contributor Author

I would also name every method and field in this mixing with "raw" to be explicit that the person using this knows the rabbit hole they are going into.

Done.

@renancaraujo
Copy link
Member

Done.

Was it?

@st-pasha
Copy link
Contributor Author

I've created an example, and it seems to be working quite well; and there are no extraneous bell sounds on a Mac.

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Lgtm, is this still a draft?

@st-pasha
Copy link
Contributor Author

Still need tests and docs

@st-pasha
Copy link
Contributor Author

Oh LOL:

[RawKeyboard] is the legacy API, and will be deprecated and removed in the future. It is recommended to always use [HardwareKeyboard] and [KeyEvent] APIs (such as [FocusNode.onKeyEvent]) to handle key events.

@st-pasha st-pasha changed the title feat: Added RawKeyboardDetector feat: Added HardwareKeyboardDetector Jan 26, 2023
@st-pasha
Copy link
Contributor Author

Here's what I'm wondering: should this even be a mixin? It could be a regular Component instead.

This could have a few potential advantages:

  • the user may add/remove this component dynamically;
  • less chance of name clash with some user field/method inside the Game class;
  • ability to pass onKeyEvent handler as a parameter instead of having to override the method.

@spydon
Copy link
Member

spydon commented Jan 26, 2023

Here's what I'm wondering: should this even be a mixin? It could be a regular Component instead.

Sounds nice, a bit inconsistent with how we have other things though?
But maybe we should start moving towards that pattern for things like this in the long run and then I think it is fine to start here.

@st-pasha st-pasha marked this pull request as ready for review January 26, 2023 20:24
@spydon spydon enabled auto-merge (squash) January 27, 2023 21:50
@spydon spydon merged commit 95b1fc0 into main Jan 27, 2023
@spydon spydon deleted the ps.raw-keyboard-events branch January 27, 2023 21:57
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.

onKeyEvent doesn't work if you press two buttons.
5 participants