godotengine / godot

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

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

Open RolandMarchand opened 1 year ago

RolandMarchand commented 1 year ago

Godot version

v4.2.dev.custom_build [2048fe5df]

System information

Godot v4.2.dev (2048fe5df) - 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

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 commented 1 year ago

Might be a duplicate of https://github.com/godotengine/godot/issues/78204, same fundamental issue. I worked around this issue in shader compiler in https://github.com/godotengine/godot/pull/78972

aaronfranke commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Can I work on this issue?

RolandMarchand commented 1 year ago

Can I work on this issue?

Of course. @jsjtxietian already has a pull request https://github.com/godotengine/godot/pull/82440 which needs more work. You can either work on this PR, or start from scratch.

AThousandShips commented 1 year ago

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 commented 1 year ago

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