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

ConfigFile saving floats with low precision and unable to handle floats > 1e+06 #82424

Open
RolandMarchand opened this issue Sep 27, 2023 · 8 comments

Comments

@RolandMarchand
Copy link

Godot version

v4.2.dev.custom_build [2048fe5]

System information

Godot v4.2.dev (2048fe5) - Gentoo 2.14 - Wayland - Vulkan (Forward+) - dedicated AMD Radeon RX 6600 (RADV NAVI23) () - AMD Ryzen 5 5600 6-Core Processor (12 Threads)

Issue description

There is an issue with the way ConfigFile encodes floating-point values. The low encoding precision is limited to the 6 biggest numbers, and since Godot's floating-point number is 32 bits, its ConfigFile encoding should fail at a minimum of 1e+14.

Examples

  • 123456.0 gets encoded as "123456.0," and parsed to 123456.0, but the number 1234567.0 will be encoded as "1.23457e+06," and parsed to 1234570.0, which is wrong.

  • 11111111111111.0, gets encoded as "1.11111e+13," and parsed to 11111100000000.0.

Proposed Fix

I suggest to only encode floats greater than 1e+14 in scientific notation, and to keep 14 digits of precision when doing so, while omitting trailing 0's. Otherwise, standard notation should be used.

Note

This is potentially a good issue to receive the label "good first issue."

Steps to reproduce

extends Node2D

func _ready():
	var cfg = ConfigFile.new()
	cfg.set_value("section", "key", 1000001.0)
	# The issue only occurs when parsing encoded text,
	# such as with `parse()` or `load()`
	cfg.parse(cfg.encode_to_text())
	assert(cfg.get_value("section", "key") == 1000001.0)

Minimal reproduction project

godot.zip

@bitsawer
Copy link
Member

Might be a duplicate of #78204, same fundamental issue. I worked around this issue in shader compiler in #78972

@aaronfranke
Copy link
Member

Added the label "good first issue", but note to anyone solving this issue, there is some nuance here. Plain Variant floats are 64-bit C++ doubles, so they have a larger range than the floats you'd find inside of Vector2. Vector math types are real_t which can be either 32-bit or 64-bit. The saved value's precision should reflect the precision of the type used.

@jsjtxietian
Copy link
Contributor

jsjtxietian commented Sep 27, 2023

I’d love to take this issue.

Vector math types are real_t which can be either 32-bit or 64-bit. The saved value's precision should reflect the precision of the type used.

I guess I can make a similar function like String::num which can take care of float real_t (a little hard in this way, since we will pass an int to String::num and cause ambiguous call to overloaded function) or pass an exter para to String::num.

Or there is a String::num_real seems to work but did not take care of %lf and %f. But I think it's fine since p_decimals has a limit for float. Let's give it a try.

@RolandMarchand
Copy link
Author

RolandMarchand commented Sep 27, 2023

As a workaround, I encode the float as a string using String.num(), and convert it back to a float using float():

extends Node2D

func _ready():
	var cfg = ConfigFile.new()
	cfg.set_value("section", "key", String.num(1000001.0))
	cfg.parse(cfg.encode_to_text())
	assert(float(cfg.get_value("section", "key")) == 1000001.0) # Passes

@aniketmdinde
Copy link

Can I work on this issue?

@RolandMarchand
Copy link
Author

RolandMarchand commented Oct 1, 2023

Can I work on this issue?

Of course. @jsjtxietian already has a pull request #82440 which needs more work. You can either work on this PR, or start from scratch.

@AThousandShips
Copy link
Member

You should communicate with the author of that PR, see if they need any help, otherwise you're better off looking at some other issue

@jsjtxietian
Copy link
Contributor

jsjtxietian commented Oct 1, 2023

@aniketmdinde Feel free to investigate and put more discussion here!
I'm on vacation now and will not be able to modify my pr in a few days. You can work based on my pr or leave it if you like.

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

Successfully merging a pull request may close this issue.

6 participants