Convert API to use std::shared_ptr by de-vri-es · Pull Request #215 · moveit/moveit

de-vri-es · 2016-09-12T20:05:45Z

This PR updates the API to use std::shared_ptr for moveit types instead of boost::shared_ptr.

I'm not sure what the opinions are on this topic, but this PR at the least provides a place to discuss this specific API change. But do note that other libraries are doing the same thing right about now (like fcl and urdfdom).

Technically, switching from boost::enable_shared_from_this to std::enable_shared_from_this is not 100% API compatible, since weak_from_this is only added in C++17. This is very unlikely to cause problems though.

One more point: pluginlib still returns boost::shared_ptr and using createUnmanagedInstance is discouraged. Instead I opted to convert the pointers to std:: instead. That method is not 100% safe when weak_ptr are involved in the general case, but it is in this case because nobody else has a copy to the original boost::shared_ptr.

Because the conversion isn't safe in general I didn't want to put it in a header (the header would have to be exported or duplicated across 5 packages). Instead I opted now to duplicate the function in an anonymous namespace in 5 source files. I've also got a PR open with class_loader to add support for std:: smart pointers (unique and shared), so when that lands it makes sense to adapt to that interface. In the mean time I didn't know a nicer solution than the current in this PR.

davetcoleman · 2016-09-12T21:32:51Z

moveit_core/collision_detection/include/moveit/collision_detection/world.h

-MOVEIT_CLASS_FORWARD(Shape);
+class Shape;
+typedef boost::shared_ptr<Shape> ShapePtr;
+typedef boost::shared_ptr<const Shape> ShapeConstPtr;


Just to make sure I understand the logic here - we don't want to use std::shared_ptr for Shape because its part of geometric_shapes, which is part of desktop-full, which is not suppose to have std in its public headers yet, correct?

https://github.com/ros-planning/geometric_shapes/blob/kinetic-devel/include/geometric_shapes/shapes.h#L273

We did already break this rule for that package because it was required for FCL/octomap, but I guess in this case we don't have a good reason to continue to break that rule. But another (cleaner) possibility would be to make the C++11 switch there, as well, correct?

I agree, it would be nice to have std::shared_ptr there. The previous deviation from REP-003 was out of necessity though. On the other hand, urdfdom also switched. And while it may not be a real ROS package that is more or less a technicality.

But yeah, as long as the geometric_shape API uses boost::shared_ptr we can't switch them here.

/edit: Apparently urdfdom is packaged as ROS package too. It's been a while since I've used the official Ubuntu distribution ^^

How about a comment for ROS-L saying we'll switch to C++11 then?

I agree, please add a comment here that explains why this code still uses boost::shared_ptr and that it should be changed for lunar.

davetcoleman · 2016-09-13T00:55:48Z

+1 but other maintainers should get a chance to review and discuss this big change
@v4hn @rhaschke @mikeferguson

davetcoleman · 2016-09-19T21:25:10Z

I would like to merge this asap so we have momentum for the kinetic release

rhaschke

+1.

rhaschke · 2016-09-22T11:38:37Z

moveit_core/collision_detection/include/moveit/collision_detection/world_diff.h


    /* used to unregister the notifier */
-    boost::weak_ptr<World> world_;
+    std::weak_ptr<World> world_;


What about a WeakPtr definition in MOVEIT_CLASS_FORWARD?

We had a short discussion at #206. Nobody seemed very interested (including me), but if you think that is the right approach you can reopen the PR.

-1 to that macro

rhaschke · 2016-09-22T11:39:13Z

moveit_core/collision_detection/src/world_diff.cpp

    changes_ = other.changes_;

-    boost::weak_ptr<World>(world).swap(world_);
+    std::weak_ptr<World>(world).swap(world_);


see above (WeakPtr forward declaration)

rhaschke · 2016-09-22T11:44:38Z

moveit_core/planning_scene/include/moveit/planning_scene/planning_scene.h

 #include <moveit_msgs/PlanningSceneComponents.h>
