Fix discrepancy in int handling between JSON::stringify and JSON::parse_string #112256
+19
−5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Refactor number parsing to differentiate between integers and floats to match the behaviour of the stringify method.
|
sorry for all the force pushes, I didn't have a gltf file for testing and there was a very dumb mistake in my code, I'm very glad the tests caught it. I just got one and tested the gltf importer it should be good now |
|
I have no clue why the last workflow failed, if anyone can have a look it would be nice, changing the parser to properly handle ints shouldn't cause all the variations that have been detected 😕 Unless there are parts of code that relied on the JSON parser always returning floats and now that it's returning ints there are int divisions happening instead of float ones or something like that... I give up, I already spent too much time on this... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I am proposing a fix for the issue #24119 which I had the displeasure of running through myself following the behavior mentioned by godotengine/godot-proposals#8830 in the hopes of helping bring an end to this (7 years old!!! debate)
Currently, Godot claims to follow the official JSON spec, stating in the documentation of stringify: "[...] converting a Variant to JSON text will convert all numerical values to float types". source.
However, this is factually not true as can be seen in the code of stringify, when an INTEGER variant is detected it uses itos (line 75 of json.cpp), which has the very strange behavior of not loosing any precision when going "out" of Godot, making it look supported but then loosing precision when imported as can be seen in the modified code.
The changes in this PR hope to bring a satisfying solution to this problem by considering values that do not contain a comma or an exponent notation as an integer and storing them as such when parsing JSON.
This would make Godot follow the OpenAPI spec for JSON instead of the official spec which does not support integers.
What does it break?
Any reliance on Variant types generated by the JSON parser not including INTEGER would break
What does it fix?
Users naïvely expecting to be able to stringify then parse an int64 or a PackedInt64Array would not be surprised to see that the value of their integers have changed when importing.
From what I have understood storing uids in a JSON is the preferred way when referencing Resources in a save system so this is in my opinion critical.