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

Data partially lost when iterating through a deserialized Dictionary #95445

Closed
gturri opened this issue Aug 12, 2024 · 4 comments
Closed

Data partially lost when iterating through a deserialized Dictionary #95445

gturri opened this issue Aug 12, 2024 · 4 comments

Comments

@gturri
Copy link
Contributor

gturri commented Aug 12, 2024

Tested versions

v4.2.2.stable.mono.official [15073af]

System information

Windows 11

Issue description

I create a Godot.Collection.Dictionary<Vector2I, Vector2I>, fill it with some values, and serialize it (with Json.Stringify). So far so good.
I then deserialize it with json.Parse and cast it back to a Godot.Collection.Dictionary<Vector2I, Vector2I>.
At this point calling .ToString() on this deserialized value shows the expected content. But looping (with a foreach) on this variable get me key and values that are all Vector2I.Zero

Steps to reproduce

Run the following code (which is pretty much the code from the Saving games documentation page):

        // Serialize data
        var d = new Godot.Collections.Dictionary<Vector2I, Vector2I>
        {
            { new Vector2I(1, 2), new Vector2I(3, 4) },
        };

        foreach (var (k, v) in d) // This loop displays '(1, 2) -> (3, 4)' as expected
        {
            GD.Print($"{k} -> {v}");
        }


        var s = Json.Stringify(d);
        GD.Print($"serialized dict: {s}"); // This line displays 'serialized dict: {"(1, 2)":"(3, 4)"}' as expected


        // Deserialize data
        var json = new Json();
        var parsedResult = json.Parse(s);
        if (parsedResult != Error.Ok)
        {
            GD.Print($"failed to parse"); // just to know if we have an
            return;
        }

        // Parse deserialized data
        var d2 = new Godot.Collections.Dictionary<Vector2I, Vector2I>((Godot.Collections.Dictionary)json.Data);
        GD.Print($"deserialized dict: {d2}"); // this line displays 'deserialized dict: { "(1, 2)": "(3, 4)" }', which is  expected
       
        foreach (var (k, v) in d2)
        {
            GD.Print($"{k} -> {v}");
        }
       // /!\  This loop displays '(0, 0) -> (0, 0)' which is clearly wrong! /!\

The expected behavior is that the last line of this code prints (1, 2) -> (3, 4), but the actual behavior is that it displays (0, 0) -> (0, 0).

Minimal reproduction project (MRP)

clone.zip

@atlasapplications
Copy link

I tested this by converting a Godot.Collections.Dictionary<Vector2I, Vector2I> to a C# Variant and back to a dictionary and compared it to when you do that with json.Parse() and there is indeed a difference in output. From my understanding of JSON, the specification doesn't include a type for Dictionary or Vector2I so Godot's implementation of json.Stringify() converts it to a String and this is where I think the data loss occurs. In order for the data to be preserved it would need to be a JSON compatible type not a Godot specific type. You can also tell because when you print out the dictionary the types are in quotes instead of just parenthesis for the vectors themselves.

However, not all is lost. The data is still there and it can be retrieved you would just need to parse it yourself. My recommendation is to break down each of the Godot types like Vector2I and create a method that takes each of the Ints (which are JSON compatible types) and json.Stringify() them individually. Then when you parse it, you basically are just doing that process in reverse by deserializing.

On a separate note, if you're using C#, it may be preferable to avoid Godot.Collections as much as possible (outlined here) because it involves expensive marshalling and Variant itself defeats type safety. There are excellent libraries out there for JSON parsing built into .NET 8 itself such as System.Text.Json that may make what you're doing more efficient.

@gturri
Copy link
Contributor Author

gturri commented Aug 13, 2024

Thanks for that explanation.
Indeed, the "Saving games" documentation page turns the position to int instead of keeping it as a Vector2I, I'm afraid I overlooked it.
Thanks also for the recommendation to avoid using this in C#.

What is not clear to me now is whether I should close this bug report (considering that I was misusing this API), or if it should still be considered a bug considering that the godot documentation for json says:

The JSON enables all data types to be converted to and from a JSON string

and does not mention any such limitation.
(or perhaps it's the doc that should be "fixed", by not saying "all data types" but mentioning this limitation?)

@raulsntos
Copy link
Member

As explained by @atlasapplications, Godot's JSON serializer only supports JSON data types (string, number, object, array, bool, and null). There's already a proposal to add support for all Godot engine types:

I can see how the JSON documentation could be misleading, although later in the documentation it seems to vaguely mention some of these limitations. Pull requests to improve the documentation are welcomed 🙂

@raulsntos raulsntos changed the title [C#] Data partially lost when iterating through a deserialized Dictionary Data partially lost when iterating through a deserialized Dictionary Aug 14, 2024
@gturri
Copy link
Contributor Author

gturri commented Aug 14, 2024

Thanks for this feedback.
Given that other opened issue, and given that anyway bug report for the doc are supposed to be on https://github.com/godotengine/godot-docs instead, I'll close that issue (and I keep in mind that it would make sense that I take the time to propose a PR to improve the doc ^^)

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

No branches or pull requests

4 participants