-
-
Notifications
You must be signed in to change notification settings - Fork 199
Implement collision margin #393
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
Conversation
src/collision/narrow_phase.rs
Outdated
| body2: &RigidBodyQueryReadOnlyItem, | ||
| collider1: &ColliderQueryItem<C>, | ||
| collider2: &ColliderQueryItem<C>, | ||
| collision_margin: Scalar, |
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.
| collision_margin: Scalar, | |
| collision_margin: CollisionMargin, |
It's weird to refer to CollisionMargin in the docs and then not actually use that type.
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 don't like having to wrap everything in newtypes unnecessarily so I changed this (and the other similar cases) to impl Into<CollisionMargin> and derived From<Scalar> for now. Not sure if it's worth it though, since the newtypes don't really give an API benefit (other than explicitness and docs) and it just means that we need to call .into() and use derefs in more places internally
src/dynamics/solver/contact/mod.rs
Outdated
| collider_entity2: Entity, | ||
| collider_transform1: Option<ColliderTransform>, | ||
| collider_transform2: Option<ColliderTransform>, | ||
| collision_margin: Scalar, |
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.
| collision_margin: Scalar, | |
| collision_margin: CollisionMargin, |
if we already have the newtypes, we should also be consistently using them imo
src/collision/narrow_phase.rs
Outdated
| let collision_margin1 = collider1.collision_margin.map_or( | ||
| rb_collision_margin1.map_or(0.0, |margin| margin.0), | ||
| |margin| margin.0, | ||
| ); | ||
| let collision_margin2 = collider2.collision_margin.map_or( | ||
| rb_collision_margin2.map_or(0.0, |margin| margin.0), | ||
| |margin| margin.0, | ||
| ); | ||
| let collision_margin_sum = collision_margin1 + collision_margin2; |
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.
This code is duplicated. Consider extracting it into a method.
|
Math checks out, just some code quality issues, nothing big :) |
Co-authored-by: Jan Hohenheim <jan@hohenheim.ch>
Objective
Collisions for thin objects such as triangle meshes can sometimes have stability and performance issues, especially when dynamically colliding against other thin objects.
To allow users to mitigate these issues when necessary, engines such as Rapier and Bullet provide a collision margin or contact skin that forces the solver to maintain an artificial separation distance between objects. It can be thought of as a kind of shell that adds thickness to colliders, reducing the issues mentioned above.
Solution
Add a
CollisionMargincomponent. It can be used to add artifical thickness to any collider for collisions.Without a collision margin:
2024-07-02.22-07-55.mp4
With a collision margin:
2024-07-02.22-09-28.mp4