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

Support Java protocol version 764+ with nameless tags #36

Open
WinX64 opened this issue Oct 4, 2023 · 6 comments
Open

Support Java protocol version 764+ with nameless tags #36

WinX64 opened this issue Oct 4, 2023 · 6 comments

Comments

@WinX64
Copy link

WinX64 commented Oct 4, 2023

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:

  • A new NbtVariant (such as NbtVariant.JavaNetwork). This would make sense since the change only affects tags sent over the network, while the ones saved to the disk remain the same. However, it could cause confusion since this changes only affects 23w31a and beyond, while prior versions remain unaffected.
  • A new general configuration option (such as encodeRootTagName = true). This would eliminate the necessity for a NbtVariant that would be version dependent. However, this option wouldn't make sense for other NbtVariants aside from Java.
@BenWoodworth
Copy link
Owner

Oh fun! 🙃 😆

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
Copy link
Author

WinX64 commented Oct 10, 2023

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:

  • data class NamedTag<T>(val name: String, val value: T)

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

  • Allows any type of tag as root: Not only it supports the nameless root tags introduced in 23w31a, but also has inherent support for any type of tag to be encoded/decoded as root (required in/after 23w40a).
  • No need for extra Variants: The Java variant could be used for both cases, with no need to implement the version-dependent JavaNetwork variant.
  • Future proofing: If the removal of the root tag name ever happens on bedrock, no changes in the library will be required, as the user will simply have to change the method for encoding the tag.
  • Easier to serialize nameless root tags: No more clunky custom serializers, as all it will take is a single call to Nbt.encodeTo*(StringTag("my value")).
  • More concise way to serialize named root tags: Currently, it is required for tags to be enclosed in a NbtCompound of a single element, with run-time validation throwing an exception if it doesn't. Having a type requiring a single name and value is more concise and can be validated at compile-time.
  • Clearer separation of concerns: This has to do with the previous point. A named root tag is inherently different from NbtCompounds, but they're currently used interchangeably. This change would clearly delineate what a NamedTag and a NbtCompound 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:

  • Although technically part of the NBT spec, custom root tag names have never had any use-cases in the Vanilla implementation (they are always written empty, and the name is always ignored when read, regardless of its actual value). Aside from some NBTs used as examples, I highly doubt any use-case exists where you'd actually need a custom root tag name.
  • This change will make the NBT format functionally identical to the Json format, aside from some minor limitations. That in and of itself has several advantages, but the main take is that it could serve as a base templete, especially in implementing missing features such as polymorphism and others.

@BenWoodworth BenWoodworth changed the title Implement nameless root tags Support Java protocol version 764+ with nameless tags Oct 19, 2023
@BenWoodworth
Copy link
Owner

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 😅)

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
Copy link
Owner

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 🥲). 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
Copy link
Author

WinX64 commented Nov 3, 2023

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
Copy link

pandier commented May 4, 2024

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.

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

No branches or pull requests

3 participants