Fix the cleanup logic for the Android render thread #94661
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
unfortunately not yet (on Samsung Tab S7, Android 13) with 1 sec delayError ResourceFormatSaverTextInstance::save(const String &p_path, const Ref<Resource> &p_resource, uint32_t p_flags) {
if (p_path.ends_with(".tscn")) {
packed_scene = p_resource;
}
Error err;
Ref<FileAccess> f = FileAccess::open(p_path, FileAccess::WRITE, &err);
ERR_FAIL_COND_V_MSG(err, ERR_CANT_OPEN, "Cannot save file '" + p_path + "'.");
Ref<FileAccess> _fref(f);
print_line("BEGIN DELAY: ", OS::get_singleton()->get_ticks_usec());
OS::get_singleton()->delay_usec(1000000); // 1 000 000 = 1 sec
print_line("END DELAY: ", OS::get_singleton()->get_ticks_usec());
local_path = ProjectSettings::get_singleton()->localize_path(p_path);Logcat: The editor does not crash due to bugfix #94593. The editor loads the default settings. I have switched to English, but then it switches to the default “German”, my system language. Test without PR #94593 singleton = ResourceLoader::load(config_file_path, "EditorSettings");
//before PR #94593
singleton->set_path(get_newest_settings_path()); // Settings can be loaded from older version file, so make sure it's newest.
if (singleton.is_null()) {
ERR_PRINT("Could not load editor settings from path: " + config_file_path);
//PR #94593
//config_file_path = get_newest_settings_path();
goto fail;
}
//PR #94593
//singleton->set_path(get_newest_settings_path()); // Settings can be loaded from older version file, so make sure it's newest.
singleton->save_changed_setting = true;Crash: Empty file:
|
|
Notes: I can't see on Android: (Dev-Build) from with print_line("BEGIN DELAY: ", OS::get_singleton()->get_ticks_usec());
OS::get_singleton()->delay_usec(50000); // 1 000 000 = 1 sec
print_line("END DELAY: ", OS::get_singleton()->get_ticks_usec());If |
m4gr3d
force-pushed
the
fix_android_render_thread_cleanup
branch
from
3f8dc42 to
d27514f
Compare
On Android the exit logic goes through `Godot#onDestroy()` who attempts to cleanup the engine using the following code:
```
runOnRenderThread {
GodotLib.ondestroy()
forceQuit()
}
```
The issue however is that by the time we ran this code, the render thread has already been paused (but not yet destroyed), and thus `GodotLib.ondestroy()` and `forceQuit()` which are scheduled on the render thread are not executed.
To address this, we instead explicitly request the render thread to exit and block until it does. As part of it exit logic, the render thread has been updated to properly destroy and clean the native instance of the Godot engine, resolving the issue.
m4gr3d
force-pushed
the
fix_android_render_thread_cleanup
branch
from
d27514f to
4d0da74
Compare
|
Thanks for testing @Alex2782! I made some additional changes which should address the issue. I was able to confirm by replicating your test - adding a delay to the saving logic - and I'm no longer able to repro the issue. Can you take another look. |
|
Looks great! Tested with up to 10 seconds delay (no ANRs) and |
akien-mga
approved these changes
Jul 24, 2024
|
Thanks! |
This was referenced Jul 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On Android the exit logic goes through
Godot#onDestroy()who attempts to cleanup the engine using the following code:The issue however is that by the time we ran this code, the render thread has already been paused (but not yet destroyed), and thus
GodotLib.ondestroy()andforceQuit()which are scheduled on the render thread queue, are not pulled out off the queue and not executed.To address this, we instead explicitly request the render thread to exit and block until it does. As part of it exit logic, the render thread has been updated to properly destroy and clean the native instance of the Godot engine, resolving the issue.
Fixes #93826