-
-
Notifications
You must be signed in to change notification settings - Fork 123
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
Improve NBT editor #401
base: master
Are you sure you want to change the base?
Improve NBT editor #401
Conversation
…ield with default values when a ValueError occurs, values field gets disabled when the tag type is a TAG_Compound or TAG_List
…ating, and split up the new tag and edit tag dialogs
__slots__ = ParsedNBT.__slots__ + [ | ||
"_value", | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just re-define __slots__
as ("_value",) and it will extend the parent
parent: ParsedNBTContainer = None, | ||
): | ||
for key, value in _nbt.items(): | ||
if isinstance(value, MutableMapping): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might break with the python version of the nbt library. Please can you test all of this with the python version as well
There are still some issues that need ironing out but it does look good. |
The tree should really have one root node and all of the NBT should be a sub-entry under that node. |
Bugs: nbt.TAG_Compound({}) # This shows nothing
nbt.TAG_String("hello") # This causes a crash
nbt.TAG_Compound({"str": nbt.TAG_String("hello")}) # This causes a crash I cannot edit arrays. |
@@ -585,7 +585,7 @@ def _save(self, _): | |||
|
|||
if __name__ == "__main__": | |||
level_dat = nbt.load( | |||
r"C:\Users\gotharbg\Documents\Python Projects\Amulet-Core\tests\worlds_src\java_vanilla_1_13\level.dat" | |||
b"\x0A\x00\x0B\x68\x65\x6C\x6C\x6F\x20\x77\x6F\x72\x6C\x64\x08\x00\x04\x6E\x61\x6D\x65\x00\x09\x42\x61\x6E\x61\x6E\x72\x61\x6D\x61\x00" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an invalid NBT bytestring, since the TAG_String
is not wrapped in a TAG_Compound
, which is the reason this string crashes the UI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No that is a valid nbt format.
The first byte (0A) is the start of the compound tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And a tag string can be standalone in the binary format it just isn't used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought all valid NBT had to be wrapped in a TAG_Compound
? The NBTFile object shouldn't be acting as that TAG_Compound
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm referencing this page for whether the byte string is valid NBT or not: https://wiki.vg/NBT#Specification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are misreading it a bit. I assume you are referring to this line:
Every NBT file will always implicitly be inside a tag compound, and also begin with a TAG_Compound
. The main word being implicitly
Normally a dictionary will own the keys and store the values under those keys. Binary NBT is defined a bit backwards.
Instead of the compound tag owning the keys, the tag itself owns the keys. This is because the root tag also has a name.
The implicit compound tag is there to store the leading name however it is only used in Java level.dat files. Everywhere else it is blank. The NBTFile class is just there to handle the leading name if it isn't blank.
It is a bit complex. I am not sure if I explained it that well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The level.dat file in Java has a double nested compound tag which might be where the confusion is coming from. Try loading up a bedrock level.dat file. I think those only have one compound tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the code you will need to read a bedrock level.dat file
with open(path, "rb") as f:
f.read(8)
dat = f.read()
level_dat = nbt.load(
dat,
little_endian=True
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also while doing some debugging I realised our nbt library cannot de-serialise binary root tags that are not compound tags. This isn't really an issue though since I don't think it is ever used but they can be constructed either from SNBT or by directly using the classes so the viewer should be able to display them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two solutions to this are allowing the NBTFile to contain all tag types and
a) make the API less specific and have code get the actual nbt object
b) modify the API to pass all calls to the actual object
or just mark the issue as won't fix and leave NBTFile as handling just compound tags
|
Reworked the NBT editor to use an abstracted representation of a NBT structure instead of actual NBT objects. This allows for much cleaner handling of adding/removing items from container types (
TAG_Compound
/TAG_List
), as well as simplifying the UI code to reduce the need to handle edge cases.Changes:
.nbt
property)