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

Serialization not compatible with many formats because of flatten #130

Closed
Frizi opened this issue May 10, 2019 · 2 comments · Fixed by #313
Closed

Serialization not compatible with many formats because of flatten #130

Frizi opened this issue May 10, 2019 · 2 comments · Fixed by #313
Labels
defect Something that isn't as or doesn't work as intended

Comments

@Frizi
Copy link

Frizi commented May 10, 2019

There is a long-standing serde bug without an intention to fix, that causes all structs that use #[serde(flatten)] attribute to be interpreted as maps during serialization. Unfortunately, that means that all formats that differentiate between structs and maps cannot properly round-trip palette types through serde. It works on JSON, because it has no distinction between structs and maps.

An example format to test against would be RON (and here is it's related issue). For binary format that suffers from the same issue, check bincode.

In order to fix this, palette needs to implement serialization manually, and use serialize_struct with all the expected fields being serialized directly. That way serde will always treat those types as structs uniformly. This is not fully trivial due to generics, but should be possible with additional crate-private traits that operate on struct serializers adding all necessary fields.

Example RON behaviour of serializing Srgba with current setup:

{"red": 0.5, "green": 1.0, "blue": 0.0, "alpha": 1.0}

Deserialization of above fails, because generated deserializer expects identifiers in place of map keys, but ron always passes strings. In order to deserialze above struct properly, the representation must be manually changed to

{red: 0.5, green: 1.0, blue: 0.0, alpha: 1.0}

With manual serialization implementation, the ideal supported serialized form would be one of those, or ideally both:

Srgba(red: 0.5, green: 1.0, blue: 0.0, alpha: 1.0)

Srgba(0.5, 1.0, 0.0, 1.0)

For reference, this is how amethyst implements transform deserialization as seq and as map to accept both formats as valid in a vector field.

@Ogeon
Copy link
Owner

Ogeon commented May 12, 2019

Thank you for highlighting this! It should definitely be fixed. I'm all for supporting both formats if it can be done in a nice way (I haven't tried to do a manual implementation in quite a while, so I don't have it fresh in mind).

@Ogeon Ogeon added the defect Something that isn't as or doesn't work as intended label May 12, 2019
bors bot added a commit that referenced this issue Apr 16, 2023
313: Improve serializing r=Ogeon a=Ogeon

This fixes a long-standing bug in how `Alpha` and `PreAlpha` are serialized. They used to be flattened to a map, which is fine in JSON but not in more expressive formats, such as RON. The replacement implementation will only handle a subset of possible values (structs, tuples and sequences), but they should be the most common ones. It should also be possible to extend it later.

I have also taken the opportunity to add some helpful utility functions for serializing and de-serializing.

## Closed Issues

* Fixes #130, using a custom implementation.

## Breaking Change

Technically breaking, since it changes the serialization format a bit, but the old behavior was buggy and not always usable. The serialized type and name for `Alpha` and `PreAlpha` is now inherited from the color's type. So, for example, `Srgba` will be serialized and de-serialized as an `Rgb` struct with an additional `alpha` field. Unit types get extended to newtypes, and newtypes get extended to tuples.

Co-authored-by: Erik Hedvall <erikwhedvall@gmail.com>
@bors bors bot closed this as completed in ecd3a8e Apr 16, 2023
@Ogeon
Copy link
Owner

Ogeon commented Apr 16, 2023

A fix has been released in 0.7.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect Something that isn't as or doesn't work as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants