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

Improve NBT editor #401

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Improve NBT editor #401

wants to merge 9 commits into from

Conversation

Podshot
Copy link
Sponsor Member

@Podshot Podshot commented Aug 20, 2021

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:

  • Input NBT objects are parsed to a representation format that keeps track of the parent containing it
  • Tree building logic is less complex and cleaner
  • NBT editing/adding dialogs are separate from each other
  • Removed the need for a callback (current NBT state can be accessed via the .nbt property)

@Podshot Podshot marked this pull request as ready for review August 23, 2021 12:16
Comment on lines 70 to 72
__slots__ = ParsedNBT.__slots__ + [
"_value",
]
Copy link
Member

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):
Copy link
Member

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

@gentlegiantJGC
Copy link
Member

There are still some issues that need ironing out but it does look good.
I re-added the old example nbt because you had it linked to a file on your PC but the old NBT cannot be displayed.
For some reason it cannot display non-container data that is either the root or in the first level.
This NBT viewer should be able to display any valid nbt or NBTFile

@gentlegiantJGC
Copy link
Member

The tree should really have one root node and all of the NBT should be a sub-entry under that node.

@gentlegiantJGC
Copy link
Member

Bugs:
These things cannot be displayed

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.
Not really a bug but I intuitively double click on items to edit them. I don't think that should be too difficult to add.
wx assertion error when deleting the "root" compound tag (note this is not actually the root of the NBT)
The add nbt tag window is not big enough by default to see the save/cancel buttons
The icons in the new tag window could be closer to the radio button they are associated with. It currently looks like they are associated to the one to the right not the left.
Creating a tag in a list asks me for a name. Tags in lists do not have names
I am able to create tags of different types in a list. NBT can only have one type in a list
I can create two tags in a compound with the same name

@@ -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"
Copy link
Sponsor Member Author

@Podshot Podshot Aug 25, 2021

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

Copy link
Member

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

Copy link
Member

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

Copy link
Sponsor Member Author

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

Copy link
Sponsor Member Author

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

Copy link
Member

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

Copy link
Member

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

Copy link
Member

@gentlegiantJGC gentlegiantJGC Aug 25, 2021

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
    )

Copy link
Member

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.

Copy link
Member

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

@Podshot Podshot marked this pull request as draft August 25, 2021 15:31
@Podshot Podshot added this to the core-features milestone Aug 25, 2021
@CLAassistant
Copy link

CLAassistant commented May 11, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ gentlegiantJGC
❌ Podshot
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

None yet

3 participants