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

Change some keys to be triggered once every key press #13883

Merged
merged 1 commit into from Oct 21, 2023

Conversation

srifqi
Copy link
Member

@srifqi srifqi commented Oct 11, 2023

Goal of the PR
This PR tries to make some keys to be triggered once every key press (using wasKeyPressed()).
Those keys are listed below:

  • KeyType::CAMERA_MODE
  • KeyType::SCREENSHOT
  • KeyType::TOGGLE_BLOCK_BOUNDS
  • KeyType::TOGGLE_HUD
  • KeyType::MINIMAP
  • KeyType::TOGGLE_CHAT
  • KeyType::TOGGLE_FOG
  • KeyType::TOGGLE_DEBUG
  • KeyType::TOGGLE_PROFILER
  • KeyType::RANGESELECT

How does the PR work?
This PR changes the key detection from wasKeyDown() to wasKeyPressed() for the keys listed above. The list contains keys that might cause sudden graphical changes (or creating new image files for KeyType::SCREENSHOT) when being pressed down continuously.

Does it resolve any reported issue?
Yes, this PR tries to fix #13868 (and also some of #11900).

Does this relate to a goal in the roadmap?
Yes, this PR tries to create a better UI/UX.

To do

This PR is Ready for Review.

How to test

  1. Play in a Minetest world.
  2. Press down (and hold) the toggle camera mode key.
  3. The action is triggered once.
  4. Release the key.
  5. Repeat step 2–4 with the rest: take screenshot, toggle block bounds, toggle HUD, toggle minimap, toggle chat, toggle fog, toggle debug, toggle profiler, and toggle (maximum) range select.

@srifqi srifqi added Feature ✨ PRs that add or enhance a feature @ Client / Controls / Input UI/UX labels Oct 11, 2023
@ghost
Copy link

ghost commented Oct 12, 2023

Darn, I liked holding down the camera button and watching the headache mode video 🤣.

Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that "no flashing" is an improvement, but the delay introduced by triggering on release instead of on press feels weird for me. If we do this, I suggest doing it for all "toggles" to make it more consistent and thus easier to get used to.

A different solution that wouldn't introduce a delay would be ignoring repeated press events, i.e. ignoring all press events except the first one until the next release event.

@srifqi
Copy link
Member Author

srifqi commented Oct 17, 2023

I suggest doing it for all "toggles" to make it more consistent and thus easier to get used to.

I was going to do that, but I do not have enough reasons. That might be a good reason to do it because your alternative solution is harder (at least for me) to implement.

@grorp
Copy link
Member

grorp commented Oct 17, 2023

I just found out that my alternative solution is trivial to implement: Just replace wasKeyDown with wasKeyPressed. I suggest doing this, then it also isn't as important anymore to do it for all keys.

More keys where using wasKeyPressed would be good:

  • SCREENSHOT
  • TOGGLE_BLOCK_BOUNDS
  • TOGGLE_CHAT
  • TOGGLE_DEBUG
  • TOGGLE_PROFILER

@srifqi
Copy link
Member Author

srifqi commented Oct 18, 2023

Thank you! TIL about how wasKeyPressed() works (a little). I changed it to wasKeyPressed() and added more keys from your list.

@srifqi srifqi changed the title Change some toggles to be triggered on key release instead Change some keys to be triggered once every key press Oct 18, 2023
Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, let's see what users think about this (will they notice?). We can always do this for more keys later, but the ones changed by this PR are the most annoying ones.

Those keys are below:
- KeyType::CAMERA_MODE
- KeyType::SCREENSHOT
- KeyType::TOGGLE_BLOCK_BOUNDS
- KeyType::TOGGLE_HUD
- KeyType::MINIMAP
- KeyType::TOGGLE_CHAT
- KeyType::TOGGLE_FOG
- KeyType::TOGGLE_DEBUG
- KeyType::TOGGLE_PROFILER
- KeyType::RANGESELECT


Co-authored-by: Gregor Parzefall <82708541+grorp@users.noreply.github.com>
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike the function name may indicate, the action is triggered right after pressing down the key. No delay, no objections. Works on my machine.

@srifqi srifqi merged commit c9655e5 into minetest:master Oct 21, 2023
13 checks passed
@srifqi srifqi deleted the toggle_on_key_release branch December 1, 2023 17:00
kawogi pushed a commit to kawogi/minetest that referenced this pull request Dec 19, 2023
Those keys are below:
- KeyType::CAMERA_MODE
- KeyType::SCREENSHOT
- KeyType::TOGGLE_BLOCK_BOUNDS
- KeyType::TOGGLE_HUD
- KeyType::MINIMAP
- KeyType::TOGGLE_CHAT
- KeyType::TOGGLE_FOG
- KeyType::TOGGLE_DEBUG
- KeyType::TOGGLE_PROFILER
- KeyType::RANGESELECT


Co-authored-by: Gregor Parzefall <82708541+grorp@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid epilepsy when keeping the camera key pressed
3 participants