Fix collision reposition with CSGShape3D
#102286
Conversation
ryevdokimov
force-pushed
the
fix-collision-reposition-csgshape
branch
from
6bd9d28 to
468cbf7
Compare
ryevdokimov
force-pushed
the
fix-collision-reposition-csgshape
branch
2 times, most recently
from
2cf3f05 to
054b6eb
Compare
| if (node->is_class("CSGShape3D")) { | ||
| rids.insert(node->call("get_root_collision_instance")); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is still breaking encapsulation. I remember we may have added call back functions to override, but I don't remember.
There was a problem hiding this comment.
No such function is used here, so probably it does not exist, or is for something else.
There was a problem hiding this comment.
CSG collision shapes from what I can tell are unique in how they're generated by having the parent hold the identity of the collision resource. Somehow which behavior is being used needs to be identified and the RIDs retrieved that way. Currently not seeing any obvious way to do that, but I'll keep thinking about it.
There was a problem hiding this comment.
I'm no expert in the matter but I faced a similar problem in #101368. Maybe one idea could be to add some kind of get_collision_rid method in VisualInstance3D that returns an empty RID and overload it in CsgShape?
There was a problem hiding this comment.
Maybe one idea could be to add some kind of get_collision_rid method in VisualInstance3D
Adding a method for getting collision related data in a class meant only for visual things seems strange to me, and I'm not sure much better than breaking encapsulation in this scenario. I'm opened to second opinions though.
There was a problem hiding this comment.
It would likely go in GeometryInstance3D I think, and not VisualInstance3D? (The former inherits the latter.)
Collision data in geometry is slightly more related I suppose.
But yeah I don't see an easy/clean solution.
There was a problem hiding this comment.
I agree that would be marginally better, but still is it worth muddying up the classes to handle niche cases like this? I just don't want a precedent to be set that might end up making things confusing down the future. I feel like the real solution to this is something more fundamental that I can't pin down quite yet.
There was a problem hiding this comment.
Yeah let's go with the local hack for now and see if we find a better architecture later.
There was a problem hiding this comment.
I added a comment in the code to point out this is a hack. I do plan on developing this snapping idea further, see: #100415 for example, so maybe something will hit me in the head during the process.
There was a problem hiding this comment.
Tested locally, it works as expected.
Testing project: test_pr_102286.zip
Note that it won't work if the CSGBox you're moving is a child of the CSGCombiner3D with the collision on it. It will just pass through instead:
test_csg_reposition.mp4
This may be difficult to resolve though, given the collision shape is regenerating itself in real-time when you're moving the object (and you don't want the object to collide with itself as you're repositioning it).
PS: I noticed that we can pan, zoom and orbit while using physics-based repositioning, but not when using the translate/rotate/style Blender-style manipulation shortcuts. It would be good to look into unlocking those options if they don't conflict with anything else (in a separate PR).
ryevdokimov
force-pushed
the
fix-collision-reposition-csgshape
branch
from
054b6eb to
708e2c8
Compare
ryevdokimov
force-pushed
the
fix-collision-reposition-csgshape
branch
from
708e2c8 to
6f0a0ba
Compare
|
Thanks! |
Fixes: #102274
2025-02-01.18-11-56.mp4