gazebosim / gazebosim/gz-rendering

contentScalingFactor uses locale-dependent conversion to string

Open
#136 9 comments 2 reactions 0 assignees View on GitHub
bug good first issue help wanted
Dominant language
C++
Stars
81
Forks
90
Avg merge
1d 13h
Merged PRs (30d)
10

Description

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);`](https://github.com/OGRECave/ogre-next/blob/c9fa139caee32e39396dbceb799e0d35dc0d9b62/RenderSystems/GL3Plus/src/windowing/OSX/OgreOSXCocoaWindow.mm#L182)

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; }`](https://github.com/OGRECave/ogre-next/blob/13f9efd19a5d7b5945242d70f6e4a5c41a52ad45/OgreMain/src/OgreStringConverter.cpp#L258-L269)

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...

Contributor guide

Open the contributing guide

Research direction

Start with the contentScalingFactor handling at ogre/src/OgreRenderEngine.cc:651 and ogre2/src/Ogre2RenderEngine.cc:715, then inspect the relevant ign-common/StringUtils utilities. Reproduce the rendering path with a locale such as cs_CZ.UTF-8 and verify that the value passed to OGRE uses a locale-independent decimal representation.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
computer-graphics
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.