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

Fix assumption on global locale in URDF parser #15

Merged
merged 2 commits into from
Aug 30, 2019

Conversation

traversaro
Copy link
Contributor

Before this fix, the URDF parser was silently failing to parse the decimal part of any real number parsed from XML if the C++ global locale was set to have a decimal separator different from the point.

Related issues in other projects :

gergondet added a commit to traversaro/mc_rbdyn_urdf that referenced this pull request Aug 27, 2019
@gergondet
Copy link
Member

Thanks for bringing up the issue. This can be confirmed by explicitly setting the locale to one that has comma decimal separator in the test.

However, the attrToList function is only used to convert double lists (such as in the xyz and rpy attributes). For single attribute double value (e.g. value in a mass tag) we use tinyxml2::XMLElement::DoubleAttribute which is unfortunately also locale dependent. I've pushed a patch to your branch.

Note: AppVeyor failure is not related to the PR, I'll work on a patch to fix this then I will merge this

@traversaro
Copy link
Contributor Author

However, the attrToList function is only used to convert double lists (such as in the xyz and rpy attributes). For single attribute double value (e.g. value in a mass tag) we use tinyxml2::XMLElement::DoubleAttribute which is unfortunately also locale dependent. I've pushed a patch to your branch.

Sorry, I missed those. Just for reference there is a tinyxml2 issue on this: leethomason/tinyxml2#160 .

@gergondet
Copy link
Member

Build should be fixed, I will merge this after the CI succeeds. Thanks.

@gergondet gergondet merged commit 6e01aa7 into jrl-umi3218:master Aug 30, 2019
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.

2 participants