[21.1+] Data Attachments Codec Support #1163

robotgryphon · 2024-06-22T12:06:07Z

This PR aims to add full support for codecs to the data attachment system, since Mojang is moving away from things like NBT Serializable and towards codecs and stream codecs.

The initial phase is to phase out and replace existing usages of pure NBT code, in favor of supporting codecs. It serves as a foundation for Stream codec support and sync-ready attachments.

Attachment Holder Extensions

IAttachmentHolderExtension is an extension interface which forwards IAttachmentHolder API to an exposed dataAttachments() reference. Using it means that any implementers that wish to have common attachment API simply need to expose an attachment holder, and the interfaces will do the rest.

By using this extension interface, it was possible to reduce several patch files by a considerable amount.

Attachment Holders

Makes AttachmentHolder final, and buildable via a codec.
Modifies serialization of attachment holders to use codecs.
Removes AttachmentHolder.AsField -- this is now the standard implementation.

Internals/Documentation

Modifies AttachmentInternals#copyAttachments to loosen from to only require the interface.
Cleans javadoc for AttachmentType to no longer reference ItemStacks, as items now use data components.

Attachment Types

Attachment types no longer support INBTSerializable, due to the shift to codecs. This will need to be weighed whether it can be fully removed or if providing a wrapper via CompoundTag.CODEC is going to be necessary.

In return, the attachment type builder has added functionality to specify a different deserializer than the serializer.

Examples

Replacements for IAttachmentSerializer/Re-capturing a "holder" reference

There is no direct replacement for manually serializing NBT data; you must switch to a codec. If a holder/parent context is needed, it is attached automatically when the attachment is created or initialized from a codec read.

// MyAttachment here is a player attachment for storing some amount of data
AttachmentType<MyAttachment> MY_ATTACHMENT = AttachmentType
    .builder((holder) -> new MyAttachment((Player) holder))
    .serialize(MyAttachment.CODEC)
    .build();

Conditionally serializing data

If one needs to NOT serialize something with a codec, such as in the case of a "default" state, use the serialize(Codec, Predicate) override:

class TrinketInventory extends ItemStackHandler {
    public final Codec<TrinketInventory> CODEC = // ...

    public TrinketInventory(int slotCount) {
        super(slotCount);
    }
    
    /**
     * Returns true if all slots are empty.
     */
    public boolean isEmpty() {
        return stacks.stream().allMatch(ItemStack::isEmpty);
    }
}

final AttachmentType<TrinketInventory> MY_ATTACHMENT = AttachmentType
    .builder(() -> new TrinketInventory(5))
    .serialize(TrinketInventory.CODEC, Predicates.not(TrinketInventory::isEmpty))
    .build();

Specifying a different deserializer than the serializer

Should you need to, you can override the default deserializer for attachment types.
This is done by overriding the deserialize(Decoder<T>) function, which takes either a codec or a decoder instance. Note that if the serializer is NOT set when this is called, it will throw an exception.

final AttachmentType<TrinketInventory> MY_ATTACHMENT = AttachmentType
    .builder(() -> new TrinketInventory(5))
    .serialize(TrinketInventory.CODEC, Predicates.not(TrinketInventory::isEmpty))
    .deserialize(TrinketInventory.NON_EMPTY_CODEC)
    .build();

…der for AttachmentHolder

neoforged-pr-publishing · 2024-06-22T12:06:21Z

Publish PR to GitHub Packages

Last commit published: a80233e95d6485bde3db4ba57229444d4b63da54.

PR Publishing

The artifacts published by this PR:

Repository Declaration

In order to use the artifacts published by the PR, add the following repository to your buildscript:

repositories {
    maven {
        name 'Maven for PR #1163' // https://github.com/neoforged/NeoForge/pull/1163
        url 'https://prmaven.neoforged.net/NeoForge/pr1163'
        content {
            includeModule('net.neoforged', 'testframework')
            includeModule('net.neoforged', 'neoforge')
        }
    }
}

MDK installation

