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

onKeyEvent doesn't work if you press two buttons. #2245

Closed
bernaferrari opened this issue Dec 30, 2022 · 14 comments · Fixed by #2257
Closed

onKeyEvent doesn't work if you press two buttons. #2245

bernaferrari opened this issue Dec 30, 2022 · 14 comments · Fixed by #2257
Labels
bug fix available A PR fixing this bug is available and waiting for a review

Comments

@bernaferrari
Copy link

How to reproduce:

  • Press "arrow right".
  • Press "arrow up".
  • Take the finger out from "arrow up".
  • onKeyEvent will stop triggering.
Screen.Recording.2022-12-30.at.19.04.01.mov

Really simple code:

  @override
  KeyEventResult onKeyEvent(
      RawKeyEvent event, Set<LogicalKeyboardKey> keysPressed) {
  print('keysPressed: ${keysPressed.map((e) => e.keyLabel)}');
   return super.onKeyEvent(event, keysPressed);
}

Using Flame 1.5.0 and Flutter (Channel main, 3.7.0-13.0.pre.127, on macOS 13.1 22C65 darwin-x64,
locale en-BR)

@spydon
Copy link
Member

spydon commented Dec 30, 2022

Have you tried on the stable channel of Flutter?

@bernaferrari
Copy link
Author

Yes, same issue.

@spydon
Copy link
Member

spydon commented Dec 30, 2022

Very strange, this definitely used to work! It seems to not happen all the time either.
I would guess that this is a Flutter bug, so it would be good to try this in a pure Flutter app.

Here is a full repro if anyone wants to try:
https://dartpad.dev/?id=96f38607b6d255a7c9b7b19954ec19a6

@bernaferrari
Copy link
Author

For me, Flutter-only seems to be working fine.

This is an old sample, it is working:

import 'package:flutter/material.dart';
import 'package:flutter/services.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  // This widget is the root of your application.
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(),
      home: const HomePage(),
    );
  }
}

class HomePage extends StatefulWidget {
  const HomePage({Key? key}) : super(key: key);

  @override
  State<HomePage> createState() => _HomePageState();
}

class _HomePageState extends State<HomePage> {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Focus(
        autofocus: true,
        onKey: (FocusNode node, RawKeyEvent event) {
          KeyEventResult result = KeyEventResult.ignored;
          if (event is RawKeyDownEvent) {
            if (event.logicalKey == LogicalKeyboardKey.keyZ ||
                event.logicalKey == const LogicalKeyboardKey(0x0000005a)) {
              // When press Z, ctrl+Z, meta+Z, shift+Z:
              print('Z pressed');
              result = KeyEventResult.handled;
            } else {
              // When press meta+shift+Z.
              print('something pressed (???)');
            }
          }
          setState(() {});
          return result;
        },
        child: Text('key is: ${RawKeyboard.instance.keysPressed}'),
      ),
    );
  }
}

@bernaferrari
Copy link
Author

Hold on. Still debugging.

@bernaferrari
Copy link
Author

bernaferrari commented Jan 2, 2023

This is not a Flutter issue, this is how the OS works. These are the possible workarounds:

image

Also this:
flutter/flutter#117841 (comment)

@spydon
Copy link
Member

spydon commented Jan 2, 2023

This is not a Flutter issue, this is how the OS works. These are the possible workarounds:

image

Also this:
flutter/flutter#117841 (comment)

Thanks for investigating it! I guess we should close this then.

@bernaferrari
Copy link
Author

bernaferrari commented Jan 2, 2023

Yes and no, ideally you could refactor onKeyEvent or add the proposed method into the framework, so would help more people, as the behavior will probably be unclear for many (like Bonfire, etc), and this affects 90% of games.

@bernaferrari
Copy link
Author

bernaferrari commented Jan 7, 2023

I made a sketch for a mixin that could somewhat be integrated into Flame, since it guarantees the button works. Just giving the idea. But ideally you could override onKeyEvent with the new system, so existing apps keep working and the bug is gone. currentKeysPressed should become keysPressed:

mixin WithCorrectTimer on Component, HasKeyboardHandlerComponents {
  Timer? _timer;
  final Set<String?> currentKeysPressed = {};
  final int interval = 16;

  @override
  Future<void>? onLoad() {
    _timer?.cancel();
    _timer = Timer.periodic(
        Duration(milliseconds: interval), (Timer t) => updateMovement());
    return super.onLoad();
  }

  void updateMovement();

  @override
  void onRemove() {
    _timer?.cancel();
    super.onRemove();
  }

  @override
  KeyEventResult onKeyEvent(
      RawKeyEvent event, Set<LogicalKeyboardKey> keysPressed) {
    currentKeysPressed
      ..clear()
      ..addAll(RawKeyboard.instance.keysPressed.map((e) => e.debugName));
    return super.onKeyEvent(event, keysPressed);
  }
}

@spydon
Copy link
Member

spydon commented Jan 7, 2023

You shouldn't need the timer if you use on Component in the mixin, then you can hook in and use update instead.

@st-pasha
Copy link
Contributor

st-pasha commented Jan 7, 2023

My worry here is that there could be a situation where a key gets released without the corresponding "KeyUp" event -- say it gets released when the game widget has no focus. In that case we would keep thinking that the key is pressed, and it will effectively become "stuck".

@st-pasha
Copy link
Contributor

st-pasha commented Jan 7, 2023

But perhaps we could create a new version of the KeyboardEvents mixin, allowing the people to gradually transition, with the possibility of falling back to the old mixin if something is not working. Similar to how we introduced new draggables and tappables mixins.

@bernaferrari
Copy link
Author

My worry here is that there could be a situation where a key gets released without the corresponding "KeyUp" event

There is a Flutter "bug" that already happens when you switch window. It will be fixed in the coming months when the key behavior is synced across window.

Right NOW, the only way to not trigger the bug behavior is using ShortcutActivator or SingleActivator, which you aren't using on keyboard event. So there wouldn't be any change.

@st-pasha st-pasha added the fix available A PR fixing this bug is available and waiting for a review label Jan 12, 2023
@bernaferrari
Copy link
Author

I can't believe you worked so hard on it. I thought I was happy with my placeholder (which came from you, lol). And you ended up doing something even better. Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix available A PR fixing this bug is available and waiting for a review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants