search

ros / urdf

Repository for URDF parsing code
63 stars 41 forks source link

Tinyxml2 support #9

Closed clalancette closed 6 years ago

clalancette commented 6 years ago

Based on this comment this PR is an alternative to #4. Instead of removing the TinyXML APIs completely, we deprecate them and add the TinyXML2 APIs alongside. That way we won't break everything that depends on us, while still moving forward. Sometime in the future we can actually remove the TinyXML APIs.

Note that I've targeted this at kinetic-devel instead of melodic-devel, since we can add this to kinetic without breaking anything, and thus not branch off. If we decide to go with this PR, then we can delete the melodic-devel branch completely.

In addition to this work, this PR also cleans up the code and adds some tests.

@rojkov, @sloretz FYI

rojkov commented 6 years ago

Looks good to me.

I guess this makes https://github.com/ros/urdf/pull/4 obsolete then.

mikaelarguedas commented 6 years ago

Adding new dependencies needs to be reflected in the package.xml (even if the tinyxml2 module is provided by cmake-modules it doesnt provide the dependency)

Building on top of the original contribution will allow the original contributor to be credited.

Testing deprecated features will generate deprecated warnings so the test compile arguments should be updated accordingly

clalancette commented 6 years ago

Adding new dependencies needs to be reflected in the package.xml (even if the tinyxml2 module is provided by cmake-modules it doesnt provide the dependency)

Oh, good call, I missed that bit. Will fix.

Building on top of the original contribution will allow the original contributor to be credited.

True, but I wanted to restructure the whole thing in a pretty significant way. I think what I'll do here is modify the last commit to make @rojkov the author, since most of the code is his.

Testing deprecated features will generate deprecated warnings so the test compile arguments should be updated accordingly

Sure, I'll add -Wno-deprecated to the list.

clalancette commented 6 years ago

Looks good to me.

I guess this makes #4 obsolete then.

Great. It sounds like you are good with this direction, so I'm going to go ahead and close out #4, and we can iterate with any changes here.

clalancette commented 6 years ago

@sloretz Gentle ping for a review here. I'd like to get this into Melodic, as this will be holding up doing a similar change in i.e. kdl_parser.

wjwwood commented 6 years ago

Now that the TinyXML interface is deprecated, can you guys mention this in the migration guide for Melodic and maybe point to an example diff for how to port it?

https://wiki.ros.org/melodic/Migration

clalancette commented 6 years ago

Now that the TinyXML interface is deprecated, can you guys mention this in the migration guide for Melodic and maybe point to an example diff for how to port it?

https://wiki.ros.org/melodic/Migration

Good idea, I'll do that now.

wjwwood commented 6 years ago

Thanks!