Capsule-box collision #135
Conversation
Originally, the capsule axis was at a 45 deg angle and intersected the cube, with the symmetry being broken differently from mjc (contact with same distance but different position & frame).
There was a problem hiding this comment.
1 comment, but deferring the full review to someone who knows the ins-and-outs of the C implementation. @yuvaltassa probably?
|
|
||
| secondpos *= wp.float32(mul) | ||
|
|
||
| # skip: |
There was a problem hiding this comment.
what does the skip comment refer to?
There was a problem hiding this comment.
Unfortunately: https://github.com/google-deepmind/mujoco/blob/main/src/engine/engine_collision_box.c#L425
This is my least favorite bit of C MuJoCo. I have promised @kbayes untold riches the day we delete these gotos.
Cursory scan of this code looks like @camevor has correctly undone the gotos - agree might be worth deleting this comment or otherwise adding # goto skip; comments elsewhere to situate this comment.
There was a problem hiding this comment.
I left this in to provide an anchor when comparing against the mjc implementation, but I agree that it's confusing more than anything else. I'll do a cleanup pass over all the comments.
|
|
||
| secondpos *= wp.float32(mul) | ||
|
|
||
| # skip: |
There was a problem hiding this comment.
Unfortunately: https://github.com/google-deepmind/mujoco/blob/main/src/engine/engine_collision_box.c#L425
This is my least favorite bit of C MuJoCo. I have promised @kbayes untold riches the day we delete these gotos.
Cursory scan of this code looks like @camevor has correctly undone the gotos - agree might be worth deleting this comment or otherwise adding # goto skip; comments elsewhere to situate this comment.
| </worldbody> | ||
| </mujoco> | ||
| """, | ||
| "capsule_box_edge": """ |
There was a problem hiding this comment.
Capsule box is complicated! Is this also testing capsule on box corner? If not, let's add it. Generally speaking, please add a few more fixtures to fully exercise that kernel logic which is pretty complex.
There was a problem hiding this comment.
Agreed! I added a two more fixtures (capsule on corner and capsule flat on face -> requires 2 contacts) and more strict contact checking in the test.
- Add explanations - Rephrase vague comments - Remove obsolete comments
|
Thanks for your reviews and comments! Added some more test fixtures and cleaned up the comments in the code. |
Capsule-box collision based on mjc
includes / depends on #129 and resolves #77