Skip to content
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.

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

Always add decimal when converting float to string #47502

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Mar 30, 2021

So that

var a: float = 1
print(a)

prints

1.0

Makes working with numbers less confusing. I remember someone mentioned it around godotengine/godot-proposals#1866

Closes godotengine/godot-proposals#7894

@KoBeWi KoBeWi added enhancement topic:core cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Mar 30, 2021
@KoBeWi KoBeWi added this to the 4.0 milestone Mar 30, 2021
@KoBeWi KoBeWi requested a review from a team as a code owner March 30, 2021 15:54
@Calinou
Copy link
Member

Calinou commented Mar 30, 2021

Does this change apply automatically to floats within Vectors/Colors/Transforms/Matrices as well?

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 30, 2021

Hmm, it doesn't. Changing it might be not as easy :I

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 30, 2021

Ok, so I extended this to these types (in addition to floats): Vector2, Rect2, Vector3.
The original aim was to differentiate integer types from floats. As we don't have AABBi or Basisi I left these types untouched.

@dalexeev
Copy link
Member

dalexeev commented Apr 2, 2021

Also need to solve this problem:

print(5.0)                    # 5
print(5.0000001234)           # 5 <--
print("%.16f" % 5.0)          # 5.0000000000000000
print("%.16f" % 5.0000001234) # 5.0000001234000004
$ python3 -q
>>> print(5.0000001234)
5.0000001234

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 2, 2021

The precision problem doesn't look related.

@dalexeev
Copy link
Member

dalexeev commented Apr 2, 2021

The precision problem doesn't look related.

The problem is not with the precision of the numbers themselves, but that print doesn't display the number as precisely as it could. This can be seen by the fact that everything works with the formatting operator.

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 2, 2021

Ah, right. I don't know how to fix this tbh. I can't use is_equal_approx() in FLOAT_STRING, because it produces stuff like 1.00001.0.

Well maybe I could search for . inside the resulting String 🤔

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 2, 2021

Ok changed it. Here's the FLOAT_STRING as it was originally:

#define FLOAT_STRING(f) (rtos(f) + (Math::floor(f) == f ? ".0" : ""))

The new one is slightly less efficient, but maybe it doesn't matter that much.

@akien-mga
Copy link
Member

We approved the change in a PR review meeting today, though we were concerned about the performance cost of the FLOAT_STRING define (doing a costly calculation twice + a find()).

A better option might be to use String::num_real() instead of String::num(), which already takes care of adding a .0 where relevant. I'm not sure how num() and num_real() differ aside from that and whether there would be some stuff handled by the one and not the other.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jun 26, 2021

Rebased and reworked. I removed the macro and just changed usages of String::num_real() to use true as second argument (which makes the .0 always appear). I did it in the operators instead of stringify() method to avoid duplicating formats. As a result, some more classes are affected, e.g. Basis, which uses vectors.

print(1.0)
print(Vector2.ONE)
print(Rect2(Vector2.DOWN, Vector2.ONE))
print(Basis())

->

1.0
(1.0, 1.0)
[P: (0.0, 1.0), S: (1.0, 1.0)]
[X: (1.0, 0.0, 0.0), Y: (0.0, 1.0, 0.0), Z: (0.0, 0.0, 1.0)]

@KoBeWi KoBeWi requested a review from a team as a code owner July 3, 2021 18:31
@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 27, 2021

So I just noticed that changing Variant::stringify() will affect generated docs (as seen in failed CI, PI and TAU lose their precision). Not sure if that's ok 🤔

@akien-mga
Copy link
Member

This seems to break the generated Mono glue:

/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/VisualScript.cs(125,84): error CS1503: Argument 1: cannot convert from 'double' to 'float' [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj]
/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/VisualScript.cs(125,89): error CS1503: Argument 2: cannot convert from 'double' to 'float' [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj]
/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/TextParagraph.cs(351,110): error CS1503: Argument 1: cannot convert from 'double' to 'float' [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj]
/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/TextParagraph.cs(351,115): error CS1503: Argument 2: cannot convert from 'double' to 'float' [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj]

