Blob API is too terse

[Selicre]

I can't tell if I'm missing something, but the API for nbt::Blob doesn't let you enumerate keys, or remove them, or even read its name. It's okay for writing output, but you can't use it for processing NBT data that you don't know the structure of. Is there any better way to read an NBT blob, maybe using the Value type?

Edit: managed to use Value by first reading a u8 tag type and a short-prefixed string.

[atheriel]

I'm happy to see the API expanded for this kind of thing if you'd like to submit a PR.

[Zettymaster]

I'll, just write some ideas from this and some observations of my own:

Is a Map<> really the best choice here? Changing a title would break the dependency back to a containing compound tag, without adding wrappers. Ideally we could skip having a break in dependency by using Vec<>, we would not even need wrappers, we could just expose the members of blob directly and let Value own the title String. Similarly Value::Compound could get the same treatment, just having a Vec<Value> instead.

I should also mention, that the current implementation does not allow for a compound tag to contain multiple tags with the same title, which is byproduct of using a Map<>.

Yet another thing is why a Blob struct even exists. Given the above changes it could just be a normal Value::Compound that gets read/written from/to by the existing functions, they'd just be returning Value::Compound now.

I might do a PR, but probably not this week.

[Selicre]

Compounds aren't allowed to have duplicate keys as per the wiki, so a Map is a good choice.

I personally feel like the Blob type should not exist, and compound values should be able to be (de)serialized directly. Named blob support should honestly be kind of a secondary thing purely because I can't find a single instance where it is used, or is relevant. Do you know of any such places?

I was also thinking of replacing the Vec<Value> type with something that would keep a single enum discriminator per vector, but that would require unsafe code, and I'm not sure if it's that beneficial to performance.

[Zettymaster]

Oops, my bad, i read the wiki at like 2am.

[atheriel]

Named blob support should honestly be kind of a secondary thing purely because I can't find a single instance where it is used, or is relevant. Do you know of any such places?

All valid NBT files emitted by Minecraft itself and every other existing NBT implementation have a name -- it's just an empty string most of the time, as signified by the byte sequence 0a 00 00. Without those zeros your NBT will cause any parser to barf. You can look at the test suite to see examples of what NBT files look like when written out as a byte vector, if you're curious.

Named compounds are explicitly part of the original spec and several of the sample NBT values provided by Notch at the time had them. We actually use these original samples in the test suite; you can see them in the tests/ directory.

It may well be the case that the vanilla client no longer uses them, but I'm not sure it's wise to actively prevent their creation. I believe that the default Blob::new() will do what you'd expect and set the name to an empty string -- you have to use Blob::named() if you want a name.

As for

Yet another thing is why a Blob struct even exists. Given the above changes it could just be a normal Value::Compound that gets read/written from/to by the existing functions, they'd just be returning Value::Compound now.

and

I personally feel like the Blob type should not exist, and compound values should be able to be (de)serialized directly.

I introduced the Blob type because it was not possible (six years ago) to write a function in Rust that indicated that it only returns one discriminant of an enum. Its purpose is to make it harder to serialize incomplete NBT data, otherwise a tuple of a name (discussed above) and compound would likely work better.

I am actually quite open to proposals for a new API to replace Blob, but only if it is actually more ergonomic than using the existing one.

[StackDoubleFlow]

Is there any reason to keep the fields of Blob private? We might as well access the contents directly without having to recreate a map-like api.

[StackDoubleFlow]

I think we're over-complicating things here. Revisiting this, I'd like to propose a simple change to the Blob API to make it more usable:

• Publicize all fields
• Remove get, insert, and the Index impl

All Blob should be is a thin container for a named compound, and we should not be trying to recreate the HashMap api here.