[Feature] Add IsKeyRepeated and key safety checks

[nickyMcDonald]

Hi, In order to make it possible to pick up repeated key events I have added the bool GetKeyRepeat(int key) function.
A good example of where this would be essential would be in the 'input box' example.
Replace:

if (IsKeyPressed(KEY_BACKSPACE))
{
    letterCount--;
    if (letterCount < 0) letterCount = 0;
    name[letterCount] = '\0';
}

With:

for (bool pressed = IsKeyPressed(KEY_BACKSPACE); pressed || GetKeyRepeat(KEY_BACKSPACE); pressed = false)
{
    letterCount--;
    if (letterCount < 0) letterCount = 0;
    name[letterCount] = '\0';
}

If you have any questions about renaming, changing, removing or anything else please let me know.
I do not expect the current changes in the pr to be set in stone.

[raysan5]

@n77y I understand this functionality returns true for one frame every certain time (i.e. every 10 frames, depending on the framerate), right?

I'm seeing several PRs requiring key-repeat detection functionality. Personally, I never needed it, it just managed it myself using exposed functions. First of all, that functionality does not seem to be available in all supported platforms, only PLATFORM_DESKTOP (correct me if I'm wrong), second, I got many programs depending on keys input and managing key-repeat myself, not sure how those changes could affect my code (or other users codebases in similar situation).

Why is that feature so important? What are the problems of just managing the key-repeats with already provided functionality?

[nickyMcDonald]

You would know more than me about whether this would work only on a desktop or not. I could not find any GLFW docs that specified any platform support for GLFW_REPEAT. If that is an automatic game breaker then this pr can be closed, however wouldn’t GetCharPressed only work with desktop as well?

I understand that the GLFW_REPEAT and GLFW CharCallback are both capped at the framerate, however when the framerate is above the char-repeat rate they behave according to the system's settings and not any hardcoded values.

A great example would be if I managed the key-repeat to match my system's. Then my app is used by another user, and they have a different repeat-rate on their system. This would result in inconsistent repeat rates with various keys.

The best workaround I can see with the current state of raylib and its bindings would be to use GetKeyPressed instead of GetCharPressed so I can write my own repeat functionality that's consistent with all the keys and implement a repeat speed setting.

The changes are not designed to change the functionality of any current code bases, that could obviously break their functionality and that would not be ideal. Rather this is intended as a feature meant for newer codebases and older if chosen to add it.

[nickyMcDonald]

If GLFW_REPEAT is confermed to be desktop only then this pr can be closed and @actondev's pr #3245 would work in this case.

[raysan5]

A great example would be if I managed the key-repeat to match my system's. Then my app is used by another user, and they have a different repeat-rate on their system. This would result in inconsistent repeat rates with various keys.

Yes, that's it, depending on the OS and maybe keyboard driver the key-repeat events could not be consistent between systems.

The best workaround I can see with the current state of raylib and its bindings would be to use GetKeyPressed instead of GetCharPressed so I can write my own repeat functionality that's consistent with all the keys and implement a repeat speed setting.

That's similar to what I do on some of my tools, I just manage the key-repeats myself checking if IsKeyDown() and measuring the timming to allow key-repeat behaviour. Timming is usually consistent between platform and functionality does not rely on the OS or driver. But in that case the user experience could not be the best for the user and the implementation cost is a bit higher.

If GLFW_REPEAT is confermed to be desktop only then this pr can be closed and @actondev's pr #3245 would work in this case.

I'm looking to the several related issues/PRs to see the best option for raylib.

[nickyMcDonald]

@raysan5, I have made a few changes. I renamed it to IsKeyRepeated because its only one repeat per frame only.

if (IsKeyPressed(KEY_BACKSPACE) || IsKeyRepeated(KEY_BACKSPACE))
{
    letterCount--;
    if (letterCount < 0) letterCount = 0;
    name[letterCount] = '\0';
}

Internally there are a few changes as well.
If you prefer my previous commit let me know so I can revert it.

[nickyMcDonald]

@raysan5, also it would appear that andriod supports repeated keys perhaps.

[ghost]

I'm sorry, but is this really necessary? It appears to be a big change from the standard logic of directly accessing CORE.* which is used everywhere (and is really handy).

IMHO, if some keypress repeated function is to be added, I'd rather something like #3245 which is more minimal and isolated.

[nickyMcDonald]

@ubkp, correct me if im wrong. The issue is this change? I was unware CORE was used so oftin if this is the case. I can make some changes to the pr if you can be more specific with what the issue is.

[ghost]

@n77y That too, but I was mostly looking at FLAGS_CHECK(KEY_STATE(key)... and KEY_STATE(.... Right now I can tap into anything CORE.* and extend it directly/easily without breaking anything and keeping fully upstream compatibility. Your PR proposes a different logic, it changes too much.

IMHO, keypress repeated should be handled by the user (it's not a one size fits all), but if that's to be added, I'd much rather an approach that keeps it mostly isolated/independent to not break what is already working really well. That's why I mentioned #3245.

[nickyMcDonald]

@ubkp, I have taken your requests and staged them.

[ghost]

@n77y That's looking so much better. But why are you implementing a KEY_SAFE(...?

Personally, I don't think that making IsKeyPressed() require IsKeyDown() and IsKeyReleased() require IsKeyUp() would be a good idea, makes them dependent. IMHO, the current implementation is very resilient due to being independent of each other. Does those functions really need to change?

[nickyMcDonald]

@ubkp, I believe KEY_SAFE(key) to be necessary due to the exposed nature of the IsKey...'s. It only seems mandatory to ensure that we aren’t reading memory we aren’t supposed to and rather simply return false when the user inputs a invalid key.

I agree, removing dependencies on the IsKey...'s is sensible. I removed them.

[nickyMcDonald]

KEY_SAFE(key) macro removed. Replaced with normal safety checks

[ghost]

@n77y Regarding the key safe check, the first sentence of this comment #3246 (comment) comes to mind. So I assumed it was up to the user to validate that. I'm not against it tho, I think that's good.

Just tested your PR on PLATFORM_DESKTOP (Linux Mint 21.1 64-bit) and PLATFORM_WEB (Chromium 115.0.5790.170 64-bit and Firefox 115.1.0esr 64-bit, both on Linux), and it's working great. Very good job.

---

Tested with:

#include "raylib.h"

int main(void) {
    InitWindow(800, 450, "test");
    SetTargetFPS(60);
    while (!WindowShouldClose()) {

        if (IsKeyRepeated(KEY_ONE)) TraceLog(LOG_INFO, "1 repeated");

        BeginDrawing();
        ClearBackground(RAYWHITE);
        EndDrawing();
    }
    CloseWindow();
    return 0;
}

[ghost]

@n77y Sorry to bump this again. But today I compared this PR with @actondev PR (#3245) and (after all the changes) this PR ended up being almost the same as @actondev PR. So, since he presented that solution first, I think his PR should be the accepted one.

This PR, however, adds key safe checks, Raspberry Pi and Android support. So, these improvements could be added after @actondev PR.

Either way, I'd suggesting waiting to see what @raysan5 decides to do about keypress repeated before moving further.

[raysan5]

@ubkp I agre that @actondev solution could be merged first and any further improvement could be added as a separate PR.

Despite it isolated nature, not affecting current functionality (something that I love), this is a big change and I prefer to keep PRs as small and self-contained as possible.

[actondev]

Not important, but some comments:
Should be sizeof(repeatKeyState). But then, use another array every time? Checking with MAX_KEYBOARD_KEYS instead could make things consistent.

That said, I think these should be asserts instead. If you call this some invalid value this is a silent failure.

bool static IsValidKey(int key) {
return key >= 0 && key < MAX_KEYBOARD_KEYS);
}
// and using it
assert(isValidKey(key));

I have no objections with merging this one instead, since more platforms seem to work as well!

[raysan5]

Checking with MAX_KEYBOARD_KEYS instead could make things consistent.

Agree.

I think these should be asserts instead. If you call this some invalid value this is a silent failure.

I'm afraid raylib does not use assert() anywhere. It can just TraceLog(LOG_WARNING, ...) if really required.

[raysan5]

@actondev Just merged your PR.

@n77y Required improvements from this PR could be added to already added PR.

[actondev]

@raysan5 I fucked up apparently in the last moment force push, apologies!
@n77y could you fix this bit? b3f82a1#diff-39fea0e3daa23741e81daf967717f35c7842426aab26a0bc3f525d6a2f03e339R3763-R3764 to return false; 🤦

[nickyMcDonald]

@actondev, @raysan5, @ubkp, #3256