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

Implemented new getPositionIK() from Kinematics Base #239

Open
wants to merge 1 commit into
base: indigo-devel
Choose a base branch
from

Conversation

Jmeyer1292
Copy link

See the new overload for getPositionIK for documentation on the new function.

We over in the Descartes project require "all" of the solutions that an analytical inverse kinematics routine can return (not just the closest to a seed). We got a few pull requests in that add such a function to moveit's KinematicsBase. Here I present an implementation of this function for this special model.

There's a little more duplication with existing code than I like, but I moved out some of the most obviously separable logic to a new helper function.

Let me know what you think; I'm eager to make the Univeral Robots trivially inter-operable with Descartes.

@shaun-edwards
Copy link
Member

@Jmeyer1292, thanks for the contribution. Sorry for taking so long to review.

@davetcoleman have you had a chance to use this?

@Jmeyer1292
Copy link
Author

@shaun-edwards This is not what I prepared for Dave, but rather what part of my testing for the IKFast Interface I added to Descartes a while back.

For work akin to dave's to be generally usable, we'll need something like this and #248 (and maybe more). I took the easy route and fixed things in our special plugin.

@gavanderhoorn
Copy link
Member

@Jmeyer1292: why would #248 be needed for this?

@Jmeyer1292
Copy link
Author

@gavanderhoorn I meant for @davetcoleman and his work on hilgendorf. Two UR robot arms and all.

@gavanderhoorn
Copy link
Member

Ah, right. I still feel that is more of a MoveIt problem than something we need to work-around on our end though. But see #248.

@UltronDestroyer
Copy link

@gavanderhoorn It's mostly a plugin problem. Most other pugins (PR2, KDL) use the method of using the joint names passed in via the group to determine where the prefix is to be added and to determine what joints are in the kin chain.

I got the idea for the fix by looking at how the PR2 grabs joints from its plugin. Fun fact: as long as you have 7 joints in your kin chain the PR2 plugin will work. It wont be optimized unless its the PR2 arm, but it will load the plugin.

That's kinda the scenario we're facing now.

@gavanderhoorn
Copy link
Member

gavanderhoorn commented Jun 28, 2016

@TheDash wrote:

@gavanderhoorn It's mostly a plugin problem. Most other pugins (PR2, KDL) use the method of using the joint names passed in via the group to determine where the prefix is to be added and to determine what joints are in the kin chain.

yes, and that works for those plugins because KDL (and trac_ik as well) dynamically setup their internal solver structures (ie: all their internal parameters are configured at runtime, based on the kinematic structure described in the URDF). pr2_arm_kinematics is essentially a KDL plugin. It just adds some niceties to it.

IKFast and ur_kinematics don't do that: IKFast plugins are generated off-line, with all parameters hard-coded into the source. ur_kinematics wasn't generated, but hand-coded based on @kphawkins calculations.

You simply cannot (re-)use IKFast or ur_kinematics for anything but the kinematic configurations they were generated for.

@UltronDestroyer
Copy link

You simply cannot (re-)use IKFast or ur_kinematics for anything but the kinematic configurations they were generated for.

Which is why we should also put a check in the code to see if it has the needed joints for the UR arms, but to ignore prefixes. That gets the best of both worlds.

I will continue that discussion on #248 as it is kinda side tracking this thread

@davetcoleman
Copy link
Contributor

@shaun-edwards no I have not used this. I'm using the ur5_robot_model included in roscon_2015 demo. This looks like it would be a nice cleanup though

@gavanderhoorn
Copy link
Member

This is probably something that should / can be reviewed in combination with #316 and #305.

I won't be able to do this before ROSCon however, and I would greatly appreciate some help.

@gavanderhoorn
Copy link
Member

Tagging this PR with wrid18 as this is a PR that would be nice to review.

#305 and #316 should be taken into account / considered as well.

@mxgrey
Copy link

mxgrey commented Jul 11, 2018

If these changes are not strictly needed in indigo-devel, I would suggest closing this PR in favor of #358 which combines these changes with #305 and targets kinetic-devel.

Alternatively, we could create a PR for the changes in #358 that targets indigo-devel if there are still indigo users that want them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants