Skip to content

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

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

Merged
merged 3 commits into from
Dec 24, 2018

Conversation

rhaschke
Copy link
Contributor

@rhaschke 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?

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 changed the title [proposal] RDFLoader / RobotModelLoader: remove parsing from TinyXML documents [proposal] RDFLoader / RobotModelLoader: remove parsing of TinyXML documents Dec 11, 2018
@jonbinney
Copy link
Contributor

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

@v4hn
Copy link
Contributor

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

@v4hn
Copy link
Contributor

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.

@rhaschke
Copy link
Contributor Author

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

@v4hn
Copy link
Contributor

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 moveit:melodic-devel Dec 24, 2018
@rhaschke rhaschke deleted the cleanup-rdf-loader branch December 24, 2018 08:15
sjahr pushed a commit to sjahr/moveit that referenced this pull request Jun 21, 2024
MoveIt Setup Assistant - Merge the Feature branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants