`ColliderParent` component #397

Aceeri · 2023-07-06T00:25:45Z

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 · 2023-07-06T07:06:52Z

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

sebcrozet · 2023-07-08T10:07:46Z

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.

Aceeri · 2023-07-08T20:56:08Z

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 · 2023-07-08T21:45:35Z

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 · 2023-07-25T22:13:46Z

This fixes #337 as well.

Aceeri · 2023-07-26T04:57:22Z

src/plugin/systems.rs

+    let child_colliders = |entity: Entity| -> Vec<Entity> {
+        let mut found = Vec::new();
+        let mut possibilities = vec![entity];
+        while let Some(entity) = possibilities.pop() {
+            if rigid_bodies.contains(entity) {
+                continue;
+            }
+
+            if colliders.contains(entity) {
+                found.push(entity);
+            }
+
+            if let Ok(children) = childrens.get(entity) {
+                possibilities.extend(children.iter());
+            } else {
+                continue;
+            };
+        }
+
+        found
+    };


This could be done better, ideally without allocations. I'm thinking of just refactoring this all so that any entity in the hierarchy between a RigidBody and a Collider has a ColliderLink marker component.

I suppose this is fine as long as the Vec is re-used in the system as a Local<Vec<Entity>>

Aceeri · 2023-07-29T17:31:17Z

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.

sebcrozet · 2023-08-16T12:31:29Z

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!

Aceeri · 2023-08-17T23:02:21Z

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 marked this pull request as ready for review July 6, 2023 08:02

Aceeri force-pushed the collider-parent branch from d4139c4 to a03663d Compare July 24, 2023 02:37

Aceeri commented Jul 26, 2023

View reviewed changes

Aceeri marked this pull request as draft July 26, 2023 13:03

Aceeri marked this pull request as ready for review July 29, 2023 17:08

Aceeri added 16 commits August 21, 2023 11:10

ColliderParent component

b3e5bb2

Initial parent setup

b785379

Add, update, remove collider parent based on hierarchy changes

8dcd6f7

Testing compounds vs child colliders

11d69d5

Re-add system for updating parent

408f75f

Clean up warnings

dbb7d6a

Re-add MapEntities to ColliderParent

63f5924

Revert cargo toml changes

941134a

Revert boxes3 example

ddec5f2

Formatting

e8b8a2a

Collider parent unit tests

2e07596

Add HierarchyPlugin for transform propagation

bf29e18

Some messing around to figure out 1 frame delays

bcc503c

Formatting, etc.

e77d3eb

Reduce allocations by re-using vecs

bbbaa5e

PhysicsSet::EveryFrame so we don't miss hierarchy events

70f2c16

Aceeri force-pushed the collider-parent branch from 7efa59b to 70f2c16 Compare August 21, 2023 18:40

Aceeri added 4 commits August 21, 2023 11:48

Configure everyframe

90e0d58

Formatting

de6c983

Improve documentation a bit

e9d10d4

1.72 formatting

eed69bf

Vrixyz added D-Difficult Needs strong technical background, domain knowledge, or impacts are high, needs testing... P-Medium A-Integration very bevy specific enhancement New feature or request labels May 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`ColliderParent` component #397

`ColliderParent` component #397

Aceeri commented Jul 6, 2023 •

edited

Aceeri commented Jul 6, 2023

sebcrozet commented Jul 8, 2023

Aceeri commented Jul 8, 2023 •

edited

Aceeri commented Jul 8, 2023 •

edited

Aceeri commented Jul 25, 2023

Aceeri Jul 26, 2023

Aceeri Jul 28, 2023

Aceeri commented Jul 29, 2023 •

edited

sebcrozet commented Aug 16, 2023

Aceeri commented Aug 17, 2023 •

edited

ColliderParent component #397

Are you sure you want to change the base?

ColliderParent component #397

Conversation

Aceeri commented Jul 6, 2023 • edited

Aceeri commented Jul 6, 2023

sebcrozet commented Jul 8, 2023

Aceeri commented Jul 8, 2023 • edited

Aceeri commented Jul 8, 2023 • edited

Aceeri commented Jul 25, 2023

Aceeri Jul 26, 2023

Choose a reason for hiding this comment

Aceeri Jul 28, 2023

Choose a reason for hiding this comment

Aceeri commented Jul 29, 2023 • edited

sebcrozet commented Aug 16, 2023

Aceeri commented Aug 17, 2023 • edited

`ColliderParent` component #397

`ColliderParent` component #397

Aceeri commented Jul 6, 2023 •

edited

Aceeri commented Jul 8, 2023 •

edited

Aceeri commented Jul 8, 2023 •

edited

Aceeri commented Jul 29, 2023 •

edited

Aceeri commented Aug 17, 2023 •

edited