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

Re-design root class name NbtCompound-nesting functionality #29

Open
BenWoodworth opened this issue Apr 10, 2023 · 22 comments · May be fixed by #44
Open

Re-design root class name NbtCompound-nesting functionality #29

BenWoodworth opened this issue Apr 10, 2023 · 22 comments · May be fixed by #44
Assignees
Milestone

Comments

@BenWoodworth
Copy link
Owner

BenWoodworth commented Apr 10, 2023

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.
  • But more importantly, and the focus of this issue, the meaning of an NBT compound tag is different depending on the context.
    • A deserializer looking at an NbtCompound needs to know where in the serialization process it is to know how to deserialize the tag.
    • Specifically for a NbtContentPolymorphicSerializer, 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.
  • This behavior has also been the source of confusion, with it being unintuitive that e.g. Nbt.encodeToNbtTag(someClass) and encoder.encodeSerializableElement(serializer, someClass) don't produce the same result.
    • And, upon understanding the issue, it becomes an inconvenience that needs to be worked around. E.g. the v0.11 NbtTransformingSerializer implementation that needs to use internal API, and this code in slack that resorts to inspecting a NbtCompound before encoding

Old redesign

This issue's initial redesign has been implemented, but is being replaced by the new design proposed in this comment

Original solution
  • Remove the special conditional logic for adding serial names at the root, so data serializes the same regardless of where in the serialization process/data it is
    • This makes deserializing based on the structure more straightforward
  • Add an annotation that, when applied to a serial descriptor, instructs the serializer unconditionally to nest the data in a single-entry NbtCompound
    • This covers the use case of top-level NBT binary/files needing to be a named tag (single-element NbtCompound)
    • From the docs:

      An NBT file consists of a single GZIPped Named Tag of type TAG_Compound.

      • So @NbtNamed might be a good annotation name. @NbtRoot and @NbtFile have been used in earlier versions of knbt, but they seem too specific

Tasks

  • StructureKind.CLASS/OBJECT
    • Covers the previous default class nesting using the serial name
  • StructureKind.MAP
  • StructureKind.LIST

New Design

This design aims to add a concept of NBT names that all serializable types have, with every type either:

  • having a static name that can be easily annotated and optionally validated.
  • being dynamically named, giving full control to the serializer for encoding/decoding the name, and the full behavior around it.

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

  • All data implicitly has @NbtName("")
  • Decoding from non-empty named NBT will fail
    • Not lenient with mismatched names by default, similar to how Json is strict by default, e.g. with isLenient and ignoreUnknownKeys
    • Can be made lenient with a ignoreNbtName configuration option or similar

Statically setting the NBT root name for a type

  • Annotating a class/interface with @NbtName (or including it in the serial descriptor annotations)

Inspecting NBT (including root name) by decoding to an in-memory representation

val decodedNbt: NbtNamed<NbtTag> = nbt.decodeFromBufferedSource(source)

Inspecting how data is encoded to NBT (including root name) through an in-memory representation

val encodedNbt: NbtNamed<NbtTag> = nbt.encodeToNamedNbtTag(data)

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:

  • Java Network (starting from 1.20.2)
  • SNBT (Based on MC Java's net.minecraft.data.Main NBT -> SNBT conversion tool, since its output SNBT doesn't include the root name anywhere)
  • The in-memory representation with NbtTags

Static NBT names for all serializable types

  • Every serializable type has an NBT name
    • This means all serializers can be used without needing additional logic, notably serializers from other modules that can't be changed.
    • The name will be an empty string unless otherwise set.
      • Minecraft's implementation always uses empty strings, so it's unimportant for most uses
  • Denoted with a @NbtName annotation, on classes or in serial descriptors
  • Name is taken from the "outermost" value
    • When one serializer delegates to another, the first serializer's NBT name is used. That way names can be overwritten.
    • Conceptually, the NBT name of a TAG_Compound element is used over its value's NBT name
  • Used only when the serializable type is the root data of a named-root NBT variant
    • Ignored when the serializable type is an element of another
    • Ignored when used with the root value of an unnamed-root NBT variant
  • When decoding, serves to validate that the NBT has the expected name
    • Strict by default, failing if decoding a different name than expected
    • Can be disabled with a configuration option

Dynamically 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.

  •   class NbtNamed<T>(val name: String, val value: T)
    • Non-nullable name, since using this class implies that the serialized NBT is a named tag
    • Nullable T, since the newer Java Network NBT supports null (TAG_End) as a root value
  • Initially very restrictive, supporting basic root name-accessing functionality until it can better designed later on
    • NbtNamed's serializer will be specially handled by the NBT encoder/decoder for now
  • Serializers should be able to delegate to NbtNamed's serializer
    1. Expected behavior is the outer serializer effectively having a dynamic NBT name.
    2. Because of this, the outer serializer should not validate against its static name.
    3. That means the writing/validating of the NBT name should be held off until a value is actually decoded/encoded
      • Since, until then, it's impossible to know if another serializer will be delegated to
  • Wrapping any value with NbtNamed should override that value's NBT name
    • This means wrapping another NbtNamed (directly, or indirectly when delegating serializers) should give precedence to the outermost dynamic name

