fabric icon indicating copy to clipboard operation
fabric copied to clipboard

Fabric Data Attachment API

Open Technici4n opened this issue 3 years ago • 22 comments

This API allows attaching arbitrary data to various game objects such as worlds, chunks, entities and block entities. Data can be attached client-side and server-side, but no syncing is provided in the API. The data can optionally be persisted on the server side.

The use case is generally to store data for a specific game object directly with the game object instead of having to store it separately and rely on load/unload events.

This is similar to the "data attachment" part of Forge capabilities, however API queries should be handled by API Lookup as before.

API overview

There are two classes in the API: AttachmentType and AttachmentSerializer.

AttachmentType<A, T> is the "key" identifying a data attachment of type A to game objects of type T. It also contains the get, computeIfAbsent, set and remove methods that actually allow one to retrieve, update, and remove attached values.

AttachmentTypes should be stored in static final variables, and created using one of the for* constructors by passing an identifier, the attachment class, and optionally a serializer:

public interface AttachmentType<A, T> {
	static <A> AttachmentType<A, BlockEntity> forBlockEntity(Identifier identifier, Class<A> attachmentClass, @Nullable AttachmentSerializer<A, ? super BlockEntity> serializer) { ... }
	static <A> AttachmentType<A, WorldChunk> forChunk(Identifier identifier, Class<A> attachmentClass, @Nullable AttachmentSerializer<A, ? super WorldChunk> serializer) { }
	// etc...

Serialization

Attachments types may be registered with an AttachmentSerializer<A, T> if they wish to be persisted to NBT. The signature is self-explanatory, and there is also a helper to create a serializer from a DFU Codec:

public interface AttachmentSerializer<A, T> {
	/**
	 * Serialize the value to a new NBT compound.
	 * If {@code null} is returned, the value will not be saved at all.
	 *
	 * @param value The current value of the attachment. Never {@code null}.
	 * @return The serialized attachment, or {@code null} if nothing should be saved.
	 */
	@Nullable
	NbtCompound toNbt(A value);

	/**
	 * Create a new instance from an NBT compound previously created by {@link #toNbt}.
	 * If {@code null} is returned, the instance will not be placed in the attachment target.
	 *
	 * @param target The target of this attachment. This can be used to capture a reference (for example to a host entity).
	 * @param nbt The nbt data of this attachment. Never {@code null}.
	 * @return The deserialized attachment, or {@code null} if no attachment should be loaded.
	 */
	@Nullable
	A fromNbt(T target, NbtCompound nbt);

API usage example

Using a mutable type

class ManaContainer {
    int mana = 0;
}

public static final AttachmentType<ManaContainer, Entity> MANA_CONTAINER = AttachmentType.forEntity(
    new Identifier("mymod:mana_container"), // identifier for serialization
    ManaContainer.class, // class for type safety
    /* serializer, omitted here */ // or null if the attachment should not be serialized
);

PlayerEntity player = ...;
// Query
ManaContainer maybeContainer = MANA_CONTAINER.get(player);
// Increment mana
MANA_CONTAINER.computeIfAbsent(player, player -> new ManaContainer()).mana++;
// Remove
MANA_CONTAINER.remove(player);

// Entities are always persisted, so no markDirty call is needed.

Using an immutable type

public static final AttachmentType<Integer, Entity> MANA = AttachmentType.forEntity(
    new Identifier("mymod:mana"), // identifier for serialization
    Integer.class, // class for type safety
    AttachmentSerializer.fromCodec(Codec.INT) // codec-based serializers are very easy to obtain
);

PlayerEntity player = ...;
// Query
int currentMana = Objects.requireNonNullElse(MANA.get(player), 0);
// Increment mana
MANA.set(player, Objects.requireNonNullElse(MANA.get(player), 0) + 1);
// Remove
MANA.remove(player);

TODO

