search

ros / urdf

Repository for URDF parsing code
63 stars 41 forks source link

Real transition to TinyXML2? #24

Closed rhaschke closed 3 years ago

rhaschke commented 5 years ago

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: https://github.com/ros/urdfdom/pull/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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@sloretz Ping.

rhaschke commented 4 years ago

@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 commented 3 years ago

@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 https://github.com/ros2/urdf/pull/17 , and we'll need to go through a deprecation/removal tick-tok cycle for the latter.

rhaschke commented 3 years ago

Ok. So we should close this PR, right?

clalancette commented 3 years ago

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.