-
-
Notifications
You must be signed in to change notification settings - Fork 21.7k
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
Make JSON write and parse incompatible values as Variant consistently #33241
Conversation
v_err = VariantParser::parse(&ss, value, v_parse_err, v_err_line); | ||
|
||
if (v_err != OK || _is_compatible_value(value)) { | ||
// Read the whole token value as a string instead | ||
value = token.value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There could be certain performance issues having to write and parse inner JSON strings for Variants, but it might be negligible, I haven't tested this. I'd prefer perfectly working feature over speed for this personally. I was thinking that some convention could be used to encode that JSON string contains a Variant, for instance @
reserved character could be inserted at the beginning:
{
"my_color": "@Color( 1, 1, 1, 1 )",
}
This could also avoid v_err != OK || _is_compatible_value(value))
check which I added because some strings could be falsely converted to other types. For instance, str2var("3D")
would be parsed as a number, returning 3
. This case is covered in the test project actually.
The JSON writer is rewritten to treat vars by JSON types. This is needed to better control the writing state. Namely, object keys determine whether a Variant can be written as another JSON compatible type, or whether it should be converted with `VariantWriter` to be embedded into a JSON string which can be parsed later with `VariantParser`. This effectively removes the need to manually convert incompatible types with `var2str` and `str2var` via scripting. JSON writer and parser use Variant serialization methods over simple conversion which makes the behavior consistent.
I'm really not sure whether something like this has a chance to be merged, considering that it's a core feature used by many, and I wouldn't want to be responsible for regressions. Nonetheless, it was nice to make this work. Feel free to close this if you do not want to compromise the stability. I'm unlikely to create a proposal at GIP to discuss this, and tbh I personally don't need this myself at the moment. |
@Xrayez Thanks for being upfront about your lack of interest, I'm closing this and adding "salvageable" in case others want to continue your work. |
There was some discussion on Discord that this feature could be made opt-in, but considering the amount of complexity this adds it's probably not worth it. But I have to say that we should either forbid converting JSON-incompatible types altogether, or actually allow the types to be converted properly by the Variant parser and writer (as this PR proposes), currently it's just a mix of both (people don't know what to expect), or perhaps come up with Godot's own way to encode that in JSON compatible manner (which is probably a lot more work in comparison) without relying on Variant parser and writer. The test project is valuable in and of itself.
Well I think I have to be more honest, the most important reason for the lack of interest is not because I'm not personally interested in this (this PR originates from the issues I've stumbled upon while developing #33137, and makes the feature set more complete), but because I haven't received positive feedback for core devs throughout this time, and sorry but I don't have enough lifetime to write formal proposals for each bug/feature I want to integrate, and given that the problem must exist, I don't see the point either. 😛 |
I could resurrect this given the fact that we have testing framework integrated #40148, the test project would have to be converted to doctest cases to ensure no regressions occur, but again this takes some work to write a test suite for JSON parser and writer, and I'd still like to receive some feedback on this in any case before doing anything. |
Tested out this code and works well. I have tested it on all the built in types Vector2, Vector3, Transform2D, Color and so forth. Converts a Godot type into it's string equivalent then restores it back into a Godot object. The way Godot currently handles JSON is odd as it allows exporting of unsupported types but breaks them. The only way to fix this is using complex scripts. This implementation allows data to be exchanged between Godot applications as JSON objects and data to be saved to JSON then restored. Only Godot understands these custom string datatypes and to any other application is just string. I think is a good compromise without breaking JSON specs. |
Fixes #33161, Closes #11866.
Remake of #33163.
Helps #33137.
TL;DR: try to write compatible JSON with native types, but when it can't, just stuff vars into JSON strings. This should work as before but "the right way".
The JSON writer is rewritten to treat vars by JSON types. This is needed to better control the writing state. Namely, object keys determine whether a Variant can be written as another JSON compatible type, or whether it should be converted with
VariantWriter
(akavar2str
) to be embedded into a JSON string which can be parsed later withVariantParser
(akastr2var
).This effectively removes the need to manually convert incompatible types with
var2str
andstr2var
via scripting. JSON writer and parser use Variant serialization methods over its visual representation which makes the conversion behavior possible, see issues like #27529, godotengine/godot-proposals#1351, #48169.Example
Here's an example JSON serialized from dictionary with
to_json(dict)
:JSON
Original dictionary
parse_json(json)
:Dictionary
Test project
I had to write some tests to keep regressions from occurring during development. See the script comments on what, how, and why.
json-convert-incompatible.zip