-
Notifications
You must be signed in to change notification settings - Fork 3
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
Re-design root class name NbtCompound-nesting functionality #29
Comments
Another issue with the way things are done at the moment is related to change I mentioned on #13 (comment). This recent change in 23w40a will allow not only Compound Tags, but also String Tags to be encoded as root tags. If we were to generalize this, any type of tag could potentially be encoded as a root tag. |
I don't think that should actually be an issue with the way I have it implemented now for this issue. You'd need a custom serializer since you can't e.g. annotate a string with @NbtNamed, but adding the annotation to the serializer descriptor like this should work: object RootStringSerializer : KSerializer<String> {
override val descriptor: SerialDescriptor =
object : SerialDescriptor by PrimitiveSerialDescriptor("RootStringSerializer", PrimitiveKind.STRING) {
@ExperimentalSerializationApi
override val annotations: List<Annotation> = listOf(NbtNamed("root string name"))
}
override fun serialize(encoder: Encoder, value: String): Unit =
encoder.encodeString(value)
override fun deserialize(decoder: Decoder): String =
decoder.decodeString()
} |
What is interesting now though. I was debating between |
Yes, the issue I'm trying to point out is that very assumption (named tags are compounds with one entry), which in my view at least has always been more of a convention, and not a hard limitation of the NBT format itself, even before the change in 23w40a. It's making very cumbersome to perform some operations that should've been trivial (writing a simple Stringtag should not require all that work mentioned on #29 (comment)). I will open a new issue detailing everything (or possibly edit #36) as to not get too off-topic. The point I'm trying to make isn't really related to root class names, but as to how the root tag itself is structured/treated during serialization/deserialization. |
As far as discussing root tags and how they're structured/treated (and the mental model for it in general), this issue's probably the best place to discuss that. (Instead of a new issue, since the structure/treatment is exactly what this issue is about). And your issue for nameless root tags could be a discussion of how the new variant fits into that mental model. I have a few ideas after sitting on it yesterday, so I'll drop a comment over there too. |
And I also 100% agree that it's cumbersome. The |
Awesome, I will gather my ideas and make a post here when I have some time then. Some of the points are still more aligned with #36, so I will post those there. |
The default root class nesting behavior will eventually be removed due to it being problematic for a number of reasons. (See #29) But until then, this provides a way to opt out of that behavior entirely.
Partially in reply to your comment on #36:
I've thought about this more than I'd like to admit (😅), wanting to find a solution for:
And I had a couple misconceptions when I originally considered how names should be modeled (and in all my messages before this):
A new approach that I think will work(Which replaces the approach outlined in this issue's description)
New Design(moved to the issue description) |
A way to serialize a class without a root tag and just have everything be a flat compound tag would be nice. Using this in actual Minecraft modding I have run into many scenarios where I don't want a root tag because it gets embedded in another bit of nbt. |
I agree. Having like a |
Right now I do this NBT.encodeToNbtTag(serializer, input).nbtCompound[serializer.descriptor.serialName]!! It's kotlin, because I write my Minecraft mods in Kotlin, but having an |
I also have some stuff for converting between the knbt representation and the Mojmap representation if anyone needs that: (expand code)@OptIn(ExperimentalContracts::class)
inline fun minecraftNbtCompound(builderAction: NbtCompoundBuilder.() -> Unit): CompoundTag {
contract { callsInPlace(builderAction, InvocationKind.EXACTLY_ONCE) }
return buildNbtCompound(builderAction).toMinecraft
}
@OptIn(ExperimentalTypeInference::class, ExperimentalContracts::class)
inline fun <T : NbtTag> minecraftNbtList(
@BuilderInference builderAction: NbtListBuilder<T>.() -> Unit,
): ListTag {
contract { callsInPlace(builderAction, InvocationKind.EXACTLY_ONCE) }
return buildNbtList(builderAction).toMinecraft
}
val NbtTag?.toMinecraft: Tag
get() = when (this)
{
null -> EndTag.INSTANCE
is NbtByte -> ByteTag.valueOf(value)
is NbtByteArray -> ByteArrayTag(this)
is NbtCompound -> toMinecraft
is NbtDouble -> DoubleTag.valueOf(value)
is NbtFloat -> FloatTag.valueOf(value)
is NbtInt -> IntTag.valueOf(value)
is NbtIntArray -> IntArrayTag(this)
is NbtList<*> -> toMinecraft
is NbtLong -> LongTag.valueOf(value)
is NbtLongArray -> LongArrayTag(this)
is NbtShort -> ShortTag.valueOf(value)
is NbtString -> StringTag.valueOf(value)
}
val NbtCompound.toMinecraft: CompoundTag
get() = CompoundTag().apply {
mapValues { it.value.toMinecraft }.forEach { (key, value) ->
put(key, value)
}
}
val NbtList<*>.toMinecraft: ListTag
get() = ListTag().apply {
this@toMinecraft.map {
it.toMinecraft
}.forEach {
add(it)
}
}
val Tag.fromMinecraft: NbtTag?
get() = when (id.toInt())
{
0 -> null
1 -> NbtByte((this as NumericTag).asByte)
2 -> NbtShort((this as NumericTag).asShort)
3 -> NbtInt((this as NumericTag).asInt)
4 -> NbtLong((this as NumericTag).asLong)
5 -> NbtFloat((this as NumericTag).asFloat)
6 -> NbtDouble((this as NumericTag).asDouble)
7 -> NbtByteArray((this as ByteArrayTag).asByteArray)
8 -> NbtString(this.asString)
9 -> (this as ListTag).fromMinecraft
10 -> (this as CompoundTag).fromMinecraft
11 -> NbtIntArray((this as IntArrayTag).asIntArray)
12 -> NbtLongArray((this as LongArrayTag).asLongArray)
else -> throw IllegalStateException("Unknown tag type: $this")
}
val CompoundTag.fromMinecraft: NbtCompound
get() = buildNbtCompound {
this@fromMinecraft.allKeys.associateWith {
this@fromMinecraft[it]?.fromMinecraft
}.forEach { (key, value) ->
if (value != null)
put(key, value)
}
}
val ListTag.fromMinecraft: NbtList<*>?
get() = when (elementType.toInt())
{
0 -> null
1 -> buildNbtList {
forEach {
add(it.fromMinecraft as NbtByte)
}
}
2 -> buildNbtList {
forEach {
add(it.fromMinecraft as NbtShort)
}
}
3 -> buildNbtList {
forEach {
add(it.fromMinecraft as NbtInt)
}
}
4 -> buildNbtList {
forEach {
add(it.fromMinecraft as NbtLong)
}
}
5 -> buildNbtList {
forEach {
add(it.fromMinecraft as NbtFloat)
}
}
6 -> buildNbtList {
forEach {
add(it.fromMinecraft as NbtDouble)
}
}
7 -> buildNbtList {
forEach {
add(it.fromMinecraft as NbtByteArray)
}
}
8 -> buildNbtList {
forEach {
add(it.fromMinecraft as NbtString)
}
}
9 -> buildNbtList<NbtList<*>> {
forEach {
add(it.fromMinecraft as NbtList<*>)
}
}
10 -> buildNbtList {
forEach {
add(it.fromMinecraft as NbtCompound)
}
}
11 -> buildNbtList {
forEach {
add(it.fromMinecraft as NbtIntArray)
}
}
12 -> buildNbtList {
forEach {
add(it.fromMinecraft as NbtLongArray)
}
}
else -> throw IllegalStateException("Unknown tag type: $this")
} This is also when I realized that knbt doesn't have an equivalent of Minecraft's |
I also have some wrappers for using kotlinx serializers for Minecraft Codecs and Codecs as serializers (expand code)open class CodecSerializer<T>(private val codec: Codec<T>) : KSerializer<T>
{
override val descriptor: SerialDescriptor
get() = NbtTag.serializer().descriptor
override fun deserialize(decoder: Decoder): T
{
return codec.parse(NbtOps.INSTANCE, decoder.asNbtDecoder().decodeNbtTag().toMinecraft).orThrow
}
override fun serialize(encoder: Encoder, value: T)
{
encoder.asNbtEncoder().encodeNbtTag(codec.encodeStart(NbtOps.INSTANCE, value).orThrow.fromMinecraft!!)
}
}
inline fun <reified T> Codec<T>.serializer(): KSerializer<T> = CodecSerializer(this) @OptIn(ExperimentalSerializationApi::class)
open class KotlinCodec<X>(private val serializer: KSerializer<X>, private val hasRootTag: Boolean = false) : Codec<X>
{
override fun <T> encode(input: X, ops: DynamicOps<T>, prefix: T): DataResult<T>
{
val element = if (serializer.descriptor.kind == StructureKind.CLASS && !hasRootTag)
NBT.encodeToNbtTag(serializer, input).nbtCompound[serializer.descriptor.serialName]!!
else
NBT.encodeToNbtTag(serializer, input)
return DataResult.success(NbtOps.INSTANCE.convertTo(ops, element.toMinecraft))
}
override fun <T> decode(ops: DynamicOps<T>, input: T): DataResult<Pair<X, T>>
{
val element = NBT.decodeFromNbtTag(serializer, ops.convertTo(NbtOps.INSTANCE, input).fromMinecraft!!.let {
return@let if (serializer.descriptor.kind == StructureKind.CLASS && !hasRootTag)
buildNbtCompound {
put(serializer.descriptor.serialName, it)
}
else
it
})
return DataResult.success(Pair.of(element, ops.empty()))
}
}
@OptIn(ExperimentalSerializationApi::class)
open class KotlinMapCodec<X>(private val serializer: KSerializer<X>, private val hasRootTag: Boolean = false) : MapCodec<X>()
{
override fun <T> encode(input: X, ops: DynamicOps<T>, prefix: RecordBuilder<T>): RecordBuilder<T>
{
val element = if (serializer.descriptor.kind == StructureKind.CLASS && !hasRootTag)
NBT.encodeToNbtTag(serializer, input).nbtCompound[serializer.descriptor.serialName]!!
else
NBT.encodeToNbtTag(serializer, input)
require(element is NbtCompound) { "Class ${serializer.descriptor.serialName} does not serialize to a CompoundTag!" }
return ops.mapBuilder().apply {
element.nbtCompound.forEach { (key, value) ->
add(key, NbtOps.INSTANCE.convertTo(ops, value.toMinecraft))
}
}
}
override fun <T> keys(ops: DynamicOps<T>): Stream<T> =
serializer.descriptor.elementNames.map { ops.createString(it) }.stream()
override fun <T> decode(ops: DynamicOps<T>, input: MapLike<T>): DataResult<X>
{
val element = NBT.decodeFromNbtTag(serializer, NbtCompound(input.entries().toList().associate {
ops.convertTo(NbtOps.INSTANCE, it.first).fromMinecraft!!.nbtString.value to ops.convertTo(
NbtOps.INSTANCE,
it.second
).fromMinecraft!!
}).let {
return@let if (serializer.descriptor.kind == StructureKind.CLASS && !hasRootTag)
buildNbtCompound {
put(serializer.descriptor.serialName, it)
}
else
it
})
return DataResult.success(element)
}
}
inline fun <reified T> codec(): Codec<T> = KotlinCodec(serializer())
inline fun <reified T> mapCodec(): MapCodec<T> = KotlinMapCodec(serializer()) |
Probably not the best place to put this stuff, you should create a gist :) If I understand it correctly, the root tag is a feature of the binary format for type checking (you can't decode without it)? If so, then only the binary format encoders/decoders (e.g. Also are there any updates on the work? I'm open to doing some contributions. |
So, here's some background of the entire mess: The root tag is just a name for the outer-most NBT tag. Although the spec lightly implies that it should be a Compound, the way it is written allows it to be any type in essence. The spec itself is a mess, and was probably not thought of thoroughly back when it was defined, considering the recent changes. The spec defines NBT tags consisting of a name and its payload, with nameless tags being considered the edge-cases (such as the values in a nbt lists). However, it's easier if you think of things in reverse, that is, as named tags being the edge-case. That way, you leave names as being specific data of Compounds, pretty much just like how Json is structured (json objects contains key value pairs). Of course, the root tag becomes the outlier in this case, since it is also still named. Up until recently, that is how things worked, but on 23w31a the root tag name was completely dropped when encoding data on the network side, since it was completely pointless (the root tag name was always empty in every single case on the Vanilla implementations). Not only that, changes were made so any type of NBT tag can be serialized as-is. These recent changes made it so that the NBT format is structurally identical to the Json format (compounds are objects, strings are text, lists are arrays, and everything can be serialized as-is without any wrapping). |
Thanks for the input and use cases! I haven't read through all the new replies just yet, but I've got a new design mostly written up, and I'm going try to finish fleshing that out and update my last comment with it later today once I'm done. As far as what needs to be done, this issue as originally stated has already been implemented, but it still has some problems and pain points that should be addressed in the new design I'll post. After that, it shouldn't be too much to implement, since I'm expecting it to be more of a refactor effort with some small API changes/additions afterwards, being that it's a change to how NBT's modeled. So I'm not sure there's a good way to contribute, but I've got more free time now after just finishing some at-home construction the past few months, and I'll be posting updates here! v0.12 will have a good number of API changes addressing pain points of the library, and this issue is the last big thing in the way before I get to pushing it out the release. Possibly finishing up this issue within a month if all goes smoothly :) |
Updated! I'll read through the comments tomorrow when I get a chance :) |
The default root class nesting behavior will eventually be removed due to it being problematic for a number of reasons. (See #29) But until then, this provides a way to opt out of that behavior entirely.
Sorry for the late reply! The new design (in the issue description) gets rid of the root compound entirely, so Then because there's no root compound naming at all anymore, there's a new (and optional) Basically all the functions you're using now will just work, without needing to add any additional logic to deal with names. I'm going to start implementing this soon, so let me know how it sounds or if you have any questions/concerns about the new way serializing will work :) |
Awesome to hear that! I do have a question regarding the new approach though. Maybe we can even use this discussion to iron things out. Is your current idea:
It sounds kind of ambiguous to me at the moment. |
Nice! I look forward to bumping my Gradle dependency |
Nice job! Exactly what I wanted and need |
Good question! My current idea is mostly #1, with Here's what I have in mind for fun <T> encodeToNamedNbtTag(serializer: KSerializer<T>, value: T): NbtNamed<NbtTag> In this design, every serializable type is treated as having a NBT name (which root-named NBT variants use for the root value). This function is exactly the same as And in general, I think of Let me know if that clears things up a little! |
Problem
Currently (v0.11) classes are serialized within a single-entry NbtCompound, with the serial name as the key. But, only if serialized at the root.
This behavior is convenient, but has a couple issues:
@SerialName
doesn't support empty strings, which is common in NBT files.@NbtSerialName
annotation #21NbtContentPolymorphicSerializer
, because extra nesting is added for a class depending on if it's at the root, looking at the shape of the tag alone isn't enough, and properly accounting for it is undesirable complexity.Nbt.encodeToNbtTag(someClass)
andencoder.encodeSerializableElement(serializer, someClass)
don't produce the same result.Old redesign
This issue's initial redesign has been implemented, but is being replaced by the new design proposed in this comment
Original solution
NbtCompound
@NbtNamed
might be a good annotation name.@NbtRoot
and@NbtFile
have been used in earlier versions of knbt, but they seem too specificTasks
StructureKind.CLASS
/OBJECT
class
nesting using the serial nameStructureKind.MAP
StructureKind.LIST
New Design
This design aims to add a concept of NBT names that all serializable types have, with every type either:
The initial implementation will mainly focus on implementing static naming, since most use cases don't need logic around the serialized NBT names, and it's also not clear yet how an API for dynamic names should behave. See the dynamic names section below for details.
For the scope of this initial design, the NBT name only applies to the root tag name. Though in the future, especially with dynamic names, it's possible the NBT name could apply more broadly. (e.g. a value within an NBT compound knowing its own element name while deserializing)
Use Cases
These are use cases that are being designed for in this new approach
Default use, without using any named NBT API
@NbtName("")
Json
is strict by default, e.g. withisLenient
andignoreUnknownKeys
ignoreNbtName
configuration option or similarStatically setting the NBT root name for a type
@NbtName
(or including it in the serial descriptor annotations)Inspecting NBT (including root name) by decoding to an in-memory representation
Inspecting how data is encoded to NBT (including root name) through an in-memory representation
Named-root NBT variants
Only some variants of NBT have root names encoded. With this new design, serializing values should be the same between named and unnamed root variants (instead of named variants being modeled as nested in an NBT compound).
Unnamed root variants:
net.minecraft.data.Main
NBT -> SNBT conversion tool, since its output SNBT doesn't include the root name anywhere)NbtTag
sStatic NBT names for all serializable types
@NbtName
annotation, on classes or in serial descriptorsDynamically serializing NBT names, and future encoding/decoding API
Add a basic
NbtNamed
class for an in-memory representation of the (root) NBT value and its name.T
, since the newer Java Network NBT supportsnull
(TAG_End
) as a root valueNbtNamed
's serializer will be specially handled by the NBT encoder/decoder for nowNbtNamed
's serializerNbtNamed
should override that value's NBT nameNbtNamed
(directly, or indirectly when delegating serializers) should give precedence to the outermost dynamic nameCare was taken when deciding how this would work, making sure there's room for full-blown dynamically serialized names to be added later in a forward-compatible way.
NbtNamed
at the root for nowTAG_Compound
entries as being a list of named tags, so there is an interpretation where it makes sense for NBT names` to be used with nested tag entriesThe text was updated successfully, but these errors were encountered: