bevy_rapier icon indicating copy to clipboard operation
bevy_rapier copied to clipboard

`ColliderParent` component

Open Aceeri opened this issue 2 years ago • 8 comments

Ease of access for finding the rigid body associated with the collider as well as updating the collider parent based on changes to the bevy hierarchy.

For an example of the issue with the current system: https://github.com/Aceeri/rapier_sync

Aceeri avatar Jul 06 '23 00:07 Aceeri

Is ColliderParent the best term for this? I'm mostly mimicking the ColliderSet::set_parent(collider, rigid_body, ...) api atm.

Aceeri avatar Jul 06 '23 07:07 Aceeri

Thank you for this PR! What use-case does this new component serve?

I’m not sure it should have any side-effect (when moved or removed) regarding the collider’s parent since this is based only on the entity hierarchy. I’m also worried of the performance implications of collect_collider_hierarchy_changes acting on all topology changes.

If the goal is only to easily find the entity of a collider’s parent rigid-body, we have the method RapierContext::collider_parent.

sebcrozet avatar Jul 08 '23 10:07 sebcrozet

I don't think we ever updated the collider parent if the entity was moved in the hierarchy though which was part of the point of this. At least I don't see any set_parent calls in bevy_rapier currently aside from initializing a collider.

I also kind of think collider_parent should be more first class usage, I've ran into plenty of bugs where I accidentally assumed that the Collider entity I got back from a shape cast/ray cast would also be the rigid body, but maybe that is more of a documentation issue?

Aceeri avatar Jul 08 '23 20:07 Aceeri

Some ways I think we could minimize the performance implications is by having a ColliderLink marker component and then checking for Query<Entity, (With<ColliderLink>, Changed<Parent>)>. This should also let us more properly update child collider positions in rapier. This would also help with properly updating translation/rotation from the hierarchy as it changes, since right now child collider Transform changes do nothing after initialization.

Aceeri avatar Jul 08 '23 21:07 Aceeri

This fixes #337 as well.

Aceeri avatar Jul 25 '23 22:07 Aceeri

This should be ready to merge now.

Thank you for this PR! What use-case does this new component serve?

Entity hierarchy changes are not reflected in the Collider::parent, that means if a collider is re-parented to another entity it will still be considered parented to the previous RigidBody entity. This also

I’m not sure it should have any side-effect (when moved or removed) regarding the collider’s parent since this is based only on the entity hierarchy.

I'm a bit confused on this, we base the parent initially on the entity hierarchy so I'd assume we'd want to keep it updated to that standard.

I’m also worried of the performance implications of collect_collider_hierarchy_changes acting on all topology changes.

Topology changes should be relatively infrequent so I'm not too worried here. There are some improvements we could make by mimicking transform propagation and only operating on Parent changes to any entity between the Collider entity and RigidBody entity. But this gets much more error-prone and I feel like might be premature optimization in this case.

If the goal is only to easily find the entity of a collider’s parent rigid-body, we have the method RapierContext::collider_parent.

The method is super useful, but I think it lacks a bit of discoverability, I don't think I've seen any uses of rapier in the wild where it was correctly used instead of just assuming that the Collider entity was also the RigidBody entity.

Aceeri avatar Jul 29 '23 17:07 Aceeri

I'm a bit confused on this, we base the parent initially on the entity hierarchy so I'd assume we'd want to keep it updated to that standard.

I was worried about the user adding or removing the ColliderParent component manually. Though that seems unlikely since that component can’t be constructed manually.

Since it is not supposed to be modified by the user, I wonder if we should call this ReadColliderParent (similar to ReadMassProperties)? I’m also not sure about whether this is worth the added code complexity considering we already have RapierContext::collider_parent. But I will deffer to your judgement on these remarks.

Otherwise that looks good!

sebcrozet avatar Aug 16 '23 12:08 sebcrozet

I'm a bit confused on this, we base the parent initially on the entity hierarchy so I'd assume we'd want to keep it updated to that standard.

I was worried about the user adding or removing the ColliderParent component manually. Though that seems unlikely since that component can’t be constructed manually.

Since it is not supposed to be modified by the user, I wonder if we should call this ReadColliderParent (similar to ReadMassProperties)? I’m also not sure about whether this is worth the added code complexity considering we already have RapierContext::collider_parent. But I will deffer to your judgement on these remarks.

Otherwise that looks good!

Just ColliderParent should be fine, Parent for bevy is similar in that you shouldn't modify it manually, but instead through commands. I'll improve the docs here a bit as well.

I think it is worthwhile just for consistency over changes to the ECS world.

We could also do it by directly setting the collider parent in the RapierContext, but this is more idiomatic for how bevy users see game entities, where the property is directly attached. Plus this has some benefits in the future when bevy finally gets relations added.

Aceeri avatar Aug 17 '23 23:08 Aceeri