Documenting why setFromIK in computeCartesianPath does not use random restarts #1076
Conversation
|
CI failed, requiring to fix clang format: https://travis-ci.org/ros-planning/moveit/jobs/436909712 |
mlautman
requested review from
davetcoleman,
rhaschke and
v4hn
and removed request for
davetcoleman,
rhaschke and
v4hn
| @@ -1827,6 +1827,8 @@ as the new values that correspond to the group */ | |||
| /** \brief This function is only called in debug mode */ | |||
| bool checkCollisionTransforms() const; | |||
|
|
|||
| static std::string name_; | |||
There was a problem hiding this comment.
This seems to be superfluos. Either use RobotState::name_ or this static const name_ everywhere.
Personally, I would prefer the latter actually.
There was a problem hiding this comment.
name_ has to be static to be used by static methods like testJointSpaceJump and static member variables have to be initialized outside the class definition. I don't know of a cleaner way to do this. If you have a cleaner syntax please add your commit on top of mine.
https://stackoverflow.com/questions/1563897/c-static-constant-string-class-member
| @@ -53,6 +53,7 @@ namespace core | |||
| * it is difficult to choose a jump_threshold parameter that effectively separates | |||
| * valid paths from paths with large joint space jumps. */ | |||
| static const std::size_t MIN_STEPS_FOR_JUMP_THRESH = 10; | |||
| std::string RobotState::name_ = "robot_state"; | |||
There was a problem hiding this comment.
If using this one (which I don't prefer), please use static const.
There was a problem hiding this comment.
This refers to the same static member variable. An easy test to demonstrate this is to make one const and not the other.
There was a problem hiding this comment.
I have made it const
| @@ -1925,7 +1925,7 @@ double RobotState::computeCartesianPath(const JointModelGroup* group, std::vecto | |||
| Eigen::Affine3d pose(start_quaternion.slerp(percentage, target_quaternion)); | |||
| pose.translation() = percentage * rotated_target.translation() + (1 - percentage) * start_pose.translation(); | |||
|
|
|||
| if (setFromIK(group, pose, link->getName(), 1, 0.0, validCallback, options)) | |||
| if (setFromIK(group, pose, link->getName(), 0, 0.0, validCallback, options)) | |||
There was a problem hiding this comment.
I think, a single attempt is used on purpose here: In a Cartesian path, it doesn't make sense to use a random seed for IK. This essentially enforces non-continuous joint-space motions.
There was a problem hiding this comment.
hrm, good point. I will drop this change, and add this as a comment.
mlautman
changed the title
|
@rhaschke changes made |
| @@ -1919,6 +1919,9 @@ double RobotState::computeCartesianPath(const JointModelGroup* group, std::vecto | |||
| traj.clear(); | |||
| traj.push_back(RobotStatePtr(new RobotState(*this))); | |||
|
|
|||
| // For each waypoint we do not use random restarts since they would very likely result in IK jumps | |||
| std::size_t ik_attempts = 1; | |||
There was a problem hiding this comment.
I don't see a benefit in introducing a new (constant!) variable here. This even more suggests that the number of IK attempts could be changed. I submitted an alternative comment.
Documenting why the setFromIK call in computeCartesianPath uses a single attempt rather than the default.
Description
Please explain the changes you made, including a reference to the related issue if applicable
Checklist