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

Update "Store OMPL Planning Data" PR #2364

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Update "Store OMPL Planning Data" PR #2364

wants to merge 12 commits into from
+58 −3

Conversation

Shobuj-Paul
Copy link
Contributor

@Shobuj-Paul Shobuj-Paul commented Sep 14, 2023 •
edited
Loading

Description

Resolves the merge conflicts and cling-tidy errors for PR #1183
Fixes #2317

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@codecov
Copy link

codecov bot commented Sep 14, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 10.00000% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 43.01%. Comparing base (d962501) to head (b08fd4c).
Report is 31 commits behind head on main.

Files Patch % Lines
...lanners/ompl/ompl_interface/src/ompl_interface.cpp 14.29% 12 Missing ⚠️
...pl/ompl_interface/src/planning_context_manager.cpp 0.00% 6 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2364      +/-   ##
==========================================
- Coverage   50.74%   43.01%   -7.73%     
==========================================
  Files         392      692     +300     
  Lines       32553    56317   +23764     
  Branches        0     7276    +7276     
==========================================
+ Hits        16517    24218    +7701     
- Misses      16036    31935   +15899     
- Partials        0      164     +164

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

@Shobuj-Paul Shobuj-Paul marked this pull request as ready for review September 16, 2023 06:49
@Shobuj-Paul
Copy link
Contributor Author

Shobuj-Paul commented Sep 16, 2023 •
edited
Loading

I have replaced std::bind function with lambdas. If this fix looks good, we can remove the defined function storePlannerData.
If there is a better way to remove std::bind, let me know.

henningkayser
henningkayser reviewed Sep 19, 2023
View reviewed changes
Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

Thank you!
While I'm not a huge fan of adding a direct rclcpp service dependency to the ompl plugin, I see why it might be userful. IMO, it would be much cleaner to use some event API that exposes detailed features like this through the planning plugin, and only bind a service to it via moveit_ros. I'm fine with merging this feature anyway, provided the comments are resolved.

moveit_planners/ompl/ompl_interface/src/planning_context_manager.cpp
{
// Store all planner data
for (const auto& entry : planner_data_storage_paths_)
{
ob::PlannerData data(planners_[entry.first]->getSpaceInformation());
planners_[entry.first]->getPlannerData(data);
if (data.numVertices() == 0)
Copy link
Member

Choose a reason for hiding this comment

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

Please add an debug info for this here. The error in OMPLInterface::storePlannerData() shouldn't make assumptions about the reason for failures in this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added debug info, please check if this is what you meant.

moveit_planners/ompl/ompl_interface/src/ompl_interface.cpp Outdated Show resolved Hide resolved
moveit_planners/ompl/ompl_interface/src/ompl_interface.cpp Outdated Show resolved Hide resolved
create_client Outdated Show resolved Hide resolved
@Shobuj-Paul
Copy link
Contributor Author

Thank you! While I'm not a huge fan of adding a direct rclcpp service dependency to the ompl plugin, I see why it might be userful. IMO, it would be much cleaner to use some event API that exposes detailed features like this through the planning plugin, and only bind a service to it via moveit_ros. I'm fine with merging this feature anyway, provided the comments are resolved.

I am not sure on how to proceed with that, but I could work on it instead of this PR if given some direction.

@Shobuj-Paul Shobuj-Paul force-pushed the feature/store_planner_data branch 4 times, most recently from 075eb7a to 5d5fb0d Compare September 28, 2023 12:35
@Shobuj-Paul Shobuj-Paul force-pushed the feature/store_planner_data branch 2 times, most recently from 5b43cd9 to 496ccbf Compare October 2, 2023 15:57
dyackzan
dyackzan reviewed Oct 4, 2023
View reviewed changes
Copy link

@dyackzan dyackzan left a comment

Choose a reason for hiding this comment

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

I have reviewed and am ok approving these changes once Henning's comments are resolved. Tagging @henningkayser for his reply here

@Shobuj-Paul Shobuj-Paul force-pushed the feature/store_planner_data branch 2 times, most recently from 5ced6eb to 0628239 Compare October 11, 2023 07:28
@Shobuj-Paul Shobuj-Paul force-pushed the feature/store_planner_data branch 3 times, most recently from 14e9186 to d682fa0 Compare October 19, 2023 06:27
@Shobuj-Paul Shobuj-Paul force-pushed the feature/store_planner_data branch 8 times, most recently from d78e718 to 3a7f68e Compare October 28, 2023 09:54
@Shobuj-Paul Shobuj-Paul force-pushed the feature/store_planner_data branch 3 times, most recently from 5b6330a to 1ab8fe4 Compare November 1, 2023 15:42
@Shobuj-Paul Shobuj-Paul force-pushed the feature/store_planner_data branch 4 times, most recently from ce014b4 to b54673d Compare November 11, 2023 09:35
@Shobuj-Paul Shobuj-Paul force-pushed the feature/store_planner_data branch 2 times, most recently from 6e5164d to 66fbc6f Compare November 27, 2023 11:51
@mergify Mergify
Copy link

mergify bot commented Dec 17, 2023

This pull request is in conflict. Could you fix it @Shobuj-Paul?

@github-actions GitHub Actions
Copy link

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

@github-actions github-actions bot added the stale Inactive issues and PRs are marked as stale and may be closed automatically. label Apr 10, 2024
@peterfryd
Copy link

Hi, what is the timeline for this PR?
I am currently doing planning for a 6 DOF in a rather complex static environment in Ros2 Humble and i have the same issue as this guy: moveit/moveit#2160.
As it is now, the successrate for the persistentLazyPrmStar planner i use from the Ompl library isnt close to being good enough without needing a huge amount of nodes which causes the saving process to slow down. Hence, the node doesnt successfully save the data before being killed by SIGTERM which then causes the data to become corrupt.
I see that there is a service available called /save_map but the saved roadmap cant be read properly by the planner for later uses.
Would love to get some help, and i see that this PR is related to it :-))

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

@henningkayser henningkayser henningkayser left review comments

@sjahr sjahr sjahr left review comments

@dyackzan dyackzan dyackzan left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
stale Inactive issues and PRs are marked as stale and may be closed automatically.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add service to store OMPL motion planner data
6 participants
@Shobuj-Paul @peterfryd @henningkayser @sjahr @dyackzan @andreas-botbuilt