Unifying how NBT is treated internally in relation to the rest of types
altrisi opened this issue ยท 0 comments
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
(extendsMapValue
)NbtListValue
(extendsListValue
or maybeAbstractListValue
)- This and map could make sure entered values are of an NBT type too
NbtNumericValue
(extendsNumericValue
)- Would therefore hold typing information, unlike
NumericValue
- Would therefore hold typing information, unlike
NbtStringValue
(extendsStringValue
)
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 makeNBTSerializableValue
implement it- The interface would contain the important (non-implementation) method headers that could be shared across all NBT values (like
getNbtElement()
)
- The interface would contain the important (non-implementation) method headers that could be shared across all NBT values (like
- 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
- Those methods would declare they return an
- 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).