Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[proposal] RDFLoader / RobotModelLoader: remove parsing of TinyXML documents #1254

Merged
merged 3 commits into from Dec 24, 2018

Conversation

Projects
None yet
3 participants
@rhaschke
Copy link
Contributor

rhaschke commented Dec 10, 2018

urdf::Model::initXml() is highly inefficient, as it first serializes the string and eventually
parses it again (see ros/urdf#24 (comment)). We should discourage and eventually remove the TinyXML API and solely rely on parsing from string.

For now, this is just a proof-of-concept that the MoveIt source repo doesn't need the TinyXML support.
A user-friendly approach would deprecate the TinyXML API first, but, maybe, that's not really necessary?

Opinions?

RDFLoader/RobotModelLoader: remove parsing from TinyXML documents
urdf::Model::initXml() is highly inefficient, as it first serializes the string and eventually
parses it again. We should discourage and eventually remove this API.

@rhaschke rhaschke force-pushed the ubi-agni:cleanup-rdf-loader branch from c294d9e to 1d4cb28 Dec 11, 2018

@rhaschke rhaschke changed the title [proposal] RDFLoader / RobotModelLoader: remove parsing from TinyXML documents [proposal] RDFLoader / RobotModelLoader: remove parsing of TinyXML documents Dec 11, 2018

@jonbinney

This comment has been minimized.

Copy link
Contributor

jonbinney commented Dec 16, 2018

+1! I hadn't realized initXml() was doing that deserialize-reserialize step....

@v4hn

This comment has been minimized.

Copy link
Member

v4hn commented Dec 16, 2018

I don't have a strong opinion here, but if we want to remove it in melodic, please add a line to the MIGRATION.md

@rhaschke rhaschke requested a review from davetcoleman Dec 17, 2018

@v4hn

This comment has been minimized.

Copy link
Member

v4hn commented Dec 18, 2018

This code triggers a deprecation warning in the build farm now, because urdf::Model deprecated TinyXML.

So I guess this request is more about whether we want to transition to tinyxml2 or only remove the old API.

Given @rhaschke's argumentation, I agree to only remove the old API in melodic-devel.

@v4hn

v4hn approved these changes Dec 18, 2018

@rhaschke

This comment has been minimized.

Copy link
Contributor Author

rhaschke commented Dec 19, 2018

Added the requested migration note. @v4hn, do I understand correctly, that you accept the removal without a deprecation phase?

@v4hn

This comment has been minimized.

Copy link
Member

v4hn commented Dec 21, 2018

do I understand correctly, that you accept the removal without a deprecation phase

Yes. I believe it is unnecessary overhead to drag this along as deprecated.
If you agree, please merge.

@rhaschke rhaschke merged commit 3a0b701 into ros-planning:melodic-devel Dec 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rhaschke rhaschke deleted the ubi-agni:cleanup-rdf-loader branch Dec 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.