Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix not using result of solution callback #48

Merged
merged 1 commit into from
Jun 14, 2023

Conversation

SammyRamone
Copy link
Contributor

This PR fixes a small bug introduced in #42 due to which the result of the solution callback was ignored. Therefore, the GroupStateValidityCallbackFn (which is often used for collision checking) in the setFromIK() call was ignored.

Independent from this error, I was also wondering if the current way to call this function makes sense. Currently, it is only called at the very end of searchPositionIK(). This means, during the process of searching for an IK solution, pick_ik constantly checks the solution_fn (which does not contain the callback). When this finally returns true, it will check the callback. If the callback then returns false, pick_ik will say that it did not find a valid solution, although neither time nor iteration limits have been reached. Instead it would also be possible to continue searching for a solution until either a limit has reached or the solution_fn and the callback are valid at the same time.
I saw that bio_ik was doing it also like pick_ik is doing it currently. Maybe it is how MoveIt expects the kinematics_plugins to behave. However, I think that there might be benefit in changing this behavior, so that a valid solution is found more often.

Copy link
Collaborator

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

I don't see how this PR changes anything, as a few lines above we define

auto const found_solution = error_code.val == error_code.SUCCESS;

I do see your point on the question in the PR description, though. Basically, restarting the optimization with new initial conditions until finding a good solution or the full time budget is expended is a great idea to explore!

Copy link
Collaborator

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Wait! The solution callback actually mutates the error_code variable. Ugh, I hate non-const reference args when it's not obvious.

I understand now -- thank you for the fix!

Have put your remaining idea into a new issue: #49

@sea-bass sea-bass merged commit fd5f5b8 into PickNikRobotics:main Jun 14, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants