-
-
Notifications
You must be signed in to change notification settings - Fork 21.9k
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
Prevent off-thread errors when generating resource previews for animation nodes #102692
Prevent off-thread errors when generating resource previews for animation nodes #102692
Conversation
Vector<String> names = get_editable_animation_list(); | ||
Vector<String> names; | ||
// Prevents off-main-thread errors when generating resource previews. | ||
if (is_current_thread_safe_for_nodes()) { |
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.
This is part of engine internals and AFAIK not really meant to be used throughout the codebase. You can see that current usage of this API is limited to core classes: Node and WorkerThreadPool.
So the problem might need a different solution that doesn't make wrong assumptions on thread safety.
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.
Ah I didn't notice that. It makes sense why you wouldn't want random non-warning thread checks. The only small obvious other thing I can think of is if get_animation_list
doesn't need to check !singleton->is_visible()
- it seems like the rest of the method doesn't rely on it as a soft thread check but it does short circuit calls in some events such as reassigning the root node of the tree.
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.
get_editable_animation_list()
is meant to be called only from the main thread because it boils down to a function in some editor plugin (and editor APIs are in general meant to be used from the main thread only). Instead of is_current_thread_safe_for_nodes()
,, you want to use Thread::is_main_thread()
. The former worls because the main thread happens to be safe-for-nodes, but both checks aren't conceptually (nor practically) equivalent.
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.
Oh ok, I'll update to use that. I originally had that there but wasn't sure if other threads could be marked as safe, and that thread local marker is ultimately what is_visible
uses (although I'm unsure why I didn't submit this with the exact same call the guard does)
efe1840
to
c82c5b0
Compare
Thanks! |
Thanks for the quick reviews! It's probably still not ideal having this thread check instead of the other thread check but in the meantime I'll appreciate having fewer errors in console. If there's a suggestion for a better way to do it I'm open to changing this again. |
Fixes #102106
This check could be moved to
AnimationTreeEditor::get_animation_list
if that's preferred.