godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
88.89k stars 20.15k forks source link

System conversions of float to string should be locale independent #48148

Open Sipaha opened 3 years ago

Sipaha commented 3 years ago

Godot version: 3.2.3

OS/device including version:

Ubuntu 20.04.2 LTS

Issue description:

If we set locale with comma as decimal delimiter we got strange behavior in editor:

image image

Problem was found in build with godot-kotlin module: https://github.com/utopia-rise/godot-kotlin-jvm/issues/166 Local fix in module already exists, but I think this is a problem of engine. Bugs like this may appear in other modules.

I don't know exactly how locale is changed, because it occur while JVM initialization, but standard function std::to_string change its behavior:

image Output: Godot-JVM: 1.100000 Godot-JVM: 1,100000

Calinou commented 3 years ago

@Sipaha Is this reproducible on a vanilla Godot build without any custom modules or GDNative add-ons? I've never seen anything of the sort before, including on systems that run in French (which uses commas as decimal separator).

akien-mga commented 3 years ago

I don't understand why this is being reported here, it's very clear from https://github.com/utopia-rise/godot-kotlin-jvm/issues/166 that it's a bug with handling of locale decimal separator in godot-kotlin-jvm / JVM, not in Godot.

CedNaru commented 3 years ago

Without a module or GDNative, I don't think it's possible to reproduce it. The change in the std::locale was a side effect of creating the JVM with JNI, it shouldn't happen in the vanilla code of Godot.

The bug was only happening on Unix systems. I couldn't reproduce it with Windows, but on Linux, it happened when the regional format for numbers was set to a language using "," instead of "." for floating numbers. That setting is independent of the OS language itself.

That issue is mostly because of our Kotlin module but it's a side effect that propagates in the rest of Godot and breaks a lot of stuff in the editor because of some hidden std setting.

It's fixed now on our side, but I guess it's worth notifying the main repo how such a minor change can have an effect on the rest of the engine. We weren't even aware that JNI could change the locale of the std so it was hard to find the source of the bug. In our code, we never do any operation related to locales, it was a hidden behavior.

akien-mga commented 3 years ago

Godot doesn't use std::locale. I'm not familiar with it but it seems to change the C++ locale and if the code that uses it changes it to something which will break the handling of floats, that's outside our control. https://en.cppreference.com/w/cpp/locale/locale

This was a very common issue plaguing Unity games a few years back due to the same issue in Mono, though eventually they fixed it. JVM might have the same bogus handling of locales on Linux that breaks serialization.

CedNaru commented 3 years ago

Are you sure there is no use of that at all ? Because our fix to that issue was basically to make a copy of the current locale before the JVM modify it. After the JVM change, we set back the locale to the previous. Those 2 new lines fix the issue:

image

Also, I noticed that here: https://github.com/godotengine/godot/blob/dc98144b99f35cc6446e297e2c6082d8c441d540/core/ustring.cpp#L1053

You are using a preprocessor related to the STD to modify the behavior of the String::num method. So it means one of the preprocessor branches somehow uses the STD.

Unless I misunderstand what is going on, it means that at least some part of Godot depends on the std::locale

akien-mga commented 3 years ago

Right, I meant we don't set std::locale ourselves, but if it's set by a thirdparty it can apparently still mess up with C++ globals and our assumptions. The code you identified probably needs to guard against such unexpected changes to the C++ locale configuration done via std::locale.