Skip to content
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

Move _scene_particles_set_view_axis to new static function to allow call to be done on render thread, preventing multi threaded error on compute shader execution. #99299

Merged

Conversation

Bonkahe
Copy link
Contributor

@Bonkahe Bonkahe commented Nov 15, 2024

Resolves #99025 by calling the function particles_set_view_axis on the render thread.
The error would occur when the number of instances in the tree went over 1k, resulting in the thread_cull_threshold being exceeded, this resulted in the multi-threaded culling method being called for particle effects, this then subsequently called particles_set_view_axis which was now being called from a thread other than the render thread, and would immediately throw errors for trying to execute a compute shader on any thread other than the render thread (this only occurred either the draw_order or transform_align settings were changed on a GPUParticles3D).

I initially was going to just call it directly using a callable_mp style call, but it would not allow it because it's calling it on a singleton, so what I did was create a static function within RendererSceneCull to call the function same as before, then called that static function using call_on_render_thread and passing it a callable_mp_static constructor.
If there is a way to do it in a single line that would be preferable, but I had no small amount of issues trying to get that work, and couldn't find any examples elsewhere of that functionality to reference.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not super familiar with the callable syntax. This seems like the right workaround if we can't create a callable in this situation

@KoBeWi Is it correct that we won't be able to create a callable that calls RSG::particles_storage->particles_set_view_axis() from renderer_scene_cull?

@KoBeWi
Copy link
Member

KoBeWi commented Nov 16, 2024

I don't see why the Callable couldn't be created.
Try callable_mp(RSG::particles_storage, &RendererParticlesStorage::particles_set_view_axis).

@Bonkahe
Copy link
Contributor Author

Bonkahe commented Nov 16, 2024

I don't see why the Callable couldn't be created. Try callable_mp(RSG::particles_storage, &RendererParticlesStorage::particles_set_view_axis).

I tried that, results in this error:

.\core/object/callable_method_pointer.h(116): error C2039: 'get_instance_id': is not a member of 'RendererParticlesStorage' .\servers/rendering/storage/particles_storage.h(36): note: see declaration of 'RendererParticlesStorage' .\core/object/callable_method_pointer.h(116): note: the template instantiation context (the oldest one first) is servers\rendering\renderer_scene_cull.cpp(3022): note: see reference to function template instantiation 'Callable create_custom_callable_function_pointer<RendererParticlesStorage,RID,const Vector3&,const Vector3&>(T *,const char *,void (__cdecl RendererParticlesStorage::* )(RID,const Vector3 &,const Vector3 &))' being compiled with [ T=RendererParticlesStorage ] .\core/object/callable_method_pointer.h(129): note: see reference to class template instantiation 'CallableCustomMethodPointer<RendererParticlesStorage,void,RID,const Vector3 &,const Vector3 &>' being compiled .\core/object/callable_method_pointer.h(113): note: while compiling class template member function 'CallableCustomMethodPointer<RendererParticlesStorage,void,RID,const Vector3 &,const Vector3 &>::CallableCustomMethodPointer(T *,R (__cdecl RendererParticlesStorage::* )(RID,const Vector3 &,const Vector3 &))' with [ T=RendererParticlesStorage, R=void ] .\core/object/callable_method_pointer.h(129): note: see the first reference to 'CallableCustomMethodPointer<RendererParticlesStorage,void,RID,const Vector3 &,const Vector3 &>::CallableCustomMethodPointer' in 'create_custom_callable_function_pointer'

@AThousandShips AThousandShips changed the title Moved _scene_particles_set_view_axis to new static function to allow call to be done on render thread, preventing multi threaded error on compute shader execution. Move _scene_particles_set_view_axis to new static function to allow call to be done on render thread, preventing multi threaded error on compute shader execution. Nov 16, 2024
@ydeltastar
Copy link
Contributor

ydeltastar commented Nov 16, 2024

The implementation is correct because non-static callables can only be used with an Object but RendererParticlesStorage is a plain class.
It needs to run format to fix CI.

@Bonkahe Bonkahe force-pushed the ResolveParticlesSetViewAxisThreadIssue branch from 33f411c to 05b79be Compare November 16, 2024 19:23
@Bonkahe Bonkahe force-pushed the ResolveParticlesSetViewAxisThreadIssue branch from 05b79be to 7e3d480 Compare November 16, 2024 19:26
@Bonkahe
Copy link
Contributor Author

Bonkahe commented Nov 16, 2024

The implementation is correct because non-static callables can only be used with an Object but RendererParticlesStorage is a plain class. It needs to run format to fix CI.

I am having an absolute nightmare trying to get clang to make any sense, but I think I may have gotten it sorted, I will see when the workflow runs.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

The syntax to make callables work kinda sucks, but its not your fault and this is the correct way to fix things, so oh well

@Repiteo Repiteo merged commit 7a5b1ed into godotengine:master Nov 18, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 18, 2024

Thanks! Congratulations on your first contribution! 🎉

Or, err, second contribution? Got in two back-to-back, gotdayum

@Bonkahe
Copy link
Contributor Author

Bonkahe commented Nov 18, 2024

Thanks! Congratulations on your first contribution! 🎉

Or, err, second contribution? Got in two back-to-back, gotdayum

Lol Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[4.4 dev4] compute_list render thread error
5 participants