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

[core] Mod-tap key presses not registered properly (PRESS-RELEASE in single frame) #2827

Closed
3 tasks done
dmlary opened this issue Dec 12, 2022 · 16 comments
Closed
3 tasks done

Comments

@dmlary
Copy link

dmlary commented Dec 12, 2022

Please, before submitting a new issue verify and check:

  • I tested it on latest raylib version from master branch
  • I checked there is no similar issue already reported
  • My code has no errors or misuse of raylib

Issue description

Very fast key presses not registering in IsKeyPressed(), but do show up in GetKeyPressed().

I've tracked this issue down to using a keyboard with QMK firmware, using mod-tap. When tapping the spacebar, I get both the KeyDown & KeyUp event in the same frame. This key pressed event isn't registered when I check IsKeyPressed(), but does show up when I call GetKeyPressed().

It exhibits most clearly when trying to play the Pang game on the raylib website: https://www.raylib.com/games/classics/loader.html?name=classic_pang

Environment

This happens both natively and for the raylib web-based demos.

  • Platforms: Web & Desktop
  • OS: OSX 11.7
  • OpenGL:
INFO: GL: OpenGL device information:
INFO:     > Vendor:   ATI Technologies Inc.
INFO:     > Renderer: AMD Radeon Pro 5500M OpenGL Engine
INFO:     > Version:  4.1 ATI-4.6.21
INFO:     > GLSL:     4.10

Issue Screenshot

Screen Shot 2022-12-11 at 7 12 00 PM

Screenshot of logging messages for code snippet below.

Top part of the log is me hitting space on laptop keyboard, bottom is me hitting space on external keyboard running QMK firmware and mod-tap on spacebar.

Code Example

minimal code logging discrepancy between IsKeyPressed() and GetKeyPressed():

                PollInputEvents();
                if (IsKeyPressed(KEY_SPACE)) {
                    log_debug("space pressed");
                }
                for (int k = GetKeyPressed(); k != 0; k = GetKeyPressed()) {
                    log_debug("pressed {}", k);
                }
@dmlary dmlary changed the title [core] Short description of the issue/bug/feature [core] fast key presses not registering in IsKeyPressed() Dec 12, 2022
@raysan5
Copy link
Owner

raysan5 commented Dec 13, 2022

@dmlary Yes, that's as expected. IsKeyPressed() consumes key polled and polling happens per frame while GetKeyPressed() accumulates key presses in an internal queue. At 60 fps it shouldn't be a big issue.

In any case, for more fine-grained control of key polling mechanisms raylib allows SUPPORT_CUSTOM_FRAME_CONTROL as a config flag, so, advance users can manage the key polling themselfs as desired.

@raysan5 raysan5 closed this as completed Dec 13, 2022
@dmlary
Copy link
Author

dmlary commented Dec 13, 2022

To make sure this is clear, it does not work at 60fps. The keyboard itself sends both the key up and down within a single frame, every time. So the game never registers the key press.

Attempting to play the raylib demo games in this state makes most of them unplayable.

I understand this is an edge case, but anyone using raylib for their game will encounter some small percentage of users who have random keys that appear not to work.

At the very least, it’s documented now.

@raysan5
Copy link
Owner

raysan5 commented Dec 14, 2022

@dmlary I can't reproduce it with my keyboard, it works as expected for me... 🤔

@dmlary
Copy link
Author

dmlary commented Dec 14, 2022

As I said in the issue description the problem is due to me using a keyboard running QMK firmware with mod-tap enabled.

https://qmk.github.io/qmk_mkdocs/master/en/mod_tap/

This is a feature that sends quick (< 1ms delay between) key up/down events when the key is tapped, but a different key code when held.

When I was evaluating raylib for use, I was struggling to understand why so many of the demos were so unresponsive to input, even when I built them locally. This is what it came down to.

Once again, I acknowledge this is an edge case, but anyone trying to play games built on raylib with mod-tap enabled is going to have a similar experience.

Thinking through the input code, is there any reason why an up/down in a single frame doesn’t register as KeyPressed? I think this question is asking about transitioning from KeyUp priority (If it’s up by the end of the frame, it wasn’t down during), to the opposite (if it was ever down since the last frame it was down). The gameplay difference is a maximum of one frame of lag before registering that the key is no longer pressed.

This would eliminate the issue of quick key presses, but I’m not confident it wouldn’t break something else in the input handling.

@raysan5
Copy link
Owner

raysan5 commented Dec 14, 2022

@dmlary Ok, thanks for the detailed explanation, I didn't know about that mod-tap feature of some keyboards.

This issue is an edge case and platform-specific (in this case keyboard-specific) so supporting it properly would require specific code for this case, increasing the complexity (and maintainability) of the sources; also I don't know how many users could have this specific requirement, in 9 years noone had reported this issue... Maybe it is a modern keyboards feature?

