The physics server is not thread-safe

[mihe]

Unlike Godot's default physics server, which gets wrapped in PhysicsServer3DWrapMT to buffer and flush commands coming in from other threads, there is currently no support in Godot Jolt for calling the physics server from anything but the main thread.

There are different ways this can be solved. Jolt itself has excellent support for interacting with its physics system from multiple threads, but sadly the code in Godot Jolt does not at the moment. The most realistic near-term solution to this is to take a similar approach to Godot Physics and wrap the physics server in another proxy server that buffers and flushes commands in a similar manner.

This also opens up for support of things like the project setting physics/3d/run_on_separate_thread.

[jrouwe]

Hello,

In godotengine/godot-proposals#7308 (comment) you mention that you plan to do this in the same way as godot does it by creating an interface like PhysicsServer3DWrapMT. I would say that with the locking interfaces that Jolt provides, it should be possible to create a far more performant version and I wouldn't expect this to be a lot of work. Is there a reason why you think this is hard?

[mihe]

To start with, I don't have a ton of experience with parallel programming, outside of your average mutex/atomic.

I would certainly much rather rely on Jolt's existing locking. If it was just about acting as a middle-man between Godot's API and the actual Jolt bodies then most of the work would have already been done for me, but considering the amount of my own mutable state in just JoltBodyImpl3D alone I have a hard time reasoning about multiple writers interacting with all this stuff.

As an example, JoltObjectImpl3D (bodies/areas) can exist in "non-initialized" form if you in Godot programmatically create a RigidBody3D or Area3D but don't actually attach it to the scene tree/root. In this case there's no actual JPH::PhysicsSystem to associate it with, so there's no JPH::Body, which leaves me with having to manipulate JPH::BodyCreationSettings instead, which obviously circumvents any locking that Jolt offers. This then gets used and destroyed once the body does get attached to the scene tree, which associates the body with what Godot calls a "physics space", which is what contains the actual JPH::PhysicsSystem.

Obviously that example isn't an insurmountable obstacle, but trying to imagine all the different ins-and-outs of this machinery is difficult for me. On top of that I typically find that multi-threading bugs manifest themselves in very random/non-deterministic ways that can be frustrating or downright impossible to debug, so this "wall of synchronization" that PhysicsServer3DWrapMT offers sounds pretty appealing.

With all that said, seeing as how JoltPhysicsServer3D acts as the gateway for literally the entire API that can be accessed from multiple threads, and considering that the physics server also deals with its objects in terms of IDs similar to Jolt, maybe it's possible to take a similar to approach to what you did with JPH::BodyManager and JPH::MutexArray. I suppose that would effectively render much of Jolt's own locking redundant though.

Anyway, that probably came off more defensive than I meant for it to sound. I just don't think I can pull this off by myself in a way that wouldn't make me lose sleep at night. If you can see some way of easily accommodating for this then I'm all ears, of course.

[jrouwe]

Ok, you convinced me :)

Let's first make a working version, then we can always decide to make it better later.

[Saul2022]

Agreed though i kind of think the safest and performant approach could be using jolt internal stuff, as it was designed for high multithreading.

[RealMadvicius]

Is there documentation for Jolt and Godot like UML graphs ( state, activity, sequence, etc ) regarding physics engine that would help anyone wanting to contribute, to better understand what is what, how it works and how they could be interconnected in an efficient way ?

[mihe]

Not that I'm aware of. You have this for Godot and for Jolt I guess you have the "Simulation Step in Detail" section in this document. Neither of them will be of much help with regards to contributing to this particular issue though.

The work required for this issue is largely "just" about recreating something similar to PhysicsServer3DWrapMT and CommandQueueMT found in Godot, and then wrapping this in that wrapper. It's a substantial chunk of code though, and one that I'm not super excited about just copy-pasting verbatim.

I've also toyed with the idea of making a pull request for Godot to allow other physics servers to leverage the existing PhysicsServer3DWrapMT, the changes of which you can find here. It would need to be opt-in from the physics server's point-of-view though, but that could be as simple as adding something like a needs_mt_wrapper() method to it I guess.

I would frankly prefer to be in control of whatever solution is used though, which is why I haven't chased this down, but it could very well be the more pragmatic solution here. Getting something like that in for Godot 4.2 at this point in the release cycle seems optimistic though, meaning we probably wouldn't see it until Godot 4.3 releases some time around March anyway, at which point I'm sure we'll have this sorted out anyway.

EDIT: Thinking about it some more, relying on the existing PhysicsServer3DWrapMT would mean that any additional physics server methods, like the ones added for the substitute joints, wouldn't be covered by the wrapper. So that's something to consider as well I guess.

[RealMadvicius]

As a side question, would making the physics server threadsafe make it also capable of doing the same kind of processing as with Unity Dots/ECS + jobs, etc regarding physics ?

[mihe]

I'm not very familiar with how DOTS works, so I can't speak to any specific discrepancies, but from what I understand PhysicsServer3DWrapMT aims to mostly fulfill the safety part of thread-safety rather than allowing for massive multi-processing. It effectively just adds a giant queue around the physics implementation. So if you're writing a bunch of stuff in parallel, then you should see some benefit from it I guess, since there's no need to block anything if you're just appending to the queue, but if you're reading stuff then I believe it will block the thread until it flushes the queue at the next physics tick.

However, many of the interesting things you'd probably want to do from separate threads, like messing with a body's state or performing physics queries, can only be done through PhysicsDirectBodyState3D and PhysicsDirectSpaceState3D, which are typically meant to only be accessed at a specific point in the game loop and are not guaranteed to be thread-safe.

None of these limitations are inherent to the API though, and could be addressed by employing more fine-grained locking at an implementation level, but that's a scary task that I'm hoping to avoid by utilizing something like PhysicsServer3DWrapMT.

[tracefree]

I would frankly prefer to be in control of whatever solution is used though, which is why I haven't chased this down, but it could very well be the more pragmatic solution here. Getting something like that in for Godot 4.2 at this point in the release cycle seems optimistic though, meaning we probably wouldn't see it until Godot 4.3 releases some time around March anyway, at which point I'm sure we'll have this sorted out anyway.

Hi! May I ask what the status of this is? Have the required changes to Godot been merged, or is there an issue on the Godot side that can be tracked?

Also, I have discovered that when loading a scene containing physics bodies using ResourceLoader.load_threaded_request with use_sub_threads = true, Godot Jolt will randomly fail to generate collision shapes (parameter "shape" == null errors) when instantiating the PackedScene. Do you think this is related to missing thread-safety and will be fixed automatically once it has been implemented, or should I open a separate issue for this?

[mihe]

May I ask what the status of this is?

I haven't worked on this at all yet, and I don't currently have any plans to.

It's looking likely that this extension might get merged into Godot as an official module in a (hopefully) not too distant future, in which case thread-safety largely comes for free through Godot's own PhysicsServer3DWrapMT.

Do you think this is related to missing thread-safety and will be fixed automatically once it has been implemented, or should I open a separate issue for this?

It's almost certainly related. No part of the API is thread-safe and will result in errors/crashes/corruption when used from multiple threads at the same time, which I would imagine includes use_sub_threads.

[Zylann]

I would like to add a note about one aspect of threading that is a bit different from what Godot does with its MT wrapper. I raised this to mihe a while ago, but didn't post it anywhere:

TLDR: I need to create mesh collision shape resources safely in my threads, and not have their baking be delayed to the last moment on the main thread. This is mostly independent of the physics server being multi-threaded or not. See also this older proposal in which I mention this.

---

When generic collision support is enabled, my voxel terrain project requires to bake mesh colliders at runtime and frequently. Chunks get meshed in multiple threads my module spawns in its own pool to handle prioritised chunk tasks that can last longer than a frame. This shouldn't require to simulate physics on different threads, but it is very much desired that the creation of shapes can happen in these threads, because baking mesh colliders is even more expensive than the meshes themselves, due to a data structure (such as BVH) having to be built from geometry.

In theory, even though I couldn't find this documented, I might be able to do this safely with GodotPhysics colliders, even if multithreaded physics is turned off (but at least set to "single-safe" so RID maps are safe), as long as I just do the baking of the shape resource, and I don't assign them to a body. This is because the creation and baking of the shape should be isolated from the physics world.
(this is unfortunately not true of all physics engines; for example, ReactPhysics can't do this due to their custom memory allocator not being thread-safe).

I'm assuming Jolt supports this under a similar assumption (correct me if not though). But with GodotJolt, from the last discussion we had, it seems the shape's baking is intentionally delayed to the very last moment, when it gets attached to a body pretty much. This is very bad for my use case, because it pushes the expensive part down to the main thread and out of my control. Also, it generally prevents from loading scenes or shape resources asynchronously if they contain GodotJolt shapes.

If changing this is still not planned, we also discussed that there could be at least an alternative way of creating mesh colliders using the PhysicsServer3D API: shape_set_data takes a RID and a Variant. So it could be provided different data and flags that could tell GodotJolt to override the default behavior, and do the baking immediately, assuming that the caller could be a different thread and the shape isn't attached to a body yet. Additionally, this could also allow to pass indexed 32-bit float vertices (which is what I generate and what Jolt wants), rather than being forced to pass de-indexed triplets of Vector3 (which is what Godot decided in its generic API, and unnecessarily has to be 64-bit in double-precision builds of Godot. This leads to a waste of effort purely because of the API).

---

Side exploratory note: if Jolt can also make assumptions about the way the mesh is structured, that could be sent as an option to speed up baking: Transvoxel meshes have all their triangles constrained into grid cells, and vertices on edges of that grid, so unless the mesh is simplified with a MeshOptimizer pass, building a BVH could get simpler? Maybe the structure itself could be made into a grid instead of a BVH? If not, just ignore this for now ;)