  • [x] Figure out why chunk attachments are not properly persisted?
  • [ ] More utility methods in AttachmentType?
  • [ ] Check that entities do indeed always get saved.
  • [ ] Decide whether using fabric:value in AttachmentSerializer.fromCodec for codecs that don't result in an NbtCompound is acceptable.

Technici4n avatar Apr 28 '22 21:04 Technici4n

I think we need a method that does this for you to reduce the code size

// Increment mana
MANA_CONTAINER.computeIfAbsent(player, ManaContainer::new).mana++;

For example:

MANA_CONTAINER.getOrCreate(player).mana++;

maityyy avatar Apr 29 '22 16:04 maityyy

I think we need a method that does this for you to reduce the code size

// Increment mana
MANA_CONTAINER.computeIfAbsent(player, ManaContainer::new).mana++;

For example:

MANA_CONTAINER.getOrCreate(player).mana++;

@maityyy Should it just find a constructor without params via reflection? Or we could also have the AttachmentType provide a default value factory.

Technici4n avatar Apr 29 '22 21:04 Technici4n

I recommend making the container getter method in AttachmentTarget abstract and having another InternalAttachmnetTarget extends AttachmentTarget in impl package that defaults the method instead. The InternalAttachmnetTarget will be injected into minecraft classes. This is to avoid mod classes implementing AttachmentTarget forgetting to override the method somehow.

liach avatar Apr 29 '22 21:04 liach

Or we could also have the AttachmentType provide a default value factory.

This is what I recommend, and we can pass the owner of the attachment container to the attachment type as well, to facilitate default value computation by context.

liach avatar Apr 29 '22 22:04 liach

I recommend making the container getter method in AttachmentTarget abstract and having another InternalAttachmnetTarget extends AttachmentTarget in impl package that defaults the method instead. The InternalAttachmnetTarget will be injected into minecraft classes. This is to avoid mod classes implementing AttachmentTarget forgetting to override the method somehow.

I'll think about it, but I think injected interfaces should be part of API. Maybe I'll do it as a nested interface.

Technici4n avatar Apr 29 '22 22:04 Technici4n

Or we could also have the AttachmentType provide a default value factory.

This is what I recommend, and we can pass the owner of the attachment container to the attachment type as well, to facilitate default value computation by context.

We'd have to pass AttachmentTarget directly though, and the supplier would have to do an instanceof check?

Technici4n avatar Apr 29 '22 22:04 Technici4n

I recommend making the container getter method in AttachmentTarget abstract and having another InternalAttachmnetTarget extends AttachmentTarget in impl package that defaults the method instead. The InternalAttachmnetTarget will be injected into minecraft classes. This is to avoid mod classes implementing AttachmentTarget forgetting to override the method somehow.

I'll think about it, but I think injected interfaces should be part of API. Maybe I'll do it as a nested interface.

The injected interface has a parent interface in the API, which should suffice. The extra interface serves solely to shut IDE noise when viewing the decompiled minecraft code, and to assure modders extending those classes that they don't need to implement the container getter as the API mixin'd it.

liach avatar Apr 29 '22 22:04 liach

I'm still not a fan since modders could be relying on it by accident. But I think custom AttachmentTargets are niche enough for this not be too much of an issue? I think Player doesn't even want custom attachments, but IMO it comes "for free". ;)

(By "custom" I mean attachment to custom game objects, i.e. attachment that are not provided by fabric API itself).

Technici4n avatar Apr 29 '22 22:04 Technici4n

We'd have to pass AttachmentTarget directly though, and the supplier would have to do an instanceof check?

I believe passing AttachmentTarget directly suffices, as they have a lot of subclasses and extra type parameter cannot cover those use cases.

If you want to go dangerous, you can declare something like

public <O extends AttachmentTarget> Builder withDefault(Function<? super O, ? extends T> defaultFactory) {}

which would avoid explicit casting and make code look more fluent, but it depends on modders to choose the correct target type O and avoid class cast exceptions.

I'm still not a fan since modders could be relying on it by accident.

The injected interface is already in the impl package, so it shouldn't really be a problem. Having modders implement AttachmentTarget but the compiler never tells them that they need to override getAttachmentContainer (and IDE won't generate stubs either) is more of a problem.

But I think custom AttachmentTargets are niche enough for this not be too much of an issue?

If modders shouldn't implement it, document it explicitly and add @ApiStatus.NonExtendable, than leaving a dangling interface that can be easily incorrectly implemented.

liach avatar Apr 29 '22 22:04 liach

I believe passing AttachmentTarget directly suffices, as they have a lot of subclasses and extra type parameter cannot cover those use cases.

Yes, I think you're right.

If you want to go dangerous, you can declare something like

public <O extends AttachmentTarget> Builder withDefault(Function<? super O, ? extends T> defaultFactory) {}

Looks quite dangerous indeed. Will probably avoid this. 😄

The injected interface is already in the impl package, so it shouldn't really be a problem. Having modders implement AttachmentTarget but the compiler never tells them that they need to override getAttachmentContainer (and IDE won't generate stubs either) is more of a problem.

It's not in the impl package? AttachmentTarget is injected and it's part of the api package.

But I think custom AttachmentTargets are niche enough for this not be too much of an issue?

If modders shouldn't implement it, document it explicitly and add @ApiStatus.NonExtendable, than leaving a dangling interface that can be easily incorrectly implemented.

I mean that they are allowed, but I expect someone implementing attachments for a custom container to be considerate enough to notice that they need to override this method. It will also be an easy-to-fix crash on the first test they do, so while not ideal I think it's "acceptable".

