-
Notifications
You must be signed in to change notification settings - Fork 943
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
hot fix #1012 #1086
hot fix #1012 #1086
Conversation
…nners (#1012) * added funcitonality for CHOMP Optimization Adapter and made some changes in planning_scene.h / .cpp files to make this work * added comments and cleaned up some code * added funcitonality for optimizing path obtained from OMPL, underconstruction but works fine right now * did some commenting and cleaning up of code * cleaned up some code and added comments * corrected my previous public/private small bug in planning_scene.h * changed CHOMP Parameter name to make more sense trajectory_initialization_method * added functionality to initialize trajectories in CHOMP when input trajectory has more points than CHOMP trajectory points * changed author for chomp_optimizer_adapter.cpp * addressed Dave's PR requested changes * addressed PR requested changes added a new helper function to make chomp_initialization from trajectory simpler, now has if, for, for * changed OMPL to fillTrajectory as the parameter value * just a dot * added chomp_motion_planner dependency in moveit_ros_planning
@raghavendersahdev, please could you have a look too? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing the planning scene!
moveit_ros/planning/package.xml
Outdated
@@ -34,6 +35,7 @@ | |||
<run_depend>dynamic_reconfigure</run_depend> | |||
<run_depend>angles</run_depend> | |||
<run_depend>eigen_conversions</run_depend> | |||
<run_depend>chomp_motion_planner</run_depend> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this necessary? it will require that we start releasing chomp as debians @130s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original PR was aiming for that, we may just as well allow releasing. The version numbers within the CHOMP packages will need to be updated to the rest of the stack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chomp_motion_planner
is part of MoveIt anyway, isn't it?moveit_planners/chomp/chomp_motion_planner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its always been more experimental and has never been released, but sure, let's release it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following this up in ros-gbp/moveit-release#5
moveit_ros/planning/planning_request_adapter_plugins/src/chomp_optimizer_adapter.cpp
Show resolved
Hide resolved
moveit_ros/planning/planning_request_adapter_plugins/src/chomp_optimizer_adapter.cpp
Show resolved
Hide resolved
const planning_interface::MotionPlanRequest& req, | ||
planning_interface::MotionPlanResponse& res, | ||
std::vector<std::size_t>& added_path_index) const | ||
{ | ||
// following call to planner() calls the OMPL planner and stores the trajectory inside the MotionPlanResponse res | ||
// variable which is then used by CHOMP for optimization of the computed trajectory | ||
bool solved = planner(planning_scene, req, res); | ||
bool solved = planner(ps, req, res); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be const
just as planning_success
.
I'm also wondering if this function should return solved && planning_success
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be const just as planning_success.
???
I'm also wondering if this function should return solved && planning_success?
Feel free to modify the return value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was returning only solved
so that in case CHOMP is not able to find a path, the planner still returns the OMPL's computed path and returns true, if we return the value as solved && planning_success
this would imply both CHOMP (with OMPL's / STOMP's initial guess) and the initial planner (e.g., OMPL/STOMP) need to produce a correct path.. In most cases (almost all cases) with OMPL's / STOMP's initial guess CHOMP produces a collision free path.. so yes doing solved && planning_success
might make sense here ..
On the other hand, In case if planning_scene == false
May be just a message using ROS_INFO could be used if required to indicate to the user that CHOMP adapter failed so only using OMPL's path to compute the solution .... I have no strong preference for this change here, what ever you feel is good can be done here....
However, I am a bit inclined with displaying the user with a message that CHOMP failed and Initial planner succeeded so using only one planner and discarding CHOMP adapter's usage..
Thanks @rhaschke for fixing this up! |
afe7bdb
to
e7d4cf8
Compare
…nners (moveit#1012) * added functionality for CHOMP Optimization Adapter and made some changes in planning_scene.h / .cpp files to make this work * added functionality for optimizing path obtained from OMPL, under construction but works fine right now * added functionality to initialize trajectories in CHOMP when input trajectory has more points than CHOMP trajectory points * hotfix moveit#1086
Thanks @rhaschke and sorry for getting to this late, I was held up with some other work, I just tested your changes, it works fine and is better than my way 👍 Small side note: I am not very well versed with Travis, but I see here this PR #1086 first built successfully (#2696) and a second later instance of it (#2697) error'ed, not sure why... maybe #2696 is a later instance than #2697 not sure....I might be wrong here.. just mentioning... |
…ent)) Hopefully this fixes the issue during bloom moveit/moveit#1083 (comment) As far as `chomp` goes, I'm not sure if we still ignore `moveit_planners_chomp`. Looks like it's not depended from `package.xml`, so it might be ok. ``` $ git log -1 kinetic-devel commit 2903081c904602ffeca8d71592688c93ca8adc90 Author: Isaac I.Y. Saito <130s@2000.jukuin.keio.ac.jp> Date: Wed Oct 24 04:23:04 2018 -0700 0.9.13 $ ack-grep -i moveit_planners_chomp . moveit_planners/chomp/chomp_interface/test/chomp_moveit.test 3: <include file="$(find moveit_planners_chomp)/test/rrbot_move_group.launch"/> 5: <test test-name="chomp_test" pkg="moveit_planners_chomp" type="chomp_moveit_test" time-limit="80.0"/> moveit_planners/chomp/chomp_interface/test/rrbot_chomp_planning_pipeline.launch.xml 12: <rosparam command="load" file="$(find moveit_planners_chomp)/test/chomp_planning.yaml"/> moveit_planners/chomp/chomp_interface/test/rrbot_move_group.launch 9: <param name="$(arg robot_description)" command="$(find xacro)/xacro --inorder '$(find moveit_planners_chomp)/test/rrbot.xacro'"/> 12: <param name="$(arg robot_description)_semantic" textfile="$(find moveit_planners_chomp)/test/rrbot.srdf" /> 16: <rosparam command="load" file="$(find moveit_planners_chomp)/test/joint_limits.yaml"/> 35: <include file="$(find moveit_planners_chomp)/test/rrbot_chomp_planning_pipeline.launch.xml" /> 46: <!--include file="$(find moveit_planners_chomp)/test/moveit_rviz.launch"> moveit_planners/chomp/chomp_interface/CMakeLists.txt 2:project(moveit_planners_chomp) moveit_planners/chomp/chomp_interface/package.xml 2: <name>moveit_planners_chomp</name> ```
…moveit#1086 (comment)) Hopefully this fixes the issue during bloom moveit/moveit#1083 (comment) As far as `chomp` goes, I'm not sure if we still ignore `moveit_planners_chomp`. Looks like it's not depended from `package.xml`, so it might be ok. ``` $ git log -1 kinetic-devel commit 2903081c904602ffeca8d71592688c93ca8adc90 Author: Isaac I.Y. Saito <130s@2000.jukuin.keio.ac.jp> Date: Wed Oct 24 04:23:04 2018 -0700 0.9.13 $ ack-grep -i moveit_planners_chomp . moveit_planners/chomp/chomp_interface/test/chomp_moveit.test 3: <include file="$(find moveit_planners_chomp)/test/rrbot_move_group.launch"/> 5: <test test-name="chomp_test" pkg="moveit_planners_chomp" type="chomp_moveit_test" time-limit="80.0"/> moveit_planners/chomp/chomp_interface/test/rrbot_chomp_planning_pipeline.launch.xml 12: <rosparam command="load" file="$(find moveit_planners_chomp)/test/chomp_planning.yaml"/> moveit_planners/chomp/chomp_interface/test/rrbot_move_group.launch 9: <param name="$(arg robot_description)" command="$(find xacro)/xacro --inorder '$(find moveit_planners_chomp)/test/rrbot.xacro'"/> 12: <param name="$(arg robot_description)_semantic" textfile="$(find moveit_planners_chomp)/test/rrbot.srdf" /> 16: <rosparam command="load" file="$(find moveit_planners_chomp)/test/joint_limits.yaml"/> 35: <include file="$(find moveit_planners_chomp)/test/rrbot_chomp_planning_pipeline.launch.xml" /> 46: <!--include file="$(find moveit_planners_chomp)/test/moveit_rviz.launch"> moveit_planners/chomp/chomp_interface/CMakeLists.txt 2:project(moveit_planners_chomp) moveit_planners/chomp/chomp_interface/package.xml 2: <name>moveit_planners_chomp</name> ```
Unfortunately, #1012 was merged while still containing severe issues:
@davetcoleman or @bmagyar Please review and merge asap, as kinetic-devel is broken.