[mihe]

I'm assuming Jolt supports this under a similar assumption (correct me if not though).

Yes, Jolt supports this just fine.

But with GodotJolt, from the last discussion we had, it seems the shape's baking is intentionally delayed to the very last moment, when it gets attached to a body pretty much. This is very bad for my use case, because it pushes the expensive part down to the main thread and out of my control. Also, it generally prevents from loading scenes or shape resources asynchronously if they contain GodotJolt shapes.

I know you're already aware of this, but for anyone in this thread who might not be, the reason I did it this way is due to how Godot deals with modified properties for scene nodes, where modified properties will "trickle in" one-by-one as the node is loaded, which would cause the shape to be rebuilt multiple times over. This applies when creating a shape node procedurally as well, since each property change will trigger the PhysicsServer3D.shape_set_data call for the underlying shape, causing a potentially expensive rebuild. When using the PhysicsServer3D API directly you don't have this problem, since you can just call shape_set_data once instead, but I have no way of distinguishing between these use-cases on my end.

This is all made worse by the fact that there is no signal/call/whatever from Godot saying "alright, all properties are set, now commit those to the underlying shape", which means I don't have much of a choice (with the current API) but to defer building the shape until it's absolutely necessary, which is either when the shape's body actually enters a physics space (i.e. NOTIFICATION_ENTER_WORLD) or when a physics query is performed with that shape.

The worst offender of this is HeightMapShape3D, which is not only fairly costly to rebuild, but would also be rebuilt 3-4 times over for every single instance. This is less of a problem for ConcavePolygonShape3D, since it at the moment only has two properties, faces and backface_collision, and backface_collision is likely rarely modified. Nevertheless, if someone does decide to override backface_collision they will have their ConcavePolygonShape3D be built twice every time for every such instance, which is doubly costly given the lack of triangle indices for these shapes in Godot.

Another reason why I did it this way was because of the fact that these "trickled" property changes also mean you'll need to build shapes that are in intermediate states, which may or may not make sense to Jolt, and may or may not emit errors when building. This was mostly a problem with HeightMapShape3D at the time, due to me relying solely on Jolt's equivalent as the backing shape, and at the time it only supported square power-of-two dimensions. Nowadays I believe the only problematic shapes are CylinderShape3D and CapsuleShape3D, which can't have a radius of 0 for obvious reasons, but that's something I've actually seen people get tripped up on (as Godot Physics does sort of allow for this) so I might change that at some point.

If changing this is still not planned, we also discussed that there could be at least an alternative way of creating mesh colliders using the PhysicsServer3D API: shape_set_data takes a RID and a Variant. So it could be provided different data and flags that could tell GodotJolt to override the default behavior, and do the baking immediately, assuming that the caller could be a different thread and the shape isn't attached to a body yet. Additionally, this could also allow to pass indexed 32-bit float vertices (which is what I generate and what Jolt wants), rather than being forced to pass de-indexed triplets of Vector3 (which is what Godot decided in its generic API, and unnecessarily has to be 64-bit in double-precision builds of Godot. This leads to a waste of effort purely because of the API).

This has been in the back of my mind since we last spoke about this, but I've hesitated a bit due to what I mentioned earlier in this thread, namely that this extension is likely to be merged into Godot as an official module, so I've tried my best to avoid introducing any API discrepancies that could make that transition more difficult. In my mind this issue would be one of several issues best addressed/discussed as part of that transition in some way.

Side exploratory note: if Jolt can also make assumptions about the way the mesh is structured, that could be sent as an option to speed up baking: Transvoxel meshes have all their triangles constrained into grid cells, and vertices on edges of that grid, so unless the mesh is simplified with a MeshOptimizer pass, building a BVH could get simpler? Maybe the structure itself could be made into a grid instead of a BVH? If not, just ignore this for now ;)

I don't know enough about the inner workings of JPH::MeshShape to comment on the viability of this, but regardless, this would fall under the same category as what's mentioned above I guess.