-
Notifications
You must be signed in to change notification settings - Fork 41
Add support for plane vs convex #107
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
base: main
Are you sure you want to change the base?
Conversation
Before we get too far into the convex/mesh algorithms, let's get #51 done. This will help us decide as a group what is a reasonable number of verts per geom to optimize performance for - is it thousands or is it 64? I think having a few scenes will help with your questions about shared mem, and I also see for loops in your implementation that perhaps ought to be parallelized depending on what we learn from benchmarking real scenes. So if it's alright with you, let's hold this PR until #51 is done, should be next week. |
# Conflicts: # mujoco_warp/_src/collision_driver_test.py # mujoco_warp/_src/collision_primitive.py # mujoco_warp/_src/types.py
# Conflicts: # mujoco_warp/_src/collision_driver_test.py # mujoco_warp/_src/collision_primitive.py
For me mjwarp-viewer --mjcf=C:\git7\mujoco_warp\mujoco_warp\test_data\aloha_pot\scene.xml fails with Cuda 700 (invalid access) errors on the main branch. This makes testing of this MR with the aloha_pot scene a bit difficult since only removing the whole if elif tree in collision_primitive.py avoids the invalid access error. Even just adding the sphere_plane case brings back the Cuda 700 error. |
No, not yet, will try to do it today. I'm still a bit absorbed with warp pull requests. |
@erikfrey The pot scene now collides correctly with the ground. I had to add some safeguards to avoid some array overflows due to the many contacts generated. Also a pot scene without the robots was added since the pot disappears after frame 1 in the scene containing the robots |
Thanks Tobi - does this new benchmark give you any insights on the number of vertices per geom to optimize for? Or are we going to defer that to another PR? |
I would defer optimizations to later. We could find the max vertices in all convexes when initializing the model and then use shared memory in case that number is small enough. But I think we always need a fallback that can handle any number of vertices in convexes. |
ok, let's get this reviewed then and add a follow-up ticket to optimize/look for better ways of doing this. |
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.
Thanks for your patience - after addressing a few small comments and rebasing this to accomodate the new kernel API, we will merge.
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.
Let's remove this file from the PR - separately we will work on stability for this scene. That should land very soon, so rather than add this and then delete it, I think we can just leave it out.
result.efc_force[:] = d.efc.force.numpy()[:nefc] | ||
result.efc_margin[:] = d.efc.margin.numpy()[:nefc] | ||
# Safely copy only up to the minimum of the destination and source sizes | ||
n = min(result.efc_D.shape[0], d.efc.D.numpy()[:nefc].shape[0]) |
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.
Hmm, you may have had to make these changes for the same reason as the TODO above (e.g. fix _realloc_con_efc)
For now, would you mind doing this:
- repeat the same TODO above down here (e.g.
# TODO(team): set these efc_* fields after fix to _realloc_con_efc
)
then you can comment out setting these efc_ fields for now and we'll uncomment them soon. Thanks for finding this!
|
||
# Find point a (first support point) | ||
a_dist = wp.float32(-_HUGE_VAL) | ||
for i in range(convex.vertnum): |
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.
let's leave a TODO that we may explore tile_min or other techniques to speed this up, as a reminder for ourselves and also a signal for anyone trying this code
Can support arbitrary many vertices in a convex. Function _manifold_points was inlined to simplify the implementation. Support values get computed very often, maybe storing them would be better. But that would come at the cost of having an upper bound of the number of vertices per convex such that shared memory or something similar can be allocated.