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

Core: Fix JSON.{from,to}_native() issues #99765

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Nov 27, 2024

  • Add support for typed arrays and dictionaries.
  • Fix script handling to be consistent with binary and text serialization.
  • Add protection against infinite recursion.
  • Fix serialization/deserialization returns value not equal to original.
    • JSON does not distinguish between int and float.
    • Godot dictionaries preserve the insertion order, while JSON dictionaries do not.
    • Make sure that custom properties/keys do not conflict with syntactic ones, as was the case with __gdtype in the case of string-only key dictionaries.
  • Fix a copy of the bug fixed in Core: Fix RefCounted handling in marshalls.cpp #90693 (premature free RefCounted).
  • Use a more compact representation.
    • Use JSON literal null instead of {"__gdtype":"Nil"} (or it would be {"type":"Nil"}).
    • Use prefixed strings for int, float, String, StringName, and NodePath.
      • Example: "i:1", "f:1.0", "s:abc", "sn:abc", "np:abc".
      • These types are used very often.
      • Number JSON type does not distinguish between int and float.
      • StringName and NodePath are used quite often.
        • Especially StringName in Lua-style dictionaries.
    • Do not use nested components for types like Rect2, Transform2D, and Transform3D.
      • All types except objects and a few simple types store data in the args key, as if it were a constructor or list initializer.
      • This doesn't hurt readability or potential parsing outside of Godot much, but it saves a lot of space.
  • Do not use separate parameters allow_classes and allow_scripts, use a single allow_objects.
    • This is consistent with binary serialization.
  • More comprehensive tests and other minor improvements to code style and consistency with binary serialization.

There were also a few minor "unrelated" improvements along the way:

  • Add Variant::get_type_by_name(), which is the inverse of Variant::get_type_name().
    • This improves performance slightly since it uses a hashmap instead of comparing strings in a loop.
    • Several similar uses have been replaced with the new method.
  • More unified container type logic for typed arrays and dictionaries.
    • This was done because marshalls.cpp already had ContainerType, which was causing an SCU build error.
Test script
@tool
extends EditorScript

var _json_from_native: Callable
var _json_to_native: Callable

func is_equal(a: Variant, b: Variant) -> bool:
    if is_same(a, b):
        return true
    if typeof(a) != typeof(b):
        return false
    if typeof(a) == TYPE_OBJECT:
        if a.get_class() != b.get_class():
            return false
        if a.get_script() != b.get_script():
            return false
        # NOTE: Ideally we would want to compare properties recursively,
        # but for this test we don't need that.
        return true
    if typeof(a) == TYPE_DICTIONARY:
        if not a.is_same_typed(b):
            return false
        if a.size() != b.size():
            return false
        var a_keys = a.keys()
        var b_keys = b.keys()
        if a_keys != b_keys:
            return false
        for key in a_keys:
            if not is_equal(a[key], b[key]):
                return false
        return true
    if typeof(a) == TYPE_ARRAY:
        if not a.is_same_typed(b):
            return false
        if a.size() != b.size():
            return false
        for i in a.size():
            if not is_equal(a[i], b[i]):
                return false
        return true
    if typeof(a) >= TYPE_PACKED_BYTE_ARRAY:
        return a == b
    assert(false)
    return false

func stringify(value: Variant) -> String:
    if value is Object:
        return "%s(%s)" % [value.get_class(), value.get_script()]
    else:
        return var_to_str(value).replace("\n", " ")

func encode_data(value: Variant, sort_keys: bool = false) -> String:
    return JSON.stringify(_json_from_native.call(value), "", sort_keys)

func decode_data(string: String) -> Variant:
    return _json_to_native.call(JSON.parse_string(string))

func test(value: Variant) -> void:
    var encoded = encode_data(value)
    var decoded = decode_data(encoded)
    if not is_equal(value, decoded):
        prints(stringify(value), "->", stringify(decoded))

func _run() -> void:
    var json := JSON.new()
    if json.get_method_argument_count(&"from_native") == 3:
        _json_from_native = json.from_native.bind(true, true)
        _json_to_native = json.to_native.bind(true, true)
    else:
        _json_from_native = json.from_native.bind(true)
        _json_to_native = json.to_native.bind(true)

    print("Non-optimal representation")
    print("==========================")
    # Prints `{"__gdtype":"Nil"}`, but JSON has `null`.
    print(encode_data(null))
    # `StringName`s are used frequently, so a short notation is desirable.
    print(encode_data({a = 1}))
    print(encode_data({"a": 1}))
    # Nested components take up a lot of space
    print(encode_data(Rect2()))
    print(encode_data(Transform2D()))
    print(encode_data(Transform3D()))
    # Godot dictionaries preserve the insertion order, while JSON dictionaries do not.
    # Therefore, the short form for string-only dictionaries is inconsistent,
    # we must always use an array for key-value pairs.
    print(encode_data({a = "a", b = "b"}, true))
    print(encode_data({b = "b", a = "a"}, true))
    print(encode_data({"a": "a", "b": "b"}, true))
    print(encode_data({"b": "b", "a": "a"}, true))

    print()
    print("Bad serialize-deserialize result")
    print("================================")

    # JSON does not distinguish between `int` and `float`.
    test(1)
    test(1.0)
    test([1, 1.0])
    test({1: 2, 1.0: 2.0})
    # Typed collections are not supported
    test([] as Array[int])
    test({} as Dictionary[int, int])
    # The string-only short form cannot escape the special key `__gdtype`.
    test({"__gdtype": "test escape"})
    test({"type": "test escape"})
    # The script after deserialization should be the same.
    test(preload("res://test.gd").new())

    print()
    print("end")
