[BULLET] Making GetContactsFromLastStepFeature optional in Collision Features

[Lobotuerk]

🎉 New feature

Makes GetContactFromLastStepFeture not a requirement for Collisions in IGN-Physics. Fixes gazebosim/gz-physics#208 (comment)

Summary

Test it

Checklist

• Signed all commits for DCO
• Added tests
• Added example and/or tutorial
• Updated documentation (as needed)
• Updated migration guide (as needed)
• 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

[azeey]

This is not compiling mainly because of https://github.com/ignitionrobotics/ign-gazebo/blob/f5a362cf579ef23d2cd9d5ec1cd2f856410db3c3/src/systems/physics/Physics.cc#L2202-L2203

The EntityFeatureMap::Get expects an EntityPtr with required/minimum FeatureList. In this snippet of code contact.collision1 has ContactFeatureList, which is not the required feature in EntityCollisionMap. Ideally, this should downcast cast to the entity with the required FeatureList, but there's an implicit conversion issue that causes an ambiguity in the compiler: the compiler is unable to decide whether to call EntityFeatureMap::Get(const Entity &) or EntityFeatureMap::Get(const RequiredEntityPtr &_physEntity). The temporary solution is to manually create an ShapePtr with the required features from contact.collision1 before calling EntityFeatureMap::Get.

diff --git a/src/systems/physics/Physics.cc b/src/systems/physics/Physics.cc
index 9b419b8c..5ce52614 100644
--- a/src/systems/physics/Physics.cc
+++ b/src/systems/physics/Physics.cc
@@ -313,3 +313,3 @@ class ignition::gazebo::systems::PhysicsPrivate
   public: using ShapePtrType = ignition::physics::ShapePtr<
-            ignition::physics::FeaturePolicy3d, ContactFeatureList>;
+            ignition::physics::FeaturePolicy3d, CollisionFeatureList>;
 
@@ -432,2 +432,3 @@ class ignition::gazebo::systems::PhysicsPrivate
             CollisionFeatureList,
+            ContactFeatureList,
             CollisionMaskFeatureList,
@@ -2201,5 +2202,6 @@ void PhysicsPrivate::UpdateCollisions(EntityComponentManager &_ecm)
     const auto &contact = contactComposite.Get<WorldShapeType::ContactPoint>();
-    auto coll1Entity = this->entityCollisionMap.Get(contact.collision1);
-    auto coll2Entity = this->entityCollisionMap.Get(contact.collision2);
-
+    auto coll1Entity =
+        this->entityCollisionMap.Get(ShapePtrType(contact.collision1));
+    auto coll2Entity =
+        this->entityCollisionMap.Get(ShapePtrType(contact.collision2));

I'll continue to think about a way to fix the implicit conversion problem, but I don't think it's needed for this PR.

[codecov]

Codecov Report

Merging #690 (0a38e38) into ign-gazebo4 (302f5ed) will decrease coverage by 11.90%.
The diff coverage is 68.87%.

❗ Current head 0a38e38 differs from pull request most recent head 7525bc4. Consider uploading reports for the commit 7525bc4 to get more accurate results

@@               Coverage Diff                @@
##           ign-gazebo4     #690       +/-   ##
================================================
- Coverage        77.37%   65.46%   -11.91%     
================================================
  Files              217      235       +18     
  Lines            12217    17019     +4802     
================================================
+ Hits              9453    11142     +1689     
- Misses            2764     5877     +3113     

Impacted Files	Coverage Δ
include/ignition/gazebo/EntityComponentManager.hh	100.00% <ø> (ø)
include/ignition/gazebo/rendering/SceneManager.hh	100.00% <ø> (ø)
.../plugins/component_inspector/ComponentInspector.cc	7.29% <0.00%> (-1.32%)	⬇️
.../plugins/component_inspector/ComponentInspector.hh	28.57% <ø> (ø)
src/systems/diff_drive/DiffDrive.hh	100.00% <ø> (ø)
...rc/systems/performer_detector/PerformerDetector.hh	100.00% <ø> (ø)
src/systems/physics/Physics.hh	100.00% <ø> (ø)
src/systems/scene_broadcaster/SceneBroadcaster.hh	100.00% <ø> (ø)
src/systems/sensors/Sensors.cc	74.75% <0.00%> (ø)
src/rendering/SceneManager.cc	22.69% <8.08%> (ø)
... and 52 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 778ff85...7525bc4. Read the comment docs.

[azeey]

The failing test on Ubuntu is also failing on our ign-gazebo4 CI https://build.osrfoundation.org/job/ignition_gazebo-ci-ign-gazebo4-bionic-amd64/54/, so it's not caused by this PR.

[Blast545]

LGTM

[gonzodepedro]

LGTM