Use urdf::*ShredPtr instead of boost::shared_ptr #144

jspricke · 2016-08-23T19:34:36Z

urdfdom_headers uses C++ std::shared_ptr. As it exports it as custom
*SharedPtr type, we can use the to sty compatible.

scpeters · 2016-08-24T03:44:14Z

I'm going to close and re-open to trigger the ROS build farm pull request testing.

scpeters · 2016-08-24T04:00:44Z

travis failed because it tests indigo and jade, but the ROS build farm is happy when testing with kinetic

scpeters · 2016-08-24T04:03:21Z

Will this be a problem for debian jessie, which doesn't have urdfdom 0.4 (it has 0.3)?

jspricke · 2016-09-14T08:20:53Z

It needs at least urdfdom 0.4.

bmagyar · 2016-11-01T21:17:16Z

Keep this one in mind too:
#153

urdfdom_headers uses C++ std::shared_ptr. As it exports it as a custom *SharedPtr type, we can use them to stay compatible.

jspricke · 2016-11-04T22:18:10Z

Rebased on current master. Note that Travis still fails due to urdfdom being too old. Also this needs #165 now as well.

rhaschke

Kinetic is also supported on Wily and Jessie which do not yet ship with urdfdom 0.4. Use the compatibility header from urdf package.

rhaschke · 2016-11-06T11:31:01Z

collada_parser/src/collada_parser.cpp

        USERDATA(double scale) : scale(scale) {
        }
        double scale;
+#if __cplusplus <= 199711L


The decision for boost::shared_ptr vs. std::shared_ptr shouldn't implicitly depend on the c++ version, but it should be an deliberative decision of the developer. You should declare a type VoidSharedPtr for this.

rhaschke · 2016-11-06T11:32:18Z

collada_parser/include/collada_parser/collada_parser.h

 #include <boost/function.hpp>

-#include <urdf_model/model.h>
+#include <urdf_world/types.h>


This include doesn't exist in Kinect on Wily and Jessie. You should use the compatibility header of the urdfpackage instead to safely define ModelInterfaceSharedPtr.

rhaschke · 2016-11-06T11:32:35Z

collada_parser/src/collada_parser.cpp

                    }

                    _getUserData(pdomjoint)->p = pjoint;
+#if __cplusplus <= 199711L


rhaschke · 2016-11-06T11:33:45Z

urdf_parser_plugin/CMakeLists.txt


 find_package(catkin REQUIRED)
-find_package(urdfdom_headers REQUIRED)
+find_package(urdfdom_headers 0.4 REQUIRED)


This is not feasible for Kinetic on Wily and Jessie.

rhaschke · 2016-11-06T11:33:56Z

urdf_parser_plugin/include/urdf_parser_plugin/parser.h

 #define URDF_PARSER_PLUGIN_H

-#include <urdf_model/model.h>
+#include <urdf_world/types.h>


rhaschke · 2016-11-06T11:34:06Z

urdf_parser_plugin/package.xml

  <buildtool_depend>catkin</buildtool_depend>
-  <build_depend>liburdfdom-headers-dev</build_depend>
-  <run_depend>liburdfdom-headers-dev</run_depend>
+  <build_depend version_gte="0.4">liburdfdom-headers-dev</build_depend>


rhaschke

Kinetic is also supported on Wily and Jessie which do not yet ship with urdfdom 0.4. Use the compatibility header from urdf package.

rhaschke · 2016-11-06T12:05:43Z

Note that Travis still fails due to urdfdom being too old. Also this needs #165 now as well.

You should update .travis.yml to build the Kinetic branch against a Kinetic ROS.

jspricke · 2016-11-06T14:20:36Z

@rhaschke Thanks for the review, I've integrated your proposals.

rhaschke

Overall looks good. Thanks for taking the effort to replace all those type definitions. Please consider minor comments.

rhaschke · 2016-11-06T15:07:05Z

.travis.yml

  matrix:
-    - CI_ROS_DISTRO="indigo"
-    - CI_ROS_DISTRO="jade"
+    - CI_ROS_DISTRO="kinetic"


Sorry. Replacing Indigo and Jade with Kinetic wasn't a good advice. Travis is stuck to a Trusty system, which doesn't support Kinetic. Please revert this. Using the compatibility header, the Kinetic branch should compile in Indigo and Jade too now.

rhaschke · 2016-11-06T15:14:41Z

urdf_parser_plugin/include/urdf_parser_plugin/parser.h

 #define URDF_PARSER_PLUGIN_H

-#include <urdf_model/model.h>
+#include <urdf/urdfdom_compatibility.h>


This requires package urdf as a build dependency too.

This also means you need to have urdf as a run_depend. If we were using package xml version 2, it would be a build_export_depend. Basically, since there is no separate -dev package urdf needs to be installed at "run time" so other packages would be able to build against it. This is likely handled implicitly as a transitive dependency of one of the other run_depend's.

This would result in a circular dependency, as we tried already ;).

rhaschke · 2016-11-06T15:23:01Z

urdf/urdfdom_compatibility.h.in


 URDF_TYPEDEF_CLASS_POINTER(ModelInterface);
+
+typedef boost::shared_ptr<void> VoidSharedPtr;


Why do you define VoidSharedPtrhere? This is a compatibility header to declare types of urdfdom. However, VoidSharedPtris only used internally by ColladaParser. Hence, the declaration should go to collada_parser/src/collada_parser.cpp:179.

I don't see how to do it, as we would need @URDFDOM_DECLARE_TYPES@ in there to ifdef between the different urdfdom versions.

..we cloud export a define in the urdf/urdfdom_compatibility.h.in, would that be ok?

I don't see your issue. VoidSharedPtris a datatype declared and used internally in ColladaParseronly. It's not related to urdfdomor urdf package. You simply typedef boost::shared_ptr<void> VoidSharedPtr just before collada_parser/src/collada_parser.cpp:179.

but we need to differentiate between boost::shared_ptr and std::shared_ptr depending on the urdfdom version.

No, not at all. VoidSharedPtr doesn't depend on urdfdom. It's introduced only in ColladaParser. It's the free choice of the ColladaParserdeveloper when to switch to std::shared_ptr for this.

but it's used to point to either point to urdf::LinkSharedPtr or urdf::JointSharedPtr
So it depends on them.
(not commenting on the programming style)

I see. Didn't look that deep into the code. Nevertheless, VoidSharedPtr is a type within ColladaParser and should be declared there and not in the urdf package. You can convert between std::sharedptr and boost::sharedptr like this:

http://stackoverflow.com/questions/12314967/cohabitation-of-boostshared-ptr-and-stdshared-ptr#comment28669000_12315035

This reverts commit f1c085c.

rhaschke · 2016-11-06T16:09:56Z

urdf_parser_plugin/package.xml


  <buildtool_depend>catkin</buildtool_depend>
  <build_depend>liburdfdom-headers-dev</build_depend>
+  <build_depend>urdf</build_depend>


Argh! This defines now a circular dependency:

07:42:37 Error: Circular dependency in subset of packages: urdf, urdf_parser_plugin 07:42:37 Can not build workspace with circular dependency

You only need to have the urdf headers to be present. As it worked for you before (compatibility_header.h was found), it's probably safe to drop this dependency.

Just saw it as well, done :)

This reverts commit 9417509.

wjwwood

Other than a small comment, the changes lgtm. Thanks for working on it guys.

wjwwood · 2016-11-07T20:04:14Z

@scpeters Do you have any feedback before this gets merged?

* Use urdf::*SharedPtr instead of boost::shared_ptr urdfdom_headers uses C++ std::shared_ptr. As it exports it as a custom *SharedPtr type, we can use them to stay compatible. * Add compatibility urdf::VoidSharedPtr * Use urdfdom compatibility header * Build kinetic for kinetic branch * Add missing namespace and includes * Revert "Build kinetic for kinetic branch" This reverts commit f1c085c. * Add build dependency * Revert "Add build dependency" This reverts commit 9417509. * Export urdfdom version to header * Add dummy version in case it's not set by urdfdom_headers * Add missing header

scpeters closed this Aug 24, 2016

scpeters reopened this Aug 24, 2016

jspricke force-pushed the urdfdom_headers_fix branch from dab2a13 to 0d684c2 Compare September 14, 2016 13:33

Use urdf::*SharedPtr instead of boost::shared_ptr

b9e7729

urdfdom_headers uses C++ std::shared_ptr. As it exports it as a custom *SharedPtr type, we can use them to stay compatible.

jspricke force-pushed the urdfdom_headers_fix branch from 0d684c2 to b9e7729 Compare November 4, 2016 21:56

rhaschke suggested changes Nov 6, 2016

View reviewed changes

jspricke added 4 commits November 6, 2016 14:57

Add compatibility urdf::VoidSharedPtr

94fadf9

Use urdfdom compatibility header

9257897

Build kinetic for kinetic branch

f1c085c

Add missing namespace and includes

affe3e2

jspricke mentioned this pull request Nov 6, 2016

Add missing namespaces #165

Closed

rhaschke suggested changes Nov 6, 2016

View reviewed changes

jspricke added 2 commits November 6, 2016 16:33

Revert "Build kinetic for kinetic branch"

dade8d1

This reverts commit f1c085c.

Add build dependency

9417509

rhaschke reviewed Nov 6, 2016

View reviewed changes

jspricke added 4 commits November 6, 2016 17:11

Revert "Add build dependency"

17a36c1

This reverts commit 9417509.

Export urdfdom version to header

4b8ed4d

Add dummy version in case it's not set by urdfdom_headers

ff73358

Add missing header

60c0aac

wjwwood approved these changes Nov 7, 2016

View reviewed changes

wjwwood merged commit 409c4b9 into ros:kinetic-devel Jan 5, 2017

jspricke deleted the urdfdom_headers_fix branch January 5, 2017 15:47

v4hn mentioned this pull request Feb 1, 2017

Use urdf::*SharedPtr instead of boost::shared_ptr (#144) #182

Merged


		URDF_TYPEDEF_CLASS_POINTER(ModelInterface);

		typedef boost::shared_ptr<void> VoidSharedPtr;

Use urdf::*ShredPtr instead of boost::shared_ptr #144

Use urdf::*ShredPtr instead of boost::shared_ptr #144

Uh oh!

Conversation

jspricke commented Aug 23, 2016

Uh oh!

scpeters commented Aug 24, 2016

Uh oh!

scpeters commented Aug 24, 2016

Uh oh!

scpeters commented Aug 24, 2016

Uh oh!

jspricke commented Sep 14, 2016

Uh oh!

bmagyar commented Nov 1, 2016

Uh oh!

jspricke commented Nov 4, 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!

rhaschke left a comment

Choose a reason for hiding this comment

Uh oh!

rhaschke commented Nov 6, 2016

Uh oh!

jspricke commented Nov 6, 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!

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!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

wjwwood commented Nov 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects