Carpet

Carpet

2M Downloads

Unifying how NBT is treated internally in relation to the rest of types

altrisi opened this issue ยท 0 comments

commented

Right now, every NBT, be it a number, a string, a list or a map, is stored under a single type: NBTSerializableValue.

Because of this, working with NBT values is a lot more limited than working with regular values:

  • You can't call most native functions that take collections with nbt collections
  • The same also applies for nbt numbers, etc
    • The worse part with this is that some will sometimes work or work with artifacts, because the result of getString() will be similar to what it being native would've been
  • Some operators behave differently for NBT types than they do with native values

Those, in my opinion, make working with NBT types more painful, and I end up always converting them to native values (even if I have to convert them back afterwards) or even make me switching entire systems based around them to other, more native methods (saving to json instead of appdata).

Therefore, my suggestion would be to split the NBT type into the following types, where all implement an interface NbtValue (or maybe the same but with Nbt as another suffix):

  • NbtMapValue (extends MapValue)
  • NbtListValue (extends ListValue or maybe AbstractListValue)
    • This and map could make sure entered values are of an NBT type too
  • NbtNumericValue (extends NumericValue)
    • Would therefore hold typing information, unlike NumericValue
  • NbtStringValue (extends StringValue)

This would make them compatible with all (or at least most) native functions that take those types, including some lately requested ones like keys() or values() for maps, and would in general improve working with those types.

It could also be considered to make type() return the specific type being held, but that would break backwards compatibility given right now they all return the literal nbt. And having it directly in the type() wouldn't be as useful given that apps checking for them as acceptable values would still have to check for both (example) list and nbt_list.

In order to keep (binary) compatibility with Scarpet extensions this is my suggested upgrade path, that could be started right away without even the need of applying this suggestion. This path assumes renaming of NBTSerializableValue to NbtValue, but that could be skipped and instead converting NBTSerializableValue into an interface (I believe that doesn't break binary compat as long as no constructor is called)

  • Create the NbtValue interface and make NBTSerializableValue implement it
    • The interface would contain the important (non-implementation) method headers that could be shared across all NBT values (like getNbtElement())
  • Deprecate for removal NBTSerializableValue and all its constructors and suggest to use factory methods that just call them
    • Those methods would declare they return an NbtValue instead
    • Those would be in that new NbtValue interface
    • Using the constructors without going through the factory method would print a warning at least to the log
  • Later, if the refactor is not done yet, remove (make package private) the old constructors and the class itself, to assert that there's no references to them missing in extension mods
    • Making the class itself package private could be delayed if it's deemed necessary because of methods in the class that necessary but only for specific subtypes (though that shouldn't happen)
  • Remove NBTSerializableValue and refactor it in the new subclasses

My current idea also considers removing the lazyness of nbt parsing (I don't see it that useful, though I haven't profiled it). Another benefit of this is that the actual implementations don't need to manage an NBT type if it's easier for them, they just need to be able to provide one when requested (if requested).