moveit / moveit

:robot: The MoveIt motion planning framework
http://moveit.ros.org
BSD 3-Clause "New" or "Revised" License
1.66k stars 947 forks source link

cartesian_path_service does not use planning request adapters #453

Open mmoerdijk opened 7 years ago

mmoerdijk commented 7 years ago

The cartesian_path_service does only (and always) use the ITPT planning request adapter. I'm aware that most people here are aware of this but it was not what I expected. I would expect the cartesian path planning service to be configurable like the ompl planning pipeline.

It would be even better if it would be possible to configure some planning request adapters for all planning pipeline/services. After all it makes sense that if an uniform sample filter is required for the ompl pipeline it also is required for the cartesian path service. The same is true if I would like to use an other time parameterisation.

Are there plans to implement something like this? If there are I'll be willing to help out, but I'm relatively new to moveit and I would need some help.

v4hn commented 7 years ago

I don't think we will change the interface to the cartesian path generator to support the general planning request adapter setup any time soon. The path generator is basically just a C++ method of the RobotState class, compared to the whole planner-interface for OMPL et. al. If someone provides the pull-request, we can talk about it, but personally I believe there is little merit in it.

On the other hand you are right in that the hard-coded time parameterization should probably be configurable once we have other working methods upstream. A simple ROS parameter to choose, or disable the parameterization if you do it yourself afterwards, could suffice imo.

Background information: Until not too long ago there was no time parameterization in the capability. I just added IPTP (:wink:) in https://github.com/ros-planning/moveit/commit/fc7da4476014e9dcf9a5da8b1c56a99a3b2e15f4

davetcoleman commented 7 years ago

+1 to configurable time parameterization feature for the cartesian service, a PR is very welcome

mmoerdijk commented 6 years ago

I Just found out that the pick and place pipeline as hardcoded time parameterizations aswel...

I think this issue is more relevant now as we have a other time paramterization method.

At this point I don't know exactly where/how I would start making this time paramterization configurable. We could add an time parameterization abstraction layer. But it would be better if the planning pipeline would be respected.

for the sake of compleetness:

grep -r trajectory_processing::IterativeParabolicTimeParameterization gives:

moveit_ros/planning/planning_request_adapter_plugins/src/add_time_parameterization.cpp:  trajectory_processing::IterativeParabolicTimeParameterization time_param_;
moveit_ros/planning_interface/move_group_interface/src/wrap_python_move_group.cpp:      trajectory_processing::IterativeParabolicTimeParameterization time_param;
moveit_ros/manipulation/pick_place/include/moveit/pick_place/approach_and_translate_stage.h:  trajectory_processing::IterativeParabolicTimeParameterization time_param_;
moveit_ros/move_group/src/default_capabilities/cartesian_path_service_capability.cpp:          trajectory_processing::IterativeParabolicTimeParameterization time_param;
moveit_planners/chomp/chomp_interface/src/chomp_planning_context.cpp:    trajectory_processing::IterativeParabolicTimeParameterization itp;
v4hn commented 6 years ago

I don't know exactly where/how I would start making this time paramterization configurable.

I believe it is well-justified to create a new class_loader plugin interface for time parameterization, now that we have different options available. It is likely that we will see other types (e.g. biological velocity profiles) in the future too.

So in this case we need a new base class, should change the other classes to implement this plugin interface and change all usages of the time parameterization module within MoveIt to instantiate a parameterized plugin to use. This also includes the add_time_parameterization planning request adapter. For guidance on how this should be done you can basically look at how KinematicsBase plugins are implemented in MoveIt.

I would very much welcome a pull-request that attempts this!

drewhamiltonasdf commented 3 years ago

Would anyone care to offer up a "temporary solution" that would be the most useful while we wait for someone more competent to tackle the bigger picture issues? I've been enjoying the process of playing with MoveIt's guts lately, and I feel like I might as well contribute what I can. My last pull request was pretty similar: just adding access to variables via parameters.

I'd be interested in tackling the bigger project too, but I would probably need quite a bit of coaching, and I doubt that would be worth it.