separating the cartesian planner #1149

mlautman · 2018-10-26T00:31:52Z

Description

This moves the Cartesian planner out of the robot_state class and into it's own class

Checklist

Required by CI: Code is auto formatted using clang-format
Document API changes relevant to the user in the moveit/MIGRATION.md notes
Created tests, which fail without this PR reference
Decide if this should be cherry-picked to other current ROS branches

moveit_core/robot_state/include/moveit/robot_state/cartesian_planner.h

moveit_core/robot_state/include/moveit/robot_state/robot_state.h

moveit_core/robot_state/include/moveit/robot_state/utils.h

moveit_core/robot_state/include/moveit/robot_state/robot_state.h

rhaschke

The old functions should be definitely deprecated and replaced by the new ones throughout the code base.

moveit_core/robot_state/include/moveit/robot_state/robot_state.h

moveit_core/robot_state/test/test_cartesian_planner.cpp

moveit_core/robot_state/include/moveit/robot_state/cartesian_planner.h

moveit_core/robot_state/src/cartesian_planner.cpp

moveit_core/robot_state/test/test_cartesian_planner.cpp

davetcoleman · 2019-03-08T02:59:17Z

moveit_core/robot_state/test/test_cartesian_planner.cpp

+protected:
+  void SetUp() override
+  {
+    static const std::string MODEL2 =


Suggested change

static const std::string MODEL2 =

// TODO(rhaschke): Use new testing framework for loading models

// https://ros-planning.github.io/moveit_tutorials/doc/tests/tests_tutorial.html

static const std::string MODEL2 =

Sorry @davetcoleman, but I really cannot understand your reasoning behind putting me as the assignee for this TODO. The change I requested is a max-half-an-hour effort. If we don't tackle this now, it's probably never done - even though there is a TODO. Why don't you ask @mlautman, who is paid to work on MoveIt, to actually do this cleanup? I already put most of my free time into this project - unsalaried. I cannot do more. Rather, I need to reduce my efforts over time to also see my family from time to time.

davetcoleman · 2019-03-08T03:00:42Z

This class really belongs in its own folder:

moveit/moveit_core/cartesian_planning

or maybe

moveit/moveit_planners/cartesian_planning

But making this first step makes the most sense for now

moveit_core/robot_state/include/moveit/robot_state/cartesian_planner.h

rhaschke

See inline comments.

moveit_core/robot_state/include/moveit/robot_state/cartesian_planner.h

moveit_core/robot_state/src/robot_state.cpp

rhaschke · 2019-03-08T13:42:44Z

moveit_core/robot_state/src/robot_state.cpp

-  return percentage_solved;
-}
-
-double RobotState::testJointSpaceJump(const JointModelGroup* group, std::vector<RobotStatePtr>& traj,


Although these testing functions are currently only used in the Cartesian path generator, I suggest to keep them in RobotState, because they can be useful in other contexts as well.

the user can just as easily use the CartesianInterpolator version. I don't think that robot_state is the ideal place for joint trajectory validation code. Maybe robot_model..?

I'm fine with robot_model too. CartesianInterpolator is very specific though...

This method is currently only used from within CartesianInterpolator. I am happy to see it moved but I don't see that as a necessary change for this PR to be merged in. I have added a todo

Who should work on all these TODOs in future? Now, it's the time to clean this up.

Anyone in the community who wants to make the change. There is no functional difference between what you are suggesting and the current state of the code. Sure we can go back and forth iterating until this relatively small change is philosophically perfect, but is that really worth our time? If you would like those methods put into robot_model please open a follow on PR and I will approve it.

@rhaschke I looked into it and moving the validation functions into moveit_trajectory_processing (which is probably where they belong) creates a dependency cycle. While I am sure we could break the cycle with some work, I think we can leave it for a future PR that refactors the cartesian planning feature more fully.

rhaschke · 2019-03-08T14:01:51Z

moveit_core/robot_state/test/test_cartesian_planner.cpp

+protected:
+  void SetUp() override
+  {
+    static const std::string MODEL2 =


Sorry @davetcoleman, but I really cannot understand your reasoning behind putting me as the assignee for this TODO. The change I requested is a max-half-an-hour effort. If we don't tackle this now, it's probably never done - even though there is a TODO. Why don't you ask @mlautman, who is paid to work on MoveIt, to actually do this cleanup? I already put most of my free time into this project - unsalaried. I cannot do more. Rather, I need to reduce my efforts over time to also see my family from time to time.

davetcoleman · 2019-03-08T23:59:10Z

@rhaschke I didn't mean to imply you needed to actually do the TODO, but we've adopted a practice of adding a user name to all TODOs to provide more reference in the future of who wanted something done. It doesn't mean you need to do it. But we totally can leave your name off of there, or add my name.

Asking Mike to refactor a test that's been in the moveit code for many years now seems to me like scope creep for the intent of this PR: to move a Cartesian path planner out of the robot state code. Yes, its a great idea to fix it, but we do not have unlimited resources either. I understand you need time with your family and certainly am not asking you to make sacrifices for this, either. Does this make senes?

davetcoleman · 2019-03-12T19:06:31Z

moveit_core/robot_state/include/moveit/robot_state/robot_state.h

@@ -49,6 +49,8 @@

 #include <boost/assert.hpp>

+// Eventually, this planner should be moved out of robot_state


this comment is in the wrong header file

rhaschke

There is still an open request from Dave and I suggest to move the implementations of all deprecated functions from robot_state.cpp to robot_state.h (see motivation below).

rhaschke · 2019-03-13T09:55:56Z

moveit_core/robot_state/include/moveit/robot_state/robot_state.h

+     NOTE: As of ROS-Melodic these are deprecated and should not be used
+     */
+  MOVEIT_DEPRECATED double
+  computeCartesianPath(const JointModelGroup* group, std::vector<RobotStatePtr>& traj, const LinkModel* link,


All deprecated functions should have their implementation inline here in the header as this a) provides a hint how to replace those functions in own code and b) doesn't generate extra function call overhead.

@rhaschke I tried that but unfortunately that would require that I move the include (#include <moveit/robot_state/cartesian_interpolator.h> ) from the src to header which fails to compile.

My understanding is that the implementation of computeCartesianPath needs MaxEEFStep and JumpThreshold, which are defined in cartesian_interpolator.h. cartesian_interpolator.h in turn includes robot_state.h to get the RobotState class definition. If I move the implementations inline to robot_state.h then I would have to include cartesian_interpolator.h in robot_state.h instead of robot_state.cpp. This results in compiler errors where the cartesian_interpolater.h included in robot_state.h includes an empty robot_state.h and fails to get the RobotState class definitions.

Also, I have moved the rogue comment that Dave found.

This results in compiler errors where the cartesian_interpolater.h included in robot_state.h includes an empty robot_state.h

Why does robot_state.h becomes empty??

mlautman · 2019-03-21T21:04:52Z

@rhaschke At almost 50 comments, I feel we have debated this PR to death considering that it is a relatively trivial change that we all agree should happen. Let's get this merged

mlautman · 2019-03-21T23:29:06Z

@rhaschke The deprecated methods are causing travis to fail.

rhaschke · 2019-03-22T12:26:50Z

Let's get this merged.

@mlautman, as Travis fails, I don't see, how we can merge this PR in the present state.

The deprecated methods are causing Travis CI to fail.

Regarding the actual Travis issue: Travis complains about deprecation warnings. Obviously, when we suggest a new API, we should also change the whole MoveIt code base to use the new API. Doing so, you will get rid of those warnings.

davetcoleman · 2019-03-26T20:07:10Z

Seems to me deprecated functions should no cause Travis to fail. If we learn to ignore Travis because of harmless warnings, we may learn also to ignore it for important stuff.

mlautman requested a review from rhaschke October 26, 2018 00:58

davetcoleman requested changes Oct 26, 2018

View reviewed changes

rhaschke requested changes Nov 19, 2018

View reviewed changes

moveit_core/robot_state/include/moveit/robot_state/robot_state.h Outdated Show resolved Hide resolved

moveit_core/robot_state/test/test_cartesian_planner.cpp Outdated Show resolved Hide resolved

moveit_core/robot_state/test/test_cartesian_planner.cpp Outdated Show resolved Hide resolved

rhaschke mentioned this pull request Nov 21, 2018

Moving computeCartesianPath out of robot_state #1142

Closed

mlautman changed the base branch from kinetic-devel to master March 8, 2019 00:09

mlautman changed the base branch from master to kinetic-devel March 8, 2019 00:10

mlautman changed the base branch from kinetic-devel to master March 8, 2019 01:15

mlautman requested review from davetcoleman and rhaschke March 8, 2019 01:51

davetcoleman approved these changes Mar 8, 2019

View reviewed changes

davetcoleman reviewed Mar 8, 2019

View reviewed changes

moveit_core/robot_state/include/moveit/robot_state/cartesian_planner.h Outdated Show resolved Hide resolved

rhaschke requested changes Mar 8, 2019

View reviewed changes

mlautman requested a review from rhaschke March 12, 2019 17:46

davetcoleman reviewed Mar 12, 2019

View reviewed changes

rhaschke requested changes Mar 13, 2019

View reviewed changes

mlautman requested a review from rhaschke March 21, 2019 21:01

mlautman requested a review from v4hn as a code owner March 21, 2019 21:02

separating the cartesian planner

6136fba

removing deprecated warnings

8c9ddab

mlautman requested a review from IanTheEngineer as a code owner March 22, 2019 22:44

rhaschke approved these changes Mar 24, 2019

View reviewed changes

rhaschke merged commit fb53bf3 into moveit:master Mar 24, 2019

mlautman deleted the separate-cartesian branch March 25, 2019 19:59

gavanderhoorn mentioned this pull request Oct 18, 2019

steep changes in configuration space after planning #1696

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

separating the cartesian planner #1149

separating the cartesian planner #1149

mlautman commented Oct 26, 2018 •

edited

Loading

rhaschke left a comment

davetcoleman Mar 8, 2019

rhaschke Mar 8, 2019

davetcoleman commented Mar 8, 2019

rhaschke left a comment

rhaschke Mar 8, 2019

mlautman Mar 8, 2019

rhaschke Mar 8, 2019

mlautman Mar 8, 2019

rhaschke Mar 9, 2019

mlautman Mar 11, 2019 •

edited

Loading

mlautman Mar 12, 2019 •

edited

Loading

rhaschke Mar 8, 2019

davetcoleman commented Mar 8, 2019

davetcoleman Mar 12, 2019

rhaschke left a comment

rhaschke Mar 13, 2019

mlautman Mar 21, 2019

rhaschke Mar 24, 2019

mlautman commented Mar 21, 2019

mlautman commented Mar 21, 2019

rhaschke commented Mar 22, 2019

davetcoleman commented Mar 26, 2019

		@@ -49,6 +49,8 @@

		#include <boost/assert.hpp>

		// Eventually, this planner should be moved out of robot_state

separating the cartesian planner #1149

separating the cartesian planner #1149

Conversation

mlautman commented Oct 26, 2018 • edited Loading

Description

Checklist

rhaschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davetcoleman commented Mar 8, 2019

rhaschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlautman Mar 11, 2019 • edited Loading

Choose a reason for hiding this comment

mlautman Mar 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davetcoleman commented Mar 8, 2019

Choose a reason for hiding this comment

rhaschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlautman commented Mar 21, 2019

mlautman commented Mar 21, 2019

rhaschke commented Mar 22, 2019

davetcoleman commented Mar 26, 2019

mlautman commented Oct 26, 2018 •

edited

Loading

mlautman Mar 11, 2019 •

edited

Loading

mlautman Mar 12, 2019 •

edited

Loading