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.

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

Upstream patches from robostack-humble #2391

Open
2 of 8 tasks
tylerjw opened this issue Sep 27, 2023 · 9 comments
Open
2 of 8 tasks

Upstream patches from robostack-humble #2391

tylerjw opened this issue Sep 27, 2023 · 9 comments
Assignees
Labels
enhancement New feature or request persistent Allows issues to remain open without automatic stalling and closing.

Comments

@tylerjw
Copy link
Member

tylerjw commented Sep 27, 2023

@tylerjw tylerjw added the enhancement New feature or request label Sep 27, 2023
@traversaro
Copy link
Contributor

https://github.com/RoboStack/ros-humble/blob/main/patch/ros-humble-moveit-common.win.patch

This is actually not a moveit issue, but rather a backward_ros issue. However, I think we fixed the issue there in RoboStack/ros-humble#102 (comment), so probably we can just drop the patch on the robostack side.

https://github.com/RoboStack/ros-humble/blob/main/patch/ros-humble-moveit-core.patch
https://github.com/RoboStack/ros-humble/blob/main/patch/ros-humble-moveit-core.win.patch

If I remember correctly, this patch is related to how MoveIt looks for fcl . In MoveIt for ROS1 this was fixed by also supporting find_package(fcl) (see https://github.com/ros-planning/moveit/blob/1.1.13/moveit_core/CMakeLists.txt#L29), not sure if this is an option also for moveit2. In MoveIt for ROS1 I think we still had to support some ancient fcl version that did not install fcl-config.cmake files, not sure if this is still a problem also in MoveIt2.

https://github.com/RoboStack/ros-humble/blob/main/patch/ros-humble-moveit-kinematics.patch

Not sure about this, perhaps @Tobias-Fischer remembers something.

https://github.com/RoboStack/ros-humble/blob/main/patch/ros-humble-moveit-planners-ompl.patch

This seems a conda-related workaround, hopefully it should not be necessary anymore.

https://github.com/RoboStack/ros-humble/blob/main/patch/ros-humble-moveit-resources-prbt-ikfast-manipulator-plugin.patch

This seems a use of variable length array (that is not supported on MSVC) and hack to implement a static assert that can be substituted with a C++11's static_assert ?

https://github.com/RoboStack/ros-humble/blob/main/patch/ros-humble-moveit-setup-assistant.patch
https://github.com/RoboStack/ros-humble/blob/main/patch/ros-humble-moveit-setup-core-plugins.patch
https://github.com/RoboStack/ros-humble/blob/main/patch/ros-humble-moveit-setup-framework.patch

Not sure about this, perhaps @Tobias-Fischer remembers something.

@tylerjw
Copy link
Member Author

tylerjw commented Sep 28, 2023

@traversaro thank you for the helpful comments. Is there a way I can test robostack to see if some of these patches are no longer needed? Should I try to run a robostack build locally or is it enough to submit a PR deleting the patch?

@traversaro
Copy link
Contributor

Should I try to run a robostack build locally or is it enough to submit a PR deleting the patch?

PR jobs should be enough, but by default packages are not rebuilt unless specified, so you need to explicit specify in the vinca config file to build moveit packages. A simpler workflow to implement locally that is not exactly as a robostack build but that covers many of the same problem would be to compile moveit from source in a conda environment with dependencies installed via conda-forge and robostack (rosdep should work fine). See https://github.com/ros-planning/moveit/blob/master/.github/workflows/robostack.yaml for a similar workflow implement in a CI for moveit1 .

@tylerjw
Copy link
Member Author

tylerjw commented Sep 29, 2023

As to fcl. I tried using find-package but it doesn't look like the version of fcl we use has a find module for cmake: https://packages.ubuntu.com/jammy/amd64/libfcl-dev/filelist

Instead it uses pkgconfig. Do you know what versions of fcl you need to support in conda?

@traversaro
Copy link
Contributor

As to fcl. I tried using find-package but it doesn't look like the version of fcl we use has a find module for cmake: https://packages.ubuntu.com/jammy/amd64/libfcl-dev/filelist

These files should ensure that find_package(fcl) works fine:

/usr/lib/x86_64-linux-gnu/cmake/fcl/fcl-config-version.cmake
/usr/lib/x86_64-linux-gnu/cmake/fcl/fcl-config.cmake
/usr/lib/x86_64-linux-gnu/cmake/fcl/fcl-targets-none.cmake
/usr/lib/x86_64-linux-gnu/cmake/fcl/fcl-targets.cmake

Instead it uses pkgconfig. Do you know what versions of fcl you need to support in conda?

It is not pinned in https://github.com/RoboStack/ros-humble/blob/main/conda_build_config.yaml nor https://github.com/conda-forge/conda-forge-pinning-feedstock, so I guess the latest one is used (0.7.0 at the time of writing, see https://github.com/conda-forge/fcl-feedstock).

@tylerjw
Copy link
Member Author

tylerjw commented Sep 29, 2023

I must just be blind. I'm sorry. Trying now to get those modules to work.

@Tobias-Fischer
Copy link
Contributor

https://github.com/RoboStack/ros-humble/blob/main/patch/ros-humble-moveit-kinematics.patch

Not sure about this, perhaps @Tobias-Fischer remembers something.

I can't recall why the changes in cached_ik_kinematics_plugin/include/moveit/cached_ik_kinematics_plugin/detail/GreedyKCenters.h are required.

The changes in the CMakeLists.txt are required as std::random_shuffle was removed in C++17 and does not compile on Windows.

@Tobias-Fischer
Copy link
Contributor

https://github.com/RoboStack/ros-humble/blob/main/patch/ros-humble-moveit-setup-assistant.patch
https://github.com/RoboStack/ros-humble/blob/main/patch/ros-humble-moveit-setup-core-plugins.patch

These changes were necessary as otherwise there was some compilation issue on Windows. I think all it does is make an implicit conversion from QString to std::string explicit. These changes should not cause any issues on the other platforms.

@Tobias-Fischer
Copy link
Contributor

https://github.com/RoboStack/ros-humble/blob/main/patch/ros-humble-moveit-setup-framework.patch

There are three separate sets of changes contained in this patch:

  1. The export header stuff is definitely required on Windows
  2. The Qt5 related changes are required for conda, which does not work with the deprecated way of finding Qt5. find_package(Qt5 COMPONENTS Core Widgets REQUIRED) is the new way of finding Qt5 and I would recommend adapting it here.
  3. The explicit string conversions as in my previous comment.

@tylerjw tylerjw self-assigned this Nov 20, 2023
@github-actions github-actions bot added the stale Inactive issues and PRs are marked as stale and may be closed automatically. label Jan 18, 2024
@tylerjw tylerjw added persistent Allows issues to remain open without automatic stalling and closing. and removed stale Inactive issues and PRs are marked as stale and may be closed automatically. labels Jan 19, 2024
@moveit moveit deleted a comment from github-actions bot Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request persistent Allows issues to remain open without automatic stalling and closing.
Projects
None yet
Development

No branches or pull requests

3 participants