Care 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.

  • Only support NbtNamed at the root for now
    • The NBT spec describes TAG_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 entries
    • Potential problem: serializers that start a new serialization, like NbtTransformingSerializer that can serialize a nested value as root NBT before re-serializing
  • Fail if used with unnamed root NBT variants
    • It's not clear how encoding an NBT name where there isn't one should work. Discard it? Fail?
    • It's also not clear how decoding a non-existent name should work. Empty string? Fail?
@WinX64
Copy link

WinX64 commented Oct 5, 2023

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.

@BenWoodworth
Copy link
Owner Author

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()
}

@BenWoodworth
Copy link
Owner Author

What is interesting now though. I was debating between @NbtNamed and @NbtRoot and landed on @NbtNamed. But with #36 being a thing now, I'm probably going to go with @NbtRoot instead since that breaks my assumption about a named tag (which is a compound with one named entry according to the spec) and the root being the same thing.

@WinX64
Copy link

WinX64 commented Oct 8, 2023

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.

@BenWoodworth
Copy link
Owner Author

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.

@BenWoodworth
Copy link
Owner Author

And I also 100% agree that it's cumbersome. The RootStringSerializer example just serves to demonstrate it's possible with the one NbtNamed line in the current encoding. And if needed, it can all be neatly wrapped into a general class from knbt, e.g. NbtNamedValue<String>("name", "value")

@WinX64
Copy link

WinX64 commented Oct 9, 2023

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.

BenWoodworth added a commit that referenced this issue Apr 11, 2024
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.
@BenWoodworth
Copy link
Owner Author

BenWoodworth commented Apr 16, 2024

Partially in reply to your comment on #36:

As for the named tag, it was more of a suggestion. If you already considered it at some point, and still decided doing it another way, I don't see any point on insisting on it.

I've thought about this more than I'd like to admit (😅), wanting to find a solution for:

  • the awkward named-tags-are-single-entry-compounds convention
  • still having the in-memory NbtTag representation fully capture/mirror the serialized NBT with named root tags
  • named tags not being seamless to work with in general
  • still giving full control to tag naming for those who want it

And I had a couple misconceptions when I originally considered how names should be modeled (and in all my messages before this):

  • I thought the NBT spec was where the root-tag-is-a-single-entry-compound convention came from
    • I think I actually got it from the wiki.vg article:

      Every NBT file will always implicitly be inside a tag compound, [...]

    • And may have confused this with it, from the NBT spec:

      An NBT file consists of a single GZIPped Named Tag of type TAG_Compound

  • I also thought that root NBT names were actually used by Minecraft
    • With test.nbt and bigtest.nbt, which I knew had root tag names.
    • But I also had it in my head that files like level.dat had non-empty named tags. But with level.dat for example, I thought the root name was "Data" and never questioned it. But actually, "Data" is actually a lone entry inside the root compound, so this with the single-entry-compound convention: {"": {Data: {...}}}

A new approach that I think will work

(Which replaces the approach outlined in this issue's description)

  • @NbtName annotation in serial descriptors
    • Everything without an explicit NbtName will have an empty name by default
    • Similar to @SerialName, both with the annotation's naming, and with how a class's @NbtName will be ignored when the class is serialized as a part of some other data
      • Instead of @NbtNamed, since everything will already be treated as named.
      • Also works nicely with the NbtNamed class below, since the annotation/class can't be named the same
    • Nameless NBT variants can be treated as having an implicitly empty name
  • NbtNamed<T>(name, data) class to override the data's name
    • Provides a concise way to replace what the name is when serializing
      • Specifically, its serializer is a copy of data's serializer, just adding/replacing its NbtName annotation and changing its serial name, and delegating everything else
    • Makes it possible to retrieve the (potentially unknown) root name when decoding
    • Allows the NbtTag hierarchy to be root-name-aware for those who want it, potentially with encodeTo/decodeFromNamedNbtTag() functions
    • Removing the single-entry-root-tag convention
  • Potential root name validation when serializing
    • Decoding a different root name than expected can throw a serialization exception
    • Encoding a non-empty root name for unnamed formats can also throw an exception
    • Configurable, so it can be strict, or also lenient like how Minecraft Java discards/ignores the root name

New Design

(moved to the issue description)

@KP2048
Copy link

KP2048 commented Apr 30, 2024

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.

@pandier
Copy link

pandier commented May 4, 2024

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 encodeToByteArrayFlat or encodeToByteArrayRaw that would just encode the payload without the root tag would be useful.

@KP2048
Copy link

KP2048 commented May 4, 2024

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 encodeToNbtTagFlat would be awesome

@KP2048
Copy link

KP2048 commented May 4, 2024

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 EndTag, so I had to use null. Unfortunately you can't have a null value in list and compound tags in knbt, so some data loss occurs

@KP2048
Copy link

KP2048 commented May 4, 2024

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())

