-
-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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
[3.x] Improve responsiveness on underpowered Android devices #42220
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! It seems like a good idea. I'll try to give it a test some time this week.
1f0513d
to
d9390d0
Compare
As for event accumulation, do we need it for input events other than InputEventMouseMotion and touch drag events? Most people can't press keys or gamepad buttons more than ~15 times in a second 🙂 (If we decide to change this, it should be done in a future PR.) |
The only other kind of event that may be accumulated is joystick axis movement, but I think that would introduce problems, like when you want to parse a stick sequence for a fighting game. Accumulation there wouldn't be as easy as keeping the most recent values. Therefore, I'd rather keep accumulation limited to pointer-like moves. In addition, just in case I'll clarify that the only thing this PR does to accumulation is enabling it for touch. The point of this PR is getting more chances of having the most recent input when code checking it is about to run and that is orthogonal to accumulation. |
I haven't measured analytically, but I can perceive that my game is now free from some minor few millisecond hiccups that were happening once in a while. I was attributing them to other causes, but with buffering active they seem to be gone. I believe that consuming the input events from Android as they come is smoothing things out. (I've added this as an additional note to the description of this PR.) |
I don't have a test case to specifically check the hiccups you describe but I've just tested on Android (low-end device - Archos 55 Helium) and didn't spot any issue with or without agile input events flushing enabled. |
Cool. Thanks for testing.
|
f25b60c
to
27c462a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me.
It would be nice however to be able to test the issue itself. Is there a way you could provide a minimal project?
This will require a rebase, but I'd like to know if there's a chance it will be accepted before doing the work. |
In think on the general idea its ok, but I don't see why you would want to turn off event accumulation for what comes from the OS, and also you need to leave input_event alone to work without accumulation so the users can throw their custom events, so I suggest just keep accumulate_event and use this instead from the OSs. |
This PR doesn't change how input accumulation works. The ability to enable/disable it was already there. Therefore, at the OS "layer" you still don't know if it's enabled or not. OS feeds the events to the engine just as it did formerly, except that happens in a way that is also compatible with the input buffering this PR adds. |
Also note that this PR, being for 3.2, adds input buffering as an option to avoid changing behavior of current projects. For 4.0 it could be just the builtin behavior in every platform supporting it, to avoid unneeded complexity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RandomShaper Can you clarify why the engine can now handles input directly from the UI thread?
For example there are several fields in os_android
(e.g: hover_prev_pos
) which after this change will be updated from the UI thread but read from the render thread.
@m4gr3d, I'm failing to see what you're pointing out about Members of
Can you elaborate or point me to another case? Maybe there's some confusion because of the pending rebase? |
@RandomShaper There may be a miscommunication; you mention there are As additional example, you update the
With the current approach in the PR, the same would need to be done for all the fields in |
virtual bool is_joy_known(int p_device); | ||
virtual String get_joy_guid(int p_device) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these methods also be moved to the AndroidInputHandler
class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My criteria for moving functions to AndroidInputHandler
has been the following:
- They were not overrides of
OS
's functions, since those are only guaranteed to be safe on the Android render thread (Godot main thread) and therefore don't need a special treatment. - They will be called only from the Android UI thread.
- They only call thread-safe functions.
All the points have turned out to be met at the same time for all the functions of interest.
4a30802
to
6054a1e
Compare
- API has been simplified: all events now go through `parse_input_event()`. Whether they are accumulated or not depends on the `use_accumulated_input` flag. - Event accumulation is now thread-safe (it was not needed so far, but it prepares the ground for the following changes). - Touch drag events now support accumulation.
Input buffering is implicitly used by event accumulation, but this commit makes it more generic so it can be enabled for other uses. For desktop OSs it's currently not feasible given main and UI threads are the same).
If enabled, key/touch/joystick events will be flushed just before every idle and physics frame. Enabling this can greatly improve the responsiveness to input, specially in devices that need to run multiple physics frames per each idle frame, because of not being powerful enough to run at the target frame rate. This will only work for platforms using input buffering (regardless event accumulation). Currenly, only Android does so, but could be implemented for iOS in an upcoming PR.
6054a1e
to
45237b3
Compare
Key, touch and joystick events will be passed directly from the UI thread to Godot, so they can benefit from agile input flushing. As another consequence of this new way of passing events, less Java object are created at runtime (`Runnable`), which is good since the garbage collector needs to run less. `AndroidInputHandler` is introduced to have a smaller cross-thread surface. `main_loop_request_go_back()` is removed in favor just inline calling `notification()` on the `MainLoop` at the most caller's convenience. Lastly, `get_mouse_position()` and `get_mouse_button_state()` now just call through `InputDefault` to avoid the need of sync of mouse data tracked on the UI thread.
45237b3
to
45c2a71
Compare
Rebased and re-tested for no regressions (mouse & keyboard on Windows and touch on Android and iOS). |
@@ -237,6 +244,7 @@ JNIEXPORT void JNICALL Java_org_godotengine_godot_GodotLib_step(JNIEnv *env, jcl | |||
// Since Godot is initialized on the UI thread, _main_thread_id was set to that thread's id, | |||
// but for Godot purposes, the main thread is the one running the game loop | |||
Main::setup2(Thread::get_caller_id()); | |||
input_handler = new AndroidInputHandler(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
step
is usually called on the render thread. For that reason and also for consistency, shouldn't input_handler
be initialized in initialize
which is invoked on the UI thread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. However, I first tried to construct the input handler at Java_org_godotengine_godot_GodotLib_initialize()
. However, it seems that's too soon for InputDefault
to be created and is causing a flood of errors and the game is unable to start. Regarding the fact it's created on the render thread and used from the UI one, shouldn't be a problem, since the step
variable issues memory barriers on increment and comparison. Not that I like that approach much, though, so if you can suggest a good place to move the construction of the input handler to, I'd listen to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Looking at the previous logic, InputDefault
was initialized for OS_Android
in OS_Android::initialize(...)
which itself is called in Main::setup2(...)
, so where you placed the init logic is actually correct.
The only alternative I can think of is to not store a reference to InputDefault
in the class and always just use InputDefault::get_singleton()
instead, but that seems overly bothersome..
I'll leave the decision to you, the current location is fine by me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright then. Let's leave it this way. We can always improve it later; this is likely to be revisited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update looks good! I've added one more feedback, once that's resolved, this should be good to merge!
Input accumulation was implemented and enabled by default in 3.1, and I don't recall major complaints around it (or bugs were fixed). In 3.4, godotengine#42220 added input buffering and apparently toggled input accumulation off by mistake. This led to multiple bug reports about degraded performance on Windows, or simply unexpected behavior change (see linked issues in godotengine#55037). Fixes godotengine#55037.
Input accumulation was implemented and enabled by default in 3.1, and I don't recall major complaints around it (or bugs were fixed). In 3.4, godotengine#42220 added input buffering and apparently toggled input accumulation off by mistake. This led to multiple bug reports about degraded performance on Windows, or simply unexpected behavior change (see linked issues in godotengine#55037). Fixes godotengine#55037.
This PR adds a new project setting to switch to a different way of delivering input events which can greatly improve the responsiveness of games in devices that need to run multiple physics frames per idle frame (and, specially, where the input is handled in the physics frame so that the pace at which the input is taken isn't limited by the visual frame rate).
Before adding that and making it work, some preliminar steps have been taken, and are in separate commits.
Notes:
Summary of commits:
UPDATE: Commit structure modified.
UPDATE: Input handling extracted from Android OS.
1. Improve input event accumulation
parse_input_event()
. Whether they are accumulated or not depends on theuse_accumulated_input
flag.2. Add input buffering framework
Input buffering is implicitly used by event accumulation, but this commit makes it more generic so it can be enabled for other uses.
For desktop OSs it's currently not feasible given main and UI threads are the same).
3. Add project setting for agile input event flushing
If enabled, key/touch/joystick events will be flushed just before every idle and physics frame.
Enabling this can greatly improve the responsiveness to input, specially in devices that need to run multiple physics frames per each idle frame, because of not being powerful enough to run at the target frame rate.
This will only work for platforms using input buffering (regardless event accumulation). Currenly, only Android does so, but could be implemented for iOS in an upcoming PR.
4. Switch to input buffering on Android
Switch to input buffering on Android
Key, touch and joystick events will be passed directly from the UI thread to Godot, so they can benefit from agile input flushing.
As another consequence of this new way of passing events, less Java object are created at runtime (
Runnable
), which is good since the garbage collector needs to run less.AndroidInputHandler
is introduced to have a smaller cross-thread surface.main_loop_request_go_back()
is removed in favor just inline callingnotification()
on theMainLoop
at the most caller's convenience.Lastly,
get_mouse_position()
andget_mouse_button_state()
now just call throughInputDefault
to avoid the need of sync of mouse data tracked on the UI thread.