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 unnamed bnbt tags #89

Open
wants to merge 9 commits into
base: 4.0
Choose a base branch
from
Open

Conversation

shBLOCK
Copy link

@shBLOCK shBLOCK commented Oct 8, 2024

Description

This PR adds support for reading/writing unnamed binary NBT tags. See #88 for potential use cases.

Reading: TODO
Writing: the name parameter of to_nbt() and save_to() now supports None, which would write an unnamed tag.

Progress

  • Reading
  • Writing
  • Doc
  • Tests

@CLAassistant
Copy link

CLAassistant commented Oct 8, 2024

CLA assistant check
All committers have signed the CLA.

@shBLOCK shBLOCK marked this pull request as ready for review October 9, 2024 09:40
@shBLOCK
Copy link
Author

shBLOCK commented Oct 9, 2024

I'm not familiar with C++ and PyBind, so my implementation is probably a bit sketchy. (I mean it works and doesn't change the current C++ API too much)
Let me know if I can improve the current implementation somehow.

@gentlegiantJGC
Copy link
Member

gentlegiantJGC commented Oct 9, 2024

You have made some good progress.
I like the writing part.
I think std::optional<std::string>(name) and std::optional<std::string>("") are redundant. I think it should cast it for you.

I don't know if I like that read_nbt can now return a NamedTag or TagNode. It makes the return type more ambiguous.
For the version that takes EncodingPreset I would prefer to include the name boolean as part of that class. The point of that version of the function is to abstract away the arguments to make it easier to use.
I think the simplest solution is just to leave the return type as NamedTag and just leave the name blank when loading an unnamed variant. The alternative is to create a new function for the unnamed variants but I like that even less.

Leaving the return type as NamedTag would mean you could add a boolean argument to the read_nbt C++ functions rather than duplicating them.

@shBLOCK
Copy link
Author

shBLOCK commented Oct 9, 2024

The problem with putting named in EncodingPreset is that it doesn't really make sense when writing, as you're passing the name parameter anyway, so the named bool in EncodingPreset seem a little redundant.
Also, I think it would make more sense to be able to do something like Write this unnamed NBT with the java encoding preset instead of making unnamed versions of the preset objects.
After all, named isn't really a "encoding" preset that's like compressed or string_encoding, which acts on the entire bnbt stream.

@shBLOCK
Copy link
Author

shBLOCK commented Oct 9, 2024

Also about the read_nbt API design, I kinda like the current implementation as it would be confusing that it returns a NamedTag when you specify named=False specifically (I definitely forgot to change the type hint of the function return types though).
I think the named parameter is clear enough to clear the confusion on the return type, but maybe union types don't play well with some IDEs?

Oops I didn't see your last line about code duplication on the C++ side, that is pretty annoying indeed...
I guess simply returning a NamedTag with an empty name would be the most straightforward solution then, just have to make sure to document this clearly.

@shBLOCK
Copy link
Author

shBLOCK commented Oct 9, 2024

Okay, the std::optional and read_nbt changes are done.
Whether to include named bool in EncodingPreset is up for further discussion though.

@gentlegiantJGC
Copy link
Member

it doesn't really make sense when writing

I forgot it was used in writing as well. It doesn't make sense there. It can be left as an extra parameter.

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.

3 participants