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

Input - prevent action missed input on Android #83438

Closed

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Oct 16, 2023

On Android, if input came in on a tick or frame after the physics_process or process, it could be missed when relying on is_action_just_pressed(). This is because by the time the action got around to being checked, the action's tick or frame would have passed.

This PR changes the timestamp for the action to be for the next tick or next frame (preventing missed input), but only immediately prior to process and physics_process, in order to minimize latency.

Alternative to #83301
Fixes #66318

Discussion

There's likely several ways of fixing this (and different ways of doing the sync, e.g. MainLoop versus Main::iteration() versus SceneTree etc). There might be some better way of protecting people making custom MainLoop - welcome suggestions here, I've just used some DEV_ASSERTS here, which (rightly or wrongly) is assuming they are building from source, as a custom MainLoop seems to be here there be dragons territory. Custom MainLoop is really difficult in general once you get down to the subtleties of timing I guess.

I was originally unsure about #83301 but it's actually not a bad approach, as essentially we want to pass any input that is occurring after the "read point" to the next tick / frame. But the latency is a bit of a downside, especially for input that could unnecessarily be shifted to the next tick which might be on a later frame.

I first experimented with storing a read tick on each action, such that if it had not been read so far on that tick, the registered tick would be placed on the current tick, but if it had been read, it would be tick+1. This would make latency pretty minimal, but I wasn't keen on the haphazard / stochastic nature, so am now of the opinion a fixed sync point might be better.

Here I've placed the sync point just before the calls to physics_process and process. It may not gain a massive amount over just using tick+1 everywhere (particularly for idle frame), but it seems slightly better for reducing latency, which is something we should strive for.

Notes

  • Fix Android logic for deferred window input events being inverted #83301 has the downside that it seems likely to unnecessarily introduced extra latency on non-Android platforms, whereas this PR should not have effects on other platforms.
  • Compared to Fix Android logic for deferred window input events being inverted #83301 this additionally fixes input on the frame as well as physics tick, as the frame also seems to share the vulnerability (although may occur less in practice).
  • This area can probably be improved in future if we proceed with Timestamped input buffering - prevent stalling and improve timing #77062, as that makes the input timing slicing a bit more sensible. In fact that PR may also already fix the problem, it's been a while since I worked on it. However these PRs are good for interim fix.
  • We discuss Android here, but really the problem is common to platforms that have separate thread for input. That may be more platforms in future.
  • I also haven't tested this yet, it would be great to get some confirmation it works (the theory is sound).
  • input_curr_tick and input_curr_process_frame could possibly be atomic, I'll defer to @RandomShaper 's opinion on this.

On Android, if input came in on a tick or frame after the physics_process or process, it could be missed when relying on is_action_just_pressed(). This is because by the time the action got around to being checked, the tick or frame would have passed.

This PR changes the timestamp for the action to be for the next tick or next frame (preventing missed input), but only immediately prior to process and physics_process, in order to minimize latency.
@Alex2782
Copy link
Contributor

I will extend my test project to measure the input latency. After that I will try this PR as well.

My guess was on the queue_redraw function, maybe the latency can be reduced further?

I think if texture_pressed is not set, then redraw is not necessary. On small smartphone displays, you do not notice much of the visual status changes when your fingers are on the buttons. Haptic feedback is usually better.

Comment on lines +105 to +106
uint64_t input_curr_tick = 0;
uint64_t input_curr_process_frame = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about using naming similar to Engine and Action: input_physics_frame and input_process_frame?

Copy link
Member Author

Choose a reason for hiding this comment

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

The terminology of tick and physics_frame has been discussed before, I won't rehash here.

Upshot was to move over to the tick terminology wherever possible. See for example the renaming to project setting physics/common/physics_ticks_per_second.

This is used internally in timing code in a number of places, but it may take some time to move existing terminology, and authors often copy terminology from existing code, and there is backward compatibility to think about, so there may be some inconsistencies.

@Calinou Calinou added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 16, 2023
@Alex2782
Copy link
Contributor

test projekt: test.zip

all actions are captured 👍
for lower input delays physics_ticks_per_second can be increased

PR_83438_Screen_recording_20231016_233508.mp4

@Alex2782

This comment was marked as off-topic.

@lawnjelly

This comment was marked as off-topic.

@Alex2782

This comment was marked as off-topic.

@lawnjelly

This comment was marked as off-topic.

@lawnjelly
Copy link
Member Author

Closing this for now as it appears the bug in #83301 may have been responsible for #66318 (and explains why it only was reported on 4.x and not in 3.x).

On Android currently buffered input should always be used so providing the buffer is flushed before physics_process() then everything should work fine. This PR effectively adds an extra safety net, allowing the system to work if _parse_input_event_impl() is called not from the main thread, but it may not be required after all in the end. 👍

@lawnjelly lawnjelly closed this Oct 24, 2023
@akien-mga akien-mga removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
@AThousandShips AThousandShips removed this from the 4.x milestone Oct 24, 2023
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.

Input.action_just_pressed and Input.action_just_released doesnt work in _physics_process on Android
6 participants