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

Add support for all engine types in JSON encoding/decoding #9510

Closed
reduz opened this issue Apr 12, 2024 · 33 comments · Fixed by godotengine/godot#92656
Closed

Add support for all engine types in JSON encoding/decoding #9510

reduz opened this issue Apr 12, 2024 · 33 comments · Fixed by godotengine/godot#92656
Milestone

Comments

@reduz
Copy link
Member

reduz commented Apr 12, 2024

Describe the project you are working on

Godot

Describe the problem or limitation you are having in your project

Several users who want to use JSON for tasks such as:

  • Communication with REST servers
  • Save games and other forms of text serialization in standard format
  • Loading generated data into the engine

have issues with the JSON support not being available to parse all types of data Godot supports, either writing to JSON and loading from it.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The idea is to add two functions to Godot:

  • One to take any Godot Variant and convert it to a JSON compatible dictionary.
  • Another to revert this process 100%.

To make it clear, for simplicity (it would become super bloated otherwise) this would not affect the JSON parser per se, but would be functions you can before encoding to JSON and after parsing from it.

Additionally, in order to carry this process perfectly, Godot should detect when numbers are integer and floating point in JSON and parse them as such (currently only parses them as float.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

This is an example of how JSON would look like in a Godot compatible format:

# Godot Elements
# Encoded as dictionary. If dictionary has _gtype key then it has a Godot type encoded with the type encoded as value.
{ 
  "__gtype":"Vector3",
  "values":[0,1,2]
},
{ 
  "__gtype":"Vector4",
  "values":[0,1,2,4]
}

# Or alternatively as members

{ 
  "__gtype":"Vector3",
  "x":0,
  "y":1,
  "z":2
},
{ 
  "__gtype":"Vector4",
  "x":0,
  "y":1,
  "z":2
  "w":4  
}

# Poll can be made to see what preference community has.

# For the rest, examples:

{ 
  "__gtype":"NodePath",
  "path":["a","b","c"]
  "subpath":["a","b","c"]
},
{ 
  "__gtype":"StringName",
  "name":"Hello" 
}

# Callable, Signal, RID not encodeable
# OBJECT can encode classes (optionally) basically using the same process as inst2dict 
# (in fact can just use the same code), but adding __gtype:"ClassName"

# Dictionaries are a problem because in Godot they can have non-string keys.
# So my recommendation here is to simply check if the dictionary has non-string keys. 
# If does not, simply use a regular JSON Object. Otherwise
# encode like this:

{
  "__gtype":"Dictionary",
  "pairs" : [ key, value, key, value, etc]
}

# The "pairs" format as seen above would be rare to see because Godot very rarely (if ever) serializes information using non String keys, but it needs to exist in case a Dictionary containing non-String keys is used.

# Arrays are just regular JSON arrays

# Packed arrays need to specify the type

{
  "__gtype":"PackedByteArray",
  "base64" : "<base64_data>"
},
{
  "__gtype":"PackedIntArray",
  "values" : [1,2,3,4]
}

Implementation in Godot:

Changing the JSON Parser to add this seems like bloating it unnecesarily. It seems to me we should add two functions:

Variant JSON::native_to_json(Variant p_native,bool p_allow_classes = false, bool p_allow_scripts = false);
Variant JSON::json_to_native(Variant p_native,bool p_allow_classes = false, bool p_allow_scripts = false);

These operate on Variant, not on text. So you would need to use this to encode a Variant before converting it to text, and after converting it from text.

It would be used like this:

json_string = JSON.stringify( JSON.native_to_json( godot_type) )

# and

godot_type = JSON.json_to_native( JSON.parse_string( json_string ) )

Implementing like this would make the implementation trivial and secure.

If this enhancement will not be used often, can it be worked around with a few lines of script?

No

Is there a reason why this should be core and not an add-on in the asset library?

An add-on should be possible, but given this is something that will need to be eventually audited (as it can become an exploitable surface), it is ideal the engine supports it internally.

@JoNax97
Copy link

JoNax97 commented Apr 12, 2024

I like the idea but I'm not a fan or the API. I think having a 2-step process can be confusing for some users.
Could a convenience API be added that combines both steps?

For example adding an optional parameter to JSON.stringify, like full_serialization = false, so default behavior is the same as now.

@reduz
Copy link
Member Author

reduz commented Apr 12, 2024

@JoNax97 I prefer not, as I stated for many reasons:

  • If you are restricting to pure JSON (and many are) you don't want the extra complexity.
  • It makes parsing and validating easier, you can tell whether the JSON is malformer and then you can tell if the Godot types are malformed.
  • Definitely want this complexity outside the JSON parser itself. As mentioned before, this is something that has potential for security exploits, so the more separate and the more simple each, the best.

@fire
Copy link
Member

fire commented Apr 12, 2024

I agree with making all engine types json encodable/decodable. We worked with a contributor @AdrienQuillet to implement a diff-ing system too. Perhaps he can join the discussion.

@reduz
Copy link
Member Author

reduz commented Apr 12, 2024

@fire right, remember again that the idea of this proposal is that we don't change our JSON encoder/decoder since users may actually not want this, (as they are working with pure JSON), but we implement this as an extra, optional layer.

@Calinou
Copy link
Member

Calinou commented Apr 12, 2024

Should the type identifier be __gdtype instead? We generally use "gd" as short for Godot rather than just "g".

@reduz
Copy link
Member Author

reduz commented Apr 12, 2024

@Calinou oh sure, makes sense

@fire
Copy link
Member

fire commented Apr 12, 2024

@reduz We wanted to save the godot engine scene as json and resources which then be easily diffable. https://github.com/V-Sekai/godot_json_scene_merge was our last proof of concept.

@reduz
Copy link
Member Author

reduz commented Apr 12, 2024

@fire yeah nothing should avoid you to save a PackedScene as JSON but I have the feeling this is not what you are after. That said you should be able to encode and decode it with some extra code on your end with this.

@fire
Copy link
Member

fire commented Apr 12, 2024

I made a diagram of our proposed process which would benefit with this feature to lossless encode and decode engine JSON types.

graph TD;
    A[PackedScene Resource] -->|Convert to Variant| B[Variant Representation];
    B --> C[Apply native_to_json];
    C --> D[JSON Dictionary with __gdtype];
    D --> E[Convert Dictionary to JSON String];
    E -->|Serialize to Binary JSON| F[Binary JSON Storage];

    G[Binary JSON Storage] -->|Deserialize from Binary JSON| H[JSON String];
    H --> I[JSON Dictionary with __gdtype];
    I --> J[Apply json_to_native];
    J --> K[Variant Representation];
    K --> L[Reconstruct PackedScene Resource];
Loading

@KoBeWi
Copy link
Member

KoBeWi commented Apr 12, 2024

You can already use var_to_str to achieve the same thing.

@fire
Copy link
Member

fire commented Apr 12, 2024

var_to_str is not an acceptable replacement feature because parsing strings is not very efficient and the str representation representation quality varies compared to json.

For a use case in validating secure input in an online user generated content system.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 12, 2024

I mean you can use var_to_str to convert native type to String and then store it inside JSON.

@foolmoron
Copy link

Tiny suggestion, how about formatting non-string dicts like this?

{
  "__gtype":"Dictionary",
  "keys" : [ key, key ],
  "values": [ value, value ]
}

@reduz
Copy link
Member Author

reduz commented Apr 12, 2024

@KoBeWi

You can already use var_to_str to achieve the same thing.

I know, but there is no point to use JSON then 😓.

@adriano-sudario
Copy link

adriano-sudario commented Apr 12, 2024

Naming and using suggestion:

json_string = JSON.variant_stringify( godot_type )

#and

godot_type = JSON.to_variant( json_string ) # 'JSON.parse_string' could be used internally

@RichoM
Copy link

RichoM commented Apr 12, 2024

How are you planning to encode special values such as Infinity or NaN?

Also, what about circular references?

@SelennLamson
Copy link

SelennLamson commented Apr 12, 2024

Hi, I actually coded something like that for a game of mine.
One of the biggest problems of Godot serialization imo is the ability for loaded objects to contain code, which might be immediately ran by the engine. This is a safety breach, so I wanted to make sure I have something as simple as possible to serialize data, without having to go through manual serialization of my classes.

It's in C#, and it works from a custom class inheriting "Godot.Resource". It simply converts the custom resource to a Godot Dictionary in a way that can be serialized to JSON using Godot functions, and reverted back to the initial custom resource (as long as you know the root type and it matches the content of the serialized string).

It can currently:

  • Serialize & deserialize any Variant type, including:
    • Primitive types
    • Godot Dicts, typed and untyped
    • Godot Arrays, typed and untyped,
    • [not tested] Complex types like vectors, transforms
  • Serialize & deserialize any custom class inheriting the class I wrote, itself inheriting Godot.Resource,
    • All and only the [Export] fields are serialized and deserialized,
  • Recursive serialization/deserialization of Variants and custom Resources.

Current limitations:

  • Only in C#
  • The root has to be a SerializableResource (the class I wrote), that might itself contain Variants
  • Not very good with type checks on Dicts and Arrays
  • Uses dynamic "object" C# notation, maybe something better could be written in C++
  • Does not handle references properly, just composition, so:
    • Objects referenced from multiple places in the tree would be serialized several times
    • Objects referenced from multiple places in the tree would be separate objects after deserialization
    • Circular references lead to infinite recursion
  • Does not handle Callables nor Signals

I'm new to the Godot community and would be glad to participate in making something that others could actually use (I mean, something that meets the quality standard).

@MMUTFX2053
Copy link

maybe its better to have a new format that replaces JSON, lets call it GDSON

@Calinou
Copy link
Member

Calinou commented Apr 12, 2024

maybe its better to have a new format that replaces JSON, lets call it GDSON

The goal of this proposal is to keep using a standard format, as it's what other applications (such as databases) use. Using a custom format would defeat the point of this proposal. Also, Godot already has its own file formats such as Resource (text or binary) and ConfigFile (text).

@MMUTFX2053
Copy link

maybe its better to have a new format that replaces JSON, lets call it GDSON

The goal of this proposal is to keep using a standard format, as it's what other applications (such as databases) use. Using a custom format would defeat the point of this proposal. Also, Godot already has its own file formats such as Resource (text or binary) and ConfigFile (text).

im an amateur/hobbyist gamedev, so im not as experienced or knowledgeable as you guys, but from my experience, its very frustrating that there are limitations to things like JSON, because i did find it easier to work with then with resource, but then i noticed i couldnt store all the data types available.

that is why i thought maybe its better to deprecate JSON and introduce a new format that supports all data types

@willnationsdev
Copy link
Contributor

maybe its better to have a new format that replaces JSON, lets call it GDSON

The goal of this proposal is to keep using a standard format, as it's what other applications (such as databases) use. Using a custom format would defeat the point of this proposal. Also, Godot already has its own file formats such as Resource (text or binary) and ConfigFile (text).

im an amateur/hobbyist gamedev, so im not as experienced or knowledgeable as you guys, but from my experience, its very frustrating that there are limitations to things like JSON, because i did find it easier to work with then with resource, but then i noticed i couldnt store all the data types available.

that is why i thought maybe its better to deprecate JSON and introduce a new format that supports all data types

@MMUTFX2053 There is no singular data format that can support all possible software. If each app tried to push its needs onto a common format, then all apps would endlessly spend their days adapting to ever-shifting standards rather than being productive.

That's why instead people convert to a simple & flexible format (like JSON) that can convey ideas from any domain (albeit with some inefficiencies / added metadata). The other applications will already have working systems that know how to parse and understand its structure, even if it may not know the meaning of certain keys or values initially.

Godot's traditional JSON serialization just builds a JSON structure out of Godot data without ensuring that it is capable of fully/safely converting the data back (it leaves out needed metadata). This proposal is for clarifying how a JSON structure could be mutated to provide that metadata and by what means the Godot API should convert to/from the metadata-filled vs. metadata-less JSON.

@WolfgangSenff
Copy link

WolfgangSenff commented Apr 13, 2024

I really like this idea and would help my GodotFirebase plugin out a lot - we have some automatic conversion stuff in our codebase that could be removed as not needed. I do think there may be some difficulty with storing stuff as "{data : [1, 2, 3, 4]}" for instance with it though, and would prefer if possible to use the member names rather than an array for vectors and things. But maybe in the end we'll need custom conversions for arrays anyway, since there's a limitation in Firebase of not supporting arrays directly (in one of its technologies, works fine in others). Conversely, if it just ends up being a string of an array, then that's totally fine no matter what, so as long as it parses back to the correct thing, I and my users would be very happy to have this!

@reduz
Copy link
Member Author

reduz commented Apr 29, 2024

@MMUTFX2053

maybe its better to have a new format that replaces JSON, lets call it GDSON

This is what you have already when you call var_to_str and str_to_var , and it fully supports all Godot types.

This proposed feature is more of a requirement for inter-operation with existing servers and protocols to allow storing the data in a way that something like databases and back-end languages and platforms can understand and optimize for.

@fire
Copy link
Member

fire commented Apr 29, 2024

Note that the gltf XMP extension has a neat way of encoding sets vs lists in json if we need that feature.

https://github.com/KhronosGroup/glTF/blob/main/extensions/2.0/Khronos/KHR_xmp_json_ld/README.md#lists-and-sets

@akien-mga
Copy link
Member

akien-mga commented May 3, 2024

I support the proposal, but one aspect I see missing is how to handle int and float types to ensure that they get properly converted. This is currently a major pain point with JSON which doesn't have these two types, but only Number.

I have yet to test godotengine/godot#91503, but I suspect it might not be able to preserve information about the difference between var i: int = 1 and var f: float = 1.0. I'll test to confirm.

CC @AThousandShips as we discussed this at length a few weeks ago, I think there might be related proposals / issues that could possibly be folded into this one as a common solution for preserving as much Godot-specific type information as possible.

@reduz
Copy link
Member Author

reduz commented May 3, 2024

@akien-mga The int/float thing to be honest is a problem, but not entirely related to this proposal (which just works on a higher level). My feeling regarding int/float is that we should explicitly add decimals to floats and parse them as such.

This is not a warranty of course that other implementations you talk to will do this, but at least if you are saving/loading this within Godot it remains consistent.

@akien-mga
Copy link
Member

akien-mga commented May 3, 2024

@akien-mga The int/float thing to be honest is a problem, but not entirely related to this proposal (which just works on a higher level). My feeling regarding int/float is that we should explicitly add decimals to floats and parse them as such.

This is not a warranty of course that other implementations you talk to will do this, but at least if you are saving/loading this within Godot it remains consistent.

Right, this is something that could be done if we go ahead with godotengine/godot#47502.

@reduz
Copy link
Member Author

reduz commented May 3, 2024

@akien-mga afaik we already are forcing decimals when writing floats, I meant more about changes to the JSON parser (that only has a token for numbers, not necessarily int or float).

Adding explicit support for ints may break compatibility if users are expecting numbers to always be float, so it also may need to be an extra option for parsing.

@akien-mga
Copy link
Member

akien-mga commented May 3, 2024

Currently we don't force decimals when stringifying floats to JSON, here's an example output from your PR:

                "_float": 42,
                "_float_var": 42,

I just tested godotengine/godot#47502 and that changes this (now all floats have a decimal point), but that's indeed not enough to properly parse back 42 specifically as int, this should be changed in the parser. And indeed this would likely be considered compat breaking if we change it.

But I agree that's not directly related to this proposal. We seem to have this proposal open already, so we can move that part of the discussion there:

@h0lley
Copy link

h0lley commented May 24, 2024

I am struggling with the point. You are basically wrapping a Godot specific format into JSON. You still have the Godot specific format. Nothing other than Godot can do anything with this Godot-JSON without extra steps. Sure, other software can parse the JSON, but then it would need a special Godot type parser to do anything with it - just like it would need with TRES. The same goes for generating this Godot-JSON, of course.

@Calinou
Copy link
Member

Calinou commented May 24, 2024

but then it would need a special Godot type parser to do anything with it - just like it would need with TRES. The same goes for generating this Godot-JSON, of course.

It's much easier to read the custom Godot datatypes within JSON than to implement a TRES parser in another language. TRES looks a bit like TOML, but a lot of its syntax is incompatible with it.

@fire
Copy link
Member

fire commented Jun 1, 2024

I worked on this a little here godotengine/godot#92656

@akien-mga
Copy link
Member

Implemented by godotengine/godot#92656.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet