search

moveit / moveit2

:robot: MoveIt for ROS 2
https://moveit.ai/
BSD 3-Clause "New" or "Revised" License
1.04k stars 510 forks source link

Upstream patches from robostack-humble #2391

Open tylerjw opened 12 months ago

tylerjw commented 12 months ago

There are a handful of patches in robostack-humble for MoveIt. It would be nice to upstream these patches so moveit can be built in robostack without modification.

Some of these patches are windows specific and we will need to learn how to test on windows. These patches are against humble, it would be nice if we can also make these patches in main.

traversaro commented 12 months ago

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 https://github.com/RoboStack/ros-humble/pull/102#issuecomment-1715621025, 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 commented 11 months ago

@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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

Tobias-Fischer commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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.