@pandier
Copy link

pandier commented May 5, 2024

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. encodeToStream) need to take the root tag into account (wrapping everything including primitives and NbtTag in a root tag). Methods like encodeToNbtTag or decodeFromString that don't use the binary format could just do without the root tag. You could also have the flat methods for binary format as mentioned earlier for encoding just the payload without the root tag. Decoding flat binary data would be unsupported.

Also are there any updates on the work? I'm open to doing some contributions.

@WinX64
Copy link

WinX64 commented May 5, 2024

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. encodeToStream) need to take the root tag into account (wrapping everything including primitives and NbtTag in a root tag). Methods like encodeToNbtTag or decodeFromString that don't use the binary format could just do without the root tag. You could also have the flat methods for binary format as mentioned earlier for encoding just the payload without the root tag. Decoding flat binary data would be unsupported.

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).

@BenWoodworth
Copy link
Owner Author

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 :)

@BenWoodworth
Copy link
Owner Author

Updated! I'll read through the comments tomorrow when I get a chance :)

BenWoodworth added a commit that referenced this issue May 7, 2024
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.
@BenWoodworth
Copy link
Owner Author

BenWoodworth commented May 10, 2024

@KP2048 (comment):

A way to serialize a class without a root tag and just have everything be a flat compound tag would be nice. [...]

@pandier (comment):

I agree. Having like a encodeToByteArrayFlat or encodeToByteArrayRaw that would just encode the payload without the root tag would be useful.

@KP2048 (comment):

Right now I do this

NBT.encodeToNbtTag(serializer, input).nbtCompound[serializer.descriptor.serialName]!!

[...], but having an encodeToNbtTagFlat would be awesome

Sorry for the late reply! The new design (in the issue description) gets rid of the root compound entirely, so encodeToNbtTag/encodeToByteArray/etc. work exactly the way you want. Nameless, "raw", and no root compound to un-nest.

Then because there's no root compound naming at all anymore, there's a new (and optional) NbtNamed("name", value) for people who do still need the root name. For that, there will be encodeToNamedNbtTag, and also using NbtNamed with the existing encodeToByteArray/etc. functions.

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 :)

@WinX64
Copy link

WinX64 commented May 10, 2024

Sorry for the late reply! The new design (in the issue description) gets rid of the root compound entirely, so encodeToNbtTag/encodeToByteArray/etc. work exactly the way you want. Nameless, "raw", and no root compound to un-nest.

Then because there's no root compound naming at all anymore, there's a new (and optional) NbtNamed("name", value) for people who do still need the root name. For that, there will be encodeToNamedNbtTag, and also using NbtNamed with the existing encodeToByteArray/etc. functions.

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:

  1. to have NbtNamed as a wrapper of sorts, with custom serialization logic to be handled internally by the encoder/decoder? In this case, the object could be passed directly to encodeToByteArray/etc, without the need of a specific encodeToNamedNbtTag method.
  2. to have the serialization logic be handled on encodeToNamedNbtTag? In this case, the input of the method could be (name: String, payload: T), without the need for the NbtNamed wrapper.
  3. something entirely different from what I mentioned above?

It sounds kind of ambiguous to me at the moment.

@KP2048
Copy link

KP2048 commented May 10, 2024

Nice! I look forward to bumping my Gradle dependency

@pandier
Copy link

pandier commented May 10, 2024

Nice job! Exactly what I wanted and need

@BenWoodworth
Copy link
Owner Author

@WinX64 (comment):

I do have a question regarding the new approach though. Maybe we can even use this discussion to iron things out. [...]

Good question! My current idea is mostly #1, with NbtNamed being a way to change the name of the payload when encoding.

Here's what I have in mind for encodeToNamedNbtTag:

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 encodeToNbtTag, except it also captures value's NBT name. So this function gives a more complete in-memory representation of how value is serialized, instead of missing the name.

And in general, I think of NbtNamed(name, value) as effectively replacing its value's NBT name. Unless a serializable type is (statically) annotated with @NbtName, its name will be an empty string by default. And NbtNamed offers a way to (dynamically) change a value's NBT name. And with the NbtNamed<NbtTag> return type, it replaces NbtTag's empty name with value's.

Let me know if that clears things up a little!

@BenWoodworth BenWoodworth linked a pull request May 16, 2024 that will close this issue
46 tasks
@BenWoodworth BenWoodworth linked a pull request May 16, 2024 that will close this issue
46 tasks
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 a pull request may close this issue.

4 participants