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

Fix repeating keys, like Enter or Backspace #3096

Closed
wants to merge 1 commit into from

Conversation

aubindetrez
Copy link

@aubindetrez aubindetrez commented Jun 4, 2023

GetCharPressed() handles character repetition as expected, but GetKeyPressed() didn't repeat keys (like ENTER and BACKSPACE).
Adding a check for GLFW_REPEAT fixes the problem.

Tested: Built and tested on Linux, GCC12.

GetCharPressed handles character repetition correctly, but GetKeyPressed didn't repeat keys (like ENTER and BACKSPACE).
Repeting a key on GLFW_REPEAT fixes the problem
@raysan5
Copy link
Owner

raysan5 commented Jun 14, 2023

@aubindetrez I've been using that function for some years and never needed that feature, please, could you provide your specific use-case for that addition?

@iacore
Copy link
Contributor

iacore commented Jun 14, 2023

Is it possible to make this configurable? For certain games, the key repeat is desired.

For example, a puzzle game might need you to repeatly press arrow keys. If you can hold it down, then it's good.

You can derive the rising/falling edge with IsKeyDown(key).

@aubindetrez
Copy link
Author

@raysan5 I was writing a text editor.

But even in a small game, if the user is supposed to enter a text, wouldn't it be more natural to repeat backspaces when kept pressed?

But if you think it's too niche or it may break existing code I understand.

@raysan5
Copy link
Owner

raysan5 commented Jun 14, 2023

I was writing a text editor.

@aubindetrez I imagined that would be one answer. :D

Actually, I'm also writting a text editor and I didn't need it, I found other ways to manage it with key-down and timming... BUT I can consider this alternative, maybe I'm just over-complicating my implementation...

In any case, is this PR somewhat related to #3089

I haven't checked those changes in depth but they look kind-of related...

@aubindetrez
Copy link
Author

aubindetrez commented Jun 14, 2023

@iacore Yes you can always explicitly handle pushed and released keys. But pressed keys are nice for entering text (among other things).

I am not sure if the repeat rate is set system wide ?! (by the Operating System)

@raysan5
Copy link
Owner

raysan5 commented Jun 14, 2023

I think the repeat rate is set by the OS/keyboard drivers but not sure.

From my perspective, I find it simpler to just poll the state and deal with that data in the user-side but key-repeat support has been reported several times and I want to analize it more carefully.

@iacore
Copy link
Contributor

iacore commented Jun 14, 2023

I think the repeat rate is set by the OS/keyboard drivers but not sure.

By X11 or Wayland, so neither

From my perspective, I find it simpler to just poll the state and deal with that data in the user-side but key-repeat support has been reported several times and I want to analize it more carefully.

Keyboard,mouse,gamepad input is event-based. If you only check the state on each frame, then if the key repeat rate is faster than FPS, then some key press/release might be skipped.

@rmn20
Copy link
Contributor

rmn20 commented Jun 27, 2023

I think that this feature is strictly mandatory, as well as IsKeyRepeated, which is currently missing, because this is a basic and essential functionality provided by the system, which makes it pointless to implement it by yourself, and without which keyboard control is incomplete. Repeated key presses are necessary when creating interfaces and games with 2D graphics, the lack of which degrades the user experience. I understand that raylib is about minimalism, but it's really essential functionality. Raylib is also about ease of use, and it would be wrong to force newbies to implement their own key repeat function, or to force them to rewrite raylib to get this functionality.

@raysan5
Copy link
Owner

raysan5 commented Jun 27, 2023

Personally, I didn't need it in any of my projects to date. Recently I created a text editor and neither needed it, just implemented this functionality with provided functions and managing times myself.

I need to review this PR carefully because it can imply my editor not working properly in some platforms like web, Android or native kernel-mode. Not sure if all platforms expose OS key-repeat.

@rmn20
Copy link
Contributor

rmn20 commented Jun 27, 2023

What about raygui? I think it would benefit significantly from keyboard-controllable lists with a key repeat.

@raysan5
Copy link
Owner

raysan5 commented Jun 27, 2023

@rmn20 All my tools use raygui and I didn't need it to date. Afaik, GuiTextBox() works ok without it.

@aubindetrez
Copy link
Author

aubindetrez commented Jul 14, 2023

I would expect getKeyPressed() and getCharPressed() to have the same repeating behaviour.
If you decide against this change, can I at least update the documentation for getKeyPressed() to document this 'no-repeat' behaviour?

@Wesbalt
Copy link

Wesbalt commented Jul 17, 2023

I am creating a text editor myself and had to implement key repeats on my own. Would love to see this added to raylib

jalsol added a commit to jalsol/raylib that referenced this pull request Jul 18, 2023
@emacstheviking
Copy link

Why not have a compile time control flag around the addition of the repeat flag, that way the core remains as it is now, but for those that are aware of what they are doing they can just add to their build process to trigger it?

#ifdef KEY_REPEAT_REQUIRED
if ((CORE.Input.Keyboard.keyPressedQueueCount < MAX_KEY_PRESSED_QUEUE)
  && (action == GLFW_PRESS || action == GLFW_REPEAT))
#else
if ((CORE.Input.Keyboard.keyPressedQueueCount < MAX_KEY_PRESSED_QUEUE)
  && (action == GLFW_PRESS))
#endif

That way the developer is in control. It does mean of course you have to know how to build from sources but I din't think it would be a problem for most. This is what I am going to try locally. I am building what I hope is a serious alternative IDE and I absolutely need key repeat behaviour 'out of the box' to make my life easier!

@raysan5
Copy link
Owner

raysan5 commented Jul 25, 2023

@emacstheviking Actually that's a good idea, allowing a flag for it, for users requiring it.

@emacstheviking
Copy link

Thank you, I saw from other posts that you are more prone to going for compile time options, as am I actually! I can make that change sometime today if I can and submit a PR if it's all good. Unless you beat me to it but I am sure you are a busy guy.

@spelufo
Copy link
Contributor

spelufo commented Aug 8, 2023

Hey. There are two implications of using a compile time flag that I'd like to point out:

  • A compile time flag means you'd decide to have repeat on all keys or none for your whole program. It is much more likely that you want to use it in some contexts (such as when editing text), but not others. From a user's point of view having a function IsKeyRepeated would be better, I think.
  • Compile time flags usually force users of language bindings to fork the bindings glue code to set the flags. I've run into this with go, but I think it would be the case for most (all?) languages. Not as important as the first point, but still, better for users of bindings if avoided.

Thanks

@nickyMcDonald
Copy link
Contributor

I unfortunatly would disagree with this change. This pr would change the functionalty of GetKeyPressed and possibly break any codebases that use GetKeyPressed functionality.

@raysan5
Copy link
Owner

raysan5 commented Aug 19, 2023

I unfortunatly would disagree with this change. This pr would change the functionalty of GetKeyPressed and possibly break any codebases that use GetKeyPressed functionality.

Agree. I'm closing this PR for now, this issue will be addressed with some of the other proposed approaches: #3245, #3248

@raysan5 raysan5 closed this Aug 19, 2023
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.

8 participants