Real transition to TinyXML2?

[rhaschke]

I don't really understand #9, attempting to transition to TinyXML2.
Passing in a TinyXML document or element (version 1 or 2 doesn't matter) actually serializes into a string and calls initString(), which eventually parses the XML string again in parseURDF. However, urdfdom's transition to TinyXML2 is still pending: ros/urdfdom#99.

While the parsing from string is mandatory to allow for the Collada parser plugin, I don't really see why we should support the highly inefficient initXml() convenience methods at all in the future?
Why not simply discourage their usage, i.e. deprecate them, and eventually remove them in future?

[clalancette]

I did a pretty extensive review of the use of the urdf APIs when coming up with #9. From what I saw, the majority of downstream users use initString(), despite the inefficiencies. So while I agree that it would be nice to deprecate it and use the more efficient versions that take the tinyxml{2} pointers, it was hard to justify. We would just be littering string -> tinyxml2 code through all downstream consumers, rather than centralizing it here.

[rhaschke]

From what I saw, the majority of downstream users use initString(), despite the inefficiencies.
... it would be nice to use the more efficient versions that take the tinyxml{2} pointers ...

Looks like you missed my point: The API has to use initString() instead of initXml(), because the actual parser needs to handle both Collada and URDF. initXml() is the inefficient API version!
Hence, my proposal was to remove TinyXML support (and not initString()).

[clalancette]

Looks like you missed my point: The API has to use initString() instead of initXml(), because the actual parser needs to handle both Collada and URDF. initXml() is the inefficient API version!
Hence, my proposal was to remove TinyXML support (and not initString()).

I see, I read it backwards. Sorry about that. Yeah, I could definitely get behind that proposal; deprecating the current initXml APIs (including the new TinyXML2 ones), and then eventually transitioning to only the initString ones. @sloretz , thoughts?

[rhaschke]

@sloretz Ping.

[rhaschke]

@sloretz, @clalancette: Unfortunately, we missed to cleanup the API of urdf and urdfdom and ultimately switch to TinyXML2 in Noetic. What are your plans regarding this?

[clalancette]

@sloretz, @clalancette: Unfortunately, we missed to cleanup the API of urdf and urdfdom and ultimately switch to TinyXML2 in Noetic. What are your plans regarding this?

In short: remove tinyxml from https://github.com/ros2/urdf and https://github.com/ros2/urdfdom. It was already done for the former in ros2#17 , and we'll need to go through a deprecation/removal tick-tok cycle for the latter.

[rhaschke]

Ok. So we should close this PR, right?

[clalancette]

Ok. So we should close this PR, right?

Yeah, that would be my preference. Since we don't have another ROS 1 vehicle to deliver this, I don't see much of a way forward here. I'm going to close this out, thanks for working on it.