-#include <boost/enable_shared_from_this.hpp>
 #include <boost/noncopyable.hpp>
 #include <boost/shared_ptr.hpp>


Is this still necessary?

Yeah, line 96 and 887 still use it for non-moveit types.

yes, but now thant you use the Ptr typedef they don't mention the symbol anymore. So this can be removed imo
The same is probably true for a number of other files..

Good point, will check.

v4hn

some minor refinements requested.
but overall this looks good to me.

v4hn · 2016-09-22T12:06:09Z

moveit_core/distance_field/include/moveit/distance_field/distance_field.h

-MOVEIT_CLASS_FORWARD(Shape);
+class Shape;
+typedef boost::shared_ptr<Shape> ShapePtr;
+typedef boost::shared_ptr<const Shape> ShapeConstPtr;


same as above.

v4hn · 2016-09-22T12:07:29Z

moveit_core/collision_detection/include/moveit/collision_detection/world_diff.h


 #include <moveit/collision_detection/world.h>
 #include <moveit/macros/class_forward.h>
-#include <boost/weak_ptr.hpp>


this is missing a <memory> include, isn't it?

Well, you can't make weak_ptrs without shared_ptrs. Since we have std::shared_ptr typedefs available from a macro in #include <moveit/macros/declare_ptr.h>, that will have pulled in #include <memory>.

But I am a fan of the rule "if you use a symbol from a header, include it", so I'll add it.

#include <memory> added

v4hn · 2016-09-22T13:11:02Z

moveit_core/robot_model/include/moveit/robot_model/link_model.h

-MOVEIT_CLASS_FORWARD(Shape);
+class Shape;
+typedef boost::shared_ptr<Shape> ShapePtr;
+typedef boost::shared_ptr<const Shape> ShapeConstPtr;


gosh, how many of these cases are there? Why is this not centralized?

I was a bit hesitant to add a macro for declaring boost::shared_ptr if they're going to turn in to std::shared_ptrs in the not-too-far future. Having macros like that lying around could be seen as an invitation to use them.

How about adding these typedefs in geometric_shapes (and possibly a new header geometric_shapes/forward.h to avoid including complex header), and until those are available keeping them explicit like this?

+1, though I think you could make the switch now since kinetic is not released yet and Travis builds geometric_shapes from source for this PR

Ah, these forward delarations where added by @rhaschke in 13d2e62 to reduce includes of geometric_shapes.
I don't like the idea to introduce a header there that only forward declares all the types.
@rhaschke is this change really worth the overhead to maintain the boost-shared_ptrs here?

On the other hand I agree with @davetcoleman. I don't think it's any good to have boost- and std- shared_ptr around in geometric_shapes in kinetic. So I would actually propose to fully move to std::shared_ptr there. This would also simplify the forward declarations here.

I like the idea of that. I think I'll have some questions but I'll open a PR there for those (or an issue if the questions are blocking).

I didn't think of it as a "solution to this problem". More as "this problem arises because the previous change that made geometric_shapes use c++11 shared_ptr was inconsistent."
And if we want to switch MoveIt! to std::shared_ptr (which is what this request is about), this should imho include geometric_shapes. Incidentally, I assumed all shared_ptr references where switched to c++11 when we discussed the issue with FCL and octomap.

Nevertheless, moveit/geometric_shapes#57 and using MOVEIT_CLASS_FORWARD here again will also solve the practical issue.

That would basically mean dropping one commit from this PR:

Don't use MOVEIT_CLASS_FORWARD for non-moveit classes.

I didn't seem your last comment when replying. But fair enough, it does make sense to switch geometric_shapes and moveit at the same time. It could get very confusing otherwise.

And in that light it may be acceptable to use MOVEIT_CLASS_FORWARD for geometric_shapes types. It feels a bit weird to define typedefs using a moveit convention in the namespace of another library, but I suppose geometric_shapes and moveit are related enough.

Shall I just drop the relevant commit then, and revert to using MOVEIT_CLASS_FORWARD?

