Fixed Bullet collision checker not taking ACM defaults into account.

[werner291]

Description

The acmCheck method in bullet_utils.h directly looked up the relevant entry in the AllowedCollisionMatrix, ignoring any default values. This PR replaced the method call with getAllowedCollision, which appears to be the correct method (please review).

Checklist

• Required by CI: Code is auto formatted using clang-format
• Extend the tutorials / documentation reference
• Document API changes relevant to the user in the MIGRATION.md notes
• Create tests, which fail without this PR reference
• Include a screenshot if changing a GUI
• While waiting for someone to review your request, please help review another open pull request to support the maintainers

[werner291]

Looking deeper into it, PlanningScene::setPlanningSceneDiffMsg seems to ignore default entries as well.

https://github.com/ros-planning/moveit/blob/ffe4697696147f0e4db116a19f70cde7c5f5d074/moveit_core/planning_scene/src/planning_scene.cpp#L1230

Is that intended?

[codecov]

Codecov Report

Merging #2871 (c931a1a) into master (ffe4697) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2871      +/-   ##
==========================================
- Coverage   61.02%   61.00%   -0.01%     
==========================================
  Files         373      373              
  Lines       31731    31731              
==========================================
- Hits        19360    19354       -6     
- Misses      12371    12377       +6     

Impacted Files	Coverage Δ
...detection_bullet/bullet_integration/bullet_utils.h	81.20% <100.00%> (ø)
.../ompl_interface/src/detail/constrained_sampler.cpp	43.25% <0.00%> (-21.62%)	⬇️
...eit_ros/manipulation/pick_place/src/pick_place.cpp	89.48% <0.00%> (-3.15%)	⬇️
...ipulation/pick_place/src/manipulation_pipeline.cpp	69.45% <0.00%> (-2.77%)	⬇️
...on/pick_place/src/approach_and_translate_stage.cpp	73.38% <0.00%> (-0.64%)	⬇️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp	77.87% <0.00%> (-0.38%)	⬇️
...raint_samplers/src/default_constraint_samplers.cpp	80.42% <0.00%> (-0.29%)	⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp	68.13% <0.00%> (-0.13%)	⬇️
moveit_core/robot_model/src/joint_model_group.cpp	63.86% <0.00%> (+2.12%)	⬆️
...e/src/parameterization/model_based_state_space.cpp	71.33% <0.00%> (+2.80%)	⬆️

---

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 ffe4697...c931a1a. Read the comment docs.

[werner291]

Update: my code still doesn't seem to work despite this fix. I'll keep working on it.

[simonschmeisser]

Maybe try reproducing your problem with small test cases that can be included as unit tests in MoveIt. We need a lot more of them especially when it comes to Bullet integration it seems

[werner291]

Nope, it was actually just due to my code linking against an old version of MoveIt. After updating the fix works, and the default ACM entries are followed correctly!

More test cases definitely wouldn't be a bad idea, though.

Please review for merging?

[v4hn]

Trivial fix for a problem that should never have existed! Thanks @werner291 🏅

More test cases definitely wouldn't be a bad idea, though.

It would be great if you could add a test for this in a followup PR. Sadly, if users do not provide tests, nobody will ever write them. Without them, it's of course difficult to ensure your use-case does not break again at some point.

[simonschmeisser]

Also this feature does not seem to be used anywhere in MoveIt so it's not that surprising its broken. What's your use-case if I may ask?

[v4hn]

True, though of course the user can fill these fields in messages. I would not expect MoveIt "to use" this anywhere else, unless a moveit_resources model makes use of it for tests.
But then, I'm really surprised to see srdfdom/moveit_setup_assistant/robot_model_loader do not have a way to set these entries!
It would be perfectly reasonable to support <disable_collisions link1="xyz" link2="*"/>, e.g. for objects that are only added for visual effect (or reference) or must never collide with anything no matter what.

[simonschmeisser]

Since noetic you can simply omit the <collision> part of the URDF for that. Or use markers. But I admit that we tend to have a visual sphere at the TCP and a collision sphere somewhere inside the gripper where it can be disabled with <disable_collisions> conveniently ...

[werner291]

Also this feature does not seem to be used anywhere in MoveIt so it's not that surprising its broken. What's your use-case if I may ask?

My use case is a quadcopter (Yes, with a floating joint! That's why I'm finding all the bugs...) with an arm which must navigate around a tree, whereby the leaves are not counted as hard "do not ever touch or else..." obstacles, but rather as a fuzzy obstacle that should generally be avoided, but can be traversed when needed, but only in certain ways. Hence, I wanted to see them in Rviz, and I may need to check collisions at some point, but I needed to disable collisions during the global path planning since otherwise the task becomes literally impossible.

[v4hn]

Nice use case! If you're allowed, please share some videos on Discord or discourse.ros.org once you have something to show :-)

[werner291]

There's a Discord server?

[v4hn]

There's a Discord server?

https://moveit.ros.org/support/ says:

Join the [MoveIt Discord channel](https://discord.com/invite/RrySut8) to view or post questions

If you can think of a better way to advertise the server, please send a pull-request for the website :)