Closed iche033 closed 5 months ago
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 :warning: |
dartsim/src/WorldFeatures.cc | 83.33% | 3 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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?
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.
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?
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 to review.
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:
gz::physics::WorldMaxTotalContacts
gz::physics::CollisionPairMaxTotalContacts
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
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
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
togz::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)
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.
CollisionPairMaxContacts
sounds good to me. I think having "total" would add confusion since it might imply the total number of contacts in the world.
π New feature
Summary
Support setting max number of contacts between collision pairs.
MaxContacts
feature.collide
function in dartsim'sOdeCollisionDetector
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 https://github.com/gazebosim/gz-sim/pull/2270 for testing with gz-sim.
Checklist
codecheck
passed (See contributing)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.