serde support for core types & VariantDispatch

[Waridley]

I got curious about how hard it would be to add serde support, and ended up basically fully implementing it so I figured I should start a PR 😂

There are no tests yet, and honestly I haven't really even tested it in my own project yet. I'll be doing so over the next few days probably, but for now I'll open this as a draft to allow for comments, feedback, and opinions.

The implementations for Object types is a bit hacky, but I really like the idea of being able to replace Godot's resource files with something more Rust-y. Since I had to wrap the references in newtypes, I suppose they could be part of a separate crate anyway, unless anyone has a better idea of how to get around the orphan rules. Even if we can implement serde's traits directly on gdnative_core types, there should probably be some discussion about whether it's actually desirable to skip properties that are set to their defaults (this would end up serializing a LOT of properties otherwise).

Would close #9

[ghost]

Wonderful. Thanks for the PR! Before you go ahead and get to the TODOs, some design concerns to consider:

[Bromeon]

Maybe a bit general, and toasteater already mentioned it to some extent:

What is the use case, or rather the exact problem we're trying to solve? Games usually have a specialized serialization mechanism (for the game logic data), so it might need some thinking which parts of godot-rust could fall under that.

[Waridley]

The biggest benefit I see from including this in the library is deriving Serialize/Deserialize for the math types and dictionaries, and a relatively simple implementation for Variant kind of falls out of that (and is necessary for dictionaries)... Until you get to VariantType::Object. The fact that it's possible to serialize Objects by iterating through all of their properties makes it enticing to try to implement it, and the fact that any Variant could be an Object makes it almost seem useful or even necessary... but it certainly has a lot of caveats.

[ghost]

After giving it a bit more thought I think I agree with Bromeon that implementing serde traits for even Resources might be overkill. Godot already has a serialization mechanism for them, and it's better than what serde can ever do -- serde can't produce cross-file references, for example. We should really keep serde to the core types (which can be useful) and leave the objects alone. Object variants should perhaps produce an error message and just serialize into Nils.

[Waridley]

Object variants should perhaps produce an error message and just serialize into Nils.

I was thinking it might be nice to be able to tell the difference between a serialized Nil and a serialized Object that got converted to a Nil... But I decided to check how Godot handles converting Nil to and from other Variant types, and it's a bit confusing... https://github.com/godotengine/godot/blob/master/core/variant/variant.cpp#L188-L194
The comment says "nil can convert to anything", but the code seems to say that you can get a Nil from anything, and Nil can only be converted to an Object. But then the conversion constructors treat Nil as 0 for basically all other types: https://github.com/godotengine/godot/blob/master/core/variant/variant.cpp#L1368

EDIT: I think for [de]serialization purposes, using serialize_unit_variant and then returning VariantDispatch::Object(Variant::new()) makes the most sense. Although, if the object is a Resource, maybe instead of trying to serialize the whole resource, we could serialize its path as a compromise.

[Waridley]

It's working now. The Variant [de]serialiation code has gotten a bit messy and repetitive. Some macros can probably help clean it up. Still need to write actual tests and deal with the leaking strings issue.

VariantDispatch was pretty straight-forward once the above discussions were settled, and I figured out how to actually deserialize enums 😆 But Variant itself was a bit trickier. I ended up deciding to go with treating non-primitive variants like an externally-tagged enum stored in a map, making use of the VariantType enum as the tag. I think this should mean that formats like JSON can deserialize the same strings as either a VariantDispatch or Variant, and it yields an acceptable syntax to me in RON (non-primitive types need to be wrapped in curly braces and tagged, but the tags can be written without quotation marks).

This RON test string has been deserializing fine for me:

{ Dictionary: {
	"pos": { Vector2: (x: 64.0, y: 42.0) },
	87: false,
	{ Rid: () }: 847,
	{ Object: None }: { Color: (r: 0.5, g: 0.0, b: 1.0, a: 1.0) },
	[
		2349,
		019.0018,
		true,
		{ Rect2: (
			position: (x: 0, y: 8),
			size: (y: 9, x: 99),
		)},
		(),
	]: 256
}}

GDScript prints it as:

{87:False, Null:0.5,0,1,1, [2349, 19.0018, True, (0, 8, 99, 9), Null]:256, [RID]:847, pos:(64, 42)}

and I checked the typeof most entries and they were correct.

[ghost]

The design looks good to me now. External tagging isn't friendly to a lot of formats, but some way to discern variants has to be here, and the alternatives aren't very good either. There's one argument for using internal tagging for a number of types, though, in that objects like the following one:

{
    "type": "Vector2",
    "x": 1.0,
    "y": 2.0
}

...would be possible to deserialize either as a Variant or directly as a Vector2 (where the type field will just get ignored).

For consistency, adjacent tagging can be considered for collections, although for them there's no real benefit over external tagging, beside looking slightly more natural in JSON:

{
    "type": "Dictionary",
    "value": { "foo": "bar }
}

I have no strong feelings either way, though. I'm fine with the design as-is.

[ghost]

Consider using serialize_newtype_variant which is specifically for externally-tagged enums. This would perform better with non-self-describing (binary) formats (e.g. Bincode) since they won't have to serialize the keys as strings.

[Waridley]

I don't think non-self-describing formats can directly deserialize Variant due to the use of deserialize_any() anyway... But they should be able to [de]serialize them as VariantDispatch, and as I said in the comment below, my most recent changes should allow most self-describing formats to deserialize a VariantDispatch directly as a Variant anyway.

Oh, and I also made it so [de]serializing VariantDispatch::Dictionary and VariantDispatch::VariantArray will keep all of the elements as VariantDispatch to ensure that non-self-describing formats can still use them.

[ghost]

I see. Perhaps this is worth noting in the type-level docs for Variant?

[Waridley]

I realized it was actually possible to use an untagged representation for Variant, and still allow deserializing a VariantDispatch as a Variant, as well as deserializing stucts such as Vector2 directly as Variants. It makes the code a bit more complicated, but I think it's much less complicated than it would have been to use the internally-tagged option.

[Waridley]

This is currently rebased onto my other 3 open PR's ( #746, #747, and #748), mostly so I can actually run the tests 😅 I should be able to rebase this, squash it, and mark it as ready to review pretty soon after those get resolved.

[Waridley]

VariantDispatch is failing to serialize with JSON, need to look in to that.

Ron is also failing to deserialize a VariantDispatch directly into a Variant. May not be that important, but I was hoping to get that working.

Also want to add some tests for deserializing sequences as structs, e.g. (1.0, 2.0, 3.0) getting correctly interpreted as a Vector3.

One big question I have left is if there are any well-known formats that are self-describing, but store field names as integers. Maybe Cbor? Theoretically they should still work with VariantDispatch, but it might be nice to also let them handle Variants, depending on whether that is useful for the format or not...

[ghost]

Also want to add some tests for deserializing sequences as structs, e.g. (1.0, 2.0, 3.0) getting correctly interpreted as a Vector3.

Isn't this ambiguous? The same sequence can also be an array.

One big question I have left is if there are any well-known formats that are self-describing, but store field names as integers. Maybe Cbor? Theoretically they should still work with VariantDispatch, but it might be nice to also let them handle Variants, depending on whether that is useful for the format or not...

The MsgPack implementation does that by default.

Also moving the discussion about Option<()> at #748 (comment) here:

But on that note, why is () treated as Null, when a unit struct represented as an empty dictionary? To me it seems more like an empty tuple, thus VariantArray::new().

I forgot whether there was something I was specifically aiming for when I wrote that. Might just be an oversight.

[Waridley]

Isn't this ambiguous? The same sequence can also be an array.

Yes, and and it's worth discussing the pros & cons of this approach. It was easy to implement, and will be easy to remove if we decide against it.

I personally like the ability to do this with hand-written Ron, and I feel like it's what the Godot developers would have done if they were implementing this (I think it's actually how the JSON singleton works as well). But it could also be surprising, and result in failed calls to from_variant on structs that derive it if the user first deserializes it as a Variant for some reason. Also, since the implementation first deserializes into a VariantArray, and then checks if it could be converted to a core type, perhaps that functionality could be left as separate functions that the user must call manually.

My initial feeling is that using Variants is already accepting some amount of magic, and if you desire more explicitness there's still VariantDispatch, or concrete types (which already succeed to deserialize from either representation anyway, even with the derived impls). But I recognize that it's not helpful to say, "just use VariantDispatch if you don't like it," as that's a much more verbose approach for text-based formats. (Though I still think it may be the best approach for binary formats, even self-describing ones like MsgPack and Cbor).

Also moving the discussion about Option<()> at #748 (comment) here:

I've been trying to make this implementation robust to any potential change in the type of ().to_variant() anyway, but it could matter slightly in some places, and might make disambiguating it from None easier.

[Waridley]

For context, if it helps inform any decisions made on this:

My use case is having some data that I mostly want to use from Rust, but also sometimes access it from GDScript and the editor (possibly even editing it in the editor, if I make a custom ResourceFormatLoader, but editing by hand is more important, so I don't want to use Godot's .tres format). I also want to be able to patch it and maybe let players customize it without re-compiling the game, and it should handle unknown and missing fields gracefully (probably using serde's flatten functionality on a map in the Rust struct).

Also, I think I want to be able to have some "common" data between the client & server, while also letting each have their own specific data, and have different modules access subsets of the data or unions of separate pieces of data. This is where a dynamic type like Variant can come in handy. bevy_reflect might be more suited to this problem in Rust specifically, but since I also want to use it in Godot, trying to map between multiple slightly different dynamic types is not worthwhile.

[Waridley]

https://stackoverflow.com/questions/51276896/how-do-i-use-serde-to-serialize-a-hashmap-with-structs-as-keys-to-json

So serde_json just doesn't even try to support serializing maps that have non-string keys, save for a few basic types like numbers that it decides to convert to strings. I really don't like the idea of warping the whole implementation around JSON, but I don't know if not supporting VariantDispatch as JSON is acceptable or not... I avoid JSON whenever possible, so I'm probably not the person to make that decision, and I know it's basically the most widely-used format in existence, so it's kinda hard to not make concessions to it at the expense of nicer syntax in other formats...

Overall it seems like it'll be impossible to come up with a single solution that will be perfect for all, or even most, formats. Perhaps more dedicated structs would be necessary with specific Serialize/Deserialize implementations for various use cases. Still, we should probably decide on a general "default" solution to add first, and add any custom implementations later, or even as separate libraries.

[ghost]

I think it's actually how the JSON singleton works as well

Just tried. Vector3 is serialized as a string containing (x, y, z). Arrays are interpreted as arrays no matter what the element count is:

print(JSON.parse('{"foo": [1.0, 2.0, 3.0]}').result)
# -> {foo:[1, 2, 3]}
print(JSON.parse('{"foo": "(1.0, 2.0, 3.0)"}').result
# -> {foo:(1.0, 2.0, 3.0)}

I think it's fine to match how the JSON singleton works for the Variant impl, but should you want to go that route, it should be a 1:1 match with no further magic added. imo.

Another option is to leave the Variant implementation out for now, and limit the scope of this PR to VariantDispatch.

JSON

My sentiment is that JSON is a must. We can just let the serializer error out on non-string keys, though. Types containing floating point numbers aren't useful as keys in general. Struct keys aren't supported in most common formats, anyway. I don't see how extra effort is required here?

[Waridley]

Just tried. Vector3 is serialized as a string containing (x, y, z). Arrays are interpreted as arrays no matter what the element count is:

Ah, right, but then Quat, Color, and Rect2 are all stored as "(a, b, c, d)" for some reason. Don't know why they wouldn't then encode Rect2 as "((x, y), (x, y))". It all seems very kludgey, but I guess there's no "best" way to encode complex types in JSON.

I think it's fine to match how the JSON singleton works for the Variant impl, but should you want to go that route, it should be a 1:1 match with no further magic added. imo.

This seems logical, but also, the JSON singleton works fine from Rust anyway, so if you want to encode Variants the way it does, why not just use it?

I think the bigger argument for deserializing sequences as structs may be that it already works to deserialize sequences as concrete structs. The derived impls provide an implementation of deserialize_seq that assumes every field is in order. Therefore, it would be possible to deserialize [a, b, c] as either Vector3::new(a, b, c) or Variant::from_vector3(...). I'm also assuming the derive macros do this perhaps because some formats actually store structs this way? If so, for those formats, MyStruct::serialize -> Variant::deserialize -> MyStruct::from_variant would always fail if we didn't treat sequences as structs, while it could break in a few situations for formats that only treat structs as maps if we don't do that.

Another option is to leave the Variant implementation out for now, and limit the scope of this PR to VariantDispatch.

JSON

[...] I don't see how extra effort is required here?

The problem is that VariantDispatch is not working at all for JSON right now because of the use of serialize_newtype_variant, so strings serialize as GodotString("string"), which JSON fails to serialize as a key.

We could just serialize all GodotString variants directly as strings, but that's likely to break other formats when deserializing.

The other option I see is to use an adjacently tagged representation for VariantDispatch, which then loses the ability to do VariantDispatch::serialize -> Variant::deserialize and get the same value. Although, it's possible that very few formats would have worked with this, anyway. In Ron it only works because deserialize_any on a serialized newtype enum variant can end up treating it as a newtype struct and thus forwarding to the code that differentiates untagged variants. I don't know how many other formats would behave that way. Although, another option to maintain this ability is to check in VariantVisitor::visit_map to see if the map could be a VariantDispatch, and then deserialize the value and throw away the tag. I guess then the only risk is a dictionary that happens to have 2 entries with the names type and value (or whatever keys we choose), which would then serialize in a way that looked like a VariantDispatch and lose the type entry when deserialized.

[Waridley]

It's really annoying that types need to decide how they should be serialized, when the formats are what have the idiosyncrasies that make them incompatible with each other... But I guess that's what newtypes are for.

[ghost]

This seems logical, but also, the JSON singleton works fine from Rust anyway, so if you want to encode Variants the way it does, why not just use it?

Beats me.

I think the bigger argument for deserializing sequences as structs may be that it already works to deserialize sequences as concrete structs. The derived impls provide an implementation of deserialize_seq that assumes every field is in order. Therefore, it would be possible to deserialize [a, b, c] as either Vector3::new(a, b, c) or Variant::from_vector3(...). I'm also assuming the derive macros do this perhaps because some formats actually store structs this way? If so, for those formats, MyStruct::serialize -> Variant::deserialize -> MyStruct::from_variant would always fail if we didn't treat sequences as structs, while it could break in a few situations for formats that only treat structs as maps if we don't do that.

Have you considered the opposite situation? What about serializing a struct that contains a Vec<f32>? That would be equally as frequent and valid a use case as yours. What about TypedArray<T>? It should be obvious that some type information would be lost when you convert data from a model to anything that isn't strictly a superset. unless you encode them with extra structures. MyStruct::serialize -> Variant::deserialize -> MyStruct::from_variant is a lost cause to begin with.

The problem is that VariantDispatch is not working at all for JSON right now because of the use of serialize_newtype_variant, so strings serialize as GodotString("string"), which JSON fails to serialize as a key.

We could just serialize all GodotString variants directly as strings, but that's likely to break other formats when deserializing.

The other option I see is to use an adjacently tagged representation for VariantDispatch, which then loses the ability to do VariantDispatch::serialize -> Variant::deserialize and get the same value. Although, it's possible that very few formats would have worked with this, anyway. In Ron it only works because deserialize_any on a serialized newtype enum variant can end up treating it as a newtype struct and thus forwarding to the code that differentiates untagged variants. I don't know how many other formats would behave that way. Although, another option to maintain this ability is to check in VariantVisitor::visit_map to see if the map could be a VariantDispatch, and then deserialize the value and throw away the tag. I guess then the only risk is a dictionary that happens to have 2 entries with the names type and value (or whatever keys we choose), which would then serialize in a way that looked like a VariantDispatch and lose the type entry when deserialized.

I see. With so many design issues surrounding the untagged Variant impl, I'd suggest leaving it (and perhaps even VariantDispatch) for a future PR. We can merge the uncontroversial parts first, and deal with Variant later. With Godot's JSON::print being available, an impl on Variant isn't immediately necessary, anyway.

[Waridley]

Have you considered the opposite situation? What about serializing a struct that contains a Vec<f32>?

Yes, I guess my point was that the cases where that could go wrong would be limited to sequences of exactly 2, 3, or 4 numbers. You did express some interest in the possibility of deserializing the same data as either a Variant or a Vector2 in this comment: #743 (review) So I figured I'd just point out that benefit of doing it this way.

I see. With so many design issues surrounding the untagged Variant impl, I'd suggest leaving it (and perhaps even VariantDispatch) for a future PR.

I agree, although I'm actually kind of surprised at how well the untagged Variant impl is working. It's mostly bikeshedding issues plaguing it, plus a few formats not liking Basis or NodePath. There are too many ways to do it rather than not enough.

I was surprised, however, that VariantDispatch would cause so many issues. I figured using objects as keys in JSON would be a solved problem by now, but I guess not. However, I do have the adjacently-tagged implementation seemingly working. The only format I'm testing that isn't working with it yet is TOML, but I'm not sure that's worth even trying to fix since TOML is so optimized for flat maps with string keys, even more so than JSON. However, it does seem a bit wasteful. I need to deserialize the type before the value, so it seems like it could be a sequence instead of a map, but that's still tricky in JSON.

Nevertheless, if there's not one clear "best" default way to handle Variant[Disptach], then at least handling the simple core type structs, and perhaps typed arrays, will make it much easier to implement custom solutions for Variants if users need it.

[Waridley]

I was making the VariantDispatch JSON problem a bit harder than it needed to be. All that needed changed was the DictionaryDispatch implementation to use a sequence of structs instead of a map. The externally-tagged newtype_variant representation for VariantDispatch works fine in all tested formats.

Glad I followed through with it and tested it on MessagePack, though. It was actually incorrect to use the existing VariantType enum to figure out which VariantDispatch variant to deserialize. Unfortunately the solution results in some code duplication, but it can probably be cleaned up with macros.

[Waridley]

I doubt this is perfectly ready to merge, but I think it's close enough that I'm ready for more feedback.

[Bromeon]

bors try

[bors]

try

Build failed:

• ci

[Waridley]

I think the other reason I thought of using serde_repr was that it seems silly to generate a sequential enum and match on it rather than just using the values directly. I probably could write an implementation that just uses the values when serializing as an integer (for formats like bincode), but it's probably not worth the effort. It would be dangerous to try to transmute it if an unknown value got deserialized anyway.

[Waridley]

I will remove serde_repr and rename the modules later tonight. I can also rebase onto the CI changes if that gets merged first.

[Bromeon]

it seems silly to generate a sequential enum and match on it rather than just using the values directly

Sure, but that assumes the number is needed at all. As mentioned, GodotError seems like a rare serialization use case, and the few that come to my mind are more useful with strings (like structured logging). But I also struggle a bit to predict exactly how serialization is going to be used, I guess we need to see how users are going to interact with it 🙂

Btw, previous CI run was aborted by me. I just merged the new CI workflow! 🚀
Let's see if bors already rebases on it by default.

bors try

[bors]

try

Build succeeded:

• full-ci

[Bromeon]

Passed with the new CI 🎉

You don't have to rebase this PR, bors does it automatically.

[Waridley]

But I also struggle a bit to predict exactly how serialization is going to be used, I guess we need to see how users are going to interact with it

Yeah, it's definitely hard to predict, especially considering there is serialization support built into Godot, it's hard to know for what all use cases that may be insufficient. Perhaps the most generally useful benefit of this feature is just enabling #[derive(ToVariant, FromVariant, Serialize, Deserialize)] for simple record types.

For me, RON support was a big motivator. I have moved the Variant serialization code to my own project (via a newtype) to enable a looser hand-written syntax to be deserialized both in Rust and in GDScript/the editor. Combined with #[serde(flatten)], It also lets me add properties that only GDScript cares about or just for quick prototyping, while letting Rust still be able to view them and not fail to deserialize them.

[Bromeon]

Apart from a few minor questions, it should be good to merge from my side. It also looks like the tests are run as expected.

[Bromeon]

This should simply serialize as the enumerator's name, right?

[Waridley]

It actually generates code for vist_str, visit_bytes, and visit_u64 and lets the Serializer/Deserializer implementations choose how to represent it via serialize_unit_variant/deserialize_identifier. The u64 representation is generated sequentially in source code order, ignoring any custom discriminants. So binary formats don't have to store unnecessary strings, but it generates more code than strictly necessary, and the serialized values don't line up with their values in code (which is of course only really relevant if you're trying to debug a binary format).
https://gist.github.com/Waridley/5fc1faa65260aef4b55a95a864ec767d

[Bromeon]

Thank you for the elaboration! 👍🏼

Ugh, that expanded code reminds me why we have macros, but also why their compilation takes so long 😬

[Bromeon]

bors try

[bors]

try

Build succeeded:

• full-ci

[Bromeon]

Great! Anything left on your side, or is it ready to merge?

[Waridley]

@Bromeon Nothing I can think of. Go ahead! ☺️

[Bromeon]

bors r+

[bors]

Build succeeded:

• full-ci

[Bromeon]

@Waridley Thank you very much for your contribution! 👍🏼