AnimatedTexture: Fix crash when loaded from a thread #93340
Conversation
| AnimatedTexture::AnimatedTexture() { | ||
| //proxy = RS::get_singleton()->texture_create(); | ||
| proxy_ph = RS::get_singleton()->texture_2d_placeholder_create(); | ||
| proxy = RS::get_singleton()->texture_proxy_create(proxy_ph); | ||
|
|
||
| RenderingServer::get_singleton()->texture_set_force_redraw_if_visible(proxy, true); | ||
| RenderingServer::get_singleton()->connect("frame_pre_draw", callable_mp(this, &AnimatedTexture::_update_proxy)); | ||
|
|
||
| MessageQueue::get_main_singleton()->push_callable(callable_mp(this, &AnimatedTexture::_finish_non_thread_safe_setup)); |
There was a problem hiding this comment.
Wouldn't using the CONNECT_DEFERRED flag achieve something similar without having to introduce a new method?
There was a problem hiding this comment.
Good question, but this is another case of the difference between the main call queue and the thread override call queue is relevant. CONNECT_DEFERRED would use MessageQueue::get_singleton(), which during threaded loading wouldn't be the good old default MessageQueue, but the override the ResourceLoader has set for the loading thread. We need to bypass it.
Maybe we could consider making that a bit more transparent. However, as I see things, we are in a de facto stage of colllection of information about the needs of resources dealing with a multi-threaded world and we're not ready yet to come up with solid API designs that are better than the manual approaches.
There was a problem hiding this comment.
Makes sense, thanks for the details!
|
Thanks! |
Signals (like most, or all?, of the
ObjectAPI) is not thread-safe.AnimatedTexturetries to connect toRenderingServer's singalframe_pre_drawin the constructor. The issue is that such constructor may be running on any thread.An alternative solution would be to make all the signal API in
Objectvirtual so thatRenderingServercan wrap all the functions with theserver_wrap_mt_common.hutilities. However, that would harm performance in every other case.In conclusion, it's OK to consider the specification for writing resource classes to include it should only deal with thread-safe APIs.