Allow CollisionShape nodes to be indirect children of bodies

[aaronfranke]

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, and should be fast.

Implements and closes godotengine/godot-proposals#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 godotengine/godot-proposals#535, #2174, godotengine/godot-proposals#1049, godotengine/godot-proposals#4559, godotengine/godot-proposals#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.

Production edit: closes godotengine/godot-roadmap#43

[fire]

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?

[aaronfranke]

@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]

Can't you use the NOTIFICATION_TRANSFORM_CHANGED?

[aaronfranke]

@fire Checking the global transform would cause it to update when the body moves, which would be every frame for some bodies.

[fire]

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?

[aaronfranke]

@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.

[ShirenY]

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:

1. Relative Transform Cache in CollisionShape3D:
   Create a relative transform cache in CollisionShape3D and 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.

2. New Signal with NOTIFICATION_LOCAL_TRANSFORM_CHANGED:
   Add a new signal, NOTIFICATION_LOCAL_TRANSFORM_CHANGED, to Node3D. When CollisionShape3D attaches itself to the body, it also listens to these nodes on the path between them for this signal. This callback can replace the use of NOTIFICATION_TRANSFORM_CHANGED in 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.

[aaronfranke]

@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.

[ShirenY]

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.

[Calinou]

Tested locally (rebased on top of master 04692d8), it works as expected with the following scene configuration:

Testing project: test_pr_77937.zip

In comparison, without this PR, none of the physics interactions work in the above testing project.

Code looks good to me.

[mihe]

Won't an exact comparison like this still cause this cached transform to be invalidated quite often just by the nature of floating-point precision?

[aaronfranke]

If the transforms are exactly the same, then multiplying them will result in the exact same outputs. Within the same machine (not including network stuff), floating-point math is deterministic. If the float is slightly off due to precision issues, it will always be off by the exact same amount when the calculation is repeated.

[mihe]

Ah, somehow I thought the transform included that of the collision object, which it obviously doesn't, but even then you'd end up invalidating it just by virtue of the object moving around, so my concern never made much sense anyway.

Nevermind. :)

[Zylann]

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 #77937 (comment) 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 Zylann/godot_voxel#555 (comment)

[Synzorasize]

This works as expected, and is just what I need for my project, so thanks!

It made me wonder though, would it be helpful to have a NodePath to choose which ancestor PhysicsBody the CollisionShape affects? That way, users can have more flexibility over their scene designs.

[fire]

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 PARENTED to ENTER_TREE notification 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?

[arthurpalm]

Any ETA on this one? This and #106048 are essential on a couple projects as they would greatly improve my workflow