In order to setup a MDK using the latest PR version, run the following commands in a terminal.
The script works on both *nix and Windows as long as you have the JDK bin folder on the path.
The script will clone the MDK in a folder named NeoForge-pr1163.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr1163
cd NeoForge-pr1163
curl -L https://prmaven.neoforged.net/NeoForge/pr1163/net/neoforged/neoforge/21.0.61-beta-pr-1163-feature-codec-attachments/mdk-pr1163.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zip

To test a production environment, you can download the installer from here.

Technici4n

IAttachmentSerializer provides important functionality that cannot be achieved with codecs:

The ability to capture the IAttachmentHolder instance.
The ability to return null, thereby skipping writing the entry entirely.

On top of that, I don't see the reason to move away from NBT serialization as long as Mojang continues to do it. Codecs can be used just fine already.

robotgryphon · 2024-06-22T12:33:45Z

IAttachmentSerializer provides important functionality that cannot be achieved with codecs:

The ability to capture the IAttachmentHolder instance.

You can still capture the holder -- the syntax has changed slightly but gained more power as a trade-off (#deserialize(CODEC, postDeserialize) functions) -- see tests

The ability to return null, thereby skipping writing the entry entirely.

Why would you even want to return null during serialization? Just.. don't serialize then.

On top of that, I don't see the reason to move away from NBT serialization as long as Mojang continues to do it. Codecs can be used just fine already.

Stream Codecs, Syncing over the network, Data Components are already moving that way...

Technici4n · 2024-06-22T13:58:01Z

You can still capture the holder -- the syntax has changed slightly but gained more power as a trade-off (#deserialize(CODEC, postDeserialize) functions) -- see tests

Okay. I think it's a bit awkward personally but there might not be a better way with codecs.

Why would you even want to return null during serialization? Just.. don't serialize then.

The use case was not serializing some sort of default instance. @pupnewfster Do you still use this functionality?

Stream Codecs, Syncing over the network, Data Components are already moving that way...

Let's not move attachments to that until the objects that they are attached to (block entities, chunks, entities, levels) are themselves moved over.

As a side note, I have the impression that this PR only benefits people that make their own attachment holders, and want to serialize them to codecs. To make that use case slightly easier, you're asking everyone who is currently serializing to NBT to move to codecs. That is not fine by me.

robotgryphon · 2024-06-22T14:16:06Z

The use case was not serializing some sort of default instance. @pupnewfster Do you still use this functionality?

If you need to control this, serialize has a second method to take in a predicate for that. It's checked in AttachmentHolder#42 before serializing an attachment.

Let's not move attachments to that until the objects that they are attached to (block entities, chunks, entities, levels) are themselves moved over.

As a side note, I have the impression that this PR only benefits people that make their own attachment holders, and want to serialize them to codecs. To make that use case slightly easier, you're asking everyone who is currently serializing to NBT to move to codecs. That is not fine by me.

Difference of opinion here. I think this is laying foundation work to better integrate with other features of vanilla, and prepare for the future. It's not a particularly large shift in thinking.

pupnewfster · 2024-06-22T16:16:00Z

Two things in regards to my usage:

When I made the PR initially that added the nullable, I also added the should serialize version for the codec constructor.
I don't really use attachments anymore except for two cases, as the majority of my attachments were for items, which now use data components instead.

neoforged-compatibility-checks · 2024-06-24T17:07:57Z

@robotgryphon, this PR introduces breaking changes.
Fortunately, this project is currently accepting breaking changes, but if they are not intentional, please revert them.
Last checked commit: a80233e95d6485bde3db4ba57229444d4b63da54.

Compatibility checks

`neoforge (:neoforge)`

net/minecraft/world/level/Level
- ❗ Class missing superclass of net/neoforged/neoforge/attachment/AttachmentHolder
net/neoforged/neoforge/attachment/AttachmentType
- serializable(Ljava/util/function/Supplier;)Lnet/neoforged/neoforge/attachment/AttachmentType$Builder;: ❗ API method was removed
- serializable(Ljava/util/function/Function;)Lnet/neoforged/neoforge/attachment/AttachmentType$Builder;: ❗ API method was removed
net/minecraft/world/level/chunk/ChunkAccess
- writeAttachmentsToNBT(Lnet/minecraft/core/HolderLookup$Provider;)Lnet/minecraft/nbt/CompoundTag;: ⚠ API method was removed
- getAttachmentHolder()Lnet/neoforged/neoforge/attachment/AttachmentHolder$AsField;: ⚠ API method was removed
- readAttachmentsFromNBT(Lnet/minecraft/core/HolderLookup$Provider;Lnet/minecraft/nbt/CompoundTag;)V: ⚠ API method was removed
net/neoforged/neoforge/attachment/AttachmentHolder
- ❗ Class was made final
net/neoforged/neoforge/attachment/IAttachmentCopyHandler
- ❗ API class no longer exists
net/neoforged/neoforge/attachment/AttachmentType$Builder
- serialize(Lnet/neoforged/neoforge/attachment/IAttachmentSerializer;)Lnet/neoforged/neoforge/attachment/AttachmentType$Builder;: ❗ API method was removed
- serialize(Lcom/mojang/serialization/Codec;)Lnet/neoforged/neoforge/attachment/AttachmentType$Builder;: ❗ API method was removed
- copyHandler(Lnet/neoforged/neoforge/attachment/IAttachmentCopyHandler;)Lnet/neoforged/neoforge/attachment/AttachmentType$Builder;: ❗ API method was removed
net/minecraft/world/level/block/entity/BlockEntity
- ❗ Class missing superclass of net/neoforged/neoforge/attachment/AttachmentHolder
net/neoforged/neoforge/attachment/IAttachmentHolder
- parent()Ljava/lang/Object;: ❗ Method was made abstract
- existingDataTypes()Ljava/util/stream/Stream;: ❗ Method was made abstract
net/minecraft/world/entity/Entity
- ❗ Class missing superclass of net/neoforged/neoforge/attachment/AttachmentHolder
net/minecraft/world/level/chunk/ImposterProtoChunk
- getAttachmentHolder()Lnet/neoforged/neoforge/attachment/AttachmentHolder$AsField;: ❗ API method was removed
net/neoforged/neoforge/attachment/IAttachmentSerializer
- ❗ API class no longer exists
net/neoforged/neoforge/attachment/AttachmentInternals
- copyChunkAttachmentsOnPromotion(Lnet/minecraft/core/HolderLookup$Provider;Lnet/neoforged/neoforge/attachment/AttachmentHolder$AsField;Lnet/neoforged/neoforge/attachment/AttachmentHolder$AsField;)V: ⚠ API method was removed
- copyEntityAttachments(Lnet/minecraft/world/entity/Entity;Lnet/minecraft/world/entity/Entity;Z)V: ⚠ API method was removed
net/neoforged/neoforge/attachment/AttachmentHolder$AsField
- ❗ API class no longer exists

…mentOps

…tachments

lukebemish

Seems to have a bunch of uses of TypeResolver which don't actually seem to be necessary...? The ones on Function especially need to be removed, cause that's an easy case where modders may have stuff nested in a utility function that ends up making the genercis unrecoverable for typetools. Basically, you can do all the same type safety and whatnot that the use of TypeResolver seems to be meant to provide, without using TypeResolver at all -- just toss out the fields you collect but never use, and pass in the one remaining type you're missing in the AttachmentHolder constructor.

src/main/java/net/neoforged/neoforge/attachment/AttachmentHolder.java

src/main/java/net/neoforged/neoforge/attachment/AttachmentType.java

src/main/java/net/neoforged/neoforge/attachment/DataAttachmentOps.java

src/main/java/net/neoforged/neoforge/codec/NBTSerializableCodec.java

src/main/java/net/neoforged/neoforge/common/util/INBTSerializable.java

src/main/java/net/neoforged/neoforge/attachment/DataAttachmentOps.java

lukebemish · 2024-06-27T02:04:11Z

src/main/java/net/neoforged/neoforge/attachment/AttachmentType.java

-                return attachment.serializeNBT(provider);
-            }
-        });
+    public static <T, P extends IAttachmentHolder> Builder<T, P> builder(Class<P> parentClass, Function<P, T> defaultValueConstructor) {


This overload with the parentClass arg is entirely unnecessary. For example, where someone might use

AttachmentType.builder(ChunkAccess.class, chunk -> new ChunkMutableInt(chunk, 0))

With this overload, they can instead just use

AttachmentType.builder((ChunkAccess chunk) -> new ChunkMutableInt(chunk, 0))

With the normal overload and all the type inference will just work -- I just tested this to be sure. So I'd toss this one out entirely

lukebemish · 2024-06-27T02:06:14Z

src/main/java/net/neoforged/neoforge/attachment/AttachmentType.java

-        public Builder<T> serialize(Codec<T> codec) {
-            return serialize(codec, Predicates.alwaysTrue());
+        public Builder<T, P> deserialize(Decoder<T> deserializeCodec) {
+            if (this.serializer == null)


Why does this have to be called after serialize? That seems rather weird. Just have it throw on build if the deserializer is set but not the serializer.

* Simplify NBTSerializableCodec * Make DataAttachmentOps work with registries (in theory)

…tachments

robotgryphon · 2024-07-09T15:24:44Z

Currently working on the stream codec support and sane typing. Turning this back into a draft PR for now while things are in heavy flux again.

Technici4n · 2024-07-10T16:41:13Z

I am still not sold on this PR. I would not invest too much effort in it until there is consensus that it is wanted in the first place.

robotgryphon · 2024-07-10T18:19:38Z

Tech, -I- want this support in Neo, since I want better serialization and synced data attachments. That's enough for me to keep working on it.

Technici4n · 2024-07-10T18:26:54Z

better serialization

I would prefer to focus on the specific features that you need as a user of the API, rather than changing all of the internals. Which feature is currently missing, and why? If it is being able to serialize an AttachmentHolder using codecs, then let's make a codec that just wraps the NBT-based internals, rather than rewriting all of the internals.

synced data attachments

I am absolutely in favor of that. That is however a matter of stream codecs - at least in the core API, and therefore seems unrelated to this PR. Some helper to use Codecs for syncing can be added on top, once again without rewriting all of the internals.

neoforged-automation · 2024-08-07T22:09:47Z

@robotgryphon, this pull request has conflicts, please resolve them for this PR to move forward.

Initial changes - remove IAttachmentSerializer, and introduce an enco…

6fb1668

…der for AttachmentHolder

robotgryphon added 1.21 Targeted at Minecraft 1.21 breaking change Breaks binary compatibility enhancement New (or improvement to existing) feature or request labels Jun 22, 2024

robotgryphon mentioned this pull request Jun 22, 2024

[1.21/1.22] Attachment Holders and Codecs #1160

Open

Technici4n requested changes Jun 22, 2024

View reviewed changes

robotgryphon added 2 commits June 22, 2024 13:53

Most of the switch-over and superclass removals

42d2da0

Move AttachmentHolderCodec to dedicated class, fix tests not compiling

8088404

robotgryphon marked this pull request as ready for review June 22, 2024 18:26

robotgryphon requested a review from Technici4n June 22, 2024 18:29

robotgryphon added 3 commits June 24, 2024 01:02

Adds NBT Serializer codec and stream codec; documentation

db3aee2

Integrate upstream changes

c5d3653

Strongly typed attachment holders, more postDeserialize testing

2bac4a0

robotgryphon mentioned this pull request Jun 24, 2024

Add several codecs for existing capabilities. Add NbtListCollector. #1035

Closed

Matyrobbrt and others added 2 commits June 24, 2024 19:58

Merge branch '1.21.x' into feature/codec-attachments

bafa2fc

Merge branch '1.21.x' into feature/codec-attachments

c3c0cd1

robotgryphon added 2 commits June 26, 2024 00:07

Clean up some more attachment internals. Better type handling; Attach…

7551e79

…mentOps

Merge remote-tracking branch 'Neoforged/1.21.x' into feature/codec-at…

2988adb

…tachments

lukebemish suggested changes Jun 26, 2024

View reviewed changes

src/main/java/net/neoforged/neoforge/codec/NBTSerializableCodec.java Outdated Show resolved Hide resolved

src/main/java/net/neoforged/neoforge/common/util/INBTSerializable.java Outdated Show resolved Hide resolved

Fix a deserialization whoops, remove typetools runtime checks, more docs

14115b1

lukebemish suggested changes Jun 26, 2024

View reviewed changes

src/main/java/net/neoforged/neoforge/attachment/DataAttachmentOps.java Outdated Show resolved Hide resolved

Far less reflection shenanigans in NBTSerializableCodec

950cbde

lukebemish suggested changes Jun 27, 2024

View reviewed changes

lukebemish and others added 5 commits June 26, 2024 22:24

Improvements to codec attachment stuff

783d0fb

* Simplify NBTSerializableCodec * Make DataAttachmentOps work with registries (in theory)

Attachments and stream codecs

31ff3e3

Small patch cleanup

518477a

WIP on improving stream codec support and better copy filtering

3fb4585

Merge remote-tracking branch 'Neoforged/1.21.x' into feature/codec-at…

a80233e

…tachments

robotgryphon force-pushed the feature/codec-attachments branch from 799c7a9 to a80233e Compare July 1, 2024 13:17

robotgryphon marked this pull request as draft July 9, 2024 14:12

robotgryphon mentioned this pull request Aug 3, 2024

Add NBT Serializable codecs and impl for ItemStackHandler/EnergyStorage #1411

Open

neoforged-automation bot added the needs rebase label Aug 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[21.1+] Data Attachments Codec Support #1163

[21.1+] Data Attachments Codec Support #1163

robotgryphon commented Jun 22, 2024 •

edited

Loading

neoforged-pr-publishing bot commented Jun 22, 2024 •

edited

Loading

The artifacts published by this PR:

Repository Declaration

MDK installation

Technici4n left a comment

robotgryphon commented Jun 22, 2024

Technici4n commented Jun 22, 2024

robotgryphon commented Jun 22, 2024

pupnewfster commented Jun 22, 2024

neoforged-compatibility-checks bot commented Jun 24, 2024 •

edited

Loading

`neoforge (:neoforge)`

lukebemish left a comment •

edited

Loading

lukebemish Jun 27, 2024

lukebemish Jun 27, 2024

robotgryphon commented Jul 9, 2024

Technici4n commented Jul 10, 2024

robotgryphon commented Jul 10, 2024

Technici4n commented Jul 10, 2024

neoforged-automation bot commented Aug 7, 2024

[21.1+] Data Attachments Codec Support #1163

Are you sure you want to change the base?

[21.1+] Data Attachments Codec Support #1163

Conversation

robotgryphon commented Jun 22, 2024 • edited Loading

Attachment Holder Extensions

Attachment Holders

Internals/Documentation

Attachment Types

Examples

Replacements for IAttachmentSerializer/Re-capturing a "holder" reference

Conditionally serializing data

Specifying a different deserializer than the serializer

neoforged-pr-publishing bot commented Jun 22, 2024 • edited Loading

The artifacts published by this PR:

Repository Declaration

MDK installation

Technici4n left a comment

Choose a reason for hiding this comment

robotgryphon commented Jun 22, 2024

Technici4n commented Jun 22, 2024

robotgryphon commented Jun 22, 2024

pupnewfster commented Jun 22, 2024

neoforged-compatibility-checks bot commented Jun 24, 2024 • edited Loading

neoforge (:neoforge)

lukebemish left a comment • edited Loading

Choose a reason for hiding this comment

lukebemish Jun 27, 2024

Choose a reason for hiding this comment

lukebemish Jun 27, 2024

Choose a reason for hiding this comment

robotgryphon commented Jul 9, 2024

Technici4n commented Jul 10, 2024

robotgryphon commented Jul 10, 2024

Technici4n commented Jul 10, 2024

neoforged-automation bot commented Aug 7, 2024

robotgryphon commented Jun 22, 2024 •

edited

Loading

neoforged-pr-publishing bot commented Jun 22, 2024 •

edited

Loading

neoforged-compatibility-checks bot commented Jun 24, 2024 •

edited

Loading

`neoforge (:neoforge)`

lukebemish left a comment •

edited

Loading