Revise the serialization semantics

[kvark]

#141 made it so we serialize the tuples for some objects. I don't quite understand why this crate needs to go such a long way to achieve serialization, with lots of manual implementations around. Can't we achieve the same (or better) effect by just requiring the space type to derive serialization and ignoring the phantom?

#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
struct TypedPoint2D<T, U> {
  pub x: T,
  pub y: T,
  #[cfg_attr(feature = "serde", serde(skip))]
  marker: PhantomData<U>,
}

cc @nox

[nox]

That would serialise the point as an object with keys "x" and "y" with serde_json, which would be unfortunate and unnecessarily verbose.

https://serde.rs/json.html

We could see with @dtolnay if he would be ok with a new attribute (or maybe one exists already? If so I didn't find it) in serde_derive to serialise a struct as a tuple.

[kvark]

Where is this output used? Are we talking about the debug printing of some structures, or does it actually affect any logic?

[nox]

Where is this output used? Are we talking about the debug printing of some structures, or does it actually affect any logic?

We are talking about the (de)serialization format of points, matrices and vectors, which shouldn't serialize to verbose JSON objects when used with serde_json. I don't know if and where such a thing happens but we do expose serde implementations and we do publish this crate on crates.io so we should think about how the serialisation is done and not blindly use serde attributes if they don't produce the output that is best for all formats.

[dtolnay]

https://github.com/kardeiz/serde_tuple provides derives that serialize a struct to JSON array.

[nox]

Thanks @dtolnay, that's neat but it doesn't support #[serde(skip)] etc. Wouldn't it be easier to maintain if it lived in serde itself? There are many serde attributes and I find it unfortunate that serde_tuple would need to duplicate a lot of work to support them all.

[nox]

A straightforward way to support those attributes would be to naively include them in the Inner type being generated, but the very fact that this type exists means we build a struct tuple with one reference per field, which may or may not optimise well. This makes me believe even more that it should be a feature of serde itself. If you feel otherwise I'll just try to hack #[serde(skip)] support in the crate you linked.

[dtolnay]

This is not a sufficiently common use case for me to accept in serde_derive. It is better for us to establish idioms for how to best implement less common use cases outside of serde_derive. Regardless of where the line is drawn between sufficiently-common and insufficiently-common, there needs to be some line drawn because it is not feasible to put everything anyone has wanted to do into serde_derive. So the idioms for doing this stuff out of tree is something we need to solve either way.

I think the comment about optimization is FUD. I would expect the same generated code either way.

[nox]

So to get back to the original topic, @kvark was your issue that Serialize and Deserialize are currently derived through our own Euclid-specific procedural macro? We can use #[serde(skip)] for these two traits, but we will still need the proc macro for Hash, PartialEq, Clone etc as long as rust-lang/rfcs#2353 is not accepted and implemented.

[kvark]

Right. I don't know what the best way to proceed here. Obviously there are downsides in any of the choices.

Double-checking the RON serialization output for stuff like rectangles:

rect: ((0, 0), (3840, 2024)),

It does appear compact, but then it's not clear where the origin and where the size...

How about we keep the tuples for vectors and points, but leave the straight-derived serialization for anything more complex, like rectangles and boxes?

[nox]

Sounds fine with me, I completely forgot about Rect2D<T> and friends which just ended up with the same serialising infra at the time because we couldn't use serde_derive and I didn't want to do multiple macros.

We can indeed use serde directly for those, but that will be a breaking change, so it would be better to group it with other breaking changes.

[nical]

I am also very keen on keeping tuples for points, vectors and sizes, and have field names for rectangles and boxes.

Waiting for other motivating breaking changes before doing this one sounds good to me unless anyone wants to do the euclid bumps just for this.

[kvark]

It's not urgent matter, just need to keep in mind before releasing a new breaking version.

I wonder if there is a place in Serde for some generic mechanism:

• I want to serialize X as Y, where there is a iml<'a> From<&'a X> for Y
• I want to deserialize X as Y, where there is a impl From<Y> for X

I could imagine this used for all the euclid types that need to be shorter, plus, say HashMap converted to BTreeMap (during serialization only) for the strong ordering of keys.

[nical]

I'm going to make some breaking changes soon. Is there still something we want to change here?

[kvark]

Basically this:

How about we keep the tuples for vectors and points, but leave the straight-derived serialization for anything more complex, like rectangles and boxes?