-
Notifications
You must be signed in to change notification settings - Fork 37
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
feat(engine): allow multiple contacts between two entities #1457
feat(engine): allow multiple contacts between two entities #1457
Conversation
|
6959b32
to
e710fd6
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1457 +/- ##
==========================================
- Coverage 53.13% 53.04% -0.09%
==========================================
Files 466 466
Lines 25980 25996 +16
Branches 2405 2413 +8
==========================================
- Hits 13804 13790 -14
- Misses 12176 12206 +30 ☔ View full report in Codecov by Sentry. |
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.
Clang-Tidy
found issue(s) with the introduced code (1/1)
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.
Thank you for working on this 🙏 Overall, looks pretty good to me, I just have a few comments, mostly regarding contact caching in this new way of doing things, and a few optimizations.
e710fd6
to
4f9f612
Compare
4f9f612
to
670ebf0
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.
Other than what I pointed out, LGTM! Ty for working on this, will be really nice to have this working 👀
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.
Just a couple more things to point out. Also, since I already merged the warm-starting PR, you can rebase and the convert the new things to work with these, it should be just a couple changes in the penetration constraint plugin.
670ebf0
to
d38632d
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.
Clang-Tidy
found issue(s) with the introduced code (1/1)
d38632d
to
69a97d7
Compare
narrow phase changes unfinished more changes narrow phase finished refactor more changes
69a97d7
to
ed6e846
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.
Alright, LGTM! Good work 👍
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.
LGTM!
Description
Remove everything from CollidingWith except entity, add a vector of manifolds.
ContactManifold is no longer a relation.
Same thing for PenetrationConstraint, add a PenetrationtConstraints relation that holds a vector of PenetrationConstraint.
Change narrow phase systems to buildup the vector of manifolds in a collidingWith relation.
Change peneration constraint plugin to create the vector of PenetrationConstraint for each manifold.
Change penetraton constraint solver to buildup the effects of all constraints in PenetrationConstraints
Checklist