-
Notifications
You must be signed in to change notification settings - Fork 935
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
Fix doxygen documentation for setToIKSolverFrame #2461
Conversation
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.
No. Looking into the code, ik_frame
indeed refers to the solver's frame, while it is assumed that pose is given w.r.t. the robot model's base frame.
I suggest making this function private (static
) within robot_state.cpp
. It's only used therein and probably not very useful externally.
moveit_core/robot_state/include/moveit/robot_state/robot_state.h
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #2461 +/- ##
==========================================
- Coverage 60.32% 60.27% -0.05%
==========================================
Files 351 351
Lines 26455 26455
==========================================
- Hits 15957 15943 -14
- Misses 10498 10512 +14
Continue to review full report at Codecov.
|
Can't be static because it uses a member variable (robot_model_). Sounds good to me, otherwise |
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.
Please make both functions private.
@@ -901,21 +901,13 @@ class RobotState | |||
*/ | |||
|
|||
/** | |||
* \brief Convert the frame of reference of the pose to that same frame as the IK solver expects | |||
* \brief Transform pose from the robot model's base frame to the reference frame of the IK solver | |||
* @param pose - the input to change | |||
* @param solver - a kin solver whose base frame is important to us | |||
* @return true if no error | |||
*/ | |||
bool setToIKSolverFrame(Eigen::Isometry3d& pose, const kinematics::KinematicsBaseConstPtr& solver); |
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.
Please make this other function private as well. It could become inline also...
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.
You mean, make both function versions inline
, right?
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.
One additional change. Otherwise, I approve.
moveit_core/robot_state/include/moveit/robot_state/robot_state.h
Outdated
Show resolved
Hide resolved
Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
This is public API because the solver's getPositionIK requires input poses in the frame and the relevant transform is otherwise inaccessible. Partially reverts moveit#2461.
This is public API because the solver's getPositionIK requires input poses in the frame and the relevant transform is otherwise inaccessible. Partially reverts #2461.
* Fix doxygen documentation for setToIKSolverFrame * "Convert" -> "Transform" * Make function private. Update comments. * Make inline and private * Longer function should not be inline Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com> Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
This is public API because the solver's getPositionIK requires input poses in the frame and the relevant transform is otherwise inaccessible. Partially reverts moveit#2461.
* Fix doxygen documentation for setToIKSolverFrame * "Convert" -> "Transform" * Make function private. Update comments. * Make inline and private * Longer function should not be inline Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com> Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
This is public API because the solver's getPositionIK requires input poses in the frame and the relevant transform is otherwise inaccessible. Partially reverts moveit#2461.
* Fix doxygen documentation for setToIKSolverFrame * "Convert" -> "Transform" * Make function private. Update comments. * Make inline and private * Longer function should not be inline Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com> Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
This is public API because the solver's getPositionIK requires input poses in the frame and the relevant transform is otherwise inaccessible. Partially reverts #2461.
I think the documentation here on setToIKSolverFrame() must be wrong. Here's what it says:
If that were correct, how would we know what the original frame of
pose
is? I think it should say:Fixes #2460