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

Implement the NBT name redesign #44

Draft
wants to merge 119 commits into
base: 0.12
Choose a base branch
from
Draft

Conversation

BenWoodworth
Copy link
Owner

@BenWoodworth BenWoodworth commented May 16, 2024

See #29

Tasks:

  • Update dependencies
    • Kotlin 2.0
    • kotlinx-serialization
    • okio
  • Refactor parameterized testing
    • Replace Kotest's property testing with Parameterize
    • Tidy up test suite / test data
  • Port over recent changes from v0.11
    • LGPL
    • "master" branch renamed to "main"
    • Shared NbtFormatBuilder interface
    • parameterizedNbtFormat() testing function
    • Builtin polymorphic serializer support
    • ...
  • General
    • Split up list-like deprecation interfaces
      • The NbtList/Arrays all sharing another interface causes issues with type inference. e.g. listOf(NbtList, NbtByteArray) is inferred as List<Any> instead of List<NbtTag>
    • Implement Back Nbt*Arrays with Lists, and encode arrays as NbtList by default, and require @NbtArray #34, fixing the problem with elements whose serializers delegate to Byte/Int/LongArrays (see here)
      • Remove fast-path optimization with array types for now, as it's making it more difficult to iterate on the implementation. (See b8d52f4 which implemented checks against descriptors instead of checking that the serializer is an instance of *ArraySerializer
  • Gradually implement new design, ideally keeping all tests valid between each change
    • Rename @NbtNamed to @NbtName, retaining the existing NBT names on test classes that have them
    • Refactor encoder/decoder-verifying testing so it will better cover named/unnamed and nullabe/non-nullable NBT formats
    • Add the NbtNamed<T> class next, that way the later changes have something to migrate to when moving away from the single-entry-root-compound name representation
      • Add convenience properties for NbtNamed<NbtTag> to match those on NbtTag, so that dealing with named tags is more seamless when the name is unimportant. E.g. namedTag.nbtInt
    • Add encodeTo/decodeFromNamedNbtTag() functions, so tests have a way to check against the serialized representation
    • Refactor general NBT serialization checks so they can be shared between the encoder and decoder
    • Add NbtName.Dynamic that disables root name validation for a type (and later allows name to be serialized dynamically with encoder/decoder functions)
      • Add checks that require @NbtName.Dynamic when delegating to another dynamic serializer
      • For consistency during the design change, all descriptors that have Dynamic will be considered to have an NbtName ("" by default). As opposed to requiring @NbtName or not being named until the name is dynamically encoded, since all types will eventually be implicitly NbtNamed anyway)
    • Make @NbtName work on any serializable type
      • A lot of the codebase assumes it's only used on conpounds/classes, so a lot of (related/unrelated) tests fail when broadening them to any type. Will do with care and comb through the code to check everything over.
    • Make the new NbtNamed class serializable. Done after implementing it as an encode/decode representation because those will be necessary for testing its serialization
    • Change buildNbtCompound(name) and buildNbtList(name) functions to return NbtNamed
    • Change @NbtName to only nest under the name for the root value
      • Since the name will eventually only apply to the root value
    • Remove the single-entry-root-compound name representation when encoding to NbtTags, binary, and snbt
      • @NbtName docs, which says it instructs the serialization to nest within an NBT compound so it's named
      • JavaNetwork docs: "Serializes unnamed NbtTags for use in network packets"
        • can remove mention of "unnamed", since there is no longer a difference in behavior with this design. (It was special because it didn't use an outer nbt compound for the internal representation, but now that's the way they all work)
      • Readme examples
    • Make all serializable types have an NBT name, defaulting to an empty name if not marked with @NbtName
    • lenientNbtName configuration option, and name validation
      • Apply to empty-named JavaNetwork, which gracefully discards the name currently
    • Replace NbtVariant with specialized NbtFormat implementations (JavaNbt, JavaNetworkNbt(val protocolVersion), etc.)
      • With some variants being named, and with some having nullable roots, it doesn't make sense for all the variants to share the Nbt class. Requiring that they all have the same encodeToNbtTag() is forcing an abstraction. In reality, they are all (slightly) different formats, so should each be its own NbtFormat implementation (with all the same binary serialization methods, but different in-memory NbtTag serialization methods)
      • Additional in-memory NbtFormat providing encodeTo/decodeFromNbtTag with all NBT capabilities (named root, nullable root, special floating point values, etc) that fully captures how a class would serialize to NBT, not just how a specific limited variant would be serialized. This is useful for testing/experimenting by library users, but also necessary for NbtTransformingSerializer so serializer's using it aren't unnecessarily limited by a specific NBT format.
    • Allow serializing null as the root tag in NBT formats that support it (Java network)
    • ...
    • Code cleanup
    • Documentation (in readme, doc comments, etc.)
      • Text
      • Code examples

@BenWoodworth BenWoodworth linked an issue May 16, 2024 that may be closed by this pull request
@BenWoodworth BenWoodworth force-pushed the 0.12-nbt-name-redesign branch 4 times, most recently from 41b70f2 to ccca50f Compare May 23, 2024 20:42
Originally the GPL 3.0 license was decided on so that any improvements
or derivative projects that benefit from this project would also have to
their source be shared and open.

