contentScalingFactor uses locale-dependent conversion to string

[peci1]

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]

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]

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

[traversaro]

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]

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

[chapulina]

More related issues:

• catkin installation does not show correct pictures in GUI osrf/subt#438
• Models are broken when inserting from Fuel gz-sim#350

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]

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:

• https://github.com/search?q=org%3Aignitionrobotics+in%3Afile+strtol+OR+wcstol+OR+strtoul+OR+wcstoul+OR+strtoll+OR+wcstoll&type=Code
• https://github.com/search?q=org%3Aignitionrobotics+in%3Afile+strtoq+OR+wcstoq+OR+strtoull+OR+wcstoull+OR+strtouq+OR+wcstouq&type=Code
• https://github.com/search?q=org%3Aignitionrobotics+in%3Afile+strtoimax+OR+wcstoimax+OR+strtoumax+OR+wcstoumax+OR+atol+OR+atoi&type=Code
• https://github.com/search?q=org%3Aignitionrobotics+in%3Afile+strtod+OR+strtof+OR+strtold+OR+strtofN+OR+strtofNx+OR+wcstod&type=Code
• https://github.com/search?q=org%3Aignitionrobotics+in%3Afile+wcstof+OR+wcstold+OR+wcstofN+OR+wcstofNx+OR+atof+OR+atoll&type=Code

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]

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: ros/urdfdom_headers#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]

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

[j-rivero]

I'm centralizing the information and issues related to locale independent conversions in gazebosim/gz-common#233. PRs are very welcome :)