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

Replace original bedrockrs_nbt with mirai-nbt #48

Closed

Conversation

RadiatedMonkey
Copy link
Member

This is quite a big PR but luckily most of it is a direct copy from the original Mirai code. I've made several changes to the original version from Mirai:

  • All references to the anyhow crate have been replaced with a new NbtError type.
  • The BinaryRead and BinaryWrite traits that this crate relies on have been copy-pasted straight into the read.rs and write.rs files respectively. The use of anyhow here has been replaced with a new StreamError type. Depending on what we end up doing this code should probably be moved to the bedrockrs_shared crate or replaced with the existing binary read/write implementation. Additionally some Mirai-specific types such as Vector and BlockPosition have been removed.

Documentation for the NBT crate can be found at https://teampathfinders.github.io/mirai/mirai_nbt/index.html (or in the code of course). Although the new error type is not documented here. The BinaryRead and BinaryWrite traits are documented at https://teampathfinders.github.io/mirai/mirai_util/index.html.

Some code has not properly been ported yet such as the NBT palette deserialization in the paletted_storage crate. I still need to look into how I can use your Cursor method with my NBT lib. I made sure the project compiles but will wait until we decide on what to do with the world format library before making more changes.

Another small issue I found is that ProtoCodecError implements clone, requiring NbtError to also implement it. In StreamError I store the original error types for errors such as Utf8Error and io::Error, the latter does not implement clone though so I instead opted to convert it to a string. It is probably useful to keep the IO error so any idea what we should do with this?

@RadiatedMonkey
Copy link
Member Author

RadiatedMonkey commented Aug 22, 2024

Seems like the modifications to the paletted_storage crate weren't included in this PR?

Edit: fixed now

@theaddonn
Copy link
Member

I do think that you could even seperate the nbt library into its own repository. NBTs are used quite often... hence why a standalone library would be helpful.

@theaddonn
Copy link
Member

Also.. how would one handle serializing a given blob of NBT data with a different endianess, I quite loved our

// IIRC it looked about like that...
bedrockrs_nbt::serialize::<NbtLittleEndianNetwork>(&nbt)

@RadiatedMonkey
Copy link
Member Author

Also.. how would one handle serializing a given blob of NBT data with a different endianess, I quite loved our

// IIRC it looked about like that...
bedrockrs_nbt::serialize::<NbtLittleEndianNetwork>(&nbt)

With my lib your example would look like

bedrockrs_nbt::to_var_bytes(&nbt)

Each version also has a function that is suffixed with _in, such as to_var_bytes_in. These functions will serialize into an existing buffer rather than creating their own. You can also serialize using a generic type like you did. That can be done like this:

let mut ser = bedrockrs_nbt::Serializer::new(&mut buffer);
ser.serialize::<_, BigEndian>(&nbt).unwrap();

I guess I should probably expose a new function that wraps this, allowing you to do it like this:

bedrockrs_nbt::to_bytes::<BigEndian>(&nbt).unwrap();
bedrockrs_nbt::to_bytes_in::<BigEndian>(&mut writer, &nbt).unwrap();

to match your original NBT lib.

I think I might close this PR and move the NBT library to a new repository (should I call it bedrockrs_nbt?) Then I'll create a new PR that ports bedrockrs to it. I think if I were to continue inside of this PR it would get quite messy.

RadiatedMonkey added a commit to bedrock-crustaceans/nbtx that referenced this pull request Aug 23, 2024
@theaddonn
Copy link
Member

Also.. how would one handle serializing a given blob of NBT data with a different endianess, I quite loved our

// IIRC it looked about like that...
bedrockrs_nbt::serialize::<NbtLittleEndianNetwork>(&nbt)

With my lib your example would look like

bedrockrs_nbt::to_var_bytes(&nbt)

Each version also has a function that is suffixed with _in, such as to_var_bytes_in. These functions will serialize into an existing buffer rather than creating their own. You can also serialize using a generic type like you did. That can be done like this:

let mut ser = bedrockrs_nbt::Serializer::new(&mut buffer);
ser.serialize::<_, BigEndian>(&nbt).unwrap();

I guess I should probably expose a new function that wraps this, allowing you to do it like this:

bedrockrs_nbt::to_bytes::<BigEndian>(&nbt).unwrap();
bedrockrs_nbt::to_bytes_in::<BigEndian>(&mut writer, &nbt).unwrap();

to match your original NBT lib.

I think I might close this PR and move the NBT library to a new repository (should I call it bedrockrs_nbt?) Then I'll create a new PR that ports bedrockrs to it. I think if I were to continue inside of this PR it would get quite messy.

Calling it bedrockrs_nbt is fine

Also I think that the way you are doing it with extra functions is completely fine, it quite reminds me of serde_json. I was just doing it in my way, up to you... though the way you proposed it might be the best

bedrockrs_nbt::to_bytes::<BigEndian>(&nbt).unwrap();
bedrockrs_nbt::to_bytes_in::<BigEndian>(&mut writer, &nbt).unwrap();

Up to you 🦐

@RadiatedMonkey
Copy link
Member Author

Closing this in favour of #49

@RadiatedMonkey RadiatedMonkey deleted the mirai-nbt branch December 29, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants