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

Use std::shared_ptr for compatibility with FCL 0.5. #47

Merged
merged 1 commit into from
Jul 27, 2016

Conversation

de-vri-es
Copy link
Contributor

FCL 0.5 uses std::shared_ptr (and std::weak_ptr) in their public API. To allow using geometric_shapes with FCL 0.5, this PR changes the relevant shared_ptrs to std. This change does affect the public API of geometric_shapes.

See also moveit/moveit_core#315

One question: should we hunt down other shared_ptrs as well? In any case, for compatibility with ROS messages, those types should continue to use boost::shared_ptr until roscpp switches (if that ever happens).

I suppose the pragmatic approach for now is to only switch to std::shared_ptr where required by external libraries, but I also wouldn't mind updating more to std::shared_ptr (where possible).

@davetcoleman
Copy link
Member

I'm going to hold off on merging any of these PRs until all of MoveIt! Kinetic builds again, that way Travis won't be failed for too long

@de-vri-es
Copy link
Contributor Author

Makes sense.

@davetcoleman
Copy link
Member

I suppose the pragmatic approach for now is to only switch to std::shared_ptr where required by external libraries, but I also wouldn't mind updating more to std::shared_ptr (where possible).

Can you open an issue for this in moveit_core as an API change for kinetic?

@davetcoleman
Copy link
Member

The set of C++11 changes required for FCL 0.5 discussed here are now passing in Kinetic in this test environment. Note that the kinetic branch will continue not to work without installing from source the latest Octomap and FCL, but these will soon be released. Merging all related PRs.

Thanks @de-vri-es for helping migrate MoveIt!

@davetcoleman davetcoleman merged commit 8162491 into moveit:kinetic-devel Jul 27, 2016
@de-vri-es
Copy link
Contributor Author

My pleasure, and thanks for all the help :)

@de-vri-es
Copy link
Contributor Author

Can you open an issue for this in moveit_core as an API change for kinetic?

Issue opened at moveit/moveit_core#319

@jonbinney
Copy link

Using -std=c++11 in the public API of geometric_shapes violates REP 0003, right? According to that REP, for kinetic "the API of the packages included in desktop-full will not use any C++11-specific feature."

@davetcoleman
Copy link
Member

I thought that must be a mistake in the REP because for Kinetic it says: "Minimum Requirements: C++11". But it was on purpose

It seems though violating it is unavoidable since octomap already violated it

@jonbinney
Copy link

Where does Octomap use std::shared_ptr in its (public facing) API? In geometric shapes, the problem isn't that it uses std::shared_ptr, just that it uses it in a public function that gets used by other ROS packages (https://github.com/ros-planning/geometric_shapes/blob/1a020ba1969b7de8d7772e2a82693411733cad65/include/geometric_shapes/shapes.h#L254). At least for geometric_shapes, couldn't that constructor take a boost::shared_ptr and convert it to an std::shared_ptr if needed for passing to octomap?

@davetcoleman
Copy link
Member

Ok, it looks like FCL is what requires octree to be wrapped in a std::shared_ptr. Hopefully you're right and we can keep the public facing APIs using boost, I'm sure @de-vri-es has a better understanding of this

@jonbinney
Copy link

What is the dependency chain between FCL and geometric_shapes? FCL doesn't directly use geometric shapes (or vice versa), right?

@davetcoleman
Copy link
Member

I believe its:

FCL depends on octomap
geometric_shapes depends on octomap
moveit_core depends on FCL and geometric_shapes

@de-vri-es
Copy link
Contributor Author

de-vri-es commented Aug 2, 2016 via email

@de-vri-es de-vri-es deleted the fcl-0.5 branch September 22, 2016 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants