Skip to content
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

fix: allow users to set agents' collision groups by depth #712

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

howird
Copy link

@howird howird commented Nov 21, 2024

  • When there are 32+ bodies in a .mjcf file, ActorBuilder.collision_groups[2] is greater than $2^{32} - 1$ which causes an error since the C++ interface expects the elements of ActorBuilder.collision_groups to be uint32
  • This is due to the way that collision groups are set here and leads to an error here when setting the collision groups since they are supposed to be uint32
  • Changes:
    • if more than 32 bodies are found, thow an error when loading the .mjcf, rather than later
    • propose an alternative which sets the collision group based on the depth from the root body node of the .mjcf so there are less collision groups
      • this is off by default and enabled by setting group_collisions_by_depth = True in the agent class
      • Not sure if this is the right way to do this, open to alternatives

@StoneT2000
Copy link
Member

Thank you for the contribution.

This is simply default behavior to ensure no adjacent links can ever collide. That being said for more complex articulations with a ton of links in sequence we may want to disable the option for setting collision bits automatically and require the user to custom define a more efficient collision disabling setup

I think the 32 maximum is a limitation of physx however, @fbxiang any way to increase it or would one have to combine more links into the same collision groups?

@howird
Copy link
Author

howird commented Nov 25, 2024

For now, I think we should add the error being thrown at load time or just stop adding collision points after 32. Since allowing this invalid state leads to a lot of confusion later on.
I am working on control of humanoids in my research so I need a lot of links.
I am interested in working on fixing this issue, and can put some time into writing a feature, can you clarify what work needs to be done to enable the user to "custom define a more efficient collision disabling setup"

@StoneT2000
Copy link
Member

Actually there is a better fix that should be done. Namely adjacent links always have collisions disabled in physx anyway. The mjcf loader should just check if the links are physically adjacent or not (whether they have collision meshes, since some links are dummy links). For links that are adjacent in terms of Mujoco (since mujoco supports link + many joints to another link), we should disable collisions if there are more than one joint.

The collision group bits maxing out at 32 is a limitation of the physx simulation backend.

For a more efficient collision disabling setup I will add documentation soon about how e.g. the G1 humanoid robot is modelled for efficiency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants