improve [get|set]JointValueTarget in python wrapper #858

rhaschke · 2018-04-25T08:48:38Z

As pointed out in #845, the signature and semantics of getJointValueTarget() doesn't match the one of the corresponding setter, which renders both functions rather useless. As I pointed out in #845 (comment) a proper solution of this issue, should consider:

Add a new method MoveGroupInterface::getJointValueTarget(std::vector<double>&) that matches the corresponding setJointValueTarget(const std::vector<double>&) method and should use RobotState::copyJointGroupPositions(const JointModelGroup*, std::vector<double>&) to do so.
Depreacate the old method MoveGroupInterface::getJointValueTarget()
(currently returning the full RobotState) and
Instead add a better-named method MoveGroupInterface::getJointStateTarget() to correctly reflect the semantics of returning the full robot state.
To make the difference even clearer, one could think about renaming the getter to getTargetRobotState(), both in MoveGroupInterface and MoveGroupInterfaceImpl.
Deprecate the dangerous functions setJointValueTarget(const robot_state::RobotState&) and setJointValueTarget(const sensor_msgs::JointState&), which allow to set joint values that are not member of the JointModelGroup.

This PR considers all of them as well as a sanity check in some other joint setters.

rhaschke · 2018-04-25T08:56:41Z

@willcbaker Please review.

henningkayser

This has been on pause for some time.
I think your changes really make sense, I'm just a little hesitant to make those setJointValueTarget() functions deprecated right away since they are really convenient and could be 'fixed' easily.

moveit_ros/planning_interface/move_group_interface/src/wrap_python_move_group.cpp

henningkayser · 2019-03-04T16:32:27Z

...ng_interface/move_group_interface/include/moveit/move_group_interface/move_group_interface.h

@@ -400,7 +399,7 @@ class MoveGroupInterface

      If these values are out of bounds then false is returned BUT THE VALUES
      ARE STILL SET AS THE GOAL. */
-  bool setJointValueTarget(const sensor_msgs::JointState& state);
+  MOVEIT_DEPRECATED bool setJointValueTarget(const sensor_msgs::JointState& state);


Why not just change the implementation to match setJointValueTarget(const std::map<std::string, double>& variable_values)? It would probably make sense to use a setJointValueTarget(const std::vector<std::string>& variable_names, const std::vector<double>& variable_names) for that.

There is already a setJointValueTarget(const std::map<std::string, double>& variable_values).
I'm not sure about your intention.

My intention is to keep support for setting target states from robot trajectory waypoints directly (from both message and class types). I don't get why setJointValueTarget(const sensor_msgs::JointState& state) should be deprecated only because there can be joints that are not member of the group. IMO this function should just ignore non-member joints just like setJointValueTarget(const std::map<std::string, double>& variable_values) does.
Alternatively I would suggest adding a setJointValueTarget(const std::vector<std::string>& variable_names, const std::vector<double>& variable_names) function that is more convenient to use than the map.

My intention is to keep support for setting target states from robot trajectory waypoints directly

Makes sense. Please feel free to augment to this PR branch.

henningkayser · 2019-03-04T16:40:41Z

...ng_interface/move_group_interface/include/moveit/move_group_interface/move_group_interface.h

@@ -362,7 +361,7 @@ class MoveGroupInterface

      If these values are out of bounds then false is returned BUT THE VALUES
      ARE STILL SET AS THE GOAL. */
-  bool setJointValueTarget(const robot_state::RobotState& robot_state);
+  MOVEIT_DEPRECATED bool setJointValueTarget(const robot_state::RobotState& robot_state);


I agree that these functions should not set any positions other than those inside the corresponding joint model group.
Rather than making these deprecated I would really prefer changing the implementation to just use RobotState::copyJointGroupPositions() or RobotState::getVariablePositions() selectively.

I my opinion, the RobotState shouldn't be exposed by move-group methods.

In that case setStartState(robot_state::RobotState) should be deprecated as well. But does this really improve usability?

rhaschke

@henningkayser LGTM.

...ng_interface/move_group_interface/include/moveit/move_group_interface/move_group_interface.h

willcbaker · 2019-03-14T20:27:59Z

Iirc I had added robotstate python bindings a while back to expose the robot state methods as at the time it was one of the only ways to get the current full state of the robot and set multidof joint goals.

henningkayser · 2019-03-18T19:10:39Z

Iirc I had added robotstate python bindings a while back to expose the robot state methods as at the time it was one of the only ways to get the current full state of the robot and set multidof joint goals.

@willcbaker so you agree with this changes or not?
@rhaschke could you fix CI so we can get this merged?

willcbaker · 2019-03-19T01:24:24Z

I haven't had much time to play around with this yet, but it from what I can tell by reading:

RE: The get target robot state being protected:

The set_start_state is difficult to use (in general). And to @henningkayser 's point improve [get|set]JointValueTarget in python wrapper #858 (comment) this does not increase usability. The public method was used to retrieve potential future robot states and nicely complemented this function.
I had in the past been using this method in the python wrapper as a way to concatenate plans together to make the robot do complex behaviors, e.g.

group.set_joint_value_target(x0,x1,..xn)
plan  = group.plan()
next_start_state = function_to_extract_robot_state_from_trajectory(plan)
group.set_start_state(next_start_state)
group.set_joint_value_target(y0,y1,..yn)
plan2 = group.plan()
complete_plan = function_to_concatenate_plans(plan, plan2)
group.execute(complete_plan)