Before
Non-optimal representation
==========================
{"__gdtype":"Nil"}
{"pairs":[{"key":{"name":"a","__gdtype":"StringName"},"value":1}],"__gdtype":"Dictionary"}
{"a":1}
{"position":{"values":[0.0,0.0],"__gdtype":"Vector2"},"size":{"values":[0.0,0.0],"__gdtype":"Vector2"},"__gdtype":"Rect2"}
{"x":{"values":[1.0,0.0],"__gdtype":"Vector2"},"y":{"values":[0.0,1.0],"__gdtype":"Vector2"},"origin":{"values":[0.0,0.0],"__gdtype":"Vector2"},"__gdtype":"Transform2D"}
{"basis":{"x":{"values":[1.0,0.0,0.0],"__gdtype":"Vector3"},"y":{"values":[0.0,1.0,0.0],"__gdtype":"Vector3"},"z":{"values":[0.0,0.0,1.0],"__gdtype":"Vector3"},"__gdtype":"Basis"},"origin":{"values":[0.0,0.0,0.0],"__gdtype":"Vector3"},"__gdtype":"Transform3D"}
{"__gdtype":"Dictionary","pairs":[{"key":{"__gdtype":"StringName","name":"a"},"value":"a"},{"key":{"__gdtype":"StringName","name":"b"},"value":"b"}]}
{"__gdtype":"Dictionary","pairs":[{"key":{"__gdtype":"StringName","name":"b"},"value":"b"},{"key":{"__gdtype":"StringName","name":"a"},"value":"a"}]}
{"a":"a","b":"b"}
{"a":"a","b":"b"}

Bad serialize-deserialize result
================================
1 -> 1.0
[1, 1.0] -> [1.0, 1.0]
{ 1: 2, 1.0: 2.0 } -> { 1.0: 2.0 }
Array[int]([]) -> []
Dictionary[int, int]({}) -> {}
{ "__gdtype": "test escape" } -> null
Resource(<GDScript#-9223370082409708004>) -> Resource(<GDScript#-9223342979152718982>)

end

If test.gd has a global name (class_name), then this also causes an error:

gdscript://-9223342907295902962.gd:2 - Parse Error: Class "Test" hides a global script class.
After
Non-optimal representation
==========================
null
{"type":"Dictionary","args":["sn:a","i:1"]}
{"type":"Dictionary","args":["s:a","i:1"]}
{"type":"Rect2","args":[0.0,0.0,0.0,0.0]}
{"type":"Transform2D","args":[1.0,0.0,0.0,1.0,0.0,0.0]}
{"type":"Transform3D","args":[1.0,0.0,0.0,0.0,1.0,0.0,0.0,0.0,1.0,0.0,0.0,0.0]}
{"args":["sn:a","s:a","sn:b","s:b"],"type":"Dictionary"}
{"args":["sn:b","s:b","sn:a","s:a"],"type":"Dictionary"}
{"args":["s:a","s:a","s:b","s:b"],"type":"Dictionary"}
{"args":["s:b","s:b","s:a","s:a"],"type":"Dictionary"}

Bad serialize-deserialize result
================================

end

@dalexeev dalexeev added this to the 4.4 milestone Nov 27, 2024
@dalexeev dalexeev force-pushed the core-fix-json-from-to-native branch 5 times, most recently from 93b46a4 to 6036d39 Compare November 28, 2024 17:41
@dalexeev dalexeev marked this pull request as ready for review November 28, 2024 18:49
@dalexeev dalexeev requested review from a team as code owners November 28, 2024 18:49
@dalexeev dalexeev force-pushed the core-fix-json-from-to-native branch 2 times, most recently from 3791fa1 to 20b2cd8 Compare November 28, 2024 23:29
@fire
Copy link
Member

fire commented Nov 29, 2024

Is this ready for review?

@dalexeev
Copy link
Member Author

Yes, it's ready.

doc/classes/JSON.xml Outdated Show resolved Hide resolved
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I support this and want this merged but I am sick with flu. @AThousandShips can you help review style and such?

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't tested but style looks good

@fire
Copy link
Member

fire commented Dec 6, 2024

I'm curious.

Do not use separate parameters allow_classes and allow_scripts, use a single allow_objects.

So allow_classes and allow_scripts is merged with allow_objects?

Edit: I prefer the granularity

@dalexeev
Copy link
Member Author

dalexeev commented Dec 7, 2024

So allow_classes and allow_scripts is merged with allow_objects?

Edit: I prefer the granularity

I think this should be consistent with binary serialization. If we want to add this option here, we should also add it there.

@fire
Copy link
Member

fire commented Dec 7, 2024

Is that a lot of trouble?

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested briefly locally and it seems to work quite well. The code looks fairly solid to me.

I asked @reduz to have a quick look and he said it looks fine.

Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marshalls changes look good

@Faless
Copy link
Collaborator

Faless commented Dec 10, 2024

If we want to add this option here, we should also add it there.

I think we can actually do that, but should go in a separate PR. The hard part is going to be how to handle compatibility with functions exposed to scripting.

@Repiteo Repiteo merged commit 8f16f86 into godotengine:master Dec 10, 2024
21 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 10, 2024

Thanks!

@dalexeev dalexeev deleted the core-fix-json-from-to-native branch December 10, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#92656 (native JSON types) does not support custom objects
8 participants