CC @godotengine/mono

@raulsntos
Copy link
Member

@akien-mga It's likely because the bindings generator now generates code using literal 0.0 (which is a double in C#) instead of 0 (which is an int). An int can be implicitly converted to a float but a double cannot be implicitly converted to a float because of precision-loss so in methods that take a float it fails.

The generator uses operator String() to generate the constructor for the default value of variants:

case Variant::PLANE: {
Plane plane = p_val.operator Plane();
r_iarg.default_argument = "new Plane(new Vector3" + plane.normal.operator String() + ", " + rtos(plane.d) + ")";
r_iarg.def_param_mode = ArgumentInterface::NULLABLE_VAL;
} break;
case Variant::AABB: {
AABB aabb = p_val.operator ::AABB();
r_iarg.default_argument = "new AABB(new Vector3" + aabb.position.operator String() + ", new Vector3" + aabb.size.operator String() + ")";
r_iarg.def_param_mode = ArgumentInterface::NULLABLE_VAL;
} break;
case Variant::RECT2: {
Rect2 rect = p_val.operator Rect2();
r_iarg.default_argument = "new Rect2(new Vector2" + rect.position.operator String() + ", new Vector2" + rect.size.operator String() + ")";
r_iarg.def_param_mode = ArgumentInterface::NULLABLE_VAL;
} break;
case Variant::RECT2I: {
Rect2i rect = p_val.operator Rect2i();
r_iarg.default_argument = "new Rect2i(new Vector2i" + rect.position.operator String() + ", new Vector2i" + rect.size.operator String() + ")";
r_iarg.def_param_mode = ArgumentInterface::NULLABLE_VAL;
} break;

case Variant::TRANSFORM2D: {
Transform2D transform = p_val.operator Transform2D();
if (transform == Transform2D()) {
r_iarg.default_argument = "Transform2D.Identity";
} else {
r_iarg.default_argument = "new Transform2D(new Vector2" + transform.columns[0].operator String() + ", new Vector2" + transform.columns[1].operator String() + ", new Vector2" + transform.columns[2].operator String() + ")";
}
r_iarg.def_param_mode = ArgumentInterface::NULLABLE_VAL;
} break;
case Variant::TRANSFORM3D: {
Transform3D transform = p_val.operator Transform3D();
if (transform == Transform3D()) {
r_iarg.default_argument = "Transform3D.Identity";
} else {
Basis basis = transform.basis;
r_iarg.default_argument = "new Transform3D(new Vector3" + basis.get_column(0).operator String() + ", new Vector3" + basis.get_column(1).operator String() + ", new Vector3" + basis.get_column(2).operator String() + ", new Vector3" + transform.origin.operator String() + ")";
}
r_iarg.def_param_mode = ArgumentInterface::NULLABLE_VAL;
} break;
case Variant::BASIS: {
Basis basis = p_val.operator Basis();
if (basis == Basis()) {
r_iarg.default_argument = "Basis.Identity";
} else {
r_iarg.default_argument = "new Basis(new Vector3" + basis.get_column(0).operator String() + ", new Vector3" + basis.get_column(1).operator String() + ", new Vector3" + basis.get_column(2).operator String() + ")";
}
r_iarg.def_param_mode = ArgumentInterface::NULLABLE_VAL;
} break;
case Variant::QUATERNION: {
Quaternion quaternion = p_val.operator Quaternion();
if (quaternion == Quaternion()) {
r_iarg.default_argument = "Quaternion.Identity";
} else {
r_iarg.default_argument = "new Quaternion" + quaternion.operator String();
}
r_iarg.def_param_mode = ArgumentInterface::NULLABLE_VAL;
} break;

For example, we use it to generate new Vector2(0, 0) but now, because of this PR, it generates new Vector2(0.0, 0.0) which won't work because the Vector2 constructor takes floats not doubles.

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

