search

owengage / fastnbt

Fast serde serializer and deserializer for Minecraft's NBT and Anvil formats
MIT License
185 stars 35 forks source link

Implement stringified NBT (sNBT) #85

Closed GrizzlT closed 1 year ago

GrizzlT commented 1 year ago

First commit in this draft to implement sNBT.

Tasks:

I went ahead and removed the cesu8 and flate2 dependencies. I also suspect that fastnbt will turn into a dev-dependency used for testing only.

GrizzlT commented 1 year ago

Currently there's a few TODO comments that I'm not sure about what to do with. NBT arrays still need to receive support.

GrizzlT commented 1 year ago

The TODOs in the Serializer are still there. The deserializer is now fully implemented and does not support complex enums. fastnbt is now a dev-dependency and nom was used for parsing the input string.

Feel free to add tests and/or add suggestions.

owengage commented 1 year ago

This looks great. Are you wanting to merge this PR and do the rest in another, or just keep going in this one?

GrizzlT commented 1 year ago

I would like to resolve the TODOs left before merging.

It's mostly just needing confirmation whether the serializer is allowed to error like fastnbt or if we should do something else.

Merging into the main branch can happen in a different PR.

owengage commented 1 year ago

For the TODOs about panic vs error when something cannot be serialized, I do think they should error. Panics feel extreme. That said it's hard to imagine it not signalling a bug in the program.

The convention for erroring is that serialize and deserialize share the same error type.

GrizzlT commented 1 year ago

In that case, I'm happy with this so far, feel free to merge upstream!

GrizzlT commented 1 year ago

@owengage any updates?

owengage commented 1 year ago

Hi @GrizzlT, sorry I didn't realise you were ready to merge. Is there anything left to do from your perspective here? At some point I'll have a serious look over the code and look to merge to master. Been relatively busy as of late!

GrizzlT commented 1 year ago

@owengage Thanks for merging!

My leftover concerns have been mostly described in #49 to improve overall performance. If I have some time again I'll look at that myself but it would need changes in both fastnbt and fastsnbt. Other than that I think there are no current issues holding snbt back from being merged into master (the performance increase is only an internal change).

I'll definitely be able to use this branch for now, awesome!