Support setting max contacts in dartsim's ODE collision detector

[iche033]

🎉 New feature

Summary

Support setting max number of contacts between collision pairs.

• Added a new MaxContacts feature.
• Support for this is currently only implemented when the ode collision detector is used in dartsim (which is the default collision detector).
  • This is done by overriding the collide function in dartsim's OdeCollisionDetector class to post process the contact results. Not ideal as it involves copying contact data. It would be nice if dart provides some hooks / callback into their collision checking process e.g. here so we can avoid the copy.

Added test to make sure the max contact value does limit the no. of contacts.

See gazebosim/gz-sim#2270 for testing with gz-sim.

Checklist

• Signed all commits for DCO
• Added tests
• Added example and/or tutorial
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[codecov]

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (0fc25a9) 78.59% compared to head (df9f174) 78.64%.

Files	Patch %	Lines
dartsim/src/GzOdeCollisionDetector.cc	82.85%	6 Missing ⚠️
dartsim/src/WorldFeatures.cc	83.33%	3 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff               @@
##           gz-physics6     #582      +/-   ##
===============================================
+ Coverage        78.59%   78.64%   +0.04%     
===============================================
  Files              140      141       +1     
  Lines             7654     7713      +59     
===============================================
+ Hits              6016     6066      +50     
- Misses            1638     1647       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[azeey]

I see the upstream Ode collision detector has the option to limit contacts https://github.com/dartsim/dart/blob/b7f5dd1cee755fe0e7a43c8c83794f22452501c1/dart/collision/ode/OdeCollisionDetector.cpp#L317. Is it not possible to use that?

[iche033]

I see the upstream Ode collision detector has the option to limit contacts https://github.com/dartsim/dart/blob/b7f5dd1cee755fe0e7a43c8c83794f22452501c1/dart/collision/ode/OdeCollisionDetector.cpp#L317. Is it not possible to use that?

That's the flag we're setting in EntityManagementFeatures.cc. I noticed that it limits the max number of contacts for all entities instead of max contacts per collision pair.

As a quick test, I tried setting it to 10 and then inserting boxes in shapes.sdf world. Eventually objects will start sinking and then falling through the ground plane.

[azeey]

That's the flag we're setting in EntityManagementFeatures.cc. I noticed that it limits the max number of contacts for all entities instead of max contacts per collision pair.

Ah, I think I understand now. So, this PR makes it so that each collision pair will have the same number of max contacts. This as opposed to the total number of contacts in the world being limited to the max contact parameter. Is that right?

I think Gazebo-classic also had a way to set the max contact parameter for each <collision>. This is different from that right?

[iche033]

This as opposed to the total number of contacts in the world being limited to the max contact parameter. Is that right?

yes that's correct

I think Gazebo-classic also had a way to set the max contact parameter for each . This is different from that right?

yeah that's a related feature which gives users finer control of max contacts for a specific collision.

[azeey]

@azeey to review.

[scpeters]

This as opposed to the total number of contacts in the world being limited to the max contact parameter. Is that right?

yes that's correct

I think Gazebo-classic also had a way to set the max contact parameter for each . This is different from that right?

yeah that's a related feature which gives users finer control of max contacts for a specific collision.

I think we should be careful about naming the features and APIs to make it clear which interpretation of max contacts is being used. Here's some suggested feature names:

• maximum total number of contacts allowed in a world: gz::physics::WorldMaxTotalContacts
• default maximum number of contacts for a collision pair: gz::physics::CollisionPairMaxTotalContacts
• maximum number of contacts for a specific collision, which overrides the CollisionPairMaxTotalContacts value, and requires some logic for merging the parameters for each collision shape in a pair if they both have it specified: gz::physics::CollisionMaxTotalContacts

[iche033]

I think we should be careful about naming the features and APIs to make it clear which interpretation of max contacts is being used. Here's some suggested feature names:

Great, thanks for the suggestions. For the feature in this PR, I renamed it from MaxContacts to gz::physics::CollisionPairMaxTotalContacts. 98b5e61

[scpeters]

I think we should be careful about naming the features and APIs to make it clear which interpretation of max contacts is being used. Here's some suggested feature names:

Great, thanks for the suggestions. For the feature in this PR, I renamed it from MaxContacts to gz::physics::CollisionPairMaxTotalContacts. 98b5e61

thanks; I just wrote the first thing that came to mind; we can see if @azeey has any different naming suggestions. At risk of bike-shedding, I think I only meant to include Total in the WorldMaxTotalContacts feature name, so the feature in this PR could be shortened to CollisionPairMaxContacts, but I'll let @azeey make the call (and apologies for the churn)

[iche033]

At risk of bike-shedding, I think I only meant to include Total in the WorldMaxTotalContacts feature name, so the feature in this PR could be shortened to CollisionPairMaxContacts, but I'll let @azeey make the call (and apologies for the churn)

ok no problem. @azeey any thoughts on naming? I'll make the change if Addisu agrees.

[azeey]

CollisionPairMaxContacts sounds good to me. I think having "total" would add confusion since it might imply the total number of contacts in the world.

[iche033]

CollisionPairMaxContacts sounds good to me.

ok updated! b140329

[scpeters]

just a few places where the documentation could be cleaned up