Skip to content

Use Grisu2 algorithm in String::num_scientific to fix serializing #98750

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Repiteo merged 1 commit into from
May 22, 2025
Merged

Use Grisu2 algorithm in String::num_scientific to fix serializing #98750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions COPYRIGHT.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,12 @@ Comment: Graphite engine
Copyright: 2010, SIL International
License: Expat

Files: thirdparty/grisu2/grisu2.h
Comment: Grisu2 float serialization algorithm
Copyright: 2009, Florian Loitsch
2018-2023, The simdjson authors
License: Expat and Apache

Files: thirdparty/harfbuzz/*
Comment: HarfBuzz text shaping library
Copyright: 2010-2022, Google, Inc.
Expand Down
18 changes: 16 additions & 2 deletions core/doc_data.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,24 @@
#include "doc_data.h"

String DocData::get_default_value_string(const Variant &p_value) {
if (p_value.get_type() == Variant::ARRAY) {
const Variant::Type type = p_value.get_type();
if (type == Variant::ARRAY) {
return Variant(Array(p_value, 0, StringName(), Variant())).get_construct_string().replace_char('\n', ' ');
} else if (p_value.get_type() == Variant::DICTIONARY) {
} else if (type == Variant::DICTIONARY) {
return Variant(Dictionary(p_value, 0, StringName(), Variant(), 0, StringName(), Variant())).get_construct_string().replace_char('\n', ' ');
} else if (type == Variant::INT) {
return itos(p_value);
} else if (type == Variant::FLOAT) {
// Since some values are 32-bit internally, use 32-bit for all
// documentation values to avoid garbage digits at the end.
const String s = String::num_scientific((float)p_value);
Comment thread
aaronfranke marked this conversation as resolved.
Outdated
// Use float literals for floats in the documentation for clarity.
if (s != "inf" && s != "-inf" && s != "nan") {
if (!s.contains_char('.') && !s.contains_char('e')) {
return s + ".0";
}
}
return s;
} else {
return p_value.get_construct_string().replace_char('\n', ' ');
}
Expand Down
34 changes: 13 additions & 21 deletions core/string/ustring.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@
#include "core/variant/variant.h"
#include "core/version_generated.gen.h"

#include "thirdparty/grisu2/grisu2.h"

#ifdef _MSC_VER
#define _CRT_SECURE_NO_WARNINGS // to disable build-time warning which suggested to use strcpy_s instead strcpy
#endif
Expand Down Expand Up @@ -1656,28 +1658,18 @@ String String::num_scientific(double p_num) {
if (Math::is_nan(p_num) || Math::is_inf(p_num)) {
return num(p_num, 0);
}
char buffer[256];
char *last = grisu2::to_chars(buffer, p_num);
return String::ascii(Span(buffer, last - buffer));
}

char buf[256];

#if defined(__GNUC__) || defined(_MSC_VER)

#if defined(__MINGW32__) && defined(_TWO_DIGIT_EXPONENT) && !defined(_UCRT)
// MinGW requires _set_output_format() to conform to C99 output for printf
unsigned int old_exponent_format = _set_output_format(_TWO_DIGIT_EXPONENT);
#endif
snprintf(buf, 256, "%lg", p_num);

#if defined(__MINGW32__) && defined(_TWO_DIGIT_EXPONENT) && !defined(_UCRT)
_set_output_format(old_exponent_format);
#endif

#else
sprintf(buf, "%.16lg", p_num);
#endif

buf[255] = 0;

return buf;
String String::num_scientific(float p_num) {
Comment thread
Ivorforce marked this conversation as resolved.
Outdated
if (Math::is_nan(p_num) || Math::is_inf(p_num)) {
return num(p_num, 0);
}
char buffer[256];
char *last = grisu2::to_chars(buffer, p_num);
return String::ascii(Span(buffer, last - buffer));
}

String String::md5(const uint8_t *p_md5) {
Expand Down
1 change: 1 addition & 0 deletions core/string/ustring.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,7 @@ class [[nodiscard]] String {
String unquote() const;
static String num(double p_num, int p_decimals = -1);
static String num_scientific(double p_num);
static String num_scientific(float p_num);
static String num_real(double p_num, bool p_trailing = true);
static String num_real(float p_num, bool p_trailing = true);
static String num_int64(int64_t p_num, int base = 10, bool capitalize_hex = false);
Expand Down
12 changes: 11 additions & 1 deletion core/variant/variant_call.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1697,6 +1697,16 @@ StringName Variant::get_enum_for_enumeration(Variant::Type p_type, const StringN
register_builtin_method<Method_##m_type##_##m_method>(sarray(), m_default_args);
#endif // DEBUG_ENABLED

#ifdef DEBUG_ENABLED
#define bind_static_methodv(m_type, m_name, m_method, m_arg_names, m_default_args) \
STATIC_METHOD_CLASS(m_type, m_name, m_method); \
register_builtin_method<Method_##m_type##_##m_name>(m_arg_names, m_default_args);
#else
#define bind_static_methodv(m_type, m_name, m_method, m_arg_names, m_default_args) \
STATIC_METHOD_CLASS(m_type, m_name, m_method); \
register_builtin_method<Method_##m_type##_##m_name>(sarray(), m_default_args);
#endif

#ifdef DEBUG_ENABLED
#define bind_methodv(m_type, m_name, m_method, m_arg_names, m_default_args) \
METHOD_CLASS(m_type, m_name, m_method); \
Expand Down Expand Up @@ -1882,7 +1892,7 @@ static void _register_variant_builtin_methods_string() {
bind_string_method(to_multibyte_char_buffer, sarray("encoding"), varray(String()));
bind_string_method(hex_decode, sarray(), varray());

bind_static_method(String, num_scientific, sarray("number"), varray());
bind_static_methodv(String, num_scientific, static_cast<String (*)(double)>(&String::num_scientific), sarray("number"), varray());
bind_static_method(String, num, sarray("number", "decimals"), varray(-1));
bind_static_method(String, num_int64, sarray("number", "base", "capitalize_hex"), varray(10, false));
bind_static_method(String, num_uint64, sarray("number", "base", "capitalize_hex"), varray(10, false));
Expand Down
46 changes: 30 additions & 16 deletions core/variant/variant_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1934,22 +1934,30 @@ Error VariantParser::parse(Stream *p_stream, Variant &r_ret, String &r_err_str,
//////////////////////////////////////////////////////////////////////////////////
//////////////////////////////////////////////////////////////////////////////////

// These two functions serialize floats or doubles using num_scientific to ensure
// it can be read back in the same way (except collapsing -0 to 0, and NaN values).
static String rtos_fix(float p_value, bool p_compat) {
if (p_value == 0.0f) {
return "0"; // Avoid negative zero (-0) being written, which may annoy git, svn, etc. for changes when they don't exist.
} else if (p_compat) {
// Write old inf_neg for compatibility.
if (std::isinf(p_value) && p_value < 0.0f) {
return "inf_neg";
}
}
return String::num_scientific(p_value);
}

static String rtos_fix(double p_value, bool p_compat) {
if (p_value == 0.0) {
return "0"; //avoid negative zero (-0) being written, which may annoy git, svn, etc. for changes when they don't exist.
} else if (std::isnan(p_value)) {
return "nan";
} else if (std::isinf(p_value)) {
if (p_value > 0) {
return "inf";
} else if (p_compat) {
return "0"; // Avoid negative zero (-0) being written, which may annoy git, svn, etc. for changes when they don't exist.
} else if (p_compat) {
// Write old inf_neg for compatibility.
if (std::isinf(p_value) && p_value < 0.0) {
return "inf_neg";
} else {
return "-inf";
}
} else {
return rtoss(p_value);
}
return String::num_scientific(p_value);
}

Error VariantWriter::write(const Variant &p_variant, StoreStringFunc p_store_string_func, void *p_store_string_ud, EncodeResourceFunc p_encode_res_func, void *p_encode_res_ud, int p_recursion_count, bool p_compat) {
Expand All @@ -1964,11 +1972,17 @@ Error VariantWriter::write(const Variant &p_variant, StoreStringFunc p_store_str
p_store_string_func(p_store_string_ud, itos(p_variant.operator int64_t()));
} break;
case Variant::FLOAT: {
String s = rtos_fix(p_variant.operator double(), p_compat);
if (s != "inf" && s != "-inf" && s != "nan") {
if (!s.contains_char('.') && !s.contains_char('e') && !s.contains_char('E')) {
s += ".0";
}
const double value = p_variant.operator double();
String s;
// Hack to avoid garbage digits when the underlying float is 32-bit.
Copy link
Copy Markdown
Contributor

@cridenour cridenour Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I found that this "Hack" is also necessary on all the Vectors, AABBs, etc. when compiling with precision=double. That or the current grisu2 implementation isn't fully stable for double precision.

Copy link
Copy Markdown
Member Author

@aaronfranke aaronfranke Sep 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hack works by checking if it's equivalent to the closest 32-bit float value. If you are seeing places where this is needed for vectors with precision=double then this means the underlying value isn't double somewhere, it's not a bug in Grisu2 itself. We could look for those places to try and fix issues, but it's also possible that the underlying value you're encountering is intended to be a 32-bit float, in which case we'd indeed need to apply this hack there if we want to avoid unnecessary digits being written to scene files and verison control.

Copy link
Copy Markdown
Contributor

@cridenour cridenour Sep 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two places I see constant git changes are when saving a mesh to a file on import, the AABB is changing between computers, even without a re-import. I haven't been able to follow that code path but it's possible that is getting downcast at some point.

The second place is surprisingly on transforms on a saved scene. Many of the node transforms will sometimes change in the scene file despite not being moved. Usually just one digit of change.

Given we use double precision for runtime physics and rendering and our individual scenes and meshes don't need the full precision, I'll likely just always cast down to single precision (when it matches) in our variant writer.

Copy link
Copy Markdown
Member Author

@aaronfranke aaronfranke Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened this PR to apply this to all float serialization: #110616

if ((double)(float)value == value) {
s = rtos_fix((float)value, p_compat);
} else {
s = rtos_fix(value, p_compat);
}
// Append ".0" to floats to ensure they are float literals.
if (s != "inf" && s != "-inf" && s != "nan" && !s.contains_char('.') && !s.contains_char('e') && !s.contains_char('E')) {
s += ".0";
}
p_store_string_func(p_store_string_ud, s);
} break;
Expand Down
2 changes: 1 addition & 1 deletion doc/classes/Animation.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,7 +689,7 @@
<member name="loop_mode" type="int" setter="set_loop_mode" getter="get_loop_mode" enum="Animation.LoopMode" default="0">
Determines the behavior of both ends of the animation timeline during animation playback. This is used for correct interpolation of animation cycles, and for hinting the player that it must restart the animation.
</member>
<member name="step" type="float" setter="set_step" getter="get_step" default="0.0333333">
<member name="step" type="float" setter="set_step" getter="get_step" default="0.033333335">
The animation step value.
</member>
</members>
Expand Down
4 changes: 2 additions & 2 deletions doc/classes/CharacterBody2D.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@
If [code]false[/code] (by default), the body will move faster on downward slopes and slower on upward slopes.
If [code]true[/code], the body will always move at the same speed on the ground no matter the slope. Note that you need to use [member floor_snap_length] to stick along a downward slope at constant speed.
</member>
<member name="floor_max_angle" type="float" setter="set_floor_max_angle" getter="get_floor_max_angle" default="0.785398">
<member name="floor_max_angle" type="float" setter="set_floor_max_angle" getter="get_floor_max_angle" default="0.7853982">
Maximum angle (in radians) where a slope is still considered a floor (or a ceiling), rather than a wall, when calling [method move_and_slide]. The default value equals 45 degrees.
</member>
<member name="floor_snap_length" type="float" setter="set_floor_snap_length" getter="get_floor_snap_length" default="1.0">
Expand Down Expand Up @@ -195,7 +195,7 @@
<member name="velocity" type="Vector2" setter="set_velocity" getter="get_velocity" default="Vector2(0, 0)">
Current velocity vector in pixels per second, used and modified during calls to [method move_and_slide].
</member>
<member name="wall_min_slide_angle" type="float" setter="set_wall_min_slide_angle" getter="get_wall_min_slide_angle" default="0.261799">
<member name="wall_min_slide_angle" type="float" setter="set_wall_min_slide_angle" getter="get_wall_min_slide_angle" default="0.2617994">
Minimum angle (in radians) where the body is allowed to slide when it encounters a wall. The default value equals 15 degrees. This property only affects movement when [member motion_mode] is [constant MOTION_MODE_FLOATING].
</member>
</members>
Expand Down
4 changes: 2 additions & 2 deletions doc/classes/CharacterBody3D.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@
If [code]false[/code] (by default), the body will move faster on downward slopes and slower on upward slopes.
If [code]true[/code], the body will always move at the same speed on the ground no matter the slope. Note that you need to use [member floor_snap_length] to stick along a downward slope at constant speed.
</member>
<member name="floor_max_angle" type="float" setter="set_floor_max_angle" getter="get_floor_max_angle" default="0.785398">
<member name="floor_max_angle" type="float" setter="set_floor_max_angle" getter="get_floor_max_angle" default="0.7853982">
Maximum angle (in radians) where a slope is still considered a floor (or a ceiling), rather than a wall, when calling [method move_and_slide]. The default value equals 45 degrees.
</member>
<member name="floor_snap_length" type="float" setter="set_floor_snap_length" getter="get_floor_snap_length" default="0.1">
Expand Down Expand Up @@ -186,7 +186,7 @@
<member name="velocity" type="Vector3" setter="set_velocity" getter="get_velocity" default="Vector3(0, 0, 0)">
Current velocity vector (typically meters per second), used and modified during calls to [method move_and_slide].
</member>
<member name="wall_min_slide_angle" type="float" setter="set_wall_min_slide_angle" getter="get_wall_min_slide_angle" default="0.261799">
<member name="wall_min_slide_angle" type="float" setter="set_wall_min_slide_angle" getter="get_wall_min_slide_angle" default="0.2617994">
Minimum angle (in radians) where the body is allowed to slide when it encounters a wall. The default value equals 15 degrees. When [member motion_mode] is [constant MOTION_MODE_GROUNDED], it only affects movement if [member floor_block_on_wall] is [code]true[/code].
</member>
</members>
Expand Down
Loading