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

NBT Component Serializer #1084

Open
wants to merge 24 commits into
base: main/4
Choose a base branch
from

Conversation

Codestech1
Copy link

Adds a support for serializing components as NBT Binary Tags.

Resolves #995

@Codestech1
Copy link
Author

Codestech1 commented Jun 9, 2024

The serialization works now, but i need to clean it up, add backwards compatibility (old nbt), add javadocs and deserialization. But the hardest part with understanding how everything realted to this (in both adventure and Minecraft) is done.

@Codestech1
Copy link
Author

Codestech1 commented Jun 9, 2024

Component deserialization works properly (tested with a large component with many children and styling), component serialization however doesn't serialize lists of components (children, etc.) as anything else than compounds. I'll check if there's a way to make it check if all types of components can be serialized as a string without looping through the entire list twice (one time for checks, second time for creating the list).

What's left (let me know if i forgot anything):

  • JUnit tests
  • Services (for custom serializer imlementation, like the gson serializer)
  • Cleanup (maybe split the serialization process to more classes?)
  • More serializer options
  • Make sure that legacy tags in hover event are serialized properly (I can't do it now, because i cannot even make them work through a /tellraw command in Minecraft)
  • Javadocs

@Codestech1
Copy link
Author

Another TODO:
The hover event serializer should serialize legacy show achievement hover events with value key instead of contents key

@Codestech1 Codestech1 marked this pull request as ready for review June 15, 2024 12:39
@Codestech1 Codestech1 marked this pull request as draft June 15, 2024 12:57
@Codestech1
Copy link
Author

I forgot to add tests for hover event and click event, i'm adding them right now

@Codestech1 Codestech1 marked this pull request as ready for review June 15, 2024 13:23
@Codestech1
Copy link
Author

Ok, it's ready now, i think

Copy link
Member

@zml2008 zml2008 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please review what the game actually does, you've dragged over so much legacy baggage that'd be completely unnecessary if you actually thought things through

@Codestech1 Codestech1 requested a review from zml2008 June 19, 2024 11:09
@Codestech1
Copy link
Author

I've added a possibility to (de)-serialize a style without a component, because it's useful for new chat type registry in Minecraft. For example Minestom uses adventure styles to (de)-serialize the chat type styles.

@Codestech1
Copy link
Author

After ~ 3 months i can say that it's stable. I've not noticed any issues with the serializer. I was using it in my server software - Jet - for that time. I tested all functionality, including item data components etc.

Copy link
Member

@kezz kezz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally speaking I'm happy with this. My only slight concern is that there's a lot of unchecked casts. I do wonder if we might be better off handling all of these and throwing nicer error messages (e.g. Expected <tag> to be of type Byte, was String)?

I don't think this is a blocker on a merge though, happy for this to go through and leave an issue for us to come back to that later.

@Codestech1
Copy link
Author

Generally speaking I'm happy with this. My only slight concern is that there's a lot of unchecked casts. I do wonder if we might be better off handling all of these and throwing nicer error messages (e.g. Expected <tag> to be of type Byte, was String)?

I don't think this is a blocker on a merge though, happy for this to go through and leave an issue for us to come back to that later.

Yeah, but in my opinion that should be done in adventure nbt instead.

@kezz
Copy link
Member

kezz commented Nov 2, 2024

How could that be done in adventure-nbt?

@Codestech1
Copy link
Author

How could that be done in adventure-nbt?

Making method such as asStringOrThrow would be useful.

@kezz
Copy link
Member

kezz commented Nov 2, 2024

I don't think that makes sense for an API.

@MelonHell
Copy link

looks good, hopefully this will be added in the near future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NBT Component serializer
5 participants