Split CollisionPluginLoader

[rhaschke]

To enable reuse CollisionPluginLoader (from moveit_ros_planning) in moveit_core, but avoid circular dependencies between these two packages, this PR separates the non-ROS part into moveit_core/moveit_collision_detection.so and the ROS part (reading the plugin name from the parameter server) into moveit_ros_planning/moveit_collision_plugin_loader.so (as before).
Additionally, to better distinguish both classes, the original class is renamed to ROSCollisionPluginLoader.

This PR is a prerequisite for further simplification of the tests in #2830.

[v4hn]

Not having looked too deeply into this, is this another case of downstream-packages as test dependencies? I don't see why this is even a problem here, as both packages are in the same repository.

I'm not particularly happy with this patch, especially because it breaks API for no reason in addition to scattering logic even more.

[v4hn]

Not changing this probably prints an error saying "no such method setupScene".
That's a damn surprising error to hit in an interface that was set in stone for 7 years.
I understand why you modified the name here instead of giving the moveit_core version a new name, but that would usually be much more appropriate.

If you insist on the names, at least add a MIGRATIONS.md entry.

[rhaschke]

Better clean than backwards compatible 😉
I will add a comment in MIGRATIONS.

[v4hn]

Better clean than backwards compatible 😉

Clean is relative and I know quite a few people who would like to see you suffer for that kind of statement...
Maybe @gavanderhoorn or @simonschmeisser want to comment?

[codecov]

Codecov Report

Merging #2834 (e235d92) into master (bef8f7c) will increase coverage by 0.06%.
The diff coverage is 39.40%.

@@            Coverage Diff             @@
##           master    #2834      +/-   ##
==========================================
+ Coverage   60.81%   60.86%   +0.06%     
==========================================
  Files         366      368       +2     
  Lines       31717    31715       -2     
==========================================
+ Hits        19286    19301      +15     
+ Misses      12431    12414      -17     

Impacted Files	Coverage Δ
...z_industrial_motion_planner/trajectory_generator.h	100.00% <ø> (ø)
...sion_plugin_loader/src/collision_plugin_loader.cpp	50.00% <ø> (+17.50%)	⬆️
...collision_detection/src/collision_plugin_cache.cpp	23.08% <23.08%> (ø)
...oveit/collision_detection/collision_plugin_cache.h	100.00% <100.00%> (ø)
...strial_motion_planner/src/trajectory_generator.cpp	100.00% <100.00%> (ø)
.../collision_plugin_loader/collision_plugin_loader.h	100.00% <100.00%> (ø)
..._interface/src/detail/constrained_goal_sampler.cpp	74.00% <0.00%> (-2.00%)	⬇️
...octomap_updater/src/pointcloud_octomap_updater.cpp	29.56% <0.00%> (-0.76%)	⬇️
...raint_samplers/src/default_constraint_samplers.cpp	80.42% <0.00%> (-0.29%)	⬇️
moveit_core/robot_state/src/robot_state.cpp	51.56% <0.00%> (-0.08%)	⬇️
... and 10 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 eb6135b...e235d92. Read the comment docs.

[rhaschke]

Not having looked too deeply into this, is this another case of downstream-packages as test dependencies? I don't see why this is even a problem here, as both packages are in the same repository.

CollisionPluginLoader was implemented in moveit_ros_planning, but needs to be used in moveit_core. This would cause a circular dependency.
To avoid that, I splitted off the core part into a CollisionPluginLoader class in moveit_core and only kept the ROS-related method setupScene() in a (renamed) class ROSCollisionPluginLoader in package moveit_ros_planning.

[v4hn]

CollisionPluginLoader was implemented in moveit_ros_planning, but needs to be used in moveit_core. This would cause a circular dependency.

That's only partially true, because it would only be used in tests of moveit_core and not in the package logic itself.
I still believe that should be perfectly valid use and the tools should support it.

I believe with the prbt IK plugin a main problem was that it's in a different repository even, but that's not the case here as moveit_core and moveit_ros_planning live in the same repository.
If there's actually a bug, this should be fixed and I guess your patch works. I was just saying I really don't like to split up logic (that is basically used in one place only!) even more because the tooling does not support downstream package in dependencies for building tests.
In that regard I'm this close to proposing to move all tests into separate moveit_tests_* ros packages to avoid such debatable circular dependencies. That should definitely solve the problem because moveit_tests_planning_scene could perfectly well depend on moveit_ros_planning.

