Abort threaded preview generators on exit #84716
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
YuriSizov
force-pushed
the
3d-resource-previews-are-out-to-get-you
branch
from
f3b30a0
to
ef49902
Compare
YuriSizov
force-pushed
the
3d-resource-previews-are-out-to-get-you
branch
from
ef49902
to
e90ea87
Compare
|
Note about testing: make sure to delete your resource preview cache in the editor data folder between runs ( |
akien-mga
approved these changes
Nov 10, 2023
There was a problem hiding this comment.
Did some testing with the GDQuest 4.0 new features showcase, and Kenney's Starter Kit 3D Platformer.
This seems to fix the deadlock I was experiencing on those projects.
Rough testing procedure:
- Delete .godot and editor cache
- Open in 4.1.3 to import everything
- Open in 4.2-beta5, trigger upgrade, see deadlock
- Exit, clean changes to git history
- Repeat all steps with a build including this PR instead of 4.2-beta5
|
Thanks! |
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.
See #84200 (comment).
Basically, after the reimport is done we start to generate previews. This is a non-blocking operation and it is out of our control (as in, we don't know how much work it needs to do or when it will be done).
As such, we can't really wait for it to finish, so our only hope is that it correctly aborts on exiting the editor. Which it doesn't. As far as I can tell, it should handle exiting correctly, but there are so many threads and mutexes and semaphores involved, everything and anything can go wrong.
I managed to identify a couple of problems, which this PR addresses. First, we avoid restarting the editor directly from the surface upgrade tool. This is likely needed, because this dialog can appear when we try to load a scene or a mesh resource, and if in turn we trigger the editor restart from it, this may bring us back to resource loaders. And possibly create a lock.
Another issue is that some preview generators are threaded in their own right, on top of the EditorResourcePreview being threaded. These have their own semaphores and checks and they await for the next rendered frame from the RenderingServer. Which never happens at this stage, no matter how many times we do the sync. So I added an explicit abort operation that unlocks the semaphore immediately. It's crudely done, but it seems to resolve the issues and I can at least import Kenney's starter project in Godot 4.2 as expected.
While investigating, I made a couple of clean up changes. I've removed an unimplemented preview generator, renamed some variables for clarity, fixed order mistakes, and flattened for readability
EditorResourcePreview::_iterate(should lead to no logical changes). Make sure to compare changes without whitespaces.