moveit / srdfdom

Semantic Robot Description Format
BSD 3-Clause "New" or "Revised" License
13 stars 68 forks source link

[Indigo] cleanup urdfdom compatibility (cherry-picking #27) #30

Closed 130s closed 7 years ago

130s commented 7 years ago

Just noticed that https://github.com/ros/robot_model/pull/160, which https://github.com/ros-planning/srdfdom/pull/27 relies on, is backported to Indigo. So I thought https://github.com/ros-planning/srdfdom/pull/27 can also be ported back to Indigo. Please review @v4hn @rhaschke

v4hn commented 7 years ago

You're right, we should be able to backport this. +1 The change has been picked and was released 27 days ago, but testing failed. Any idea why?

130s commented 7 years ago

Looks like typedef added in https://github.com/ros-planning/srdfdom/pull/18 is still necessary for IJ. Now tests passed.

v4hn commented 7 years ago

ok, but if that is the case (right now I don't see why) then the remaining part of this pull-request reduces to adding a dependency on the "urdf" package. We don't need that dependency though! The idea was to add it and rely on that package to provide the typedefs.

130s commented 7 years ago

Makes sense, then I kind of found a cause of the issue but am not sure how it should be tackled.

LinkConstSharedPtr is called here but without typedefing, it's not available.

In robot_model, LinkConstSharedPtr was added in Kinetic, but not in Indigo.

v4hn commented 7 years ago

Hm, yes. The pull-request you found is not merged into indigo. I think it should be - to ease the transition indigo->kinetic. (edit: I just added a pull-request to do this here) I don't see why this is important to back-port #27 into indigo though.

@130s could you please change this pull-request back to a cherry-pick of #27? Without the parts you removed it does not make sense to discuss it and currently there is not even an error message to look at.

@jspricke It would be great if you could have a look.

130s commented 7 years ago

change this pull-request back to a cherry-pick of #27?

Done.