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 ompl_interface with latest version of OMPL #2994

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

Update ompl_interface with latest version of OMPL #2994

wants to merge 12 commits into from
+21 −21

Conversation

MarqRazz
Copy link
Contributor

@MarqRazz MarqRazz commented Sep 9, 2024 •
edited
Loading

Description

I'm working on debugging some issues and require building MoveIt2 and OMPL from source. This PR updates the package moveit_planners_ompl to work with the latest changes made in OMPL and it's more modern approach to exporting/linking the library but will require building it from source until a new release is made (I am working on getting it released and will keep this PR updated with the status).

I was also having issues with OMPL but that package was recently updated to properly work with Colcon. Here is a link to the issue.

Big thanks to @JafarAbdi and @mamoll for helping me debug some of these issues and getting this updated! 🥇

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

@MarqRazz MarqRazz mentioned this pull request Sep 9, 2024
Closed
Ryanf55
Ryanf55 reviewed Sep 9, 2024
View reviewed changes
Copy link

@Ryanf55 Ryanf55 left a comment

Choose a reason for hiding this comment

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

LGTM.

moveit_planners/ompl/ompl_interface/CMakeLists.txt Outdated Show resolved Hide resolved
@JafarAbdi
Copy link
Member

Would be good to see if lines https://github.com/moveit/moveit2/pull/2994/files#diff-18741df7cbb95f9292a57457e88318346213653f65f3932a5a03a1eaf7b2655bR27-R30 are still needed

JafarAbdi
JafarAbdi reviewed Sep 9, 2024
View reviewed changes
moveit2.repos
@@ -12,3 +12,7 @@ repositories:
type: git
url: https://github.com/moveit/moveit_resources.git
version: ros2
ompl:
Copy link
Member

Choose a reason for hiding this comment

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

Should we wait till the new ompl version is released?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the ompl crew would be willing to release although they still have at least one unresolved regression lying around.

But I'm not sure it's so difficult to support both ompl cmake interfaces here for now. We do that in MoveIt.

v4hn
v4hn reviewed Sep 10, 2024
View reviewed changes
moveit_planners/ompl/ompl_interface/CMakeLists.txt Outdated Show resolved Hide resolved
moveit2.repos
@@ -12,3 +12,7 @@ repositories:
type: git
url: https://github.com/moveit/moveit_resources.git
version: ros2
ompl:
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the ompl crew would be willing to release although they still have at least one unresolved regression lying around.

But I'm not sure it's so difficult to support both ompl cmake interfaces here for now. We do that in MoveIt.

@MarqRazz
Copy link
Contributor Author

Would be good to see if lines https://github.com/moveit/moveit2/pull/2994/files#diff-18741df7cbb95f9292a57457e88318346213653f65f3932a5a03a1eaf7b2655bR27-R30 are still needed

According to OMPL's build documentation OMPL_LIBRARY_DIRS does not exist any more and the environment variables are not recommended any more so I removed it. If someone with a Mac could test this PR out that would be great!
image

@Ryanf55
Copy link

Ryanf55 commented Sep 10, 2024 •
edited
Loading

Would be good to see if lines https://github.com/moveit/moveit2/pull/2994/files#diff-18741df7cbb95f9292a57457e88318346213653f65f3932a5a03a1eaf7b2655bR27-R30 are still needed

According to OMPL's build documentation OMPL_LIBRARY_DIRS does not exist any more and the environment variables are not recommended any more so I removed it. If someone with a Mac could test this PR out that would be great! image

To support the previous CMake module behavior, the following variables are defined. These are for backwards compatability, but not recommended anymore.

It was my intent to preserve them for backwards compatability, but the reality on ompl main is they only exist for consuming ompl through submodule, not find_package. This was an oversight/mistake on my part, and I only realized it yesterday. OMPL did not change its major version, so there should not be ABI breaks. I have recognized the downstream impact this is having is larger than it should be. If MoveIT has build failures for something that worked previously, I am in the wrong. Please let me know if you have a minimum reproducible example and I will do my best to bring those variables back in find_package.

@Ryanf55
Copy link

Ryanf55 commented Sep 10, 2024 •
edited
Loading

This PR should get us closer to fixing the accidental breakage of the old variables in find_package. https://github.com/ompl/ompl/pull/1182/files#diff-148715d6ea0c0ea0a346af3f6bd610d010d490eca35ac6a9b408748f7ca9e3f4R10

We need to then add this back into the export, along with the other variables.

@MarqRazz MarqRazz mentioned this pull request Sep 11, 2024
Merged
@mergify Mergify
Copy link

mergify bot commented Oct 13, 2024

This pull request is in conflict. Could you fix it @MarqRazz?

sjahr
sjahr reviewed Oct 14, 2024
View reviewed changes
moveit2.repos Outdated Show resolved Hide resolved
@MarqRazz
Copy link
Contributor Author

Thanks for keeping this moving @sjahr!

@sjahr
Copy link
Contributor

sjahr commented Oct 14, 2024

@MarqRazz I'd just merge it now, since there aren't any CI failures related to the PR. Any concerns?

@MarqRazz
Copy link
Contributor Author

MarqRazz commented Oct 14, 2024 •
edited
Loading

Nope, I was just waiting for the 👍

Actually we need to release OMPL for this change to work right. I have not tested this against the latest release to ROS2.

@mamoll I would be happy to help get the OMPL release to ROS updated. Is there anything I can help with? @JafarAbdi also said he was will to help maintain the release.

@sjahr
Copy link
Contributor

sjahr commented Oct 14, 2024 •
edited
Loading

Ok, I'll wait for now 👍 Let me know when it's ready

@sjahr sjahr self-requested a review October 14, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JafarAbdi JafarAbdi JafarAbdi left review comments

@v4hn v4hn v4hn left review comments

@Ryanf55 Ryanf55 Ryanf55 approved these changes

@sjahr sjahr Awaiting requested review from sjahr

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

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

Successfully merging this pull request may close these issues.
5 participants
@MarqRazz @JafarAbdi @Ryanf55 @sjahr @v4hn