let's see what comes out of your new request for geometric_shapes.

I was assuming we were not switching these because it would violate the ROS policy of no std::shared_ptrs in the public headers of packages in ros-desktop-full. But then again, we already had to break that rule for FCL and Octomap, so it doesn't make a difference going all the way now.

v4hn · 2016-09-22T13:11:57Z

moveit_experimental/collision_distance_field/src/collision_world_distance_field.cpp

 #include <moveit/collision_distance_field/collision_world_distance_field.h>
 #include <moveit/collision_distance_field/collision_common_distance_field.h>
 #include <moveit/distance_field/propagation_distance_field.h>
-#include <boost/make_shared.hpp>


this is missing <memory>

Same as previous, so will also add.

#include <memory> added

v4hn · 2016-09-22T13:26:54Z

moveit_plugins/moveit_ros_control_interface/src/joint_trajectory_controller_plugin.cpp

                                                                     const std::vector<std::string> &resources)
  {
-    return boost::make_shared<moveit_simple_controller_manager::FollowJointTrajectoryControllerHandle>(
+    return std::make_shared<moveit_simple_controller_manager::FollowJointTrajectoryControllerHandle>(


<memory> is not included here, but <boost/shared_ptr.hpp> still is. Is the latter still required here?

Good catch. Indeed, boost header removed, #include <memory> added

v4hn · 2016-09-22T13:28:12Z

moveit_ros/manipulation/pick_place/include/moveit/pick_place/pick_place.h

 #include <moveit_msgs/PickupAction.h>
 #include <moveit_msgs/PlaceAction.h>
 #include <boost/noncopyable.hpp>
-#include <boost/enable_shared_from_this.hpp>


<memory> should be included for std::enable_shared_from_this

#include <memory> added

v4hn · 2016-09-22T13:29:34Z

moveit_ros/perception/semantic_world/include/moveit/semantic_world/semantic_world.h

-MOVEIT_CLASS_FORWARD(Shape);
+class Shape;
+typedef boost::shared_ptr<Shape> ShapePtr;
+typedef boost::shared_ptr<const Shape> ShapeConstPtr;


and yet another case that should have a comment saying this should be changed in lunar.

de-vri-es · 2016-09-22T17:30:58Z

hmm, seems something went wrong during a rebase

de-vri-es · 2016-09-22T17:54:31Z

Ah, the new BenchmarkExecutor wasn't using the typedefs. Added a commit for that. Looks like that commit doesn't need to be cherry picked to Indigo and Jade either.

davetcoleman · 2016-09-22T18:28:30Z

correct, BenchmarkExecutor only exists in Kinetic and will not be backported

de-vri-es · 2016-09-22T18:41:00Z

Looks like the ~~tests are~~ build is failing because the version of pluginlib is too old. Probably needs to wait for a docker image rebuild?

Bit off-topic: apparently the version_gte tags in package.xml are just informative? Couldn't find any documentation on what they actually do, but it seems catkin and rosdep are ignoring them.

gavanderhoorn · 2016-09-22T18:51:12Z

@de-vri-es wrote:

Bit off-topic: apparently the version_gte tags in package.xml are just informative? Couldn't find any documentation on what they actually do, but it seems catkin and rosdep are ignoring them.

This is documented in REP-140, but you're right that rosdep doesn't use that information. See also ros-infrastructure/rosdep#325 (specifically: ros-infrastructure/rosdep#325 (comment)).

Bloom translates those into the appropriate key:value pairs in Debian control and RPM spec files.

davetcoleman · 2016-09-22T22:09:00Z

I'm looking into why the shadow-fixed CI is not building on Dockerhub

davetcoleman · 2016-09-22T22:29:19Z

Docker error is described here: ros/ros_comm#904

de-vri-es · 2016-09-24T10:03:02Z

@gavanderhoorn: Thanks for the information. Not sure how I missed that.

Update regarding this PR:

Dropped the commit that undid MOVEIT_CLASS_FORWARD(Shape).
Switched out a boost::dynamic_ptr_cast for the std equivalent.
Replaced boost::scoped_ptr with std::unique_ptr.

I think this addresses all the remaining review points, but correct me if I'm wrong.

Only the first change affects the MoveIt API. The other two are implementation changes. The last point wasn't technically needed, but I think it makes sense to remove all boost smart pointers in one go.

Locally this builds (with moveit/geometric_shapes#57), including the tests. I've never been able to run all tests successfully on my system though (probably due to python3 issues which I intend to investigate later). I don't expect any problems though, considering the interface and semantics of boost:: and std::shared_ptr are virtually identical (so the compiler should have caught any problems).

de-vri-es · 2016-09-24T10:12:48Z

Actually, there are still boost::shared_ptrs left. It is possible that many of these can also be changed, but it needs to be verified that the library owning the type doesn't use boost::shared_ptr in their own API.

There are a number of cases in moveit_core and moveit_ros. Mostly these are shared_ptrs to tf, srdf, and urdf types. The real tricky bit here is knowing if it conflicts with other libraries besides the one owning the type. Many of these shared_ptrs might not even be part of the MoveIt API though, in which case it is safe. Should I look into these cases in more detail?

v4hn · 2016-09-24T11:53:25Z

For tf I would stick with boost::shared_ptr for kinetic as there are interfaces in other packages that want to take ownership of a tf-listener and expect the boost ptr.

I'm unsure about the others. grep -r boost::shared_ptr --include *.h | grep -v tf::Transform basically lists moveit_experimental/collision_distance_field that still uses boost ptrs internally and a small number of diverse cases that need individual review.

But yes, I would welcome changing these cases (and also the non-API cases) too.

de-vri-es · 2016-09-24T21:09:55Z

The last commit converts a number of internal shared_ptr to std::shared_ptr. I believe they were all safe to be converted.

Ignoring boost::shared_ptr<tf::...> and everything in moveit_experimental, chomp and sbpl, there is only one explicit boost::shared_ptr<urdf::Model> left in the setup assistant. But I think that is best left until ros/robot_model#153 is available.

de-vri-es · 2016-09-26T18:21:20Z

Rebased on latest kinetic-devel again, now with collision_distance_field and chomp.

davetcoleman · 2016-09-26T18:55:33Z

Need #250 to be merged before test build https://github.com/davetcoleman/moveit_kinetic_cpp11 passes, indicating this PR is good

davetcoleman · 2016-09-27T17:21:25Z

@de-vri-es can you rebase this PR with the head of kinetic-devel? The build is failing because of the now fixed fcl dependency

de-vri-es · 2016-09-27T17:26:50Z

Rebased. Note that there are two commits that we probably want to cherry-pick to I/J (using commit messages in case of future rebases):

Use srdf::ModelPtr typedefs.
Use shared_ptr typedefs in collision_distance_field and chomp.

de-vri-es · 2016-09-27T18:33:14Z

The travis test passes \o/
https://travis-ci.org/davetcoleman/moveit_kinetic_cpp11

davetcoleman · 2016-09-27T18:59:37Z

darn there is a merge conflict now, and i just merged moveit/geometric_shapes#57

can you fix asap?

de-vri-es · 2016-09-27T19:01:43Z

On it, rebase worked automatically, but I'd like to compile before pushing to be sure (I find automatic merges scary). Should be about 5 to 10 minutes.

de-vri-es · 2016-09-27T19:08:43Z

Compiled fine locally, pushed.

davetcoleman · 2016-09-27T19:10:50Z

👏 C++11!

de-vri-es · 2016-09-27T19:11:56Z

\o/

v4hn · 2016-09-29T11:40:47Z

I fully agree with this change, but want to point this out:

This change entails that all packages that compile with a moveit_core dependency in Xenial (the major ubuntu distribution supported by ROS kinetic) have to explicitly specify a standard >= c++11 in the build scripts to compile their packages. This is because Xenial ships gcc 5.3.1 which does not yet default to c++11.
At the moment this breaks our tutorials and packages in moveit_pr2 for kinetic (requests in progress).

de-vri-es force-pushed the std-shared-ptr-api branch 2 times, most recently from a2a8c00 to d71592e Compare September 12, 2016 20:30

davetcoleman reviewed Sep 12, 2016
View reviewed changes

davetcoleman mentioned this pull request Sep 12, 2016

Add weak_ptr typedefs to MOVEIT_DECLARE_PTR #206

Closed

davetcoleman approved these changes Sep 15, 2016

View reviewed changes

rhaschke assigned rhaschke, v4hn and mikeferguson Sep 16, 2016

rhaschke approved these changes Sep 22, 2016

View reviewed changes

v4hn requested changes Sep 22, 2016

View reviewed changes

de-vri-es force-pushed the std-shared-ptr-api branch from d71592e to 432e1ed Compare September 22, 2016 17:22

de-vri-es force-pushed the std-shared-ptr-api branch from 432e1ed to 9cdfbd4 Compare September 22, 2016 17:37

v4hn mentioned this pull request Sep 22, 2016

Switch boost::shared_ptr to std::shared_ptr. moveit/geometric_shapes#57

Merged

de-vri-es force-pushed the std-shared-ptr-api branch from a81a7ae to 6404ac3 Compare September 24, 2016 09:29

de-vri-es force-pushed the std-shared-ptr-api branch from c949c87 to e7b28c1 Compare September 24, 2016 12:03

v4hn approved these changes Sep 26, 2016

View reviewed changes

de-vri-es force-pushed the std-shared-ptr-api branch from 174af0e to a2b9c2d Compare September 27, 2016 17:25

de-vri-es added 12 commits September 27, 2016 20:59

Switch to std::shared_ptr for the moveit API.

c8c90ac

Switch to std::weak_ptr.

6073ecb

Switch to std::enable_shared_from_this.

0818ce8

Switch to std::make_shared.

7d2424a

Convert pluginlibs shared_ptrs to std::

a300f18

Use shared_ptr typedefs in BenchmarkExecutor.cpp

734cae9

Switch to std::dynamic_ptr_cast.

d293883

Switch to std::unique_ptr (instead of boost::scoped_ptr).

b5a6198

Use srdf::ModelPtr typedefs.

6f459b1

Convert assorted internal shared_ptrs.

9824962

Use shared_ptr typedefs in collision_distance_field and chomp.

cb8f002

Convert collision_distance_field to std::shared_ptr.

fa5a9a8

de-vri-es force-pushed the std-shared-ptr-api branch from a2b9c2d to fa5a9a8 Compare September 27, 2016 19:08

davetcoleman merged commit 22c7aa6 into moveit:kinetic-devel Sep 27, 2016

davetcoleman mentioned this pull request Sep 27, 2016

Allow travis to pass only for shadow-fixed #257

Merged

v4hn mentioned this pull request Jul 24, 2018

[WIP] Moving to C++ 14 PointCloudLibrary/pcl#2382

Closed

8 tasks

JafarAbdi pushed a commit to JafarAbdi/moveit that referenced this pull request Mar 24, 2022

Apply missing clang-tidy fixes (moveit#215)

11fb275

Conversation

de-vri-es commented Sep 12, 2016

Uh oh!

davetcoleman Sep 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

de-vri-es Sep 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davetcoleman commented Sep 13, 2016

Uh oh!

davetcoleman commented Sep 19, 2016

Uh oh!

rhaschke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v4hn Sep 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v4hn left a comment

Choose a reason for hiding this comment

Uh oh!

v4hn Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v4hn Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

davetcoleman Sep 12, 2016 •

edited

Loading

de-vri-es Sep 12, 2016 •

edited

Loading

v4hn Sep 24, 2016 •

edited

Loading

v4hn Sep 22, 2016 •

edited

Loading

v4hn Sep 22, 2016 •

edited

Loading

de-vri-es commented Sep 22, 2016 •

edited

Loading

gavanderhoorn commented Sep 22, 2016 •

edited

Loading