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

[DRAFT] Add a way to fully create a ConcavePolygonShape3D in calling thread #100413

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Zylann
Copy link
Contributor

@Zylann Zylann commented Dec 14, 2024

This is a work-in-progress, mainly for discussion. I don't consider it ready for now.

Attempts to solve the issue mentionned in #99895 (comment)
This gives control about when the heavy work is executed when creating a lof or mesh colliders at runtime, typically in procedural terrains. So creating chunks in multiple background threads, away from frame-dependent stuff, becomes easy.

Before this, a typical chunk would look like this in the profiler. Lots of these, executed in serial, either on the main thread or the physics thread:
image

After this, I can do the following in multiple background threads (including priority sorting and cancellation in case of spams, which wouldn't be easy if it was deferred):
image

  • Adds a new method to create concave polygon shapes on PhysicsServer3D and ConcavePolygonShape3D, which expects all properties to be specified at once. Contrary to shape_set_data, it is not deferred even when the physics server is wrapped inside PhysicsServer3DWrapMT
  • Expects an indexed mesh instead of triplets of Vector3 for each triangle. This saves the hassle of having to de-index meshes that are usually indexed already. And Jolt uses indexed meshes natively, so it also saves it from using Indexify internally
  • Allows to specify a sub-range of the passed mesh data. This saves the caller from having to allocate extra temporary packed arrays just to select a specific range of the mesh.
  • Optimizes AABB calculation a little bit by moving a branch outside of the loop and having it work on the indexed mesh, which likely has less vertices
  • Don't update the physics shape in Shape3D when setting its debug color or debug fill properties. These should only invalidate the debug mesh and emit a change signal, not invalidate the physics shape (which would cause it to be rebuilt, throwing out the fastpath out the window)
  • Only implemented with Jolt for now

There are more details and questions in TODO comments in the code.

While this seems to work, I'm not super happy about the approach. It is a very specific API, separate from how everything else is done. I actually wish that shapes (especially those involving heavy cooking like meshes) should be creatable in that specific way by default, while also using their properties like usual. Like, somehow making set_shape_data non-deferred when called the first time, and deferred if called more after? It wouldn't only give control when balancing threaded tasks in projects like mine, but it would also do the expected thing when loading scenes or resources with Godot's threaded loader.

However, so far I could not think of any other way (not without refactoring more things), because in Godot, properties can be set anywhere anytime, and the fact that the run in separate thread option is implemented by deferring every relevant method. So it is quite difficult to guarantee that the work is done on the caller thread, while still benefiting from multi-threaded physics simulation, while also not disrupting the physics engine if the shape has already been attached to a body (not my case, but this is a possibility to guard for; and it is guaranteed to not be the case in a function that fully creates a shape in one call).


Side note: I currently realize a part of this code could be split into a separate PR that could be easier to merge: support for indexed mesh when creating a mesh shape. As you can see in the profiling screenshot, re-indexing a mesh is expensive and it currently happens because the Godot API is expecting it to be so. If an index buffer could be passed optionally, it would optimize that step, and wouldn't break compatibility.

@Zylann Zylann requested review from a team as code owners December 14, 2024 20:41
@Zylann Zylann marked this pull request as draft December 14, 2024 20:54
@Chaosus Chaosus added this to the 4.x milestone Dec 16, 2024
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.

2 participants