I had also been using the robot state functions to create robot markers from robot state messages, I'm sure there are other ways to do this, but it was a nice to have at this level. (was heading down a route for more interactive planning other than the rviz gui, future work to come!)

RE: Deprecating robot state joint value setter:

can we confirm we can set multi dof joints via the setJointValue target?

return a variable values matching to setJointValueTarget()

…etRobotState()

allowing to set joint values for non-group joints is not a good idea

henningkayser · 2019-03-19T09:03:35Z

@willcbaker Your described behavior would still be possible if the robot state is removed from the interface. You just have to pass the joint states instead of the full RobotState message. Also the waypoint from a RobotTrajectory message returned from plan() would have to be converted either way.

can we confirm we can set multi dof joints via the setJointValue target?

I think this wouldn't be possible anymore. But what is the use case for this? To my knowledge it's not possible to plan for different multi-dof joint poses via the MoveGroupInterface.

….set_state_value_target()

rhaschke · 2019-03-19T10:25:41Z

I rebased and targeted this PR against master. As setJointValueTarget(RobotState) is still used in the rviz plugin (as pointed out by the Travis warning), I suggest to revert the deprecation of this function as well, but not expose it in the Python interface anymore, i.e. remove set_state_value_target.

henningkayser · 2019-03-19T16:44:46Z

@rhaschke do you want this to be cherry-picked into other branches as well?

rhaschke · 2019-03-19T16:55:06Z

As this changes API, I wouldn't backport this to any other branch. @henningkayser, thanks for merging.

This was referenced Apr 25, 2018

Fix get joint value target #845

Closed

Expose the getJointValueTarget c++ method in python wrapper #851

Closed

henningkayser requested changes Mar 4, 2019

View reviewed changes

henningkayser force-pushed the python-wrapper branch from a17dac0 to 348bc87 Compare March 14, 2019 08:07

rhaschke commented Mar 14, 2019

View reviewed changes

henningkayser approved these changes Mar 14, 2019

View reviewed changes

henningkayser requested a review from mlautman March 14, 2019 08:45

mlautman reviewed Mar 14, 2019

View reviewed changes

...ng_interface/move_group_interface/include/moveit/move_group_interface/move_group_interface.h Show resolved Hide resolved

rhaschke and others added 6 commits March 19, 2019 08:24

fix getJointValueTarget()

6c2ce0a

return a variable values matching to setJointValueTarget()

deprecate public getJointValueTarget() in favour of protected getTarg…

bed6b6d

…etRobotState()

rename impl's getJointStateTarget() -> getTargetRobotState()

983b936

sanity checks for joint setters: don't allow non-group joints

7c0e029

deprecating dangerous setJointValueTarget() functions

863137d

allowing to set joint values for non-group joints is not a good idea

Undeprecate setJointValueTarget(sensor_msgs::JointState)

475f48f

rhaschke force-pushed the python-wrapper branch from 348bc87 to 475f48f Compare March 19, 2019 10:18

rhaschke changed the base branch from kinetic-devel to master March 19, 2019 10:18

undeprecate setJointValueTarget(), remove python's MoveGroupInterface…

a598936

….set_state_value_target()

henningkayser merged commit c6e8df9 into moveit:master Mar 19, 2019

rhaschke deleted the python-wrapper branch March 19, 2019 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve [get|set]JointValueTarget in python wrapper #858

improve [get|set]JointValueTarget in python wrapper #858

rhaschke commented Apr 25, 2018

rhaschke commented Apr 25, 2018

henningkayser left a comment

henningkayser Mar 4, 2019

rhaschke Mar 4, 2019

henningkayser Mar 4, 2019

rhaschke Mar 4, 2019

henningkayser Mar 4, 2019

rhaschke Mar 4, 2019

henningkayser Mar 4, 2019

rhaschke left a comment

willcbaker commented Mar 14, 2019

henningkayser commented Mar 18, 2019

willcbaker commented Mar 19, 2019

henningkayser commented Mar 19, 2019

rhaschke commented Mar 19, 2019

henningkayser commented Mar 19, 2019

rhaschke commented Mar 19, 2019

improve [get|set]JointValueTarget in python wrapper #858

improve [get|set]JointValueTarget in python wrapper #858

Conversation

rhaschke commented Apr 25, 2018

rhaschke commented Apr 25, 2018

henningkayser left a comment

Choose a reason for hiding this comment

henningkayser Mar 4, 2019

Choose a reason for hiding this comment

rhaschke Mar 4, 2019

Choose a reason for hiding this comment

henningkayser Mar 4, 2019

Choose a reason for hiding this comment

rhaschke Mar 4, 2019

Choose a reason for hiding this comment

henningkayser Mar 4, 2019

Choose a reason for hiding this comment

rhaschke Mar 4, 2019

Choose a reason for hiding this comment

henningkayser Mar 4, 2019

Choose a reason for hiding this comment

rhaschke left a comment

Choose a reason for hiding this comment

willcbaker commented Mar 14, 2019

henningkayser commented Mar 18, 2019

willcbaker commented Mar 19, 2019

henningkayser commented Mar 19, 2019

rhaschke commented Mar 19, 2019

henningkayser commented Mar 19, 2019

rhaschke commented Mar 19, 2019