Eliminate ability to keep multiple collision detectors updated

[AndyZe]

MoveIt1 had the ability to keep multiple Collision Detectors updated simultaneously. One was the active_collision_ and they were all stored in a std::map<std::string, CollisionDetectorPtr>. The string in the std::map was a name which (currently) is either "FCL" or "Bullet".

This PR deletes active_collision_ and the std::map. Now, there is only one collision detector active at any time.

I think this should be removed because:

• I can't think of many use cases for switching between multiple collision detectors. @felixvd agrees, per comment here.
• If somebody DID want to switch collision detectors at runtime, they still can with the setCollisionDetector() function. It will just take a little longer because the new collision detector needs to be allocated. In other words, the only downside of this PR is a bit of delay when switching collision detectors at runtime.
• If somebody REALLY wanted to keep multiple collision detectors active at once, they could have a child planning scene with a different collision detector type (I believe - haven't tried that myself).
• There is less iteration over the std::map to find the active collision detector. That should help with speed when the planning scene is updated.
• Removing this feature simplifies the code quite a bit.
• It seems nobody kept multiple collision detectors loaded simultaneously. Otherwise, this bug would have been caught much sooner.

I have not tested this yet but I will soon. Obviously it needs a lot of testing and I'm curious to see if CI passes. But, I would be glad to hear feedback now.

[felixvd]

As mentioned in the linked comment, this is probably a vestigial feature that (close to) no one uses, but you never know. It's easy to project your own view here.

Have you tested that setCollisionDetector() still works? I could imagine this having some use.

[AndyZe]

Have you tested that setCollisionDetector() still works? I could imagine this having some use.

Well, some testing for that is built in. setCollisionDetector() gets called every time a planning scene is initialized.

Also, FCL is the only collision detector type available in MoveIt2 for now, so there is not much point in switching to anything else and it would be hard to test doing so.

I expect all tests to pass now! There is still a bit more testing and cleanup I would like to do before unmarking WIP.

[codecov]

Codecov Report

Merging #364 (a1f8acb) into main (0eb366f) will decrease coverage by 0.01%.
The diff coverage is 52.50%.

@@            Coverage Diff             @@
##             main     #364      +/-   ##
==========================================
- Coverage   53.23%   53.22%   -0.00%     
==========================================
  Files         209      209              
  Lines       18849    18805      -44     
==========================================
- Hits        10032    10007      -25     
+ Misses       8817     8798      -19     

Impacted Files	Coverage Δ
...ene/include/moveit/planning_scene/planning_scene.h	29.63% <0.00%> (-12.67%)	⬇️
.../collision_plugin_loader/collision_plugin_loader.h	100.00% <ø> (ø)
...sion_plugin_loader/src/collision_plugin_loader.cpp	28.13% <0.00%> (ø)
...nning_scene_monitor/src/planning_scene_monitor.cpp	56.30% <ø> (+0.09%)	⬆️
moveit_core/planning_scene/src/planning_scene.cpp	43.79% <72.42%> (-0.07%)	⬇️
...collision_detection/collision_detector_allocator.h	80.00% <0.00%> (-20.00%)	⬇️
.../collision_detection_fcl/src/collision_env_fcl.cpp	87.81% <0.00%> (-0.60%)	⬇️
...e/collision_detection_fcl/src/collision_common.cpp	76.95% <0.00%> (-0.31%)	⬇️
... and 2 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 0eb366f...ba67d4b. Read the comment docs.

[henningkayser]

@AndyZe I'm not sure I follow the motivation of why this should be removed. It seems like a major API change that doesn't bring a lot of upsides besides less code to maintain (which could be a strong argument for it). Someone must have used this feature at some point, otherwise it wouldn't be there. Maintaining multiple planning scenes just for running multiple collision detectors seems like a regression to the current approach. I also don't really follow why it shouldn't be used for custom collision detectors that are currently not open source.

In general, I'm not really against this change, but we should get some more opinions on this by other maintainers.

[AndyZe]

Besides making the code more readable, there will be a bit of a performance benefit. A lot of this type of iteration got eliminated:

for (const std::pair<const std::string, CollisionDetectorPtr>& it : collision_)

And you can still use whatever custom collision detector you want. You just can't keep it running in the background while another one is active. (As it currently stands, only one can be ACTIVE. The others just update in the background.)

[AndyZe]

I've tested with the moveit_cpp and Servo demos now, so I'm going to remove WIP.

[AndyZe]

I added a basic unit test for switching collision detector types. For now, it just switches from one FCL to another FCL detector, but it's better than nothing.

[henningkayser]

@AndyZe I approve these changes in general. When testing this with the run_moveit_cpp demo the collision check actually fails, so there is still something broken in here.

[AndyZe]

Thanks for catching that. The bug comes from deleting a line in PlanningScene::PlanningScene(const PlanningSceneConstPtr& parent). Now I just have to fix it ;)

[AndyZe]

Fixed!

[henningkayser]

Fixed!

Great, thanks! I'll update the branch and merge it if it succeeds CI.