Ability to convert native engine types to JSON and back.

[fire]

trafficstars

Implements Add support for all engine types in JSON encoding/decoding

Co-Authored-By: Juan [email protected] Co-Authored-By: Rémi Verschelde [email protected]

• [x] Don't Handle Callable
• [x] Don't Handle RID
• [x] Check signal

@AThousandShips I added tests but I need some help debugging the failing tests:

./tests/core/io/test_json.h:40:
TEST CASE:  [Variant][SceneTree] Conversion between native and JSON formats

./tests/core/io/test_json.h:66: ERROR: CHECK( native_converted == native_data ) is NOT correct!
  values: CHECK( <null> == RID(0) )
  logged: Conversion from native to JSON type RID and back failed. JSON: { "__gdtype": "RID" } Native Converted: { "__gdtype": "Nil" }

./tests/core/io/test_json.h:66: ERROR: CHECK( native_converted == native_data ) is NOT correct!
  values: CHECK( <JSON#-9223368359372913330> == <JSON#-9223368359389690654> )
  logged: Conversion from native to JSON type Object and back failed. JSON: { "type": "JSON", "properties": { "resource_local_to_scene": false, "resource_name": "", "data": { "__gdtype": "Nil" }, "script": { "__gdtype": "Nil" } }, "__gdtype": "Object" } Native Converted: { "__gdtype": "Object" }

./tests/core/io/test_json.h:66: ERROR: CHECK( native_converted == native_data ) is NOT correct!
  values: CHECK( <null> == String::humanize_size )
  logged: Conversion from native to JSON type Callable and back failed. JSON: { "__gdtype": "Callable" } Native Converted: { "__gdtype": "Nil" }

./tests/core/io/test_json.h:66: ERROR: CHECK( native_converted == native_data ) is NOT correct!
  values: CHECK( <null> == null::[signal] )
  logged: Conversion from native to JSON type Signal and back failed. JSON: { "__gdtype": "Signal" } Native Converted: { "__gdtype": "Nil" }

===============================================================================
[doctest] test cases:     939 |     938 passed | 1 failed | 1 skipped
[doctest] assertions: 2405808 | 2405804 passed | 4 failed |
[doctest] Status: FAILURE!

[AThousandShips]

You need to remove the Add initial JSON to_native and from_native tests. from the commit message or the co-author won't count (also the extra space before Rémi)

I'll take a look tomorrow or on monday!

[reduz]

I think its ok to not handle callable or RID, you dont want those serialized.

[AThousandShips]

I second the lack serialization of those types not being an issue, RID shouldn't at all in any kind of semi-permanent or communication format, because the majority of use cases for this would be external to a single instance of the engine (i.e. either storage or transfer over network etc.), and the same could be said for both Callable and Signal, it has been discussed previously with regards to the latter two but there's no such way currently for permanent or transfer cases (it'd have to involve just Node based cases, or locally if the object is in the transfer itself, or we couldn't realistically get a reference to the object in question)

But regardless the current capabilities of the engine doesn't support transferring or storing those types so it makes sense to not have that here either

[Mickeon]

I find weird that RID cannot be "stored" because at the end of the day, from an outsider's perspective, it's just an integer ID. Worst case scenario, if someone absolutely needs it, they may just have to convert it to an integer.

[AThousandShips]

It doesn't make sense to store it because you can't use it, so in order to prevent weirdness it's better IMO to prevent it than have reports over people being confused over why their stored RID doesn't work, it shouldn't be stored because it isn't valid outside one specific run of the engine.

Edit: I'll write up some documentation improvements to clarify that RIDs are not static and shouldn't be stored or transferred (they can't easily as you can't create a new RID from an arbitrary number anyway)

[zodiepupper]

I think it might make more sense from a phraseology standpoint to call these from_variant or maybe variant_to_dict or variant_to_json

I don't often see the word "native" used in similar contexts elsewhere in the engine.

Also would it make sense to add a flag or option for this to also automatically serialize children of Nodes? or would that usecase need to be met by the user creating a recursive function to get the child nodes since it's not a root class in the engine?

Having a built in way to serialize any object or node at any point in the scene tree to json would be really nice for my project, especially since i've already spent a ton of time trying to work out how i could do exactly that. would be great for safely syncing things over the network, since it could be parsed much more easily before being converted to an instance.

i would be glad to implement that myself if that is something that would be considered in-scope for this feature.

[fire]

The standard way to serialize Node or a tree of Node is PackedScene.

I think it might make more sense from a phraseology standpoint to call these from_variant or maybe variant_to_dict or variant_to_json

@AThousandShips @reduz Thoughts on the naming? I have no preferences.

[AThousandShips]

I think the "native" naming is absolutely relevant, the difference is between it and the ordinary encoding, the specific naming can be tweaked but the suggested variant_to_dict is far too generic and doesn't convey it's special and different from the ordinary encoding, naming it that would, IMO, make it seem like it's just a version of stringize that just returns a dictionary rather than a custom format

[zodiepupper]

i guess. but it is quite literally using a dictionary and then returning it as a string. if everyone else thinks it makes sense then whatever, but from an end-user pov it would be confusing since the term isn't used elsewhere in the engine.

[AThousandShips]

The point being is that it's different from the stringize encoding, specifically for Godot, I'd say it would be more confusing if the non-specially named method would be different from the stringize one and wouldn't work with other JSON things, again the word native isn't necessarily the absolutely required one, only that it's clearly different from the stringize and parse methods

In the best of worlds we'd rework this to not have stringize and parse but instead different primary methods named more clearly, or just an argument, but we're stuck expanding on the existing class here

(Also, not to nitpick, but the phrase "native" is used extensively in the engine, though not necessarily specifically for this kind of thing, it's used, for example, for things related to getting the OS specific handle for windows and other things)

[fire]

How about?

static Variant convert_variant_from_native(const Variant &p_variant, bool p_allow_classes = false, bool p_allow_scripts = false);
static Variant convert_variant_to_native(const Variant &p_json, bool p_allow_classes = false, bool p_allow_scripts = false);

See also:

static Variant json_to_variant(const Variant &p_json, bool p_allow_classes = false, bool p_allow_scripts = false);
static Variant variant_to_json(const Variant &p_variant, bool p_allow_classes = false, bool p_allow_scripts = false);

[fire]

~~I don't think Object is serialized correctly.~~

Need to toggle the security options.

[fire]

Rebased on master with no other changes.

[fire]

Moved the json native tests to use Node for the Object variant and enable tests so they work without 3d.

Edited:

Test logs.

https://gist.github.com/fire/3e7bba9d3e6dd2339a2acadacc79903c

[fire]

Moved the memnew(Node) variable so it outlasts the for loop and then memdelete afterwards to avoid the memory leak.

[fire]

Instead of Node * node, I have used Ref<JSON> variable, which is a RefCounted and an object, and I don't need to handle object lifetimes.

[akien-mga]

Thanks @reduz for the original PR and @fire for bringing it to the finish line!

[neth392]

Is this not meant to work with objects of custom classes?

Did some testing with the code below, does not seem to be any combination of parameters that allows for converting Object -> JSON -> NewObject with the two objects being of the same type ("MyClass" in this example)

var my_class: MyClass = MyClass.new()
var json: String = JSON.stringify(JSON.from_native(my_class, true, false))
# Prints: # {"__gdtype":"Object","properties":{"resource_local_to_scene":false,"resource_name":"","serialize_this":"test"},"type":"Resource"}
print(json, "\n")
var deserialized: Variant = JSON.to_native(JSON.parse_string(json), true, false)
# Prints: false
print(deserialized is MyClass)

var json_with_script: String = JSON.stringify(JSON.from_native(my_class, true, true))
# Prints:
# {"__gdtype":"Object","properties":{"resource_local_to_scene":false,"resource_name":"","script":{"__gdtype":"Object","properties":{"resource_local_to_scene":false,"resource_name":"","script/source":"class_name MyClass extends Resource\n\t\n@export var serialize_this: String = \"test\"\n"},"type":"GDScript"},"serialize_this":"test"},"type":"Resource"}
print(json_with_script, "\n")
# Below throws: Parser Error: Class "MyClass" hides a global script class.
var deserialized_with_script: Variant = JSON.to_native(JSON.parse_string(json_with_script), true, true)

[fire]

You set allow scripts to be false so it will filter.

[fire]

I don’t think it is the job of the json serialize to allow two objects with the same class name

[neth392]

I don’t think it is the job of the json serialize to allow two objects with the same class name

So this will only work with objects if they are a new type that isn't defined in the project? I am a bit confused here, seems that the only use case of JSON & objects is to store your object, then load it back as the same type/class of object, which didn't work with my code above.

[fire]

Can you create a new issue? I'll have to nudge the gdscript team about this problem.

[neth392]

Sure thing, and thanks for these changes