For the Mono bindings generator, we'll probably need to manually generate each piece of the constructor.

core/variant/variant.cpp Show resolved Hide resolved
@dmbro
Copy link

dmbro commented Nov 13, 2024

I'm not experienced enough as a developer to comment on whether this change should be made, but as a user, it feels inelegant and mostly unnecessary. Not only when looking at the inspector, but when writing code to display a number in-game.

I'm working on a small RPG project and all my stat values are floats, and as @Lippanon mentioned above, I can't just cast those floats to an int because oftentimes I want the decimal value when displaying them in the UI. Creating a workaround through code feels unnecessarily convoluted. It seems to me that in the vast majority of cases you wouldn't want to see ".0" at the end of a number, particularly in-game.

If this change stays, would it be possible to have a setting on Label/RichTextLabel that toggles this change on/off for floats set as text? Or a setting you can change in the Theme?

@fire
Copy link
Member

fire commented Nov 13, 2024

I am trending towards using the old behaviour, adding a new method for the new behaviour, and making each change case by case to the latest behaviour to keep backwards compatibility.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 13, 2024

would it be possible to have a setting on Label/RichTextLabel that toggles this change on/off for floats set as text

No. You can't set float to a Label, only set the final String.

We can add a utility method that converts float to String and trims zero when necessary.

@dmbro
Copy link

dmbro commented Nov 13, 2024

No. You can't set float to a Label, only set the final String.

Oh yeah, duh haha.

We can add a utility method that converts float to String and trims zero when necessary.

Would that look something like float_to_str(float_value) or str(float_value).trim_zero()?

@Mickeon
Copy link
Contributor

Mickeon commented Nov 13, 2024

Tangentially related, the more time passes the more appropriate it looks to me to trim the trailing zero out of SpinBoxes. Whether that should be a setting or just done intrinsically is up to debate.

We can add a utility method that converts float to String and trims zero when necessary.

Could this be a placeholder modifier for format Strings? Is there also a programming language that faces a similar issue? If yes, what's their solution?

It's looking like this PR is only objectively favoured when printing floats, but actually having that work would be vastly inconsistent with the str() method which print itself uses, so that's not an option.

@aaronfranke
Copy link
Member

Regardless, we should provide methods for each style of behavior. Users shouldn't need to write str(float_value).trim_zero().

Would anyone like to propose APIs for these cases?

  • If this PR is kept, what should the non-.0 function be called?
  • If this PR is reverted, what should the .0 function be called?

@PraxisMapper
Copy link

I think this change causes my project that runs fine in 4.3 to throw errors in 4.4 dev4.

I have several data files generated by a separate C# app, where keys in a dictionary are ints that can be referenced by other data files. The trick is that C# exports JSON dictionaries with int keys as strings instead, and I have not found a way to disable/change that behavior, so when I go to do that lookup I end up with something like
DataDict[str(RefKey)]
which works fine in 4.3. In 4.4 dev4, these need to be changed to
DataDict[str(int(RefKey))]
because RefKey is now being interpreted as a float when it's read from JSON, even though there's no decimal on it anywhere.

This isn't a difficult change for me to make, but it's a surprise than this applies to all numeric values loaded from JSON.

@aaronfranke
Copy link
Member

The trick is that C# exports JSON dictionaries with int keys as strings instead, and I have not found a way to disable/change that behavior

You can't change this behavior, because the JSON format only supports strings as keys. Trying to use an int as a key would make it no longer a valid JSON file.

I'm curious where you are getting RefKey from. Is it a read as a value from elsewhere in the file, as a number value? If so, that makes sense, because JSON does not have integers, all numbers are just "number". If the data format you are using has values that refer to keys, then I would suggest making those values strings, because that is type-correct with JSON. So the value would be "5" instead of 5 for example. This also gives you freedom to move to other conventions in the future, such as if you wanted to point to a key of "apple" or a UUID instead of a number.

@TheYellowArchitect
Copy link
Contributor

TheYellowArchitect commented Nov 15, 2024

var current_level: int = 12
print(current_level)

would print 12 before this PR, but now prints 12.0
My code for what broke in my project:

var current_level: int = 12
	while current_level > target_level:
		level_holder.get_node("Level" + String.num(current_level)).hide()

the fix after this PR

var current_level: int = 12
	while current_level > target_level:
		level_holder.get_node("Level" + String.num(current_level).replace(".0","").hide()

Edit: This PR has broken all my serialization, its not just the above. All my integers which are converted to string suffixes to search files, don't work

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 15, 2024

would print 12 before this PR, but now prints 12.0

It doesn't

image

the number is int.

String.num() works with floats. Just use str().

@TheYellowArchitect
Copy link
Contributor

TheYellowArchitect commented Nov 15, 2024

Thank you for the swift response - was already changing my conversions so I am glad you replied before I continued.
You are right that just using str() instead of String.num() doesn't print integers as floats 👍

@jerryjrowe
Copy link

I'm pretty sure this is the right thread---
Integers are getting the extra ".0" on them all over the place, in internal functions, even.

With sign(), specifically, it used to return 1, 0, -1.
Now it returns 1.0, 0, -1.0

And since 1 as an integer isn't the same thing as 1.0, anything that tries to, say, match a value with sign() will not work as intended.

Also, a related weird thing that is definitely happening on Linux...

If you have a variable like @export var neato_dictionary : Dictionary[String,Variant] { "thing_one" : 1, "thing_two" : 1.0, }

..if you iterate over the values in the dictionary, it'll recognize the former as an int, and the latter as a float.

But when you use str(neato_dictionary.values()[0] (or anything similar to get to that specific value), it'll print "1.0" not "1"

@Mickeon
Copy link
Contributor

Mickeon commented Dec 2, 2024

Neither of the above seem to happen in v4.4.dev.custom_build [06bb994]:

	print(type_string(typeof(sign(2))))   # Prints int
	print(type_string(typeof(sign(2.5)))) # Prints float
	
	var dict: Dictionary[String, Variant] = { "thing_one" : 1, "thing_two" : 1.0, }
	print(dict.values()[0]) # Prints 1

And they would not be caused by this PR, either way.

@KoBeWi
Copy link
Member Author

KoBeWi commented Dec 2, 2024

Note that sign() takes and returns Variant, so if you got float, it means your argument is float. Make sure the argument is int or use signi() instead.

@akien-mga
Copy link
Member

To be clear, the only thing changed by this PR is that floating-point numbers with a whole value (which is not the same thing as integers, 3 != 3.0) will now be printed with a trailing .0 to make it clear that they are indeed floating point numbers and not integers.

So if you thought you were dealing with integers and this PR has changed those numbers to have a .0 suffix, you were actually dealing with floats and the purpose of this PR is precisely to help prevent this confusion.

@iegehang
Copy link

iegehang commented Dec 3, 2024

Hello, I needed a feature included only in 4.4 version. So I switched my project. But I'm currently having a problem with this pull feature now. All integer values are returning wrong in WebApi responses. For example; item_id stored as 1 is now 1.0. When I set this responses in a "json" object, all i see is values like 1.0, 2.0 etc. Now I have to convert whole project responses prior to that which is 19k lines of codes. Isn't there an option to change this in a projectwide?

Edit: I suspected something during serialization; while getting HTTP responses; I am using "JSON.parse_string(body.get_string_from_utf8()" as suggested in official documents. If ".0" appears at the end of integer values just because we are using "get_string_from_utf8" function, this needs to be fixed.

@Calinou
Copy link
Member

Calinou commented Dec 4, 2024

Edit: I suspected something during serialization; while getting HTTP responses; I am using "JSON.parse_string(body.get_string_from_utf8()" as suggested in official documents. If ".0" appears at the end of integer values just because we are using "get_string_from_utf8" function, this needs to be fixed.

JSON doesn't have an integer type. Instead, it only has a number type which is parsed as a float in most implementations. Some implementations have a way to force integers to be used when parsing JSON; see godotengine/godot-proposals#8830.

@iegehang
Copy link

iegehang commented Dec 4, 2024

Edit: I suspected something during serialization; while getting HTTP responses; I am using "JSON.parse_string(body.get_string_from_utf8()" as suggested in official documents. If ".0" appears at the end of integer values just because we are using "get_string_from_utf8" function, this needs to be fixed.

JSON doesn't have an integer type. Instead, it only has a number type which is parsed as a float in most implementations. Some implementations have a way to force integers to be used when parsing JSON; see godotengine/godot-proposals#8830.

Problem here is you can't deny lots of people using serialization and api results are always the same just because of a .0 extension to an integer value as i never saw before in any other languages. Now all data we have been using requires a deep refactoring. PR should have been confirmed after a little thinking about consequences.

@Zireael07
Copy link
Contributor

JSON is a standard. It does not have "integers". You seem to have been misusing the standard. This PR's main goal was to make floats vs integers explicit and it surfaced that a lot of folks have been using one when they meant the other.

@iegehang
Copy link

iegehang commented Dec 4, 2024

Oh sir, please read what i am telling up there and try to understand my point. JSON has integers or not, that's no matter. One way or another serialization is part of lots of projects. IF a PR effecting a common usage, you have to "think" about the consequences before applying it. This PR is affecting usage of serialization clearly because it seems like anything inside the "get_string_from_utf8" forced out to print ".0". In a very basic response of mine; sql stores the value as 1, WebAPI stores the value as 1. But Godot converts it to 1.0 during serialization. Just why? What would happen if you apply this PR to a language like Javascript. Can you imagine the chaos? Well I took a look at whole issues and repository logs here and I don't understand why Godot developers became so arrogant. Everyone has a meaningless fanaticism over things. Anyway I'm simply saying again; this PR made a method meaningless that has been used frequently.

@dalexeev
Copy link
Member

dalexeev commented Dec 4, 2024

As said above, the JSON specification does not distinguish between int and float, there is only Number. Regardless of the presence or absence of trailing .0 in a JSON string obtained using JSON.stringify(), after deserializing the string using JSON.parse_string() you always get a float. This has not changed, in 4.3 the same behavior, you can check this with typeof() or var_to_str().

@tool
extends EditorScript

func test(value):
    var decoded = JSON.parse_string(JSON.stringify(value))
    prints(value, type_string(typeof(value)),
            "->", decoded, type_string(typeof(decoded)))

func _run():
    test(1)
    test(1.0)

4.3:

1 int -> 1 float
1 float -> 1 float

master:

1 int -> 1.0 float
1.0 float -> 1.0 float

Although 1 == 1.0 evaluates to true, these are values ​​of different types and in many cases they are considered unequal (dictionary keys, match patterns, etc.). This change is that other string conversions (str(), print(), and some others) now make this more obvious. The previous behavior was confusing and misleading (for instance, print({ 1: "a", 1.0: "b"}) prints { 1: "a", 1: "b" } in 4.3).

I'm sorry to hear that this seems to have been a significant nuisance for you. However, you probably already had hidden bugs due to Godot's non-obvious behavior. It's possible that the bugs didn't show up because of implicit type conversions, but it's too risky and fragile to rely on that. This change just exposed the real problem, the mismatch between your expectations and how it actually works.

Regarding conversions between native and JSON types, we recently added a new API that is intended to be as preserving and unambiguous a conversion as possible. Unfortunately the current implementation has several flaws, so I've opened PR #99765 to fix them.

@iegehang
Copy link

iegehang commented Dec 4, 2024

Thank you for your reply sir. Honestly I had upgraded from 4.2 to 4.4. I literally had no idea, 4.3 also had this conversion. Thanks for the #99765

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Always print floats as float literals (ex: trailing decimal)