[rhaschke]

That's only partially true, because it would only be used in tests of moveit_core and not in the package logic itself.
I still believe that should be perfectly valid use and the tools should support it.

In theory, I agree with you: As tests are (usually?) build after all packages in a repo, test dependencies shouldn't create a circular dependency. However, in practice, that's not the case.

Of course, I tried using the existing CollisionPluginLoader directly first. But that fails with a circular dependency error:
https://github.com/ubi-agni/moveit/runs/3428653136?check_suite_focus=true (see step build_target_workspace)

Removing the <test_depend>moveit_ros_planning</test_depend> declaration from moveit_core resolves the circular dependency, but makes catkin_lint - naturally - complain about a missing dependency declaration.

[rhaschke]

As a compromise: What about splitting the added functionality of setupScene() completely off?
That is, just provide a free utility function getCollisionPluginName(ros::NodeHandle&) to retrieve the name of the plugin and then call the CollisionPluginLoader::activate() from moveit_core. This would be a much cleaner API.

[v4hn]

As a compromise: What about splitting the added functionality of setupScene() completely off?
That is, just provide a free utility function getCollisionPluginName(ros::NodeHandle&) to retrieve the name of the plugin and then call the CollisionPluginLoader::activate() from moveit_core. This would be a much cleaner API.

Looking further into it, I'm honestly wondering why we should not just migrate the whole class to moveit_core and be done with it. We already have a few mentions of NodeHandle there, so it's not a matter of header includes...

[rhaschke]

I don't understand your crossed-out statement. But, you are right: The usage of NodeHandle was the only reason I proposed to split the ROS-part into moveit_ros. If node handles are used in moveit_core anyway, there is no reason for this artificial split.

[v4hn]

If node handles are used in moveit_core anyway, there is no reason for this artificial split.

I'm not sure whether this should qualify as "use", but we do have a template method for user code that uses it.
However, now that I look into it, we could actually use a forward declaration for that use because the template is never instantiated in moveit_core. But then again, if we do that we might break user code that relies on the template without additional includes.
Same for the only other mention that uses a reference. roscpp is currently not listed in moveit_core/package.xml, so something should be changed either way.

As a compromise: What about splitting the added functionality of setupScene() completely off?
That is, just provide a free utility function getCollisionPluginName(ros::NodeHandle&) to retrieve the name of the plugin and then call the CollisionPluginLoader::activate() from moveit_core. This would be a much cleaner API.

Then you would need to write

CollisionPluginLoader cpl_;
cpl_.activate(collision_detection::getCollisionPluginName(nh_), scene, true);

instead of

CollisionPluginLoader cpl_;
cpl_.setupScene(nh_, scene);

I don't think that's a nicer API. :)
If we want to keep roscpp out of moveit_core (and possibly use forward declarations for the places where they are used),
I would propose to give the moveit_core class in your current PR a new name like CollisionPluginLoaderCore to keep the current API functional.

[rhaschke]

I don't think that's a nicer API.

CollisionPluginLoader cpl_;
cpl_.activate(getCollisionPluginName(nh_), scene, true);

I do think this is cleaner: It is modular and more explicitly states what actually happens.
In any case, I'm open to any of the proposed solutions. Just, let's decide for one of them and progress.

[v4hn]

I do think this is cleaner: It is modular and more explicitly states what actually happens.

I managed to convince myself that you are right... Until I tried to implement it and noticed that it would actually read something like that:

collision_detection::CollisionPluginLoader cpl_;
auto plugin_name = collision_detection::getCollisionPluginName(nh_);
if(!plugin_name.empty())
  cpl_.activate(plugin_name, scene, true);

(I don't think adding an early return for empty string to activate would be any nicer)

Instead I renamed the internal class to keep the current API.
I went with CollisionPluginCache, which just highlights a different aspect of the class.
Because you worked from your organization's repository, I pushed the changes to my own fork:
https://github.com/v4hn/moveit/tree/pr-split-coll-plugin-loader

[rhaschke]

I don't think adding an early return for empty string to activate would be any nicer.

I think, it would be a good idea to augment activate along this line 😉
However, I'm tired to discuss this tiny PR. Just merge with your recent changes.