Skip to content

Fix constrained-based planning#3615

Merged
rhaschke merged 2 commits intomoveit:masterfrom
ubi-agni:fix-pose-model-state-space
Jul 2, 2024
Merged

Fix constrained-based planning#3615
rhaschke merged 2 commits intomoveit:masterfrom
ubi-agni:fix-pose-model-state-space

Conversation

@rhaschke
Copy link
Copy Markdown
Contributor

@rhaschke rhaschke commented Jun 27, 2024

Constrained-based planning enables the PoseModelStateSpace by default, meaning that state samples are created by sampling in SE(3) and then performing IK. This state space frequently caused issues, eventually leading to the introduction of the ROS parameter enforce_joint_model_state_space in PR #541 as a work-around.
A more detailed analysis shows that the state space's interpolation approach is the culprit - performing interpolation in Cartesian space. If the two interpolation ends are close in Cartesian but distant in joint space, the interpolated state (obtained from IK) can be far away from both start and end (particularly for redundant arms), while its Cartesian pose is naturally close to both interpolation states. This will lead to consecutive waypoints being close in Cartesian space but distant in joint space. If such a trajectory is executed, the controller will interpolate in joint space, leading to a large and unchecked motion:

old: broken new: fixed
joint-space-interpolation joint-space-dist

To avoid this issue, interpolation and distance measurement should be performed in joint space. The only drawback of this change is that trajectories are less linear in Cartesian space (see videos below). However, as the trajectory is random-sampled anyway, this shouldn't be a big issue.

old: linear in Cartesian space new: linear in joint space
cartesian joint

Constrained-based planning enables the PoseModelStateSpace by default, meaning that state samples are created by sampling
in SE(3) and then performing IK. This state space frequently caused issues, eventually leading to the introduction of the
ROS parameter enforce_joint_model_state_space in PR moveit#541 as a work-around.

A more detailed analysis shows that the state space's interpolation approach is the culprit - performing interpolation in
Cartesian space. If the two interpolation ends are close in Cartesian but distant in joint space, the interpolated state
(obtained from IK) can be far away from both start and end (particularly for redundant arms), while its Cartesian pose is
naturally close to both interpolation states. This will lead to consecutive waypoints being close in Cartesian space but
distant in joint space. If such a trajectory is executed, the controller will interpolate in joint space, leading to a large
and unchecked motion.

To avoid this issue, interpolation should be performed in joint space during planning as well.
The only drawback of this change is that trajectories are less linear in Cartesian space.
However, as the trajectory is random-sampled anyway, this shouldn't be a big issue.
@rhaschke rhaschke requested review from henningkayser and v4hn June 27, 2024 19:26
@rhaschke rhaschke requested a review from mamoll as a code owner June 27, 2024 19:26
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 27, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 47.10%. Comparing base (e8eb9c1) to head (07e7e8c).
Report is 21 commits behind head on master.

Files with missing lines Patch % Lines
...meterization/work_space/pose_model_state_space.cpp 66.67% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3615      +/-   ##
==========================================
- Coverage   47.16%   47.10%   -0.06%     
==========================================
  Files         603      603              
  Lines       59196    59215      +19     
  Branches     7024     6994      -30     
==========================================
- Hits        27915    27886      -29     
- Misses      30863    30911      +48     
  Partials      418      418              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rhaschke
Copy link
Copy Markdown
Contributor Author

rhaschke commented Jun 27, 2024

I think it would be also sensible to switch distance computation from Cartesian space to joint space: Similar eef poses in Cartesian spaces can be generated by very different joint poses. What actually matters when connecting states is the joint-space distance. Any thoughts, @mamoll and @zkingston?

Edit: Turns out that both interpolation and distance measurement in joint-space are required: Without joint-space interpolation, only a few states will be generated, leading to an invalid trajectory. Without joint-space distance measurement, an actually valid path (with multiple states) will be simplified to an invalid one with only the two far-apart start and end states:

arm_torso/arm_torso[RRTConnect]: Created 42 states (8 start + 34 goal)
SimpleSetup: Path simplification took 0.015057 seconds and changed from 10 to 2 states

