Skip to content
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

cleanup LMA kinematics solver #1318

Merged
merged 2 commits into from Feb 4, 2019

Conversation

Projects
None yet
4 participants
@rhaschke
Copy link
Collaborator

commented Jan 23, 2019

  • perform similar cleanup as in #1294
  • actually use KDL's LMA solver (was instantiated but not used)
  • remove notion of mimic joints, as the LMA solver doesn't support this

@rhaschke rhaschke requested a review from mlautman Jan 23, 2019

@rhaschke rhaschke force-pushed the ubi-agni:lma-cleanup branch from 1619f50 to f3d07e5 Jan 23, 2019

@mlautman
Copy link
Member

left a comment

Great improvement! Looks good to me :)

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 24, 2019

@mlautman Any reason you didn't yet merged this?

@rhaschke rhaschke referenced this pull request Jan 24, 2019

Merged

ikfast improvements #1320

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

There is so much removed code - how is it possible?

You say that you "remove notion of mimic joints, as the LMA solver doesn't support this" but doesn't the custom version in the deleted files provide support for this?

actually use KDL's LMA solver (was instantiated but not used)

Are you sure about this?

@fsuarez6 can you review since the original PR was yours: ros-planning/moveit_ros#723

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 25, 2019

LMA solver was instantiated but not used

Are you sure about this?

Not anymore. Looks like I confused variable names. Will need to fix this later. Closing this PR for now.
Thanks for being attentive!

@rhaschke rhaschke closed this Jan 25, 2019

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 30, 2019

There is so much removed code - how is it possible?

Most of this code was just copied 1:1 from the KDL solver, but not actually used. That's why the cleanup.

You say that you "remove notion of mimic joints, as the LMA solver doesn't support this" but doesn't the custom version in the deleted files provide support for this?

No. The KDL::ChainIkSolverVel_pinv_mimic, copied from the KDL IK plugin, and instantiated here, indeed would support mimic joints for the computation of the Jacobian. However, this class instance isn't actually used for any computation. Instead, the LMA plugin instantiates the standard KDL::ChainIkSolverPos_LMA solver (which instantiates its own VelocityIKSolver) here. This PositionIK solver is then wrapped by ChainIkSolverPos_LMA_JL_Mimic, which adds a thin layer of additional joint harmonization and limit checking. All the other code is essentially unused.

So, my original comment was slightly wrong, because the LMA solver was actually used before already.
Looks like I mixed this up, with the KDL::ChainIkSolverVel_pinv_mimic solver that was not used.
Sorry for that.

Although, I personally think that joint-limit rejection should be better handled in the solution_callback, I added joint harmonization and limit checking back to the cleaned code.
@fsuarez6 Can you motivate why your harmonization code uses the range -2 Pi ... 2 Pi instead of -Pi .. Pi ? The new code uses the default interval.

@rhaschke rhaschke reopened this Jan 30, 2019

@rhaschke rhaschke force-pushed the ubi-agni:lma-cleanup branch from 29d7d9c to ea1d646 Jan 30, 2019

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 30, 2019

Rebased to resolve conflicts.

@rhaschke rhaschke requested a review from davetcoleman Jan 30, 2019

@rhaschke rhaschke force-pushed the ubi-agni:lma-cleanup branch from ea1d646 to b79338d Jan 30, 2019

@fsuarez6

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

Most of this code was just copied 1:1 from the KDL solver, but not actually used. That's why the cleanup.

That's right. It was copied 1:1 from the KDL solver.

Can you motivate why your harmonization code uses the range -2 Pi ... 2 Pi instead of -Pi .. Pi ?

I cannot recall this. I think the default interval (-pi, pi) should work better.

Related question: Do we have any tests for the kinematics plugins?

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 31, 2019

Do we have any tests for the kinematics plugins?

@fsuarez6 We have, but only since a few weeks (#1272). You are invited to add a test for the LMA solver.

@fsuarez6

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

Great. If you guys are OK with holding this PR for a short while, I'll add some tests along side to be sure that we aren't breaking anything here.

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 31, 2019

Thanks for contributing more test. We will hold back this cleanup.
If you have questions, how to setup the unit tests, please contact me. All you should need to to is setup a rostest similar to https://github.com/ros-planning/moveit/pull/1320/files#diff-63b259ba139f6ad13f7f3a8e5248e81b. I hope I will get merged this PR in today...

@fsuarez6

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

You guys already have tests for this: https://github.com/ros-planning/moveit/blob/melodic-devel/moveit_kinematics/test/fanuc-kdl.test

@davetcoleman, based on the tests and the removed code, this PR looks good to me.

@rhaschke rhaschke force-pushed the ubi-agni:lma-cleanup branch from b79338d to 00a2f04 Feb 4, 2019

@rhaschke rhaschke force-pushed the ubi-agni:lma-cleanup branch 2 times, most recently from d6ffe7f to d26fc73 Feb 4, 2019

cleanup LMA kinematics solver
- perform similar cleanup as in #1294
- actually use KDL's LMA solver (was instantiated but not used)
- remove notion of mimic joints, as the LMA solver doesn't support this

@rhaschke rhaschke force-pushed the ubi-agni:lma-cleanup branch from d26fc73 to fe15ad4 Feb 4, 2019

@rhaschke rhaschke merged commit c7a31d5 into ros-planning:melodic-devel Feb 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rhaschke rhaschke deleted the ubi-agni:lma-cleanup branch Feb 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.