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.

Sign up for GitHub

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

Jump to bottom

Remove (redundant) random seeding and #attempts from RobotState::setFromIK #1288

Merged
merged 11 commits into from
Jan 31, 2019
Merged

Remove (redundant) random seeding and #attempts from RobotState::setFromIK #1288

merged 11 commits into from
Jan 31, 2019

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Dec 30, 2018 •
edited
Loading

As originally pointed out in #725, both RobotState::setFromIK() and (all?) actual kinematics solvers in searchPositionIK() perform random seeding if IK attempts fail, until the given timeout is reached. In setFromIK() the number of attempts was - additionally to the timeout - limited by a max number of attempts.

@v4hn suggested to remove the random sampling from RobotState as all important IK solvers (KDL, IKFast, TracIK) perform this random seeding internally anyway, and sometimes (IKFast) they rely on it.

This PR removes the random sampling from RobotState::setFromIK() and, simultaneously, removes the notion of max IK attempts from MoveIt. Thus, the number of attempts can only be limited by a timeout or appropriate measures in IK solvers (but there is no general support by KinematicsBase for this).

In my point of view, the only relevant use for a max number of attempts is to request a single attempt only (finding the one and only solution closest to the initial seed). In all other cases, when we just want to find any solution, there is no need to limit the attempts other than by a timeout.

If this PR is accepted, we should also remove attempts from PositionIKRequest.
Please merge in conjuction with #1272 and #1294.

@rhaschke rhaschke changed the title Remove ik attempts Remove (redundant) random seeding and #attempts from RobotState::setFromIK Dec 30, 2018
@rhaschke
Copy link
Contributor Author

Due to API changes, this PR requires also changes to downstream packages, e.g. moveit_visual_tools.

mlautman
mlautman previously requested changes Jan 2, 2019
View reviewed changes
moveit.rosinstall Outdated Show resolved Hide resolved
moveit_core/robot_model/src/joint_model_group.cpp Show resolved Hide resolved
moveit_core/robot_state/src/robot_state.cpp Outdated Show resolved Hide resolved
moveit_core/robot_state/src/robot_state.cpp Show resolved Hide resolved
moveit_core/robot_state/src/robot_state.cpp Outdated Show resolved Hide resolved
moveit_ros/manipulation/pick_place/src/pick.cpp Show resolved Hide resolved
moveit_ros/planning/kinematics_plugin_loader/src/kinematics_plugin_loader.cpp Outdated Show resolved Hide resolved
@rhaschke rhaschke force-pushed the remove-ik-attempts branch 4 times, most recently from 2563bc0 to ff9dfa0 Compare January 4, 2019 10:48
@rhaschke
Copy link
Contributor Author

rhaschke commented Jan 4, 2019

Rebased to resolve merge conflicts.

@rhaschke
Copy link
Contributor Author

rhaschke commented Jan 4, 2019

Ping @mlautman, @v4hn. Would be great to get some feedback on these API changes.

@rhaschke rhaschke mentioned this pull request Jan 4, 2019
Merged
@mlautman mlautman dismissed their stale review January 7, 2019 18:54

All critical feedback has been discussed and a resolution reached

@mlautman
Copy link
Contributor

mlautman commented Jan 7, 2019

I dismissed my "Request Changes" review since I am happy with this but I did add one last suggestion that I think would be an improvement. +1 after at least considering new feedback

@rhaschke rhaschke mentioned this pull request Jan 10, 2019
Open
@rhaschke
Copy link
Contributor Author

Rebased to resolve conflicts. Added deprecated function versions.

@rhaschke rhaschke force-pushed the remove-ik-attempts branch 2 times, most recently from 12529e2 to b607212 Compare January 14, 2019 14:04
@davetcoleman
Copy link
Member

I'm still not a fan of this big of change for something that isn't "broken" but rather just "sub-optimal". This approach has been working all this time and no one noticed the weird duplicate functionality. But I'll defer to @v4hn's decision / approval

@rhaschke
Copy link
Contributor Author

@v4hn Ping again.

@rhaschke
setFromIK(): remove 'attempts' argument
eebfe9c 
randomized seeding happens in the kinematics solvers and shouldn't repeated in setFromIK()
@rhaschke
RobotState::setFromIKSubgroups(): do random seeds on constraint failures
80a5dc2
davetcoleman
davetcoleman reviewed Jan 25, 2019
View reviewed changes
Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

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

I approve this after the rosinstall is restored and travis passes

moveit.rosinstall Outdated
- git:
local-name: moveit_tutorials
uri: https://github.com/ros-planning/moveit_tutorials.git
version: melodic-devel
Copy link
Member

Choose a reason for hiding this comment

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

it sure would be nice if all these repos were merged together - we wouldn't have this issue

moveit.rosinstall Outdated Show resolved Hide resolved
@rhaschke
add MIGRATION note
8c8c7a1
@rhaschke
setFromIKSubgroups(): improve timeout handling
7b27fe7 
Distribute the remaining time equally to all sub group solvers.
Also allow random seeding after first loop, by setting first_seed=false.
@rhaschke
re-added deprecated versions of setFromIK() methods
c8b15e3
@rhaschke rhaschke merged commit fc2aa52 into moveit:melodic-devel Jan 31, 2019
@rhaschke rhaschke deleted the remove-ik-attempts branch January 31, 2019 14:56
This was referenced Feb 21, 2019
Merged
Merged
@henningkayser henningkayser mentioned this pull request Mar 18, 2019
Closed
@ShotaAk ShotaAk mentioned this pull request May 8, 2019
Closed
@agutenkunst agutenkunst mentioned this pull request Jun 24, 2019
Closed
This was referenced May 14, 2020
Merged
Merged
@JafarAbdi JafarAbdi mentioned this pull request Mar 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davetcoleman davetcoleman davetcoleman left review comments

@mlautman mlautman mlautman approved these changes

@v4hn v4hn Awaiting requested review from v4hn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rhaschke @mlautman @davetcoleman