search

ros / robot_model

Contains packages for modeling various aspects of robot information, specified in the Xml Robot Description Format (URDF). The core package of this stack is urdf, which parses URDF files, and constructs an object model (C++) of the robot.
http://ros.org/wiki/robot_model
35 stars 104 forks source link

Change urdf::Model to use std::shared_ptrs in urdfdom > v0.4 #206

Closed davetcoleman closed 7 years ago

davetcoleman commented 7 years ago

In the latest ROS Lunar version of URDFDom all shared_ptr typedefs were converted to std:: from boost:: except one was forgotten - urdf::Model. This class inherits from urdf::Modelnterface which is using a std::shared_ptr, so it seems to me this should be fixed despite Lunar already being released. In this PR I propose we maintain the boost shared ptr in Kinetic but allow for an std shared ptr in Lunar.

This change is not a big deal because the typedefs being changed were only added in Sep 2016 by @bmagyar - way after Kinetic was released - so there should be almost no existing code affected by this.

This is important for MoveIt because currently this bug is blocking our full Lunar release due to this line where a urdf::ModelSharedPtr is downcast into a urdf::ModelInterfaceSharedPtr. This won't work since one is boost and one is std.

I've tested this change with MoveIt in Kinetic and Lunar and it fixes our issue.

sloretz commented 7 years ago

I think you're right that urdf::ModelSharedPtr should have been part of the compatibility header prior to the Lunar release.

As an alternative, What do you think about merging this into a branch for ROS M, and adding a work around to MoveIt? https://stackoverflow.com/questions/6326757/conversion-from-boostshared-ptr-to-stdshared-ptr

bmagyar commented 7 years ago

I strongly agree that this should make it into Lunar or even Kinetic.

:+1:

sloretz commented 7 years ago

All tests passed with a lunar pre-release test of depth 2.

mikaelarguedas commented 7 years ago

If all tests passed, should we move forward and merge this ?

davetcoleman commented 7 years ago

Into kinetic and lunar, yes please

davetcoleman commented 7 years ago

Can this be released into lunar asap so there is soak time before the next buildfarm sync? Thanks!

mikaelarguedas commented 7 years ago

:+1: the earlier the better for testing / soak time. I think @sloretz faced some issues running bloom due to the packages that disappeared from this repo recently. I think that @sloretz or @clalancette will give us an update here once it's resolved

sloretz commented 7 years ago

@clalancette, would you mind doing the release? I'm on the road all day today.

clalancette commented 7 years ago

@sloretz Sure, will do. If you have a minute, what kind of problems did you run into running bloom before?

mikaelarguedas commented 7 years ago

@clalancette please see https://github.com/ros-infrastructure/bloom/issues/425 for the description of the problem and the suggested solutions

clalancette commented 7 years ago

Just FYI, not only did we have trouble with bloom, this is also not building on all supported distros (in particular, we know right now that it is not going to work on at least yakkety and stretch). Unfortunately, I don't have time to debug it this week, and I won't be around next week. @sloretz I think you'll have to do the release here.

davetcoleman commented 7 years ago

thanks all!

sloretz commented 7 years ago

@davetcoleman FYI this has been released into kinetic and lunar. I think both will get a sync this week.