Remove outdated and unused code

[henningkayser]

This epic is meant to coordinate efforts that make MoveIt leaner which helps us with keeping up with maintenance and and with making bigger changes in the future. It is also a first step for disentangling the code base. Here is a list of things that can either be removed or better be replaced with other libraries.

moveit_core

• background_processing: only used for visualization, also there are probably better options for this. Move out of moveit_core or replace with better third-party library
• backtrace: not used or needed, remove
• dynamics_solver: remove, this should be a property of an IK solver if needed, not a library in moveit_core, only used in MotionPlanningPanel for showing dynamic metrics. If we want to keep it, it doesn’t belong into moveit_core
• exceptions: remove, we don’t use exceptions (https://github.com/ros-planning/moveit2/pull/1117)
• kinematics_metrics: remove, only used in MotionPlanningPanel. If we want to keep it, it doesn’t belong into moveit_core
• macros: move out of moveit_core or remove entirely? we might not want to use these in moveit_core, but in general I don’t care too much about these
• profiler: remove and replace with external tool like ros2_tracing?
• python: move into separate package
• sensor_manager: remove, only used in plan_with_sensing which should be removed as well.
  • I think there would be a need for a “SensorManager” which really only maintains the available sensors (camera, laser scanner etc) and provides interfaces for these, but this would be a separate effort
• transforms: not sure about this one, it’s used very often but it seems like this should be an external lib like geometry2
• utils: different package? at least split into separate test_utils

moveit_ros

• planning/plan_execution/plan_with_sensing
• manipulation

moveit_planners

• sbpl

[AndyZe]

I think the controller_manager should also be removed since:

• MoveIt's use of controllers is becoming more diverse and advanced
• it's easy enough to manage controller switching outside of MoveIt
• The controller manager is confusing. I get questions about it all the time. (Like, why are there 2 yaml files that are almost identical?)

[v4hn]

I just saw this issue, so let me give my two cents for the discussed modules

• ROS manipulation

yes, the interfaces in MGI should target the MTC capability instead (my fault for not reviewing/reworking the plugin there, I believe it needs more work)

• controller_manager should also be removed

Yes and no. It should be severely simplified at least and doesn't need the plugin interface. I do see a lot of merit in being able to specify which controllers should be used to actuate a given trajectory inside MoveIt. But to do that we could simply forward the requested controllers to the typical ros_control service API (assuming it exists) before execution. The obscure logic to decide the controller to use before execution should just be dropped. My main point about this years ago was that ros_control does it better anyway, so why even give yet another plugin API in MoveIt.

• dynamics_solver: remove, this should be a property of an IK solver if needed, not a library in moveit_core, only used in MotionPlanningPanel for showing dynamic metrics. If we want to keep it, it doesn’t belong into moveit_core

So you would want to remove RobotState::getJacobian as well and ban it to the IK solver plugins (which would implement it in very similar ways each)? These two methods are rather similarly important from my perspective and basic operators in robotics in their respective aspects. It's just that we (sadly) don't provide more interfaces that use dynamics handling. For example it would be brilliant to see this integrated with OMPL and the planner interfaces for better optimization objectives.

• kinematics_metrics: remove, only used in MotionPlanningPanel. If we want to keep it, it doesn’t belong into moveit_core

This could be renamed/simplified to something directly stating "ManipulabilityIndex", but aside from that it's a smaller sibling version of the other two. This is free "you can compute that with MoveIt" functionality, that some people consider basic computations on robot kinematic structures.

• macros: move out of moveit_core or remove entirely? we might not want to use these in moveit_core, but in general I don’t care too much about these

What's wrong with MOVEIT_CLASS_FORWARD which is used virtually everywhere? The deprecation.h header is indeed outdated, but aside from that I don't see what you would remove? (console_colors.h would be a candidate if you had an equally simple workaround)

• transforms: not sure about this one, it’s used very often but it seems like this should be an external lib like geometry2

It's how the planning scene maintains transforms and @JafarAbdi even failed to remove it for ROS at some point in the past because the API permeates quite a lot.

• utils: different package? at least split into separate test_utils

yes to splitting out the test utils. The rest seems core enough to me (it's basically a few free functions you need everywhere). But if you insist on keeping moveit_utils separately for no gain but more overhead, go ahead?

• python: move into separate package

Is it common for python wrappers to be in separate packages? Sounds weird to me, but if you think you gain something that justifies more overhead with package version synchronization, it might make sense...

• sensor_manager

I guess I gave my opinion here already.

• sbpl

It sure is dead code. :+1:

[v4hn]

For two additional "external" components we depend on I would like to add to the list

• random_numbers (this should have died/adjusted looong ago. #include <random> should suffice for all we need. We might be interested to keep the functionality from one or two functions there, but nothing more.

• eigen_stl_containers. To the best of my knowledge (I did not investigate again writing this) this whole module should be obsolete with C++17.

[schornakj]

The obscure logic to decide the controller to use before execution should just be dropped.

@v4hn There are several important cases where the TrajectoryExecutionManager is expected to just figure out which controller to use when executing subtrajectories that do not specify a controller. For example, MTC's ExecuteTaskSolution capability doesn't have a way to get controller names for the different parts of the solution when putting together an ExecutableMotionPlan. I agree that the TEM's controller prioritization logic is a good thing to eliminate (it's incompletely implemented anyway -- it doesn't even have a way to get whether a given controller is active or default currently). How should we approach introducing the requirement that trajectories need to include controller info to be executed?

[v4hn]

I believe you got me wrong there. I meant the obscure logic, that compares subsets of controllers to choose which to activate using joint subsets. there's quite a lot of code there that makes you wonder what its use-case ever was.

for all existing use-cases I know of there is only a single such subset because each joint is mapped to exactly one controller. For all further use-cases I can come up with it would suffice to be able to specify a different controller with a specific trajectory for the execution managers to change the controller if necessary.

[mikeferguson]

Just noticed this ticket - I'd like to pipe in with one additional note about controller stuff: not everyone uses ros2_control on their robot (there are other interfaces out there) - so ideally whatever simplify towards does not require leveraging the ros2_control configuration stuff (control_msgs/FollowJointTrajectory is really the kind of API level that should be leveraged - mapping joints to particular action servers).

[henningkayser]

I'm closing this since the original purpose of this epic is fulfilled (identifying and removing stale, commented, unused code). There are still some ongoing refactorings that go beyond removal (random_numbers, exceptions) so I removed them from this epic. I like the discussions about better controller support, but they are clearly off-topic here and should be continued in a different issue.