Skip to content
This repository was archived by the owner on Aug 3, 2020. It is now read-only.

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

Merged
sloretz merged 1 commit intoros:kinetic-develfrom
davetcoleman:kinetic-model-shared-ptr
Jun 24, 2017
Merged

Change urdf::Model to use std::shared_ptrs in urdfdom > v0.4#206
sloretz merged 1 commit intoros:kinetic-develfrom
davetcoleman:kinetic-model-shared-ptr

Conversation

@davetcoleman
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor

sloretz commented Jun 23, 2017

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
Copy link
Copy Markdown
Contributor

bmagyar commented Jun 23, 2017

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

👍

@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented Jun 23, 2017

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

@mikaelarguedas
Copy link
Copy Markdown
Member

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

@davetcoleman
Copy link
Copy Markdown
Contributor Author

Into kinetic and lunar, yes please

@sloretz sloretz merged commit 399b75d into ros:kinetic-devel Jun 24, 2017
@davetcoleman davetcoleman deleted the kinetic-model-shared-ptr branch June 26, 2017 06:10
@davetcoleman
Copy link
Copy Markdown
Contributor Author

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

@mikaelarguedas
Copy link
Copy Markdown
Member

👍 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
Copy link
Copy Markdown
Contributor

sloretz commented Jun 26, 2017

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

@clalancette
Copy link
Copy Markdown
Contributor

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

@mikaelarguedas
Copy link
Copy Markdown
Member

@clalancette please see ros-infrastructure/bloom#425 for the description of the problem and the suggested solutions

@clalancette
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor Author

thanks all!

@sloretz sloretz mentioned this pull request Jun 27, 2017
@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented Jun 27, 2017

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

v4hn added a commit to v4hn/robot_model that referenced this pull request Jul 18, 2017
In the kinetic-devel branch ros#153 introduced typedefs
for the `Model` class of this package.
Later, ros#206 moved these typedefs over to the
`urdfdom_compatibility.h` header.

The main purpose of the header is to ease compatibility between indigo
and kinetic+, so these typedefs should also be provided in indigo-devel.
v4hn added a commit to v4hn/robot_model that referenced this pull request Jul 18, 2017
In the kinetic-devel branch ros#153 introduced typedefs
for the `Model` class of this package.
Later, ros#206 moved these typedefs over to the
`urdfdom_compatibility.h` header.

The main purpose of the header is to ease compatibility between indigo
and kinetic+, so these typedefs should also be provided in indigo-devel.
clalancette pushed a commit that referenced this pull request Jul 27, 2017
In the kinetic-devel branch #153 introduced typedefs
for the `Model` class of this package.
Later, #206 moved these typedefs over to the
`urdfdom_compatibility.h` header.

The main purpose of the header is to ease compatibility between indigo
and kinetic+, so these typedefs should also be provided in indigo-devel.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants