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

Return all IK solutions with MoveIt plugin #553

Closed

Conversation

marip8
Copy link
Member

@marip8 marip8 commented Nov 19, 2020

I have a use case where I would like to use the MoveIt kinematics plugin getPositionIK function to return a vector of valid joint solutions for a UR robot. The default implementation of this function in the KinematicsBase class simply calls another getPositionIK overload to return a single joint position that is closest to the seed state. This PR overrides this function to return all valid joint states, given a single input tool pose. I also created a simple unit test to verify the behavior of the getPositionFK and getPositionIK functions

In the future, it would also be nice to return the all of the joint solution permutations considering the full [-2 * pi, 2 * pi] range of some of the joints. I wrote some code to do this a while back that I could probably integrate into this plugin if it's valuable

@gavanderhoorn
Copy link
Member

Thanks for the PR.

I haven't checked in detail yet, so perhaps this would be clear by reading the code, but could you check whether and how #453 and the PRs on universal_robot/projects/1 are related / offer similar functionality?

It would be really great if we could get someone to review those, so we can merge them.

@marip8
Copy link
Member Author

marip8 commented Nov 20, 2020

It seems like this PR is effectively the same as #239 and #358. I did this a while back and figured I would contribute; I didn't look to deeply into the open/unresolved PRs before opening this one. It looks like #358 would address all the changes I wanted to make. Is there a reason it hasn't been merged yet?

@gavanderhoorn
Copy link
Member

gavanderhoorn commented Nov 20, 2020

I did this a while back and figured I would contribute;

Which is certainly very much appreciated 💯

Is there a reason it hasn't been merged yet?

Yes: there hasn't been anyone with both sufficient insight into the subject and availability to review it all properly. As these PRs potentially change behaviour for all users of these packages, proper reviews were and are in order.

We've (nicely) asked a few people / users, but so far no one has stepped up.

If you'd be willing to take a look, that would get us moving again.

@marip8
Copy link
Member Author

marip8 commented Dec 8, 2020

I'll close this PR in favor of #358. I can port the unit test I made here to try to verify the functionality in that PR. I think unit tests would probably be the most effective way of testing the changes

@marip8 marip8 closed this Dec 8, 2020
@gavanderhoorn
Copy link
Member

I can port the unit test I made here to try to verify the functionality in that PR.

Yes, that would be great 👍

I think unit tests would probably be the most effective way of testing the changes

Agreed.

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

Successfully merging this pull request may close these issues.

2 participants