I'm thinking about possible simple and not too invasive solutions but the GLFW callback mechanism provided to register keys is the following:

// GLFW3 Keyboard Callback, runs on key pressed
static void KeyCallback(GLFWwindow *window, int key, int scancode, int action, int mods)
{
    // WARNING: GLFW could return GLFW_REPEAT, we need to consider it as 1
    // to work properly with our implementation (IsKeyDown/IsKeyUp checks)
    if (action == GLFW_RELEASE) CORE.Input.Keyboard.currentKeyState[key] = 0;
    else CORE.Input.Keyboard.currentKeyState[key] = 1;
}

So, if I understand it correctly, when this callback is executed the received action is already GLFW_RELEASE but a GLFW_PRESS has also previously occurred in the same frame, is that correct?

@dmlary
Copy link
Author

dmlary commented Dec 14, 2022

With the patch inlined below, I get logs showing both GLFW_PRESS and GLFW_RELEASE happening in the same millisecond. Not every one of them, but more than half:

2022-12-14T17:08:10.281 key 32, scancode 49, action 1, mods 0
2022-12-14T17:08:10.281 key 32, scancode 49, action 0, mods 0
2022-12-14T17:08:10.710 key 32, scancode 49, action 1, mods 0
2022-12-14T17:08:10.711 key 32, scancode 49, action 0, mods 0
2022-12-14T17:08:11.177 key 32, scancode 49, action 1, mods 0
2022-12-14T17:08:11.178 key 32, scancode 49, action 0, mods 0
2022-12-14T17:08:11.647 key 32, scancode 49, action 1, mods 0
2022-12-14T17:08:11.647 key 32, scancode 49, action 0, mods 0
2022-12-14T17:08:12.172 key 32, scancode 49, action 1, mods 0
2022-12-14T17:08:12.173 key 32, scancode 49, action 0, mods 0
2022-12-14T17:08:12.627 key 32, scancode 49, action 1, mods 0
2022-12-14T17:08:12.628 key 32, scancode 49, action 0, mods 0
2022-12-14T17:08:13.026 key 32, scancode 49, action 1, mods 0
2022-12-14T17:08:13.026 key 32, scancode 49, action 0, mods 0
2022-12-14T17:08:13.499 key 32, scancode 49, action 1, mods 0
2022-12-14T17:08:13.499 key 32, scancode 49, action 0, mods 0
2022-12-14T17:08:13.970 key 32, scancode 49, action 1, mods 0
2022-12-14T17:08:13.970 key 32, scancode 49, action 0, mods 0
2022-12-14T17:08:14.426 key 32, scancode 49, action 1, mods 0
2022-12-14T17:08:14.426 key 32, scancode 49, action 0, mods 0
diff --git a/src/rcore.c b/src/rcore.c
index 7acedf6c..6995db33 100644
--- a/src/rcore.c
+++ b/src/rcore.c
@@ -5262,11 +5262,31 @@ static void WindowFocusCallback(GLFWwindow *window, int focused)
     else CORE.Window.flags |= FLAG_WINDOW_UNFOCUSED;            // The window lost focus
 }

+#include <sys/time.h>
+static void
+debug(const char *fmt, ...)
+{
+  struct timeval tv;
+  char timestamp[32];
+  va_list args;
+
+  /* write the timestamp with milliseconds */
+  gettimeofday(&tv, NULL);
+  strftime(timestamp, sizeof(timestamp), "%FT%T", localtime(&tv.tv_sec));
+  printf("%s.%03ld ", timestamp, tv.tv_usec/1000L);
+
+  va_start(args, fmt);
+  vprintf(fmt, args);
+  va_end(args);
+}
+
 // GLFW3 Keyboard Callback, runs on key pressed
 static void KeyCallback(GLFWwindow *window, int key, int scancode, int action, int mods)
 {
     if (key < 0) return;    // Security check, macOS fn key generates -1

+    debug("key %d, scancode %d, action %d, mods %d\n",
+            key, scancode, action, mods);
     // WARNING: GLFW could return GLFW_REPEAT, we need to consider it as 1
     // to work properly with our implementation (IsKeyDown/IsKeyUp checks)
     if (action == GLFW_RELEASE) CORE.Input.Keyboard.currentKeyState[key] = 0;

@raysan5 raysan5 reopened this Dec 15, 2022
@raysan5
Copy link
Owner

raysan5 commented Dec 15, 2022

@dmlary As per that output, both events (key-down + key-up) are processed in one single frame...

I reopen the issue for further review but at a first glance I can't see a simple solution for this specific situation, it would require a complete redesign of the implementation with additional variables... that's out of scope for raylib at the moment...

@raysan5 raysan5 changed the title [core] fast key presses not registering in IsKeyPressed() [core] Mod-tap key presses not registered properly (PRESS-RELEASE in single frame) Jan 10, 2023
@raysan5
Copy link
Owner

raysan5 commented Aug 28, 2023

@actondev Could this issue be somewhat addressed with newly available key-repeat mechanism?

@n77y commented here:

a user could without too much difficulty get around this issue by GetKeyPressed() because when a key is released it does not remove it from the queue

For me this is a good-enough solution considering this very specific use case, could you provide a simple examples?

@actondev
Copy link
Contributor

Hi @raysan5 ! Yes, I mentioned in my comment as well #3245 (comment) . Note that his can happen not only with keys but with mouse buttons (as I mentioned it happens with my macbook touchpad a lot!)
From the top of my head we'd internally store a {Key/MouseButton}PressedInFrame which would get ored everytime we get a key/mouseButton press from the os/glfw & cleared at the end of the frame. What I haven't thought thoroughly though is

  • what about key release? same logic as key press I assume?
  • Does this mean we can remove the prevKeyState completely since we now don't implement the press logic ourselves but get the events from os? Or do we use the os key press info in a complementary way only?

@raysan5
Copy link
Owner

raysan5 commented Aug 28, 2023

@actondev thanks for the quick answer! I'm not sure if I'm understanding it properly...

  • Would it be required a new keys/mouse array to detect that specific situation? Many users probably won't benefit of that extra overload...
  • About key-pressed/key-released, if my understanding is correct, it should return both for a same key and same frame, to indicate a press-release was performed within a frame... not sure how that could affect many codebases that check both to make sure a key has been pressed and later released.

Does this mean we can remove the prevKeyState completely since we now don't implement the press logic ourselves but get the events from os? Or do we use the os key press info in a complementary way only?

Not sure if I understand this, we are already getting the events from the OS... In any case, I'd prefer to keep this separate from the current system that has been proved to work as expected in multiple platforms...

In any case, if this specific case could be moved to user side with GetKeyPressed() usage, I prefer that approach than increasing the raylib internals complexity.

@Julianiolo
Copy link
Contributor

Oh wow I just wrote about that on #3354 lol.
Yeah so I had this issue with a laptop track pad thingy (pretty much what @actondev said, however on win11, lenovo) that double clicks weren't registered because the release and press of the second click was within the same frame.
My idea, (knowing this is kinda wierd, but should work) is to do the following:

Inside the callback, after setting the currentButtonState we check if that now equals the previousButtonState. If it does we know that the button was released&pressed in the same frame(or the other way around) and we are currently on the pressed event. Know we do something to make it register as a Press.

My solution was to set the previousButtonState to the inverse of currentButtonState, but I guess it is debatable if that is smart depending on when the callback is called in relation to the part of the code where it basically goes previousButtonState=currentButtonState

That would not require any extra variables etc., just a check.

Imo it would probably be important to add some fix to this issue, since there are like 3 issues on this, and I can imagine that theres a big group of people having that problem (mostly on laptops prob) but just don't notice it, as double clicking isn't that common of a thing in games.

@actondev
Copy link
Contributor

Hey @raysan5 , apologies for not having replied all this time, hi @Julianiolo !

it should return both for a same key and same frame, to indicate a press-release was performed within a frame... not sure how that could affect many codebases that check both to make sure a key has been pressed and later released
Yeah indeed this seems quite problematic.

@Julianiolo 's proposal, while quite hacky indeed on a first read, might actually do the trick & would account for most cases.

@raysan5
Copy link
Owner

raysan5 commented Nov 1, 2023

@actondev @Julianiolo If you think it's possible to find a simple solution with not much extra code complexity or extra variables or extra functions or breaking any current codebase and also easy to maintain, please feel free to send a PR for review.

If there is no intention for someone to work on this, I'm just closing the issue and let someone else on a close/distant future to worry again about this specific usage side-case issue.

@raysan5
Copy link
Owner

raysan5 commented Nov 10, 2023

As there was no answer I understand this improvement is not required any more. Feel free to comment if situation changes.

@raysan5 raysan5 closed this as completed Nov 10, 2023
@isidroas
Copy link

I have the same issue as @dmlary (QMK mod-tap feature). But I understand that it is not worth it if it complicates the code

@ghost
Copy link

ghost commented Dec 18, 2023

if it complicates the code

@isidroas It does. IMHO, custom firmware on that level of non-standard are always a coin flip. I could be wrong, but I think it also doesn't work on No Man's Sky and/or The Outer Worlds. Either way, I doubt the userbase is large enough to justify refactoring the input code.

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

No branches or pull requests

5 participants