Remove (redundant) random seeding and #attempts from RobotState::setFromIK #1288
Conversation
rhaschke
force-pushed
the
remove-ik-attempts
branch
from
63feeea
to
e7f86b8
Compare
rhaschke
changed the title
|
Due to API changes, this PR requires also changes to downstream packages, e.g. |
rhaschke
force-pushed
the
remove-ik-attempts
branch
from
a0a3319
to
82040a1
Compare
moveit_ros/planning/kinematics_plugin_loader/src/kinematics_plugin_loader.cpp
Outdated
Show resolved
Hide resolved
rhaschke
force-pushed
the
remove-ik-attempts
branch
4 times, most recently
from
2563bc0
to
ff9dfa0
Compare
|
Rebased to resolve merge conflicts. |
mlautman
dismissed
their stale review
All critical feedback has been discussed and a resolution reached
|
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
force-pushed
the
remove-ik-attempts
branch
from
8fa5b16
to
1af9ad2
Compare
rhaschke
force-pushed
the
remove-ik-attempts
branch
from
1af9ad2
to
17cedef
Compare
|
Rebased to resolve conflicts. Added deprecated function versions. |
rhaschke
force-pushed
the
remove-ik-attempts
branch
2 times, most recently
from
12529e2
to
b607212
Compare
|
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 |
|
@v4hn Ping again. |
randomized seeding happens in the kinematics solvers and shouldn't repeated in setFromIK()
rhaschke
force-pushed
the
remove-ik-attempts
branch
from
b607212
to
9317c9d
Compare
moveit.rosinstall
Outdated
| - git: | ||
| local-name: moveit_tutorials | ||
| uri: https://github.com/ros-planning/moveit_tutorials.git | ||
| version: melodic-devel |
There was a problem hiding this comment.
it sure would be nice if all these repos were merged together - we wouldn't have this issue
Distribute the remaining time equally to all sub group solvers. Also allow random seeding after first loop, by setting first_seed=false.
rhaschke
force-pushed
the
remove-ik-attempts
branch
from
9317c9d
to
c8b15e3
Compare
As originally pointed out in #725, both
RobotState::setFromIK()and (all?) actual kinematics solvers insearchPositionIK()perform random seeding if IK attempts fail, until the given timeout is reached. InsetFromIK()the number of attempts was - additionally to the timeout - limited by a max number of attempts.@v4hn suggested to remove the random sampling from
RobotStateas 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 byKinematicsBasefor 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
attemptsfrom PositionIKRequest.Please merge in conjuction with #1272 and #1294.