MoveIt! Kinetic Modifications for FCL 0.5 and octomap 1.8

[de-vri-es]

FCL 0.5 was released which supports octomap 1.8. That is good, but additionally, FCL 0.5 uses std::shared_ptr in their API whereas for example geometric_shapes and moveit use boost::shared_ptr.

I forked both geometric_shapes and moveit_core for now and switched them to std::shared_ptr for octomap and fcl types. However, I'm wondering how to continue. I could put in pull requests but there is probably more work to be done besides geometric_shapes and moveit_core. In specific, the change for geometric_shapes includes a public interface change (from boost to std::shared_ptr).

I explored another alternative to see if it is possible to mix boost::shared_ptr and std::shared_ptr. In general, it is possible to mix the shared_ptrs by overwriting the default deleter, but when weak_ptr enters to the mix issues arise [1]. I quickly checked and FCL does seem to use std::weak_ptrs, which can not reliable be made to work for objects originally managed by a boost::shared_ptr.

So, I think the only way forward really is to switch geometric_shapes, moveit and everything depending on it to std::shared_ptr. The question is, how should we proceed with that? I'm willing to put in the work, but I would like to make sure that we agree on the way forward, and how to coordinate this (and possibly see what more needs to be done, and where).

Note that is is not necessarily as bad as it sounds, since it only needs to affect packages directly using FCL or geometric_shapes::OcTree.

[1] http://stackoverflow.com/a/12315035

[de-vri-es]

For reference, the changes to geometric_shapes [1] and moveit_core [2] are available in forks under the Delft Robotics github organization.

[1] https://github.com/delftrobotics/geometric_shapes/tree/fcl-0.5 [2] https://github.com/delftrobotics/moveit_core/tree/fcl-0.5

[v4hn]

On Mon, Jul 25, 2016 at 06:00:46AM -0700, Maarten de Vries wrote:

I forked both geometric_shapes and moveit_core for now and switched them to std::shared_ptr for octomap and fcl types. However, I'm wondering how to continue. I could put in pull requests but there is probably more work to be done besides geometric_shapes and moveit_core.

Yes, there is more work. Nevertheless, for kinetic we plan to move MoveIt! to c++11 std where possible. Pull requests are welcome! (Keep in mind we did not yet release for kinetic, so the interfaces will see some change before that.

[de-vri-es]

Does that mean that in general std::shared_ptr should be preferred over boost::shared_ptr, even if not required by a dependency (such as fcl in this case)? In some cases of course, dependencies may actually require boost::shared_ptr (such as roscpp).

I do believe that geometric_shapes has been released for kinetic already though? So can we still modify the interface there?

[davetcoleman]

geometric_shapes is almost exclusively used by MoveIt!, though the wiki says it is also used by collada_urdf, so I think its safe to change that interface. we're going to need to re-release it soon anyway for Kinetic because we've added C++11 support to the CMake file since the last release

for kinetic we plan to move MoveIt! to c++11 std where possible

indeed - any help with getting FCL to work with moveit kinetic is welcomed. i'm happy to add more kinetic-devel branches where needed across the moveit repos

[de-vri-es]

Alright, I'll make pull requests of what I have so far. Should we use this issue to track what else needs to be done?

[davetcoleman]

Sounds good - I renamed the issue to reflect this

[davetcoleman]

Sorry to ask this late - but its not clear to me whether we'll use FCL 0.5 in Kinetic or the previous version, I asked this question here

[de-vri-es]

Ah right. If we want to use octomap 1.8 there isn't really a choice. I personally think we should aim for that, but lets keep that discussion there. These PR's should wait until that is decided.

Alternatively, we can add a cmake_option to switch, but that is a higher maintenance burden.

[de-vri-es]

An intermediate recap:

These PRs should cover all packages in their respective repositories:

• [x] ros-planning/geometric_shapes#47
• [x] ros-planning/moveit_core#316
• [x] ros-planning/moveit_ros#726
• [x] ros-planning/moveit_plugins#22
• [x] ros-planning/moveit_setup_assistant#122

It would also be useful to have a list of repositories that should still be tested. Quickly scanning all ros-planning repositories, this is what I came up with (without knowing for sure if they really use octomap or fcl):

• moveit_experimental
• moveit_planners
• moveit_plugins
• moveit_pr2
• navigation

If the list is incorrect, corrections are welcome :)

[v4hn]

@davetcoleman: I answered to you there. So we need the c++11 patches in kinetic.

[de-vri-es]

Quick note: unit tests are still failing to build. I got them build locally but a lot seem to be failing. I don't think the failures are related to the shared_ptr change, but I will investigate a bit more before drawing that conclusion. Part of it may also be my python3-gcc6-archlinux environment.

[de-vri-es]

Turns out many of the unit test build failures are related to using gcc 6. Only minor changes were needed to moveit_core which are now included in the existing PR (#316). I will put the changes needed for compiling the unit tests with c++11 in a different PR.

The tests that I could run pass, although there were some python 3 issues that I will look at later (and with a separate PR).

[de-vri-es]

Added ros-planning/moveit_plugins#22 to the list above. All it does is enable C++11 support for moveit_plugins, which is all that is needed there as far as I could see.

[davetcoleman]

@de-vri-es you've now a custom rosinstall and travis build for all these PRs that includes the latest FCL and Octomap. I also included some loose instructions for running this locally on your machine if you feel inclined. Feel free to change the moveit.rosinstall file in that repo to include whatever other PRs you need, but you'll need to keep your changes to one PR per repo to test this.

Once that Travis passes we can merge all these in

[de-vri-es]

Awesome, thanks. I added the updated version of moveit_plugins to the rosinstall file. Its getting quite late for me here, so I will probably do the actual testing tomorrow :)

[de-vri-es]

moveit_setup_assistant also needs to be compiled with C++11 support, so we need a kinetic_devel branch for the PR :)

/edit: Which I now see is also noticed elsewhere :)

[v4hn]

On Tue, Jul 26, 2016 at 05:37:31AM -0700, Maarten de Vries wrote:

moveit_setup_assistant also needs to be compiled with C++11 support, so we need a kinetic_devel branch for the PR :)

done

[de-vri-es]

Updated the list above with ros-planning/moveit_setup_assistant#120

Still need to get the travis build to pass. It's not compiling FCL currently (it's not a catkin package).

[davetcoleman]

I added a script that pulls in the package.xml - not sure if its getting into the correct folder though: https://github.com/davetcoleman/moveit_kinetic_cpp11/commit/ab0de4fe567810a326ec57d26bb6e87e0dc3cc38

Also another issue is that moveit_core is still pulling in libfcl-dev here Its causing the built to re-install libfcl after we manually uninstall it. Could you change that in your branch to just read fcl?

[de-vri-es]

Also another issue is that moveit_core is still pulling in libfcl-dev here Its causing the built to re-install libfcl after we manually uninstall it. Could you change that in your branch to just read fcl?

Done. As I understood FCL will be shipped in the ROS repos anyway as catkin package, right? Otherwise depending on fcl isn't correct I'd think.

I added a script that pulls in the package.xml - not sure if its getting into the correct folder though: davetcoleman/moveit_kinetic_cpp11@ab0de4f

Regarding the wget destination, it probably needs something added, but I'm not familiar with travis so I'm not sure what the working directory is at that point. I'd guess it needs -O fcl/package.xml added. Won't the CMakeLists.txt also require patching to invoke catkin_package()?

[davetcoleman]

I'd guess it needs -O fcl/package.xml added.

I already added cd fcl but your solution is more elegant

Won't the CMakeLists.txt also require patching to invoke catkin_package()?

No it will build as a pure cmake package with catkin_tools

[de-vri-es]

Ah, nice. I didn't spot the cd. So, it might actually pass, or alteast get further into the build now :)

[davetcoleman]

The package.xml we are using appears to be outdated - it depends on libccd when it should be libccd-dev. Will attempt to fix by moving package.xml to our test repo

based on: https://github.com/ros/rosdistro/blob/master/rosdep/base.yaml#L1324

[davetcoleman]

the kinetic build is passing with FCL 0.5! great job @de-vri-es. there are some failing tests but i think its fcl/octomaps fault... they aren't setup to work properly with catkin's test facility. perhaps you could take a look though

[de-vri-es]

Nice. Thanks for the help. I'll look into the unit tests tomorrow.

[de-vri-es]

I added an option to blacklist packages for the unit test phase for the CI server, and used that to blacklist fcl, octomap, octovis and dynamic_edt_3d (the last three are all part of the octomap repository). It would appear the unit tests are now all passing.

[davetcoleman]

Yes its passing! I've merged all but https://github.com/ros-planning/moveit_core/pull/316 because it needs to be rebased

[dbolkensteyn]

@v4hn Could you set the new kinetic-devel branch to be the default one on the moveit_setup_assistant repo?

[v4hn]

On Wed, Jul 27, 2016 at 11:10:40PM -0700, Dinesh Bolkensteyn wrote:

@v4hn Could you set the new kinetic-devel branch to be the default one on the moveit_setup_assistant repo?

I would like to do that, but it seems I'm not allowed to. @davetcoleman Could you do that please?

[davetcoleman]

done

[davetcoleman]

I believe the necessary modifications are complete for FCL & Octomap, we're just waiting on the new debians to be released for FCL. Closing this issue, feel free to reopen if needed.