implement (de)serialize for ItemStack and ItemKind

[maxomatic458]

Objective

• Make it possible to serialize/deserialize ItemStacks without having to implement custom logic

Solution

• implement serde::Serialize and serde::Deserialize, ItemKinds will be serialized to their snake case name: e.g white_wool

[lukashermansson]

I thought about doing this PR myself also, however I felt like it was a bit dangerous to expose this feature in the "core" of the framwork.

If people start relying on this serialization to work across versions we are limited in what kind of changes we can do to the generation of ItemKind, and the internal structure of what a ItemStack is without causing breakage.

My personal stance on this is that we should keep serialization out of the scope for the framework(core), as the added overhead of serialized items will result in one of two things (or both):

1. We are scared to break previously serialized structs of this type (making any adjustments to the way we store these internally harder and may limit what kind of changes we can do)
2. We do not care about breaking previously serialized structs between versions, possibly silently breaking between versions. Would be error prone for our users (the changes in the code would not break silently between versions). Additionally, measures taken to try to not break silently here might tie into option 1, some changes might negatively impact what type of changes we can do.

I think we would be leaning a bit more towards approach 2 here right? from the perspective of what commitment we make for the serialization and de serialization to work over time.

I'm not really sure what would be the ideal solution for this (possibly a valance_serialize module that could deal with all (or part of the complexity)? (at the very least tag a serialized "thing" with a version so we can reject de-serializing and make sure we never fail silently)) until then i think it might be wise to leave any kind of serialization and de-serialization to the user.

I would be happy to hear all of your opinions on it, but I personally would be a bit scared of us going around and making the structs serializable without some kind of abstraction layer. I think it could be a false sense of security to the users of the framework that this serialization can be safely used, and if we want to commit to that assumption users might make, we are limited in the ways we can change these structs.

[dyc3]

Definitely needs some tests for the ItemKind impls.

[dyc3]

What if we serialized this into the Minecraft identifier for the item instead? Would that make more sense?

[maxomatic458]

like minecraft:white_wool or the numerical ID?

[dyc3]

Yeah the string ident. Minecraft doesn't have public facing numerical IDs anymore

[lukashermansson]

From what I can tell from the latest efforts to update the string ids are not stable across versions either, updates to Minecraft versions could break de-serilization.