However, GPL imposes itself even if this library is only being used as a
dependency. LGPL would've been more in line with the original intent
for the license, since it more so applies to modification of the
project/binaries, and doesn't impose itself on other projects when used
as a dependency.

(cherry picked from commit 0d5cf0b)
Both use Node 16, and had warnings about it being deprecated.

(cherry picked from commit 17412e3)
(cherry picked from commit 6f73e0c)
The new GitHub macOS host uses ARM, and the Adopt/Temurin JDKs no longer work since they aren't built for that architecture until Java 11.

See: actions/setup-java#625

(cherry picked from commit 6580e06)
@BenWoodworth BenWoodworth force-pushed the 0.12-nbt-name-redesign branch 2 times, most recently from 84865c8 to 4d49885 Compare May 29, 2024 01:00
It has been a frequent cause of issues while rebasing by frequently
changed without adding anything of substance, and hasn't been
contributing any value to the project.
@BenWoodworth BenWoodworth force-pushed the 0.12-nbt-name-redesign branch 5 times, most recently from f2b06bb to 1119e71 Compare June 6, 2024 19:11
@BenWoodworth BenWoodworth force-pushed the 0.12-nbt-name-redesign branch 4 times, most recently from ac18958 to 7050a30 Compare June 17, 2024 06:04
@BenWoodworth BenWoodworth force-pushed the 0.12-nbt-name-redesign branch 6 times, most recently from a066834 to 9caf9f7 Compare August 10, 2024 17:08
Makes it possible to simplify `VerifyingNbtReader` and, then also the
implementation of `parameterOfVerifyingNbt` since the
verifying readers/writers can all be constructed the same from the
`capabilities` that are already provided.

In the future, this can also be used when implementing
`decodeSequentially`.
Will also be used for the root tag, since its internal representation is
going to be a named tag instead of a nested compound.
These were originally added for symmetry with JSON's API. However,
the JSON convenience properties are on `JsonPrimitive` instead of
`JsonElement`, and are there out of necessity because the underlying
type is unspecific and obscured by `JsonPrimitive`. That's not the case
for NBT, so these don't provide much value.
Equivalent to the ones that exist for `NbtTag`, making it more seamless
to work with named tags when the name is unimportant.
All serializable types now have an `NbtName`, defaulting to an empty
name.

Simplify naming code by switching away from single-entry-compound code
with verification.
All serializable types now have an `NbtName`, defaulting to an empty
name.

Simplify naming code by switching away from single-entry-compound code
with verification.
All serializable types now have an `NbtName`, defaulting to an empty
name.

Simplify naming code by switching away from single-entry-compound code
with verification.
All serializable types now have an `NbtName`, defaulting to an empty
name.

Simplify naming code by switching away from single-entry-compound code
with verification.

SNBT now doesn't print root name since it's an unnamed variant, which
matches the behavior of Minecraft Java's net.minecraft.data.Main
NBT -> SNBT data generator tool with the --dev flag.
All serializable types now have an `NbtName`, defaulting to an empty
name.

Simplify naming code by switching away from single-entry-compound code
with verification.

SNBT now doesn't print root name since it's an unnamed variant, which
matches the behavior of Minecraft Java's net.minecraft.data.Main
NBT -> SNBT data generator tool with the --dev flag.
Simplify naming code by switching away from single-entry-compound code
with verification.

SNBT now doesn't print root name since it's an unnamed variant, which
matches the behavior of Minecraft Java's net.minecraft.data.Main
NBT -> SNBT data generator tool with the --dev flag.
TODO: Document in @NbtName.

Types that don't have an explicit `@NbtName` will default to having
an empty name.

The root name is largely redundant, since Minecraft's implementation
entirely discards it and just uses an empty string when writing NBT.
Giving all types a default name matches Minecraft's behavior, makes
serializing with named NBT formats more straightforward for those who
don't care about the NBT name, and makes any (3rd-party) serializable
type compatible with named NBT out of the box.

Tests can now all be run against named-root `VerifyingNbt`, since tests
values can now work in named NBT without needing an `NbtName` to be
provided.
Element was accurate when copying from `JsonTransformingSerializer`,
since it worked with `JsonElement`s, but should've been changed when
adapting the serializer to NBT.
There were design issues around providing an `NbtFormat` instance as the
default since not all the formats *can* have a default instance.
Previously this was worked around by having an illegal instance with
values that would be explicitly ignored by the builder using a
reference check.

The previous approach feels like a code smell, treating an incompletely
configured instance as if it were complete when passing it as a default,
while requiring special treatment for the "default" instance since
it isn't a behavioral subtype semantically.

With more specific `NbtFormat` subtypes coming later on, this new
approach with objects that provide the defaults should be more wieldy to
implement and maintain. The `Default` implementations that required
special handling have been removed, and there is now a single source of
truth for each option's default value.
Will be split into different implementations for each NBT variant, since
each will have slightly different APIs e.g. with Java requiring a named
NbtCompound at the root, JavaNetwork supporting unnamed nullable tags at
the root, etc.
Does not have any affect currently, but safeguards against format
changes being introduced in the future.
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 this pull request may close these issues.

Re-design root class name NbtCompound-nesting functionality
2 participants