contentScalingFactor uses locale-dependent conversion to string

peci1 commented 4 years ago

https://github.com/ignitionrobotics/ign-rendering/blob/e22fd80e25aa89454a2be1d69891c83c7900f4c0/ogre/src/OgreRenderEngine.cc#L651

https://github.com/ignitionrobotics/ign-rendering/blob/e22fd80e25aa89454a2be1d69891c83c7900f4c0/ogre2/src/Ogre2RenderEngine.cc#L715

std::to_string respects locale, so if you run ign rendering on a system with e.g. LC_NUMERIC=cs_CZ.UTF-8, it fails passing correct parameters to OGRE. See the decimal comma (not dot) in contentScalingFactor:

17:01:29: GL3PlusRenderSystem::_createRenderWindow "OgreWindow(0)_0", 1x1 windowed  miscParams: FSAA=0 border=none contentScalingFactor=1,000000 currentGLContext=true externalGLControl=true gamma=true stereoMode=Frame Sequential

OGRE parses the value with

mContentScalingFactor = StringConverter::parseReal(opt->second);

which is

Real StringConverter::parseReal(const String& val, Real defaultValue) { StringStream str(val); if (msUseLocale) str.imbue(msLocale); Real ret = defaultValue; if( !(str >> ret) ) return defaultValue; return ret; }

The behavior of OGRE thus depends on whether StringUtils::setUseLocale(true) has been called or not, and I haven't found a reference to this function neither in OGRE nor in ign-rendering. Thus I assume in this use-case, OGRE parses the value in C locale.

The ign-rendering code should either use https://en.cppreference.com/w/cpp/utility/to_chars (if C++17 is available), or boost::lexical_cast. Or set the locale before the call, but that might be unwanted in other parts...

It'd be best to add a utility function for float->string and string->float conversions to ign-common/StringUtils, so that people can easily get the correct behavior...

iche033 commented 4 years ago

C++17 is available so we can use to_chars: https://github.com/ignitionrobotics/ign-rendering/blob/master/src/CMakeLists.txt#L15

Note that there're likely other parts in the code that are affected by locale issue, e.g. #101

peci1 commented 4 years ago

Yep, I just wanted to point out this one ;)

traversaro commented 4 years ago

C++17 is available so we can use to_chars: https://github.com/ignitionrobotics/ign-rendering/blob/master/src/CMakeLists.txt#L15

Note that to_chars with double arguments was merged only recently in GCC (https://gcc.gnu.org/pipermail/gcc-patches/2020-July/550029.html) and still needs to be merged in clang ( https://reviews.llvm.org/D70631 ). Interestingly, VS2019 is the only compiler that in a released version has a fully compliant to_chars.

traversaro commented 4 years ago

Other points of the code that use locale-dependent conversions: https://github.com/search?q=org%3Aignitionrobotics+atof&type=code .

chapulina commented 4 years ago

More related issues:

set the locale before the call, but that might be unwanted in other parts...

Note that SDFormat sets LC_NUMERIC to C when parsing params and it looks like it never resets it: https://github.com/osrf/sdformat/blob/master/src/Param.cc#L368

add a utility function for float->string and string->float conversions to ign-common/StringUtils

It looks like to_chars isn't available as @traversaro said, so this seems to be our only option.

Other points of the code that use locale-dependent conversions

Nice catch

j-rivero commented 3 years ago

Other points of the code that use locale-dependent conversions: https://github.com/search?q=org%3Aignitionrobotics+atof&type=code .

Trying to complete the list (split due to limitations in github search) based on https://www.gnu.org/software/libc/manual/html_node/Parsing-of-Numbers.html:

Until we get the to_chars support in all our supported compilers, one option could be to implement helper functions in ignition-common to deal with scenarios where we want the conversions to be locale independent.

clalancette commented 3 years ago

Just in case it helps: we went through this a couple of years ago in the urdf world. It is actually tricky to get right; what we settled on was this PR: https://github.com/ros/urdfdom_headers/pull/42 . That seems to have solved the issue, at least in the places that use it, so you may want to copy that code.

traversaro commented 3 years ago

If anyone is curious, for fun I usually drop a link or a cross-link in https://github.com/robotology/idyntree/issues/288 whenever I found a locale-related bug, the collection has been constantly growing over the years.

j-rivero commented 3 years ago

I'm centralizing the information and issues related to locale independent conversions in https://github.com/ignitionrobotics/ign-common/issues/233. PRs are very welcome :)

gazebosim / gz-rendering

contentScalingFactor uses locale-dependent conversion to string #136