These changes should not be silent (unless a rename (from A to B) and a new item gets introduced with the name A. Where the produced result would yeild another item than what was intended when serializing.

[maxomatic458]

i assume that if minecraft were to change their IDs, then the enum variants generated by the build script would also change.
If thats the case id say thats out of scope for valence.

[maxomatic458]

yes i think then we probably want to create a valence_serde or valence_serialize crate where we can create wrapper structs for commonly serialized structs, like Items, Inventory-content, etc.

Ill try to implement that if I have time for that.

[maxomatic458]

thoughts on something like this? would live in its own valence_serde crate and be behind a serde feature flag

/// A Wrapper around [`ItemKind`] that provides serialization and deserialization support.
#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug, Default)]
pub struct SerItemKind(pub ItemKind);

impl Serialize for SerItemKind {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: serde::Serializer,
    {
        self.0.to_str().serialize(serializer)
    }
}

impl<'de> Deserialize<'de> for SerItemKind {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: serde::Deserializer<'de>,
    {
        let s: &str = serde::Deserialize::deserialize(deserializer)?;
        let item_kind = ItemKind::from_str(s).ok_or_else(|| {
            serde::de::Error::invalid_value(serde::de::Unexpected::Str(s), &"the snake case item name, like \"white_wool\"")
        });

        item_kind.map(SerItemKind)
    }
}

impl From<ItemKind> for SerItemKind {
    fn from(kind: ItemKind) -> Self {
        SerItemKind(kind)
    }
}

impl From<SerItemKind> for ItemKind {
    fn from(kind: SerItemKind) -> Self {
        kind.0
    }
}

[lukashermansson]

I think a wrapper like this should store a private field about what version it was serialized on so we can prevent silent misuse of these across versions. (By giving an error when de-serilization happens in case the version is not the same as the constant defined as "current")

Other than that I think its along the lines of what i expect.

It also needs some documentation about what compability we provide (or in this case lack of compability), doc comments on the wrapper is probably a good place to place such a note.

Also the use cases I primarily imagined dealt mostly with item stacks, but i assume that might also be in scope of what you want to provide in the upcoming pr, there you might also want to support lists of items with a single version tag for space efficiency.

[lukashermansson]

it could also defeer the "failing" to the call to into() (on the SerItemKind) (making it a try_into() and failing when version does not match)

optionaly it could expose a way to get at the itemkind, as long as its really clear that its dangerous, maybe something like unsafe fn get_itemkind_unchecked() -> ItemKind, should ofc have clear docs on why its dangerous and what should be considered
But it really depends on if we think people are going to use that, it could be left out if we think it would not bring any value

Note: this requires the internal item stack to be private to the module (so people cant access it without using the propper api) and thats probably a good default anyway

[maxomatic458]

Its pretty much a tradeoff between ease of use and safety.

Either we can have those safety checks, but it wouldnt be as easy to use (because of the wrapper types)
or we dont have safety checks and directly implement serialization on the raw types (behind a feature flag).

or we could also implement both maybe, as a serde and serde_unsafe feature?

where one would perform those version checks and the other one wouldnt.

I do really think being able to serialize and deserialize the raw structs would be more convenient.

[maxomatic458]

@lukashermansson

My personal stance on this is that we should keep serialization out of the scope for the framework(core), as the added overhead of serialized items will result in one of two things (or both):

we could put that behind a feature flag

We do not care about breaking previously serialized structs between versions, possibly silently breaking between versions.

we could either not care, or we could also implement a custom deserializer that would be backwards compatible, but i think its pretty unlikely that the ItemStack / ItemKind format will change any time soon.

[lukashermansson]

@lukashermansson

My personal stance on this is that we should keep serialization out of the scope for the framework(core), as the added overhead of serialized items will result in one of two things (or both):

we could put that behind a feature flag

We do not care about breaking previously serialized structs between versions, possibly silently breaking between versions.

we could either not care, or we could also implement a custom deserializer that would be backwards compatible, but i think its pretty unlikely that the ItemStack / ItemKind format will change any time soon.
@maxomatic458

You are right in that we could add a custom deserializer that is backwards compatible, but does such a concern really belong in valance_protocol or valance_genereated? Imo it does not.

I also don't see why ItemStack might not change in the near future, one limitation currently its hardcoded to only be able to understand vanilla item. As far as I am aware the vanilla client registers its items in a "registry" this allows for mods to register their items in said registry. I could see a lot of benefits that valance replicates this approach of an item_registry (this might mean the enum for ItemKind might just be an abstraction for easily naming the vanilla items in an item_registry and ItemStack might not refer to an enum of vanilla items at all) (similar to the biome registry although currently the item_registry system that Minecraft has does not need to be synced by the server while the biome registry does, (atleast as of the current Minecraft version))

I could see a future where ItemStack looks something like this in the code

#[derive(Clone, PartialEq, Debug, Default)]
pub struct ItemStack {
    pub item: ItemRegistyItemRef, // <- where this is just an identifier to an item registry that has all the vanilla items and all of the user defined items that might be present in mods.
    pub count: i8,
    pub nbt: Option<Compound>,
}

^ im not saying we can't support a backwards compatible de-serialize here either by referencing the registry, but it doubt it would be trivial, and definitely not something that should live in this module imo.

While everybody might not consider supporting communicating inventory updates and similar stuff containing itemstacks with items that are added via some mod to be in scope of valance, I think a change such as this could have the ability to limit what kind of changes are easy to do (while also being backwards compatible with a serialization feature, and I think most people expect a serialized format to be stable). For a change as the above if serialization to still work with the old format would have to take a dependency on wherever the registry lives just for the sake of being able to support deserializing, while it might not if serialization is not a consern of the valance_protocol crate.

I also think all my concerns goes away if we put all of this serialization stuff in its own module, that can deal with the eventual complexities that might arrive if the core framework evolves to for example support the above (and possibly other cases too)

[maxomatic458]

ill close that and open up a new PR where the serialization will be implemented in its own crate (using wrapper structs that can be serialized as @lukashermansson suggested)