bullet-featherstone: Use a single contact manifold for each convex decomposed mesh collision

[iche033]

🦟 Bug fix

Summary

Contact point optimization for convex decomposed mesh collisions in bullet-featherstone plugin. This PR reduces the number of contact points generated for convex decomposed mesh collision by using a single contact manifold.

Previously when a mesh collision is decomposed into multiple convex hull shape, each convex hull shape has a corresponding contact manifold which can generate up to 4 contact points (4 is the default limit in bullet). This slows down sim especially when querying contact points (via GetContactsFromLastStep). This PR overrides the btCollisionDispatcher class in order to update the contact manifolds stored in bullet. It post processes the contact points, generates a single contact manifold for each collision associated with convex hull shapes, and clears the exiting contact manifolds.

Note: it would be more efficient if we can do this inside bullet to avoid generating all the contact manifolds in the first place.

Before: many points can be generated from one convex decomposed mesh collision.

After: limit contact points to <= 4

Here're remotery graphs showing before and after the fix for a scene with multiple convex decomposed meshes. Note objects were auto-deactivated when these graphs were captured.

Before: >200 contact points

After: 20 contact points

Notable speedups are:

• PhysicsPrivate::Step: 0.153ms -> 0.097ms
• PhysicsPrivate::UpdateCollision: 0.528ms -> 0.135ms

Checklist

• Signed all commits for DCO
• Added tests
• 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.

[azeey]

Looks like there are several test failures in windows and I don't see any in the stable branch (https://build.osrfoundation.org/view/gz-harmonic/job/gz_physics-7-win/)

[iche033]

Looks like there are several test failures in windows and I don't see any in the stable branch (https://build.osrfoundation.org/view/gz-harmonic/job/gz_physics-7-win/)

Fixed and this PR is ready for review again. The test failures were due to RTTI access violation when doing a dynamic_cast from btCollisionShape to btCompoundShape. I saw a similar issue reported in dartsim and fixed by adding pragmas in base classes but that's not viable here so I switched to static_cast.

[azeey]

@azeey to review.

[azeey]

Well, this was not a walk in the park :), but I think I finally understand how things are working. I don't see any major problems, but it would be good to test what happens when objects are removed. Another point is about the ambiguity with the collision index. I'm wondering if we can find a way around that. No worries if you don't have time to tackle that right now though.

[azeey]

This seems like something that could happen fairly frequently, i.e, a link that has a primitive collider as well as a convex decomposed mesh. Or it could be a link that has multiple convex decomposed mesh colliders. I guess returning nullptr here means that we won't be consolidating the contact points from this shape right? Just checking my understanding.

[azeey]

I don't think any of the for loops are entering the block. I checked this with a Coverage build locally. Can we add a test that covers this?

[iche033]

updated MeshContact test in collisions.cc to remove the model with convex decomposed mesh collision. It should trigger this call. 15d4ede

[azeey]

Can we add a test that covers this?

[azeey]

Do we need to also remove it from customManifolds?

[iche033]

I think this is only be called for contact manifolds owned by bullet, and not the custom ones created by gz-physics. I tested manually by removing models with convex decomposed collisions and did not notice that custom manifolds were passed to this function.

[azeey]

Is this necessary? Do you know that Bullet itself won't be doing this?

[iche033]

I remember I had to keep this call for the multi body link transform to be updated. As a test, I dropped a model with convex decomposed collisions on the ground -> this worked fine, but as soon as I moved it to a different pose, its transforms are not updated.

[azeey]

Could we use the getUserIndex or getUserPointer to help with this somehow?

[iche033]

I addressed most of the comments but not all. I still need to figure out how to resolve the issue with ambiguous collision index and then add a test.

[codecov]

Codecov Report

❌ Patch coverage is 91.33858% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.54%. Comparing base (92e02c3) to head (15d4ede).
⚠️ Report is 67 commits behind head on gz-physics7.

Files with missing lines	Patch %	Lines
bullet-featherstone/src/SimulationFeatures.cc	72.00%	7 Missing ⚠️
bullet-featherstone/src/Base.cc	95.87%	4 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff               @@
##           gz-physics7     #664      +/-   ##
===============================================
+ Coverage        78.32%   79.54%   +1.21%     
===============================================
  Files              140      144       +4     
  Lines             8069     8799     +730     
===============================================
+ Hits              6320     6999     +679     
- Misses            1749     1800      +51     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.