Use stringstream instead of stod to work around locale issues. by clalancette · Pull Request #42 · ros/urdfdom_headers

clalancette · 2017-11-20T15:59:20Z

The URDF schema (https://github.com/ros/urdfdom/blob/master/xsd/urdf.xsd#L47) defines doubles in URDF as xs:double. Looking at the XSD document at https://www.w3.org/TR/xmlschema-2/#double, it says that the mantissa for a double is represented as a "decimal" number. And looking at the XSD document at https://www.w3.org/TR/xmlschema-2/#decimal, a decimal number is a "finite-length sequence of decimal digits (#x30-#x39) separated by a period as a decimal indicator". Thus, the locale should have no effect on how the document is parsed, and using std::stod is incorrect. This patch switches the Vector3 class to using the stream operator, which seems to ignore locale.

fixes #41

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

traversaro · 2017-11-20T17:13:32Z

This patch switches the Vector3 class to using the stream operator, which seems to ignore locale.

If I understood correctly the C++ documentation, the stringstream default locale is not affected by calls to the C-style std::setlocale function (the one called by Qt, that is actually exposing the std::stod problem). However, it is affected by calls to std::locale::global, so a call to this global function in any part of the thread or the process (this is implementation defined) could still cause a failure in the URDF parser. Adding a call to the imbue method:

ss.imbue(std::locale::classic());

should avoid this problem (see gbionics/idyntree#288 (comment)).

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2017-11-20T19:35:17Z

Adding a call to the imbue method:

Thanks for that. Done in 678ab93

This will be useful elsewhere (i.e. urdfdom). Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

gavanderhoorn · 2017-11-22T09:52:13Z

@clalancette: minor, but none of the three commits seem to have been made using your gh registered email, so they are not attributed to your account.

clalancette · 2017-11-22T13:44:37Z

@clalancette: minor, but none of the three commits seem to have been made using your gh registered email, so they are not attributed to your account.

Ah, thanks. When we got the new "@openrobotics.org" domain, I updated my .gitconfig, but I had forgotten to verify with GitHub. Done now.

sloretz

Looks good with a few includes. Think it's worth adding a test to this package?

sloretz · 2017-12-18T17:18:08Z

urdf_model/include/urdf_model/utils.h

+// thrown.
+static inline double strToDouble(const char *in)
+{
+  std::stringstream ss;


#include <sstream>?

sloretz · 2017-12-18T17:18:29Z

urdf_model/include/urdf_model/utils.h

+static inline double strToDouble(const char *in)
+{
+  std::stringstream ss;
+  ss.imbue(std::locale::classic());


#include <locale>?

sloretz · 2017-12-18T17:18:49Z

urdf_model/include/urdf_model/utils.h

+  ss >> out;
+
+  if (ss.fail() || !ss.eof()) {
+    throw std::runtime_error("");


#include <stdexcept> ?

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2018-02-02T16:10:13Z

I've fixed up all of the includes in caf2e8c. As far as testing goes, the only thing is that there is no test infrastructure in here. I could see about adding it (gtest or similar), but then this will grow into a much larger PR. I'm inclined to kick that to another PR, but I'll let you make the final decision.

traversaro · 2018-02-02T16:14:05Z

As far as testing goes, the only thing is that there is no test infrastructure in here. I could see about adding it (gtest or similar), but then this will grow into a much larger PR. I'm inclined to kick that to another PR, but I'll let you make the final decision.

I am totally not a fan of the situation, but historically tests of urdfdom_headers were added in the urdfdom repo, see ros/urdfdom#67 (comment) .

clalancette · 2018-02-02T21:30:44Z

@traversaro Thanks a lot for the pointer. I'll go ahead and add some tests over there.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2018-02-21T15:21:27Z

@scpeters Gentle ping; this PR (and the related ros/urdfdom#109) are ready for review and merging, and I'd like to get them in before Melodic. Thanks!

scpeters · 2018-02-21T17:24:34Z

urdf_model/include/urdf_model/utils.h

+// for parsing, as that matches up with what the XSD for double specifies.
+// On success, the double is returned; on failure, a std::runtime_error is
+// thrown.
+static inline double strToDouble(const char *in)


Why use const char * instead of const std::string &? Just curious, not asking you to change it, just hoping to learn something.

There is a good (but sneaky) reason for it. I'm hoping that once we get this basic support in, we can use this more generally throughout the urdfdom libraries to avoid this problem. As one example, https://github.com/ros/urdfdom/blob/master/urdf_parser/src/link.cpp#L120 probably suffers from a similar problem (as do several other std::stod conversions in there). Several of those get a const char * from tinyxml, so I figured this interface was more generic rather than having to wrap those inside strings.

Oh, right, I forgot I also did another PR to actually fix all of that as well: ros/urdfdom#105

I believe the bug with respect to locale was fixed a while back ros/urdfdom_headers#42 so setting LC_NUMERIC is no longer necessary

I believe the bug with respect to locale was fixed a while back ros/urdfdom_headers#42 so setting LC_NUMERIC is no longer necessary (cherry picked from commit 611f948)

… (#3072) I believe the bug with respect to locale was fixed a while back ros/urdfdom_headers#42 so setting LC_NUMERIC is no longer necessary (cherry picked from commit 611f948) Co-authored-by: Matthew Elwin <10161574+m-elwin@users.noreply.github.com>

… (#3073) I believe the bug with respect to locale was fixed a while back ros/urdfdom_headers#42 so setting LC_NUMERIC is no longer necessary (cherry picked from commit 611f948) Co-authored-by: Matthew Elwin <10161574+m-elwin@users.noreply.github.com>

… (#3074) I believe the bug with respect to locale was fixed a while back ros/urdfdom_headers#42 so setting LC_NUMERIC is no longer necessary (cherry picked from commit 611f948) Co-authored-by: Matthew Elwin <10161574+m-elwin@users.noreply.github.com>

Use stringstream instead of stod to work around locale issues.

739624e

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Use imbue to always use the "classic" locale.

678ab93

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Make a strToDouble facility in the utils.h header.

c5f6b9f

This will be useful elsewhere (i.e. urdfdom). Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette mentioned this pull request Nov 20, 2017

Switch all stod callers in urdfdom to use a helper ros/urdfdom#105

Merged

sloretz reviewed Dec 18, 2017

View reviewed changes

Add some additional includes.

caf2e8c

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Add in an actual error message to the strToDouble throw.

567ba15

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette mentioned this pull request Feb 2, 2018

Add in some tests for the strToDouble conversion. ros/urdfdom#109

Merged

scpeters reviewed Feb 21, 2018

View reviewed changes

scpeters approved these changes Feb 21, 2018

View reviewed changes

clalancette merged commit e7e0972 into master Feb 21, 2018

clalancette deleted the stream-vector branch February 21, 2018 20:16

This was referenced Jul 9, 2018

rviz RobotModel broken in melodic: <visual><origin> not used ros-visualization/rviz#1249

Closed

Release for bionic doesn't include #42 #45

Open

Pro mentioned this pull request Aug 7, 2018

[melodic] UR5 Moveit model not showing correctly ros-industrial/universal_robot#374

Closed

clalancette mentioned this pull request Oct 23, 2018

Melodic: Decimal values are not correctly parsed ros/urdf#22

Closed

traversaro mentioned this pull request Apr 5, 2020

Conversion from double to string is locale-dependent if TOML_FLOATING_POINT_CHARCONV is 0 marzer/tomlplusplus#19

Closed

clalancette mentioned this pull request Nov 19, 2020

contentScalingFactor uses locale-dependent conversion to string gazebosim/gz-rendering#136

Open

j-rivero mentioned this pull request Jul 26, 2021

Helper functions for conversions that are locale independent gazebosim/gz-common#233

Open

m-elwin mentioned this pull request Oct 3, 2022

Update Building-a-Visual-Robot-Model-with-URDF-from-Scratch.rst ros2/ros2_documentation#3071

Merged

marcbone mentioned this pull request Aug 1, 2023

collision meshes not shown in RViz frankarobotics/franka_ros2#20

Closed

Conversation

clalancette commented Nov 20, 2017

Uh oh!

traversaro commented Nov 20, 2017

Uh oh!

clalancette commented Nov 20, 2017

Uh oh!

gavanderhoorn commented Nov 22, 2017

Uh oh!

clalancette commented Nov 22, 2017

Uh oh!

sloretz left a comment

Choose a reason for hiding this comment

Uh oh!

sloretz Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

sloretz Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

sloretz Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

clalancette commented Feb 2, 2018

Uh oh!

traversaro commented Feb 2, 2018

Uh oh!

clalancette commented Feb 2, 2018

Uh oh!

clalancette commented Feb 21, 2018

Uh oh!

scpeters Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

clalancette Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

clalancette Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants