Skip to content

Remove Jolt Physics project setting "Areas Detect Static Bodies" #105746

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
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Apr 25, 2025

As talked about in the recent update of Jolt Physics (#104449) and originally discussed in jrouwe/JoltPhysics#1476, this PR removes the project setting physics/jolt_physics_3d/simulation/areas_detect_static_bodies, effectively making it permanently true, in favor of leveraging the newly introduced JPH::PhysicsSystem::SetSimCollideBodyVsBody functionality, and the JPH::CollideShapeVsShapePerLeaf helper function, which lets us configure the amount of contacts that are cached by Jolt as part of the collision detection that it does during simulation.

Warning

This is technically a breaking change, and potentially quite pervasive, depending on the project, since areas that were maybe not meant to detect static bodies now will. This does however bring Jolt Physics in line with how Godot Physics behaves (although it has its own issues with areas and static bodies) so I feel there's reasonable justification for just removing this setting completely rather than just changing its default to true and allowing people to disable it, especially when considering the current experimental label of the Jolt Physics integration.

This project setting has become a fairly popular one to enable over the years, largely due to Godot Physics supporting this already, and despite efforts to warn people about its ill effects in Jolt Physics specifically, most seem to just go ahead and enable it anyway, since the alternatives/workarounds can be inconvenient and/or unintuitive.

As such, this removes a fairly significant performance footgun, since by default Jolt would otherwise (when enabling this project setting) generate contacts for every overlapping triangle when an Area3D overlaps with a ConcavePolygonShape3D, which when dealing with complex collision meshes (or many/large Area3D) can end up being a lot. These contacts can add a lot of overhead both in terms of CPU and memory during the simulation step in ways that might not be easy to correlate to the source of the problem.

This new way of doing it is essentially exploiting the fact that Area3D in Godot doesn't actually expose the exact contacts between itself and the thing it's overlapping. This means that the underlying collision detection during simulation really only needs exactly one contact per shape, and that contact can be anywhere, which means we can set Jolt's underlying collision collector to be the much cheaper JPH::AnyHitCollisionCollector for any collision involving sensors/areas.

I still need to do some profiling to see the exact gains (and potential losses) from this, but this should hopefully be relatively safe merge, apart from the breaking change.

Note however that since we're now forced to always allow interactions between the broadphase layers belonging to areas and static bodies, this will presumably add some amount of additional cost to Area3D's collision detection, for anyone who wasn't previously relying on this project setting, but it is what it is. Being diligent about using collision masks and layers properly (which should be done anyway) should hopefully mitigate some of that.

CC: @jrouwe

@mihe
Copy link
Contributor Author

mihe commented Apr 25, 2025

This means that the underlying collision detection during simulation really only needs exactly one contact per shape, and that contact can be anywhere, which means we can set Jolt's underlying collision collector to be the much cheaper JPH::AnyHitCollisionCollector for any collision involving sensors/areas.

It's probably worth linking this specific comment as well, to highlight one compromise of this CollideShapeVsShapePerLeaf<AnyHitCollisionCollector<...>> helper, which is the heap allocation that it incurs when either of the two bodies overlap by more than 32 of its shapes.

Note however (as mentioned there) that Jolt caches collision results inbetween physics steps if the two objects haven't moved by more than a certain amount relative to eachother, so you'll only end up paying this cost when you have an Area3D actually moving around during such overlaps.

@Mickeon Mickeon removed the request for review from a team April 25, 2025 16:01
@jrouwe
Copy link
Contributor

jrouwe commented Apr 25, 2025

Change looks good.

Reminder to update https://github.com/godotengine/godot-docs/blob/master/tutorials/physics/using_jolt_physics.rst

Also there is one gotcha: In the case of an area colliding with a mesh shape, you might receive a hit for a different triangle if the area or body moves. This results in a different SubShapeID. This would trigger a ContactListener::OnContactAdded for the new ID followed by a ContactListener::OnContactRemoved for the old ID. I think you handle this case in JoltContactListener3D::_flush_area_shifts but I'm mentioning it just in case.

@mihe mihe force-pushed the jolt/area-vs-static branch from fe49594 to ca9f07f Compare April 28, 2025 19:21
@mihe mihe force-pushed the jolt/area-vs-static branch from ca9f07f to 9d42f9f Compare April 28, 2025 21:27
@mihe
Copy link
Contributor Author

mihe commented Apr 28, 2025

Also there is one gotcha: In the case of an area colliding with a mesh shape, you might receive a hit for a different triangle if the area or body moves. This results in a different SubShapeID. This would trigger a ContactListener::OnContactAdded for the new ID followed by a ContactListener::OnContactRemoved for the old ID. I think you handle this case in JoltContactListener3D::_flush_area_shifts but I'm mentioning it just in case.

It's good that you bring this up actually. I had forgotten to apply the fix that I had in mind for this, but now that I'm actually testing it out I'm seeing some odd results that I think stem from this breaking some fundamental assumptions made by _flush_area_shifts.

I'm gonna set this PR to draft for now and do some more testing at a later point.

@mihe mihe marked this pull request as draft April 28, 2025 21:32
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