godotengine
Allow CollisionShape nodes to be indirect children of bodies
EDIT: Note that when this PR was first opened there were some caveats, but those have been resolved now. It should now be fully working including supporting when any node in the tree updates its transform.
Implements and closes https://github.com/godotengine/godot-proposals/issues/535
This PR allows CollisionShape2D and CollisionShape3D nodes to be indirect children of bodies (CollisionObject2D and CollisionObject3D). A shape can only be connected to one body.
This is a highly demanded feature, see the discussion in https://github.com/godotengine/godot-proposals/issues/535, https://github.com/godotengine/godot/issues/2174, https://github.com/godotengine/godot-proposals/issues/1049, https://github.com/godotengine/godot-proposals/issues/4559, https://github.com/godotengine/godot-proposals/issues/5746 and https://ask.godotengine.org/31701/possible-rigidbody-have-colliders-that-not-direct-children.
Recently, Eoin from Microsoft has convinced me here that this is a vital feature. While I did debate with him about whether that's something worth standardizing... in terms of just whether the feature is good, I agree with him full stop, I don't see any reason to not have this. It just seems like a universally good idea.
The current code has the CollisionShape(2D/3D) looking for the CollisionObject(2D/3D) by running get_parent() and casting it to CollisionObject(2D/3D). So I made this a loop in the case that this cast fails, continue going up the tree until a body is found. All the code in CollisionObject(2D/3D) already works with a cached body reference and notifies it when things about the shape change like the transform.
~~The one caveat I can think of is that changing the transform of an intermediate node would not cause the shape to update. I don't know what the best solution to this is. We could just document that this doesn't work, or state that you would need to update the shapes manually if you do this via .update_in_shape_owner().~~ EDIT: This works now.
Production edit: closes godotengine/godot-roadmap#43
What is the workaround for changing the transform of an intermediate node would not cause the shape to update we can document?
Recreate the node?
@fire Changing the transform and changing it back would work. Or maybe we should expose _update_in_shape_owner? EDIT: I have updated this PR to expose update_in_shape_owner.
@fire Checking the global transform would cause it to update when the body moves, which would be every frame for some bodies.
Was there a bandaid for changing the transform of an intermediate node would not cause the shape to update? What is the most popular behaviour for this case?
@fire ~~Bandaid is I exposed the update method.~~
~~Another option would be to have CollisionShape3D detect when it's not a direct child, and if so, listen for the global transform changed notification in addition to local transform changed.~~
@fire @lyuma I was mistaken before, I expected that there would be a challenge with using the global transform to handle the intermediate node case, but actually after investigation we can do this in a performant way with no downsides. I have updated the PR, the intermediate node case now works fine with no performance cost to the regular case.
Saw a call for testing on the physics channel on rocket chat, the code looks good and tested well with a small test project I made
To clarify my comment with an actual usecase, imagine loading a level in a thread and adding it to the scene tree on the main thread, in particular, if the scene tree uses convex decomposition and primitives, rather than a single large ConcavePolygonShape3D. In large scenes this could reach into the thousands, so let's do a thought experiment on a particularly large number of collision shapes and see how this could impact things.
This example would involve instantiating a scene with 10000 collision shapes from a thread, and then later on the main thread, adding this sub-tree to the main scene tree using add_child.
Previously, this would have been pretty well optimized because most of the work of creatiing collision shape owners is done in the thread. My concern is how this change will impact the overhead of add_child in this case.
And a related concern is what happens when reparent or replace_child is used on such a subtree.
In both cases, I'm talking about an ancestor of the RigidBody3D / StaticBody3D / CollisionObject3D, such as a whole game level, not changing parents between the object and the shapes.
Perhaps one way to resolve this question is to do benchmarks on calling reparent of a large level with 10000 collision shapes to show what the performance impact is before vs after.
@lyuma Good catch, yes that is indeed a problem that may cause a performance regression in some projects. I have pushed a change to the PR that avoids the problem by running the code on PARENTED, and if that code successfully finds a collision object, the ENTER_TREE code will exit quite quickly.
Here is some test code:
func _ready():
print("a")
var test = TEST_PHYSICS.instantiate()
print("b")
add_child(test)
print("c")
remove_child(test)
print("d")
add_child(test)
print("e")
remove_child(test)
print("f")
var child = test.get_child(0)
test.remove_child(child)
print("g")
add_child(test)
print("h")
test.add_child(child)
print("i")
Here is the result when running that code, plus with some prints added into the C++ code:
a
col shape parented
col shape create shape owner in collision object
b
col shape enter tree
c
col shape remove shape owner from collision object
d
col shape enter tree
col shape create shape owner in collision object
e
col shape remove shape owner from collision object
f
g
h
col shape enter tree
col shape create shape owner in collision object
i
This should work correctly for all cases, with high performance for all common cases (direct children will be fast, instancing ahead of time then adding to the tree later will be fast).
Updated and re-tested this PR, still works as expected both for content made in Godot and imported from GLTF.
The main problem with this PR is that NOTIFICATION_TRANSFORM_CHANGED in this context is extemely costly, whereas the local variant has zero cost. If you have several rigid bodies moving, this will trigger the above notification every time they move, running the logic to adjust the shape in the parent body, hence incur into a very serious performance penalty.
As such I think this PR as-is is a no go, and this is the reason why this feature does not work as you would expect (performance). You would need to figure out a way to check if the intermediate nodes transform has changed without hooking up to the transform changed notification.
@reduz Yes, that is how this works. However note that it only uses the global transform changed for indirect children, it will still be fast for all existing cases of direct children.
As for "You would need to figure out a way to check if the intermediate nodes transform has changed without hooking up to the transform changed notification." I don't see how this can be done with Godot's current design.
Another option would be to just require the user to run update_in_shape_owner manually, which is how I set up this PR initially, but when I figured out I could keep the existing cases fast I thought the notification was nicer.
Also note that I would expect the common use cases of importing deeply nested colliders in GLTF would be mostly used for static level geometry where the categorization is useful. For most moving objects you usually don't have many colliders and therefore they would be direct children and fast.
Someone wanted to add a similar feature to my module https://github.com/Zylann/godot_voxel/pull/558 and faced the same issue. In the current state of things there doesn't seem to be a nice way to avoid the extra costs. Checking difference from cached transforms is as close as it gets, but still relies on global transform change. I can imagine how to do that if the whole parent chain is made of classes that actually know each other (internally communicating directly in code), but not when it's any other class inheriting Node3D.
Random thought: if the tree depth of the node from which the transform change originates is passed to the notification, we could figure out whether we have to update things or not with a simple instigator_depth >= body_depth, without having to store any extra thing in any node, apart from a uint8_t in shapes to store body_depth, which can likely be done at no cost if strategically packed with other members. This would also not require to evaluate the global transform each time, it just needs to know that a parent has moved. body_depth can stay up to date if the body is reparented since the body should know its shapes, or shapes could store a pointer to it using a bit more space and indirection. Of course there are likely other possible approaches, just happened to think of that
(Apologies for jumping in at the end of a long thread.)
I'm also hitting various limitations with having to have all collision shapes as direct children of the relevant physics bodies. An example I didn't see above; it makes it tricky to write an asset importer that generates a scene containing multiple collision shapes, without imposing a particular physicsbody root node.
Another option would be to just require the user to run update_in_shape_owner manually, which is how I set up this PR initially, but when I figured out I could keep the existing cases fast I thought the notification was nicer.
Speaking personally - I would much rather have this feature with the additional manual step, than not have it at all. Even better - would it be possible to have it 'automatic' at edit time, with the manual call only needed when there's a runtime change?
would it be possible to have it 'automatic' at edit time, with the manual call only needed when there's a runtime change?
That would be the case regardless, as the code to set up the physics object would run once at runtime anyway.
I have a use case where for ragdolls I want my physical bone 3d collision shapes to inherit rotation and location but not scale since my animations change the scale of bones, but this causes issues with scaled collision shapes. I would solve this by using remote transforms and only inheriting rotation and location, but I am unable to do this with the current implementation of collision shapes needing to be direct children of the bones.
Recently, I encountered a blocker because I need to make some of my collision sets into 'Prefabs,' so I don't have to modify them repeatedly for the same changes. However, collisions must be direct children of bodies, making this setup impossible. This limitation is hard to swallow, as nearly all the engines I've encountered allow similar setups.
Regarding the NOTIFICATION_TRANSFORM_CHANGED issue that @reduz mentioned, I have two ideas:
-
Relative Transform Cache in CollisionShape3D: Create a relative transform cache in
CollisionShape3Dand check it to see if a collision update is really needed before applying it to the physics part. This is a straightforward solution. The downside is the extra transform calculation and comparison every frame if the body keeps moving. However, considering how important this feature is, I'm totally fine with this approach. I would say let's implement it first and optimize it later. -
New Signal with NOTIFICATION_LOCAL_TRANSFORM_CHANGED: Add a new signal, NOTIFICATION_LOCAL_TRANSFORM_CHANGED, to
Node3D. WhenCollisionShape3Dattaches itself to the body, it also listens to these nodes on the path between them for this signal. This callback can replace the use ofNOTIFICATION_TRANSFORM_CHANGEDin the current PR. In my opinion, this is the most efficient and reasonable approach. Users only pay for what they do based on how far between the body and collisions in the hierarchy and how often they change the relative transforms of collisions.However, I have two concerns with this solution that go beyond my knowledge:
- First, how efficient is the signal? Will this signal introduce too much cost even when not hooked?
- Second, does this signal align with the overall design in the engine team's roadmap? Will we have something cooler than a signal specific to dealing with transform chain change notifications? I hope the core team can address these questions.
@ShirenY Note that there is already a NOTIFICATION_LOCAL_TRANSFORM_CHANGED, and this is used when the shape is a direct child. The tricky part is in figuring out how to make this happen when any node between the body and shape changes its transform, because it currently only happens for that specific node.
As for the first idea, that's a good idea. We can skip all calls into the physics server if the relative transform is unchanged by caching it and comparing to the cache.
The tricky part is in figuring out how to make this happen when any node between the body and shape changes its transform
Yeah, that's exactly what 2nd solution trying to promote. Currently node only can recieve NOTIFICATION_LOCAL_TRANSFORM_CHANGED from itself (AFAIK, is it?). What I'm trying to say is make NOTIFICATION_LOCAL_TRANSFORM_CHANGED a signal like visibility_changed, so that anyone else who is interested can listen to it. So that the shape can listen to all the nodes between it and the body.
It's been a while but now I got 2 PRs to add something similar to my module, because I have nodes that need the same thing (not the physics part specifically, but rather the pattern of having child nodes that affect a common ancestor when transforms change). I already commented about it in https://github.com/godotengine/godot/pull/77937#issuecomment-1946992889 but got no reactions so I don't know if anyone considered it.
Today I wrote up a deeper post about how I would handle the transform notifications https://github.com/Zylann/godot_voxel/issues/555#issuecomment-2432962849
My comments about 2D and 3D are the same. Most of the runtime performance is pretty similar: I think it could be adjusted slightly to avoid any need to call
get_parent()in the hot path if this is important, but it's not my main concern.Basically, my main concern is the change from
PARENTEDtoENTER_TREEnotification drastically impacts scenes instantiated in another thread or instantiated in advance of being added to the scene tree, such as level loading.Currently (<= godot 4.2) the collision object "shape owners" are created at the moment the CollisionObject3D is parented to its CollisionShape, not when it enters the scene tree. This also impacts threaded instantiation: the CollisionObject3D used to create the shape owners on a thread in this case.
With this change, the collision shapes will be created and destroyed every time any subtree is re-parented, added or removed. In addition, especially for large scene trees / levels, this change will force Godot to wait until the tree is added to the tree to create the shapes.
@lyuma is this still an issue?
Any ETA on this one? This and https://github.com/godotengine/godot/pull/106048 are essential on a couple projects as they would greatly improve my workflow
Would this also work for a CharacterBody3D, with colliders inside bone attachments?
edit: also, could a NodePath or a signal be helpful to avoid making the recursive call if they are set up?
@aaronfranke could I still add an Area3D with different layers just to detect other Area3Ds and that wouldn't interfere with the colliders? or would they be treated as part of the main CharacterBody3D and override their layers and masks?
@venilark The shapes will use whatever CollisionObject3D node is their closest ancestor, so if an Area3D node is a closer ancestor than the CharacterBody3D, then the shape will be a part of the Area3D.
I think the change is looking much better, and I think most of the performance issues from our original changes have been largely mitigated.
@fire Since you quoted my old comment, Aaron added some mitigations to cache the shape in the owner during PARENTED and preserve the existing state in ENTER_TREE, which is good.
The one thing that is still possibly an issue is that this change is removing the shape from owner in EXIT_TREE, which will trigger during reparent. I understand that this might be necessary for indirect children, but for direct child, I think we should skip the EXIT_TREE logic and perform the remove_shape_from_owner in UNPARENTED, since that is guaranteed to be called for the direct child case
Regarding reduz's comment, the cache should largely reduce the performance impact of the moving nodes. Furthermore, another mitigation was added to subscribe to local transform for direct children and only use NOTIFICATION_TRANSFORM_CHANGE for indirect children, so it is much safer than before, and I think we have done the best we can do for addressing reduz's feedback.
I think overall other than my concern about the direct child case for EXIT_TREE, I'm pretty happy with this and I think most of the performance impact has been mitigated. What can we do to get the necessary reviews in place and unblock this PR?
Feature freeze for 4.6 is around the corner, is there still a chance this will be implemented soon now the performance issues have been resolved?