@zkingston
Copy link
Copy Markdown
Contributor

In my experience, using the joint space metric is better than an EE-based metric in workspace for the exact reason you listed, and is what I usually use when doing any kind of constrained planning. There are many configurations that are similar in EE pose that are very far apart in joint space, and interpolation is far more likely to fail when they are far apart in joint space.

@mamoll
Copy link
Copy Markdown
Contributor

mamoll commented Jun 28, 2024

If you interpolate in joint space, how would the motion still be constrained? I am confused: it sounds like the fix to constrained planning is to remove constraints. If the constrained planning is correctly implemented, then it should be possible to find constrained EE paths (with probabilistic completeness guarantees). Does the IK solver use the correct seed to find nearby solutions? The constrained interpolator should reject motions with big jumps in joint space.

@rhaschke
Copy link
Copy Markdown
Contributor Author

If you interpolate in joint space, how would the motion still be constrained?

The interpolated state will be validated (and rejected if not satisfying the constraints).
The example video on the right shows a path constrained to have the eefs's z-axis upright within a 20° range, which is perfectly satisfied.

The constrained interpolator should reject motions with big jumps in joint space.

Interpolating in EE space and computing IK can result in joint-space poses far away from the poses of one or the other interpolation states (or both). Thus we had big joint-space jumps. Interpolation in joint-space will allow to reject those path connections because the interpolated state probably will not satisfy the constraints anymore.

Configurations may be similar in Cartesian EE pose, but very far apart in joint space.
What actually matters when connecting states is the joint-space distance.
@mamoll
Copy link
Copy Markdown
Contributor

mamoll commented Jun 30, 2024

I see. When I saw the description, I was thinking of the new constrained-based planning in MoveIt2 based on @zkingston's work in OMPL. This is not that. Your changes look fine to me and make the code look a lot cleaner and simpler. I suspect that this will make planning also much faster (since it's not calling IK as often).

@rhaschke rhaschke merged commit 7c0343f into moveit:master Jul 2, 2024
@rhaschke rhaschke deleted the fix-pose-model-state-space branch July 2, 2024 11:56
LeroyR pushed a commit to CentralLabFacilities/moveit that referenced this pull request Jul 11, 2024
* Fix constrained-based planning

Constrained-based planning enables the PoseModelStateSpace by default, meaning that state samples are created by sampling in SE(3) and then performing IK. This state space frequently caused issues, eventually leading to the introduction of the ROS parameter enforce_joint_model_state_space in PR moveit#541 as a work-around.

A more detailed analysis shows that the state space's interpolation approach is the culprit - performing interpolation in Cartesian space. If the two interpolation ends are close in Cartesian but distant in joint space, the interpolated state (obtained from IK) can be far away from both start and end (particularly for redundant arms), while its Cartesian pose is naturally close to both interpolation states. This will lead to consecutive waypoints being close in Cartesian space but distant in joint space. If such a trajectory is executed, the controller will interpolate in joint space, leading to a large and unchecked motion.

To avoid this issue, interpolation should be performed in joint space during planning as well.
The only drawback of this change is that trajectories are less linear in Cartesian space.
However, as the trajectory is random-sampled anyway, this shouldn't be a big issue.

* PoseModelStateSpace: Use joint-space distance

Configurations may be similar in Cartesian EE pose, but very far apart in joint space.
What actually matters when connecting states is the joint-space distance.
rhaschke added a commit that referenced this pull request Oct 31, 2024
#3615 made PoseModelStateSpace identical to ModelBasedStateSpace, both using joint-space interpolation and distance measurements. This resulted in planning timeouts for code that previously worked. Obviously, using joint-space interpolation much more often results in the rejection of tight path constraints than Cartesian interpolation did.

The problem with potential jumps due to Cartesian interpolation in principle remains, which is a dilemma of the current design:

- We do need Cartesian interpolation to reduce the risk of state rejection due to a failing path constraint, which currently cannot be checked during interpolation.
- Ideally, we would do joint interpolation and only resort to Cartesian interpolation if the path constraint is violated, i.e. follow a similar approach as in #3618. However, this would require access to the path constraint...
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.

4 participants