Technici4n avatar Apr 29 '22 22:04 Technici4n

Other question: Should removing or updating values even be allowed? I think keeping components around forever is a lot simpler, and it shouldn't cause too much overhead... It also simplifies the API surface a lot.

Technici4n avatar Apr 29 '22 22:04 Technici4n

It's not in the impl package? AttachmentTarget is injected and it's part of the api package.

No, I mean something like this:

package net.fabricmc.fabric.api.attachment.v1;
// ...
public interface AttachmentTarget {
	AttachmentContainer getAttachmentContainer();
}
package net.fabricmc.fabric.impl.attachment;
// ...
public interface InternalAttachmentTarget extends AttachmentTarget {
	@Override
	default AttachmentContainer getAttachmentContainer() {
		throw new UnsupportedOperationException("Implemented via mixin, must be overridden for custom game objects.");
	}
}

and then you inject net.fabricmc.fabric.impl.attachment.InternalAttachmentTarget onto the minecraft classes.

liach avatar Apr 29 '22 22:04 liach

To be determined, I'm still not fan of exposing this since modders could end up referencing it by mistake. I really think the exception in the default method is not a huge deal. 😄

Other question: should AttachmentTarget include some sort of markDirty method too?

Technici4n avatar Apr 29 '22 22:04 Technici4n

still not fan of exposing this Fabric API implementations have tons of internal accessor mixins in impl packages, and you don't fear exposing those though mods can call it freely. Curious.

Other question: should AttachmentTarget include some sort of markDirty method too?

I'd suggest moving the dirty checking mechanism onto the attachment container. if an attachment needs to do something when it updates, its type should be able to handle it.

liach avatar Apr 29 '22 22:04 liach

Fabric API implementations have tons of internal accessor mixins in impl packages, and you don't fear exposing those though mods can call it freely. Curious.

I know but if we use interface injection to inject something in MC source code, we're exposing these interfaces as part of an API imo as it's hard to tell what is impl and what is API. Whereas internal accessor mixins would need to be directly referenced by modders.

Technici4n avatar Apr 29 '22 23:04 Technici4n

Fabric API implementations have tons of internal accessor mixins in impl packages, and you don't fear exposing those though mods can call it freely. Curious.

I know but if we use interface injection to inject something in MC source code, we're exposing these interfaces as part of an API imo as it's hard to tell what is impl and what is API. Whereas internal accessor mixins would need to be directly referenced by modders.

Sure, I just hope the interface mixing can create explicit dummy implementations so the downstream compiler and IDE won't make noise one day

liach avatar Apr 29 '22 23:04 liach

I updated the PR with a slightly different approach after some discussion with Player on discord. The support for custom attachments has been removed since it's somewhat useless, and the IdentityHashMap<AttachmentType<?>, Object> has been inlined into the attachment targets.

Technici4n avatar Apr 30 '22 16:04 Technici4n

Updated again after more discussion with Player, AttachmentTarget has been removed, and instead the target is now a generic parameter of AttachmentType.

Technici4n avatar May 01 '22 11:05 Technici4n

Updated to 1.19, added a nice testmod. Now waiting on @sfPlayer1 for a review of the overall design. Others: feel free to review too.

Technici4n avatar Jun 21 '22 10:06 Technici4n

Updated to 1.19, added a nice testmod. Now waiting on @sfPlayer1 for a review of the overall design. Others: feel free to review too.

Technici4n avatar Jun 21 '22 10:06 Technici4n

Updated to 1.19.2. A few remarks:

  • Syncing entity data to clients is a headache that I don't want to deal with for the moment. However it can easily be added after-the-fact (possibly by someone more familiar with it than me), which means that it can be omitted from the first version. For example, one could imagine the following API extension to register a sync handler:
AttachmentType<Something, Entity> SOMETHING = ...;
AttachmentSyncing.registerForEntity(SOMETHING, new AttachmentSyncHandler<>() { ... });
  • It turns out that when an entity changes dimension on the server, it keeps any serializable attachments. I need to check the other corner cases, but we probably don't need some to specify an entity copying strategy since this is already handled correctly by saving to NBT and deserializing.

Technici4n avatar Sep 23 '22 19:09 Technici4n

I just went for a second documentation pass, and I updated the PR description. I resolved all previous comments as they were mostly outdated. Please open new ones if you would like to discuss something.

Waiting for reviews now! :wink:

Technici4n avatar Sep 23 '22 23:09 Technici4n

This PR can be closed safely as #3476 has implemented a Data Attachment API

mrjasonn avatar Jan 19 '24 16:01 mrjasonn