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.

Sign up for GitHub

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

Jump to bottom

implement (de)serialize for ItemStack and ItemKind #660

Closed
wants to merge 4 commits into from
Closed

implement (de)serialize for ItemStack and ItemKind #660

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
05615c3
(de)serialize for ``ItemStack`` and ``ItemKind``
maxomatic458 Oct 11, 2024
627f540
remove ``use serde`` in macro
maxomatic458 Oct 12, 2024
2baa435
add tests for serialization
maxomatic458 Oct 12, 2024
75e9b6c
Merge branch 'valence-rs:main' into item-stack-serialize
maxomatic458 Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/valence_generated/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ workspace = true
valence_math.workspace = true
valence_ident.workspace = true
uuid.workspace = true
serde = { workspace = true, features = ["derive"] }

[build-dependencies]
anyhow.workspace = true
Expand Down
24 changes: 24 additions & 0 deletions crates/valence_generated/build/item.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,9 @@ pub(crate) fn build() -> anyhow::Result<TokenStream> {
.collect::<TokenStream>();

Ok(quote! {
use serde::{Deserialize, Deserializer, Serialize, Serializer};
use serde::de::{self, Unexpected};

/// Represents an item from the game
#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug, Default)]
#[repr(u16)]
Expand All @@ -193,6 +196,27 @@ pub(crate) fn build() -> anyhow::Result<TokenStream> {
pub snack: bool,
}

impl Serialize for ItemKind {
maxomatic458 marked this conversation as resolved.
Show resolved Hide resolved
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
serializer.serialize_str(self.to_str())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@maxomatic458 maxomatic458 Oct 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like minecraft:white_wool or the numerical ID?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@lukashermansson lukashermansson Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

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

impl ItemKind {
/// Constructs a item kind from a raw item ID.
///
Expand Down
3 changes: 2 additions & 1 deletion crates/valence_protocol/src/item.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
use std::io::Write;

use serde::{Deserialize, Serialize};
pub use valence_generated::item::ItemKind;
use valence_nbt::Compound;

use crate::{Decode, Encode};

/// A stack of items in an inventory.
#[derive(Clone, PartialEq, Debug, Default)]
#[derive(Serialize, Deserialize, Clone, PartialEq, Debug, Default)]
dyc3 marked this conversation as resolved.
Show resolved Hide resolved
pub struct ItemStack {
pub item: ItemKind,
pub count: i8,
Expand Down
Loading