BenWoodworth / knbt

Kotlin NBT library for kotlinx.serialization
GNU Lesser General Public License v3.0
72 stars 2 forks source link

Support Java protocol version 764+ with nameless tags #36

Open WinX64 opened 11 months ago

WinX64 commented 11 months ago

As of 23w31a, NBT tags sent over the network no longer encode the usual "empty tag name", and instead only the tag type id and payload. A comparison can be seen here.

A configuration option to not encode the root tag's name should be included in the library, but I'm unsure what would be the best approach. Here are some ideas and arguments in favor/against:

BenWoodworth commented 11 months ago

Oh fun! :upside_down_face: :laughing:

Thanks for the heads up! I think you're right with adding a JavaNetwork variant. And it would be something that's clearly documented that anything before 23w31a should use Java instead.

And I agree with your configuration option downside, since it really is its own format in the way it's encoded differently.

I should have time in the coming days to thoroughly think this through (and reply to your other comments) :)

WinX64 commented 11 months ago

There are also two other ideas that I think are worth considering:

Separate named root tags from nameless root tags

The idea here is to clearly delineate the differences between named and nameless root tags. One way this could be implemented, is by encoding/decoding types as nameless by default, and by adding a special type with a custom serializer to indicate a named tag:

There are several benefits in structuring things in this way, some of which are:

Remove root tag names from the API and make them implementation-specific

The idea is essentially to remove from the API ability of defining custom root tag names in the first place, and making it so a hard-coded value (the usual empty string) is always written when necessary, and ignored when read. This may seem a bit extreme/controversial as it would break the "NBT specification", but there are also arguments in support of it:

BenWoodworth commented 10 months ago

Sorry for the late reply, it's been a hectic few weeks but My schedule's freeing up so I should have more time.

I like a lot of the ideas from your first option though, thanks for taking the time to flesh all that out so clearly :)

I went ahead implementing a JavaNetwork variant, treating all tags as unnamed tags similar to how Nbt.encodeToNbtTag() works, e.g. Nbt.encodeToNbtTag("string") gets serialized as NbtString("string")

And to avoid any confusion with the format change, it takes in a protocolVersion determines if it should implicitly encode an empty string for the old protocols, or not at all for the newer one. (or a potential future version, which... hopefully not :sweat_smile:)

I'll probably give BedrockNetwork the same treatment, and also give it a protocolVersion so any future changes wouldn't be a breaking API change in knbt.

Let me know what you think of that, and if you don't have any additional feedback, I can go ahead and merge that in!

BenWoodworth commented 10 months ago

As for changing the API for named/unnamed/etc. in general, feel free to open up a separate issue about that. For now I'm going to focus up my efforts on pushing out the long-in-development v0.12 release (which has been slow because it's been hard to find free time, but I'm newly laid off and have a lot more bandwidth now :smiling_face_with_tear:). I can tell you that your mind's exactly where mine was when I first started when I was first designing the library, and I can share why I went the route I did if you open that new issue.

WinX64 commented 10 months ago

Sorry for the late reply as well. It works just perfectly for my use case. As for the named tag, it was more of a suggestion. If you already considered it at some point, and still decided doing it another way, I don't see any point on insisting on it.

pandier commented 4 months ago

I've looked at the JavaNetwork variant implementation and I think the user should be able to specify whether empty or unnamed tags should be used without the protocol version abstraction. For example, if you had a boolean isUnnamed that specifies whether unnamed tags should be used, you would have to convert it to a protocol version: NbtVariant.JavaNetwork(isUnnamed ? 764 : 763), which I think is quite cumbersome. Getting the variant based on the protocol version could be a utility method instead.