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

Map ompl's APPROXIMATE_SOLUTION -> TIMED_OUT / PLANNING_FAILED #2455

Merged
merged 3 commits into from
Oct 17, 2023
Merged

Map ompl's APPROXIMATE_SOLUTION -> TIMED_OUT / PLANNING_FAILED #2455

merged 3 commits into from
Oct 17, 2023

Conversation

rhaschke
Copy link
Contributor

ompl's APPROXIMATE_SOLUTION is not suitable for actual execution. It just states that we got closer to the goal:

If the top found solution is approximate, does not actually reach the desired goal, but hopefully is closer to it

As the most prominent reason for an approximate solution is a timeout, also checking for this case.

Fixes moveit/moveit_task_constructor#485

@rhaschke
Map ompl's APPROXIMATE_SOLUTION -> TIMED_OUT / PLANNING_FAILED
1c1c21f 
ompl's APPROXIMATE_SOLUTION is not suitable for actual execution.
It just states that we got closer to the goal...
The most prominent reason for an approximate solution is a timeout.
@rhaschke
Copy link
Contributor Author

Interestingly the issue was fixed in the MoveIt1 backport (moveit/moveit#3257 (review)), but then not back-ported to MoveIt2 again!

@rhaschke rhaschke mentioned this pull request Oct 16, 2023
Closed
@codecov
Copy link

codecov bot commented Oct 16, 2023 •
edited
Loading

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (7cfebe8) 50.86% compared to head (05803f8) 50.84%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2455      +/-   ##
==========================================
- Coverage   50.86%   50.84%   -0.02%     
==========================================
  Files         386      386              
  Lines       32000    32002       +2     
==========================================
- Hits        16275    16269       -6     
- Misses      15725    15733       +8
Files Coverage Δ
...mpl_interface/src/model_based_planning_context.cpp 58.35% <33.34%> (-0.21%) ⬇️

... and 1 file with indirect coverage changes

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

Abishalini
Abishalini approved these changes Oct 16, 2023
View reviewed changes
Copy link
Contributor

@Abishalini Abishalini left a comment

Choose a reason for hiding this comment

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

Thanks for this fix!
I tested with a setup that often executed partial solutions and this fixes it. I had to increase the planning time in my setup.

moveit_planners/ompl/ompl_interface/src/model_based_planning_context.cpp Outdated Show resolved Hide resolved
@rhaschke rhaschke merged commit c0c6baf into moveit:main Oct 17, 2023
12 checks passed
@rhaschke rhaschke deleted the fix-approximate branch October 17, 2023 12:34
@leander2189
Copy link

Thanks for the fix!!

What is the procedure to get this PR merged also into humble branch? I recall there is a bot that does that, but have no idea how to invoke it...

@rhaschke rhaschke added the backport-humble Mergify label that triggers a PR backport to Humble label Oct 18, 2023
mergify bot pushed a commit that referenced this pull request Oct 18, 2023
@rhaschke @mergify
Map ompl's APPROXIMATE_SOLUTION -> TIMED_OUT / PLANNING_FAILED (#2455)
9cff3ce 
ompl's APPROXIMATE_SOLUTION is not suitable for actual execution. It just states that we got closer to the goal...
The most prominent reason for an approximate solution is a timeout. Thus, return TIMED_OUT and print the used timeouts for convenience.

(cherry picked from commit c0c6baf)
@mergify mergify bot mentioned this pull request Oct 18, 2023
Merged
@rhaschke
Copy link
Contributor Author

@Mergifyio backport humble

@mergify
Copy link

mergify bot commented Oct 18, 2023

backport humble

✅ Backports have been created

@rhaschke
Copy link
Contributor Author

What is the procedure to backport? I recall there is a bot that does that, but have no idea how to invoke it...

Usually, it's done by providing the label backport-humble (before merging), which I forgot to do...
However, one can manually trigger a backport by providing a comment (see #2455 (comment)). I will merge the backport if CI succeeds.

Note, that we will need a new release to actually have these changes deployed.

@leander2189
Copy link

Note, that we will need a new release to actually have these changes deployed.

Yes, I understand. Thank you

@sjahr sjahr added the backport-iron Mergify label that triggers a PR backport to Iron label Oct 18, 2023
mergify bot pushed a commit that referenced this pull request Oct 18, 2023
@rhaschke @mergify
Map ompl's APPROXIMATE_SOLUTION -> TIMED_OUT / PLANNING_FAILED (#2455)
36fb243 
ompl's APPROXIMATE_SOLUTION is not suitable for actual execution. It just states that we got closer to the goal...
The most prominent reason for an approximate solution is a timeout. Thus, return TIMED_OUT and print the used timeouts for convenience.

(cherry picked from commit c0c6baf)
@mergify mergify bot mentioned this pull request Oct 18, 2023
Merged
sjahr pushed a commit that referenced this pull request Oct 18, 2023
@mergify @rhaschke
Map ompl's APPROXIMATE_SOLUTION -> TIMED_OUT / PLANNING_FAILED (#2455) (
7e5b6ce 
#2459)

ompl's APPROXIMATE_SOLUTION is not suitable for actual execution. It just states that we got closer to the goal...
The most prominent reason for an approximate solution is a timeout. Thus, return TIMED_OUT and print the used timeouts for convenience.

(cherry picked from commit c0c6baf)

Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
henningkayser pushed a commit that referenced this pull request Oct 24, 2023
@mergify @rhaschke
Map ompl's APPROXIMATE_SOLUTION -> TIMED_OUT / PLANNING_FAILED (backp…
782a408 
…ort #2455) (#2460)

ompl's APPROXIMATE_SOLUTION is not suitable for actual execution. It just states that we got closer to the goal...
The most prominent reason for an approximate solution is a timeout. Thus, return TIMED_OUT and print the used timeouts for convenience.

(cherry picked from commit c0c6baf)

Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
medvedevigorek pushed a commit to AivotRobotics/moveit2 that referenced this pull request Nov 2, 2023
@mergify @rhaschke @medvedevigorek
Map ompl's APPROXIMATE_SOLUTION -> TIMED_OUT / PLANNING_FAILED (backp…
bf81f14 
…ort moveit#2455) (moveit#2460)

ompl's APPROXIMATE_SOLUTION is not suitable for actual execution. It just states that we got closer to the goal...
The most prominent reason for an approximate solution is a timeout. Thus, return TIMED_OUT and print the used timeouts for convenience.

(cherry picked from commit c0c6baf)

Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
@leander2189
Copy link

leander2189 commented Mar 19, 2024 •
edited
Loading

Just to note that these changes are still not deployed in latest binary version from apt repos (2.5.5-1jammy.20240217.122953). Building from sources works correctly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henningkayser henningkayser henningkayser approved these changes

@Abishalini Abishalini Abishalini approved these changes

Assignees
No one assigned
Labels
backport-humble Mergify label that triggers a PR backport to Humble backport-iron Mergify label that triggers a PR backport to Iron
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Discontinuity in trajectory
5 participants
@rhaschke @leander2189 @henningkayser @Abishalini @sjahr