updated comments for getPositionFK

[gavanderhoorn]

Improving documentation is always good. However, I don't have any direct experience with the referenced methods, so @davetcoleman: any input?

[mintar]

I agree it would be great if @davetcoleman could double-check my comments, especially the first one (that there is currently no way that the solver's getPositionFK() is called by MoveIt). It took me quite a while to dig through all the code; in the end, I figured that the solver's searchPositionIK() method is called by RobotState, but getPositionFK() is never called. Still, I might have missed something.

I'm pretty confident about the second comment (that the current implementation of getPositionFK() only works with Transform6D). I've tested it in Fuerte, where it was still possible to make arm_navigation call it (by setting the use_plugin_fk param and merging uos/arm_navigation@261873f).

[mikeferguson]

+1

[davetcoleman]

-1 Actually I believe there is one use case for getPositionFK() - OMPL uses it when planning in workspace/cartesian space (as opposed to joint space) in its computeStateFK. I've also asked @isucan before why this function was really needed, and I suspect it could be entirely removed from the API if we changed the OMPL usage to instead use RobotState. You can find its current usage here: moveit_planners/ompl/ompl_interface/src/parameterization/work_space/pose_model_state_space.cpp

[davetcoleman]

@isucan can you weigh in?

[isucan]

The only reason getPositionFK() is used for planning using the work_space parameterization is because it operates in the same coordinate frame as getPositionIK. Of course, the same info can be computed with RobotState, but it requires an additional matrix multiplication and a matrix inversion. I could go either way on this one, but if we go for not using this function, it should also be removed from the base class. As long as this plugin implements a base class, it should not make assumptions about which functions get used, so I'm leaning towards @davetcoleman

[davetcoleman]

Github auto closed this PR because I just deleted the mater branch (in favor of hydro/indigo devel branches)

I like your second set of comments - could you move that to the indigo branch? As for the others:

• This FK routine is never used, since there is currently no way to configure MoveIt to use it.

This should be removed for now. I agree that the KinematicsBase would be simpler/cleaner if we removed the getPositionFK(), but that discussion should be moved to moveit_core. I would very much support removing it, but you might get resistance from others.

• * ROS TF is used to calculate the forward kinematics instead.

Technically, MoveIt! barely uses TF but instead uses its more efficient RobotState class.

[davetcoleman]

p.s. thanks for adding comments, MoveIt needs more!

[mintar]

Thanks for everyone's response, I completely agree that a base class shouldn't make assumptions about which functions get used. Also, the function does get used, which I didn't know. I've opened a new pull request (#42) without my first set of comments. I did remove the old arm_navigation-related comment that was there before. Also, I've improved my second comment. Please give some feedback at #42.

Regarding whether to remove getPositionFK() or not: Perhaps it's useful for kinematics plugins to be able to provide their own FK implementation. It would be nice if there was a default implementation in the base class though, so a plugin can choose whether to override it or not. Also, it would be nice if the FK that's used (plugin vs. RobotState) could be consistently switched (via some parameter) for all of MoveIt/OMPL. If there's a performance penalty to that, at least it should instead be properly documented which module uses which FK.

@davetcoleman : having separate hydro and indigo branches seems like a good idea. Are you planning on keeping the two in sync, or will you abandon hydro? As long as they don't diverge (which they haven't yet), it's not a big deal, which is why I'm asking this question now rather than later.

[davetcoleman]

It would be nice if there was a default implementation in the base class

+1, although that might be tricky because KinematicsBase is actually not very tightly integrated with RobotState, RobotModel, etc

if the FK that's used (plugin vs. RobotState) could be consistently switched (via some parameter) for all of MoveIt/OMPL

That sounds very tricky, because the transforms MoveIt uses is often greater than just a single kinematic chain. I don't think this is a very valuable feature

Are you planning on keeping the two in sync, or will you abandon hydro?

I do not have the need/interest/time to think about the hydro branch, so unless someone else wants to deal with backwards compatibility issues I am a fan of just keeping old releases stable and only making improvements in the latest release. Side note: @gavanderhoorn is a maintainer too, now, and I should be focusing my efforts elsewhere in MoveIt!