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
Allow CollisionShape nodes to be indirect children of bodies #77937
base: master
Are you sure you want to change the base?
Conversation
e635433
to
e195156
Compare
85806bc
to
f030b96
Compare
f030b96
to
9f2b977
Compare
What is the workaround for Recreate the node? |
@fire Changing the transform and changing it back would work. Or maybe we should expose |
Can't you use the NOTIFICATION_TRANSFORM_CHANGED? |
@fire Checking the global transform would cause it to update when the body moves, which would be every frame for some bodies. |
9f2b977
to
1fe74ed
Compare
ceb3567
to
6b76f08
Compare
Was there a bandaid for |
@fire
|
6b76f08
to
063656f
Compare
@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. |
063656f
to
e92344d
Compare
e92344d
to
c8de913
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Godot Engine 4.3 I approve the design, but we need to test this and get physics team approval, as this is a physics thing.
- design approval
- physics team sync
- technical review
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 |
c8de913
to
5af946f
Compare
5af946f
to
576b1ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
scene/2d/collision_shape_2d.cpp
Outdated
Transform2D transform_to_col_obj = get_transform(); | ||
Node *parent = get_parent(); | ||
while (parent != collision_object) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather store the child of the collision_object. then we don't need to call get_parent() repeatedly in the loop, but can just check this == child_of_collision_object
.
This is the only real performance impact I can see in terms of collision_object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't understand what you mean by this. Can you explain further, maybe write some code and push it onto a branch?
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 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 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 And a related concern is what happens when 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. |
576b1ee
to
6328f9e
Compare
@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 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:
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the code, it looks good to me.
1c31e90
to
3fca4d6
Compare
Updated and re-tested this PR, still works as expected both for content made in Godot and imported from GLTF. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me, it needs to be tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a different way to receive notifications.
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 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 Zylann/godot_voxel#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 |
3fca4d6
to
103eed7
Compare
(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.
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? |
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. |
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 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.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 viaEDIT: This works now..update_in_shape_owner()
.Production edit: closes godotengine/godot-roadmap#43