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

Add support for KDL::ChainIkSolverPos_NR_JL, which takes joint limits… #928

Merged
merged 2 commits into from
Sep 25, 2023

Conversation

rjoomen
Copy link
Contributor

@rjoomen rjoomen commented Aug 21, 2023

… into account

Copy link
Contributor

@Levi-Armstrong Levi-Armstrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After going through this and seeing how the q_min and q_max are used, I would recommend updating KDLChainData to automatically parse this data. Then add a option to current KDLInvKinChainNR to enable limits to avoid a new class being introduce. Let me know if you see any issues with this request.

@rjoomen
Copy link
Contributor Author

rjoomen commented Aug 21, 2023

Reason for this being a new class is that ChainIkSolverPos_NR_JL is a different solver, just like ChainIkSolverPos_LMA. On the other hand it is just a variation upon _NR, so adding use_limits as a parameter to differentiate between ChainIkSolverPos_NR and ChainIkSolverPos_NR_JL would also make sense. It's also easy to implement, so let me know if this is indeed how you'd like it and I'll modify the PR.

@Levi-Armstrong
Copy link
Contributor

Reason for this being a new class is that ChainIkSolverPos_NR_JL is a different solver, just like ChainIkSolverPos_LMA. On the other hand it is just a variation upon _NR, so adding use_limits as a parameter to differentiate between ChainIkSolverPos_NR and ChainIkSolverPos_NR_JL would also make sense. It's also easy to implement, so let me know if this is indeed how you'd like it and I'll modify the PR.

Yea, lets do that. It will limit the additional unit tests needed.

- Moved parsing of joint limits from parseChainData() to parseSceneGraph()
@rjoomen
Copy link
Contributor Author

rjoomen commented Sep 25, 2023

I added the joint limits to the KDLChainData.

Upon reconsideration I'm not sure about removing ChainIkSolverPos_NR_JL and adding it as an option to ChainIkSolverPos_NR. In KDL and hence MoveIt these are separate plugins, with ChainIkSolverPos_NR_JL being the one that is used much more often. In my testing, ChainIkSolverPos_NR_JL also performed a lot better. To prevent confusion for people familiar with KDL or MoveIt I'd suggest to keep the current naming (NR and NR_JL).

If you still are in favor of adding JL as an option to ChainIkSolverPos_NR, I think it would be good to make using joint limits the default, or making joint limits an option without a default, forcing users to make a deliberate choice. The first case might silently break/improve existing setups, the second would throw errors in existing setups, forcing users to add the option. Leaving the default at no joint limits would set the inferior solver as default, though, which I'd consider undesirable.

@Levi-Armstrong
Copy link
Contributor

As long as it is unit tested, I don't have a preference.

@rjoomen
Copy link
Contributor Author

rjoomen commented Sep 25, 2023

I did add unit tests, so it's ready to be merged.

@Levi-Armstrong
Copy link
Contributor

Thank you, the unit test coverage is the same as the others.

@Levi-Armstrong Levi-Armstrong merged commit e1ec20f into tesseract-robotics:master Sep 25, 2023
10 checks passed
@rjoomen rjoomen deleted the nr_jl branch September 25, 2023 13:05
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