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

Add IsKeyPressedRepeat (desktop only) #3245

Merged
merged 1 commit into from
Aug 20, 2023

Conversation

actondev
Copy link
Contributor

@actondev actondev commented Aug 16, 2023

Hi Ray!

Currently IsKeyPressed ignores key repeats. This PR keeps the current behavior the same while adding the additional IsKeyPressedRepeat.
Another approach could be changing the existing IsKeyPressed functionality to take into account key repeats as well, and add IsKeyPressedExt, expecting an extra bool repeat flag for taking into account key repeats or not.

Personally I would be in favor of the second approach (changing existing behavior), but it's up for discussion.

@raysan5
Copy link
Owner

raysan5 commented Aug 18, 2023

@actondev 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?

@actondev
Copy link
Contributor Author

@raysan5 If I understood your question correctly, yes.

Regarding it's usefulness, in keyboard-driven application where on can navigate/perform actions on the GUI, I feel this feature being necessary. Sure one can implement key repeat themselves but a) it's more work, and b) it doesn't respect the system wide setting.
Another issue is if one enables "waiting for events" (so that the main loop isn't rerun without any user input). Is implementing key-repeat yourself also viable in that case?

As I mentioned in the PR description, existing functionality should stay as it is.

Also, if you agree with the general approach here, the same could be used for fixing the "pressed" logic when both the down & up events arrive at the same frame. (see #2827 (comment) ). So similarly as done in this PR, we'd have a keyPressInFrame. I also noted this issue in macbook touchpad: when quickly tapping (for clicking) I rarely get mouse presses, since in the same frame while processing the events, a mousedown and a mouseup.

@actondev
Copy link
Contributor Author

Btw I find it funny that 2 PRs for this issue got opened back to back. Personally I'm building a custom GUI & was adding some keyboard navigation, so not having key repeat was such a bummer heh

@raysan5
Copy link
Owner

raysan5 commented Aug 19, 2023

@actondev thanks for your answer, here my comments:

Another issue is if one enables "waiting for events" (so that the main loop isn't rerun without any user input). Is implementing key-repeat yourself also viable in that case?

Yes, it works because keeping a key down is an event, so loop keeps executing.

As I mentioned in the PR description, existing functionality should stay as it is.

Agree, that's my main concern with all the related PRs/Issues.

I have to review all the other related PRs more carefully but this one seems to me the most feasible option to support this feature.

@rmn20
Copy link
Contributor

rmn20 commented Aug 20, 2023

It's not a big deal, but maybe IsKeyPressedRepeat could be renamed to IsKeyRepeated for convenience?

@nickyMcDonald
Copy link
Contributor

nickyMcDonald commented Aug 20, 2023

It's not a big deal, but maybe IsKeyPressedRepeat could be renamed to IsKeyRepeated for convenience?

@actondev, I also think IsKeyPressedRepeat should be renamed. Could you rename it to perhaps IsKeyRepeated or something else in case you can think of something better?

If you do rename it I can close my pr and reopen it to add my additional changes if @raysan5 merges your pr.

Also a side request I have is could you make your IsKey... repeat body consistent with the other IsKey...'s?

bool IsKey...(int key)
{
    if (CORE.Input.Keyboard.keyRepeatInFrame[key] == 1) return true;
    else return false;
}

@raysan5
Copy link
Owner

raysan5 commented Aug 20, 2023

@rmn20 @n77y About renaming, I also though about that but actually IsKeyPressedRepeat() is the most accurate naming for the provided functionality. It returns IsKeyPressed() events that are repeated every certain time.

Probably for an advance programmer IsKeyRepeat() is clear enough but for a person that starts on programming it can be confusing.

@nickyMcDonald
Copy link
Contributor

nickyMcDonald commented Aug 20, 2023

@raysan5, I agree and change my mind. IsKeyPressedRepeat() Is the most accurate even if it’s more verbose.
It looks as if it’s an addition to the IsKeyPressed... functionality:

if (IsKeyPressed(KEY_BACKSPACE) ||
    IsKeyPressedRepeat(KEY_BACKSPACE))

if (IsKeyPressed(KEY_BACKSPACE) || IsKeyPressedRepeat(KEY_BACKSPACE))

if (IsKeyPressed(KEY_BACKSPACE) ||
    IsKeyRepeated(KEY_BACKSPACE))

if (IsKeyPressed(KEY_BACKSPACE) || IsKeyRepeated(KEY_BACKSPACE))

Since the key pressed are handle by comparing current vs previous
state (ie frame), a special way is needed to handle key repeats.
@actondev
Copy link
Contributor Author

forced pushed formatting changes

@raysan5 raysan5 merged commit b3f82a1 into raysan5:master Aug 20, 2023
12 checks passed
@raysan5
Copy link
Owner

raysan5 commented Aug 20, 2023

@actondev Thank you very much for this new feature and the required reviews.

@n77y thank you for the review and related improvements.

@actondev actondev deleted the feat/key-repeat branch August 20, 2023 22:23
raysan5 added a commit that referenced this pull request Sep 17, 2023
* Prettified a comment

* fixed broken indentation caused by another commit.
the commit renamed a bool to int and broke indentation: 233cf39

* Changed 0.001 and 0.00001 to EPSILON
This commit is untested.
I don't know what consequences this has.
Since the commits that added these numbers were before epsilon was added,
I have assumed that epsilon could replace them.

* Prettied up indentation in a few places

* removed spacing around *, standardizing it.

* I may have gotten overboard with indentation

* removed a few useless parenthesis

* Added fortran-raylib

* Fix examples/others/rlgl_standalone.c compilation issue (#3242)

* Update BINDINGS.md

* Ignore unused return value of GetCodepointNext in GetCodepointCount (#3241)

* Ignore unused return value of GetCodepointNext in GetCodepointCount

Removes the last warning from non-external libraries when compiling with
the default build configuration on x64 Linux.

* Remove unnecessary void cast in GetCodepointCount

* Fix #3246

* Revert "Fix #3246"

This reverts commit e4dcbd5.

* Fix text_unicode.c example crashing (#3250)

* Fix text_unicode.c example crashing

* Adjust the text_unicode.c example crashing fix

* tweaks

* add build.zig options for individual modules (#3254)

* Add `IsKeyPressedRepeat` (desktop only) (#3245)

Since the key pressed are handle by comparing current vs previous
state (ie frame), a special way is needed to handle key repeats.

* Reviewed `IsKeyPressedRepeat()` #3248

* Update rcore.c (#3255)

* Match CMakeOptions.txt options default values (#3258)

* Fix SetClipboardText for web (#3257)

* [Image] Validate that ImageDrawRectangleRec is drawing entirely inside the image (#3264)

* Add a function to clone a sound and share data with another sound.

* rename items based on feedback

* PR Feedback, use custom unload for sound alias, not variant of normal sound unloading

* sound_multi example

* Validate that image rect drawing is inside the image so we don't overflow a buffer

* remove files that should not have been added.

* remove changes that should not have been

* revert

* adsfasdfsdfsdf

* Add Vector3 Projecting and Rejection to Raymath (#3263)

* Update raymath.h

* formatting

* [Feature] IsKey... safety checks and more (#3256)

* [Feature] Add GetKeyRepeat

* Update rcore.c

* Simpler design, only one repeat per frame

* Update config.h

* Update rcore.c

* Add KEYBOARD_KEYS_MASK

* Update config.h

* reversions

* Update rcore.c

* Update rcore.c

* change docs

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update raylib.h

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Fix bug where default shaders was not linking. (#3261)

* Formating review

* Add missing cmake options (#3267)

* Fix CMake extraneous -lglfw (#3266)

Closes #3265.

The problem: LIBS_PRIVATE is a list of library names (used by pkg-config), but the shared library of the same name doesn't always exist.

* Fix example/models/models_loading_gltf.c controls (#3268)

* Fix example/models/models_loading_m3d.c controls (#3269)

* Remove e from secondes (#3270)

* Fix example/audio/audio_module_player.c help instructions and small bug (#3272)

* Fix example/audio/audio_module_player.c help instructions and small bug

* Update example/audio/audio_module_player.png screenshot

* Use type name instead of valid specifier

long long --> long long int

* REVIEWED: `GetFileLength()`, added comment #3262

* Update examples/models/models_loading_gltf.png;m3d.png screenshots (#3273)

* Remove a duplicated screenshot and add missing one (#3275)

* Add examples/shaders/shaders_lightmap.c to Makefiles (#3276)

* Fix examples/others/easings_testbed.c help instructions and small tweak (#3277)

* Fix examples/shaders/shaders_texture_outline.c help instructions (#3278)

* Fix examples/shapes/shapes_collision_area.c help instructions (#3279)

* RENAMED: LoadFont*() parameter names for consistency and coherence

* Fix uninitialized thread-locals in stbi #3282 (#3283)

* REVIEWED: Added `SetTextLineSpacing()` to multiline examples

* REVIEWED: Data size type consistency between functions #3168

* Some tweaks

* Use internal default allocators, instead of user-exposed ones

* Added rudimentary SVG support. (#2738)

* Added rudimentary SVG support. Added 2 functions ImageLoadSvg and ImageLoadSvgWithSize.

* Added an example on how to use ImageLoadSvgWithSize and adjusted Makefiles accordingly.

* Added actual correct example file.

* Reviewed the code to keep the raylib coding conventions in mind.
Moved the LoadImageSvg() code into LoadImage() guarded by SUPPORT_FILEFORMAT_SVG.
Renamed LoadImageSvgWithSize() to LoadImageSvg().
Added a LoadImageSvgFromString() function to parse the loaded SVG into an actual image. This does the bulk of the work.

* Fixed typo.

---------

Co-authored-by: Ray <[email protected]>

* REVIEWED: `LoadImageSvg()`

* REVIEWED: `LoadImageSvg()`

* Add SUPPORT_FILEFORMAT_SVG to cmake (#3284)

* Fix examples/textures/textures_fog_of_war.c help instructions (#3285)

* Fix examples/textures/textures_image_rotate.c help instructions (#3286)

* Update rtextures.c

* Fix #3247

* Update config.h

* Fix #3293

* Disable UBSAN in zig builds. (#3292)

Zig debug builds automatically enable ubsan.
As the fix for #1891 had to be reverted, debug builds using zig will crash like so:

```
Illegal instruction at address 0x3237d2
raylib/src/rlgl.h:3690:91: 0x3237d2 in rlDrawVertexArrayElements (/home/rcorre/src/raylib-zig-template/raylib/src/rcore.c)
    glDrawElements(GL_TRIANGLES, count, GL_UNSIGNED_SHORT, (const unsigned short *)buffer + offset);
```

This disables UBSAN when using zig to build raylib.

* Update README.md (#3290)

specially -> especially

* Update cmake SUPPORT_FILEFORMAT_SVG default value (#3291)

* Mouse offset and scaling must be considered also on web!

* Update rcore.c

* Update Makefile : clean raygui.c & physac.c (#3296)

* Remove PLATFORM_RPI (#3232)

* Remove PLATFORM_RPI

* remove build artifacts

---------

Co-authored-by: MichaelFiber <[email protected]>
Co-authored-by: Ray <[email protected]>

* Review to avoid UBSAN complaining #1891

* added raylib-raku to bindings (#3299)

* examples: core: adds 2D camera two player split screen (#3298)

* Reviewed examples for consistency

* Update rtext.c

* Some code restructuring for input functions, consistency review

* Remove unneeded #if (#3301)

Co-authored-by: MichaelFiber <[email protected]>

* Revert "Disable UBSAN in zig builds. (#3292)" (#3303)

This reverts commit a316f9e.

Issue #1891 was fixed again, so this is no longer needed.

* rtextures: Fix ImageDraw() source clipping when drawing beyond top left (#3306)

* REVIEWED: `TextToPascal()` issue when first char is uppercase

* Implement FLAG_WINDOW_RESIZABLE for web (#3305)

Fixes #3231

* Update BINDINGS.md (#3307)

Fix Kaylib binding. Reroute to a new repository.
Binding renamed.

* Update webassembly.yml

* Add claw-raylib to BINDINGS.md (#3310)

* Add SetWindowMaxSize for desktop and web (#3309)

* Add SetWindowMaxSize for desktop and web

* Remove SizeInt and respective adjustments

* Update rtextures.c

* Reviewed parameters for consistency

* Rename windowM* to screenM* (#3312)

* Update BINDINGS.md (#3317)

Update TurboRaylib bindings

* Update rmodels.c

* Update BINDINGS.md with vaiorabbit/raylib-bindings (#3318)

* fixed spelling mistake

* put back parenthesis

* reverted major allignment changes

* reverted parser output changes

* reverted one more indentation change

---------

Co-authored-by: Brian-E <[email protected]>
Co-authored-by: Ray <[email protected]>
Co-authored-by: ubkp <[email protected]>
Co-authored-by: ashn <[email protected]>
Co-authored-by: actondev (Christos) <[email protected]>
Co-authored-by: vitopigno <[email protected]>
Co-authored-by: Asdqwe <[email protected]>
Co-authored-by: Jeffery Myers <[email protected]>
Co-authored-by: Ethan Simpson <[email protected]>
Co-authored-by: Nickolas McDonald <[email protected]>
Co-authored-by: Branimir Ričko <[email protected]>
Co-authored-by: iacore <[email protected]>
Co-authored-by: Ethan Conneely <[email protected]>
Co-authored-by: Johannes Barthelmes <[email protected]>
Co-authored-by: bXi <[email protected]>
Co-authored-by: Ryan Roden-Corrent <[email protected]>
Co-authored-by: Ikko Eltociear Ashimine <[email protected]>
Co-authored-by: SuperUserNameMan <[email protected]>
Co-authored-by: MichaelFiber <[email protected]>
Co-authored-by: MichaelFiber <[email protected]>
Co-authored-by: Dan Vu <[email protected]>
Co-authored-by: Gabriel dos Santos Sanches <[email protected]>
Co-authored-by: Rob Loach <[email protected]>
Co-authored-by: Peter0x44 <[email protected]>
Co-authored-by: Kenta <[email protected]>
Co-authored-by: bohonghuang <[email protected]>
Co-authored-by: turborium <[email protected]>
Co-authored-by: Wilson Silva <[email protected]>
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.

None yet

4 participants