WIP: [Feature] Add GJK-EPA algorithm for rigid body collision detection #1213
Conversation
SonSang
requested review from
Genesis-Embodied-AI,
YilingQiao and
duburcqa
as code owners
| @@ -139,7 +139,7 @@ def _load_primitive(self, morph, surface): | |||
|
|
|||
| elif isinstance(morph, gs.options.morphs.Cylinder): | |||
| tmesh = mu.create_cylinder(radius=morph.radius, height=morph.height) | |||
| geom_data = None | |||
| geom_data = np.array([morph.radius, morph.height]) | |||
There was a problem hiding this comment.
Since this PR is already big, I would recommend moving changes unrelated to GJK-EPA into another PR.
examples/rigid/grasp_bottle.py
Outdated
| @@ -25,6 +25,7 @@ def main(): | |||
| viewer_options=viewer_options, | |||
| rigid_options=gs.options.RigidOptions( | |||
| dt=0.01, | |||
| use_gjk_collision=False, | |||
There was a problem hiding this comment.
Is this just a temporary patch?
There was a problem hiding this comment.
Yes, it will be removed once the implementation is done.
| @@ -38,6 +39,8 @@ def __init__(self, rigid_solver: "RigidSolver"): | |||
| self._init_verts_connectivity() | |||
| self._init_collision_fields() | |||
| self._mpr = MPR(rigid_solver) | |||
| self._gjk_epa = GJKEPA(rigid_solver) | |||
| self.use_gjk = self._solver._options.use_gjk_collision | |||
There was a problem hiding this comment.
I would suggest following the same pattern than RigidSolver:
- Add the attribute
self._use_gjk = self._options.use_gjk_collisioninRigidSolver.__init__ - do not add any argument to
self._func_narrow_phasebut rather access the attribute (statically) within the methodif ti.static(self._use_gjk):.
There was a problem hiding this comment.
Yes, I agree. Now I used self._use_gjk as an argument to avoid recompilation every time I switch from MPR to GJK or vice versa. Will change it to static variable when the implementation is complete.
There was a problem hiding this comment.
Ok I get it. But this does not prevent you to stick to the usual pattern (except for the ti.static part of course!). I would work the same as passing it has input argument I guess, am I wrong? Maybe it is still pre-compiled to some extend in this case?
There was a problem hiding this comment.
Yes, you're correct. In fact the main reasoning behind this was for the unit tests. In the unit tests, I'm changing the use_gjk flag after compilation like this. So this choice is quite temporary for the unit test -- will change the unit test code to set the flag during compilation time and follow the usual pattern!
| @@ -188,6 +188,8 @@ class RigidOptions(Options): | |||
| Acceleration threshold for hibernation. Defaults to 1e-2. | |||
| max_dynamic_constraints : int, optional | |||
| Maximum number of dynamic constraints (like suction cup). Defaults to 8. | |||
| use_gjk_collision: bool, optional | |||
| Whether to use GJK for collision detection. Defaults to False. | |||
There was a problem hiding this comment.
Would be nice to add more information about the rationale behind this choice. Why someone would opt for GJK instead of MPR++ and if it is so great, why it is not the default? Why both options are still available?
There was a problem hiding this comment.
Hmm I was thinking that it would be better to give both options to the users, so that they could opt from them for their use. How about discuss it when we finalized the implementation and get the test results?
There was a problem hiding this comment.
Of course, but for that you need to give a few pointer to the end-user to guide their choice :) But yes we can discuss this once you finalise the implementation!
| @@ -0,0 +1,2139 @@ | |||
| import math | |||
There was a problem hiding this comment.
I would suggest not versioning stuffs you do not plan to include in the final PR. You can keep some commits locally on your machine. Just push what you want me to look at :)
There was a problem hiding this comment.
Got it, sorry for pushing temporary commits together.
Description
Adding GJK-EPA algorithm for rigid body collision detection.
Related Issue
Resolves Genesis-Embodied-AI/Genesis#
Motivation and Context
GJK-EPA could be an alternative to MPR in terms of stability or efficiency.
How Has This Been / Can This Be Tested?
Running unit tests in
test_rigid_physics_gjk.py.Screenshots (if appropriate):
Checklist:
Submitting Code Changessection of CONTRIBUTING document.