-
-
Notifications
You must be signed in to change notification settings - Fork 145
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
implement (de)serialize for ItemStack
and ItemKind
#660
Closed
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,5 +7,6 @@ mod layer; | |
mod player_list; | ||
mod potions; | ||
mod scoreboard; | ||
mod serialization; | ||
mod weather; | ||
mod world_border; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
use crate::ItemKind; | ||
|
||
#[test] | ||
fn test_serialize_item_kind() { | ||
let item = ItemKind::WhiteWool; | ||
let serialized = serde_json::to_string(&item).unwrap(); | ||
assert_eq!(serialized, "\"white_wool\""); | ||
|
||
let item = ItemKind::GoldenSword; | ||
let serialized = serde_json::to_string(&item).unwrap(); | ||
assert_eq!(serialized, "\"golden_sword\""); | ||
|
||
let item = ItemKind::NetheriteAxe; | ||
let serialized = serde_json::to_string(&item).unwrap(); | ||
assert_eq!(serialized, "\"netherite_axe\""); | ||
|
||
let item = ItemKind::WaxedWeatheredCutCopperStairs; | ||
let serialized = serde_json::to_string(&item).unwrap(); | ||
assert_eq!(serialized, "\"waxed_weathered_cut_copper_stairs\""); | ||
} | ||
|
||
#[test] | ||
fn test_deserialize_item_kind() { | ||
let id = "\"diamond_shovel\""; | ||
let deserialized: ItemKind = serde_json::from_str(id).unwrap(); | ||
assert_eq!(deserialized, ItemKind::DiamondShovel); | ||
|
||
let id = "\"minecart\""; | ||
let deserialized: ItemKind = serde_json::from_str(id).unwrap(); | ||
assert_eq!(deserialized, ItemKind::Minecart); | ||
|
||
let id = "\"vindicator_spawn_egg\""; | ||
let deserialized: ItemKind = serde_json::from_str(id).unwrap(); | ||
assert_eq!(deserialized, ItemKind::VindicatorSpawnEgg); | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
orvalence_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.
There was a problem hiding this comment.
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 aserde
feature flagThere was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 consideredBut 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
There was a problem hiding this comment.
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
andserde_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.