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

Undocumented thread (un)safety #20964

Open
lupoDharkael opened this issue Aug 13, 2018 · 4 comments
Open

Undocumented thread (un)safety #20964

lupoDharkael opened this issue Aug 13, 2018 · 4 comments

Comments

@lupoDharkael
Copy link
Contributor

As I commented in #18714 we have some basic concepts of thead safety documented in the docs.
I've been working on a procedural generator and trying to speed-up the generation. I decided to use multithreading but I have several doubts/problems.
context: I'm working using C++ GDN.

  1. I'm using std::thread and OpenMP pragmas, does it have implications compared to the use of godot::Thread in relation to interacting with the engine API?

  2. I have the following code to generate some meshes:

Spatial *meshes_node = new Spatial;
//#pragma omp parallel for
for (int i = 0; i < meshDataArray.size(); i++) {
	const Array &arr = meshDataArray[i];
	MeshInstance *meshInst = new MeshInstance;
	meshInst->set_material_override(material);
	
	Ref<ArrayMesh> mesh_ref(new ArrayMesh);
	mesh_ref->add_surface_from_arrays(Mesh::PRIMITIVE_TRIANGLES, arr);
	meshInst->set_mesh(mesh_ref);
	meshes_node->add_child(meshInst);
	meshInst->add_to_group(TERRAIN_MESH_GROUP_NAME);
	
}
m_world->root_node->add_child(meshes_node);

If I uncomment the #pragma it only generates the meshes processed by one thread. That's the reason of that? Do I have to generate it only with the main thread? Can that behaviour be circumvented using Nodes or the VisualServer?

  1. Other code example, in this case is for collision generation:
Spatial *collisions_node = new Spatial;
//#pragma omp parallel for
for (int i = 0; i < meshDataArray.size(); i++) {
	const Array &arr = meshDataArray[i];
	Ref<ConcavePolygonShape> shape(new ConcavePolygonShape);
	shape->set_faces(arr[Mesh::ARRAY_VERTEX]);
	StaticBody *body = new StaticBody;
	int owner_id = body->create_shape_owner(body);
	body->shape_owner_add_shape(owner_id, shape);
	collisions_node->add_child(body);
	body->add_to_group(BODIES_GROUP_NAME);
}
m_world->root_node->add_child(collisions_node);

After removing the comment in the pragma te generation of concave collisions is way faster (without multithreading takes some seconds to complete) but the generation is unrealiable, it sometimes finishes and sometimes crashes with the following error:

ERROR: get: Condition ' !id_map.has(p_rid.get_data()) ' is true. returned: __null
At: core/rid.h:155.
ERROR: shape_set_data: Condition ' !shape ' is true.
At: modules/bullet/bullet_physics_server.cpp:146.
ERROR: get: Condition ' !id_map.has(p_rid.get_data()) ' is true. returned: __null
At: core/rid.h:155.
ERROR: body_add_shape: Condition ' !shape ' is true.
At: modules/bullet/bullet_physics_server.cpp:503.
handle_crash: Program crashed with signal 11

What's the cause of this error? Do I need to use the PhysicsServer in order to have a safe concurrent access?

This class is being loaded as a Singleton and the docs say it's safe to access the server in that case. What about nodes as in my examples?

@Zylann
Copy link
Contributor

Zylann commented Aug 13, 2018

I wonder if there is an option to check in project settings for servers to be truly thread-safe?

@lupoDharkael
Copy link
Contributor Author

I've found the answer to the second point, the project settings contains Rendering/Thread/thread_model which is defined as Single-Safe by default, that's why trying to instance from multiple threads isn't working.

There is a problem: mesh_ref->add_surface_from_arrays(Mesh::PRIMITIVE_TRIANGLES, arr); seems to require synchronization, adding #pragma omp critical to prevent misbehaviour kills the performance gain of using multiple threads.

@Calinou
Copy link
Member

Calinou commented Feb 27, 2021

I wonder if there is an option to check in project settings for servers to be truly thread-safe?

This should be addressed out of the box in 4.0 thanks to #45852.

@snouk-z
Copy link

snouk-z commented Feb 27, 2021

I got this error in a similar context, I'm constructing ArrayMeshes and Shapes in several threads and I send them to the main thread which take care of the scene tree. (I use the Mono Task library to spawn my threads).

Sometimes things work flawlessly, sometimes I got the exact same errors as OP in my terminal and sometimes the game crashes. I tried to set the rendering in "Multi-threaded" but it doesn't seem to work.

Until now I admitted that nodes and resources instantiation in parallel threads is simply not possible in Godot. But is that really the case ? The doc about the thread-safe API seems to be telling something different about resources :

Modifying a unique resource from multiple threads is not supported. However handling references on multiple threads is supported

Can someone explain the exact mechanics that happen under the hood and cause these "casual" errors and crashes ? Thanks.

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

No branches or pull requests

4 participants