Use stringstream instead of stod to work around locale issues. #42

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 [email protected]

Signed-off-by: Chris Lalancette <[email protected]>

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 robotology/idyntree#288 (comment)).

Signed-off-by: Chris Lalancette <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

… (#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 <[email protected]>

… (#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 <[email protected]>

Use stringstream instead of stod to work around locale issues.

739624e

Signed-off-by: Chris Lalancette <[email protected]>

Use imbue to always use the "classic" locale.

678ab93

Signed-off-by: Chris Lalancette <[email protected]>

Make a strToDouble facility in the utils.h header.

c5f6b9f

This will be useful elsewhere (i.e. urdfdom). Signed-off-by: Chris Lalancette <[email protected]>

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 <[email protected]>

Add in an actual error message to the strToDouble throw.

567ba15

Signed-off-by: Chris Lalancette <[email protected]>

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 frankaemika/franka_ros2#20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use stringstream instead of stod to work around locale issues. #42

Use stringstream instead of stod to work around locale issues. #42

clalancette commented Nov 20, 2017

traversaro commented Nov 20, 2017

clalancette commented Nov 20, 2017

gavanderhoorn commented Nov 22, 2017

clalancette commented Nov 22, 2017

sloretz left a comment

sloretz Dec 18, 2017

sloretz Dec 18, 2017

sloretz Dec 18, 2017

clalancette commented Feb 2, 2018

traversaro commented Feb 2, 2018

clalancette commented Feb 2, 2018

clalancette commented Feb 21, 2018

scpeters Feb 21, 2018

clalancette Feb 21, 2018

clalancette Feb 21, 2018

Use stringstream instead of stod to work around locale issues. #42

Use stringstream instead of stod to work around locale issues. #42

Conversation

clalancette commented Nov 20, 2017

traversaro commented Nov 20, 2017

clalancette commented Nov 20, 2017

gavanderhoorn commented Nov 22, 2017

clalancette commented Nov 22, 2017

sloretz left a comment

Choose a reason for hiding this comment

sloretz Dec 18, 2017

Choose a reason for hiding this comment

sloretz Dec 18, 2017

Choose a reason for hiding this comment

sloretz Dec 18, 2017

Choose a reason for hiding this comment

clalancette commented Feb 2, 2018

traversaro commented Feb 2, 2018

clalancette commented Feb 2, 2018

clalancette commented Feb 21, 2018

scpeters Feb 21, 2018

Choose a reason for hiding this comment

clalancette Feb 21, 2018

Choose a reason for hiding this comment

clalancette Feb 21, 2018

Choose a reason for hiding this comment