Add per-AttachmentType sync size limits & Allow modifying large payload max size

fabric-data-attachment-api-v1/src/client/java/net/fabricmc/fabric/impl/attachment/client/AttachmentSyncClient.java

@@ -20,7 +20,6 @@
 import net.fabricmc.fabric.api.client.networking.v1.ClientConfigurationNetworking;
 import net.fabricmc.fabric.api.client.networking.v1.ClientPlayNetworking;
 import net.fabricmc.fabric.impl.attachment.AttachmentEntrypoint;
-import net.fabricmc.fabric.impl.attachment.sync.AttachmentChange;
 import net.fabricmc.fabric.impl.attachment.sync.AttachmentSync;
 import net.fabricmc.fabric.impl.attachment.sync.AttachmentSyncException;
 import net.fabricmc.fabric.impl.attachment.sync.s2c.AttachmentSyncPayloadS2C;
@@ -39,14 +38,11 @@ public void onInitializeClient() {
 		ClientPlayNetworking.registerGlobalReceiver(
 				AttachmentSyncPayloadS2C.ID,
 				(payload, context) -> {
-					for (AttachmentChange attachmentChange : payload.attachments()) {
-						try {
-							attachmentChange.tryApply(context.client().level);
-						} catch (AttachmentSyncException e) {
-							AttachmentEntrypoint.LOGGER.error("Error accepting attachment changes", e);
-							context.responseSender().disconnect(e.getText());
-							break;
-						}
+					try {
+						payload.attachment().tryApply(context.client().level);
+					} catch (AttachmentSyncException e) {
+						AttachmentEntrypoint.LOGGER.error("Error accepting attachment changes", e);
+						context.responseSender().disconnect(e.getText());
 					}
 				}
 		);

fabric-data-attachment-api-v1/src/main/java/net/fabricmc/fabric/api/attachment/v1/AttachmentRegistry.java

@@ -159,6 +159,18 @@ public interface Builder<A> {
 		 */
 		AttachmentRegistry.Builder<A> syncWith(StreamCodec<? super RegistryFriendlyByteBuf, A> packetCodec, AttachmentSyncPredicate syncPredicate);
 
+		/**
+		 * Declares that this attachment type may be automatically synchronized with some clients, as determined by {@code syncPredicate}.
+		 *
+		 * <p>The max size limit should be increased with care, as syncing large amounts of data may result in network lag and excessive bandwidth usage.
+		 *
+		 * @param packetCodec the codec used to serialize the attachment data over the network
+		 * @param syncPredicate an {@link AttachmentSyncPredicate} determining with which clients to synchronize data
+		 * @param maxSyncBytes the max number of data bytes that can be synced, defaults to 1 MiB minus some small padding
+		 * @return the builder
+		 */
+		AttachmentRegistry.Builder<A> syncWith(StreamCodec<? super RegistryFriendlyByteBuf, A> packetCodec, AttachmentSyncPredicate syncPredicate, int maxSyncBytes);
+
 		/**
 		 * Builds and registers the {@link AttachmentType}.
 		 *

fabric-data-attachment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/AttachmentRegistryImpl.java

@@ -25,24 +25,30 @@
 import java.util.function.Supplier;
 
 import com.mojang.serialization.Codec;
+import io.netty.buffer.ByteBufUtil;
 import org.jspecify.annotations.Nullable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import net.minecraft.network.RegistryFriendlyByteBuf;
+import net.minecraft.network.VarInt;
 import net.minecraft.network.codec.StreamCodec;
 import net.minecraft.resources.Identifier;
 
 import net.fabricmc.fabric.api.attachment.v1.AttachmentRegistry;
 import net.fabricmc.fabric.api.attachment.v1.AttachmentSyncPredicate;
 import net.fabricmc.fabric.api.attachment.v1.AttachmentType;
+import net.fabricmc.fabric.api.networking.v1.PayloadTypeRegistry;
 import net.fabricmc.fabric.impl.attachment.sync.AttachmentSync;
+import net.fabricmc.fabric.impl.attachment.sync.AttachmentTargetInfo;
+import net.fabricmc.fabric.impl.attachment.sync.s2c.AttachmentSyncPayloadS2C;
 
 public final class AttachmentRegistryImpl {
 	private static final Logger LOGGER = LoggerFactory.getLogger("fabric-data-attachment-api-v1");
 	private static final Map<Identifier, AttachmentType<?>> attachmentRegistry = new HashMap<>();
 	private static final Set<Identifier> syncableAttachments = new HashSet<>();
 	private static final Set<Identifier> syncableView = Collections.unmodifiableSet(syncableAttachments);
+	private static int currentMaxPayloadSize = AttachmentSync.INITIAL_MAX_ATTACHMENT_SYNC_PAYLOAD_SIZE;
 
 	public static <A> void register(Identifier id, AttachmentType<A> attachmentType) {
 		AttachmentType<?> existing = attachmentRegistry.put(id, attachmentType);
@@ -84,6 +90,7 @@ public static class BuilderImpl<A> implements AttachmentRegistry.Builder<A> {
 		@Nullable
 		private AttachmentSyncPredicate syncPredicate = null;
 		private boolean copyOnDeath = false;
+		private int maxSyncBytes = -1;
 
 		@Override
 		public AttachmentRegistry.Builder<A> persistent(Codec<A> codec) {
@@ -107,7 +114,7 @@ public AttachmentRegistry.Builder<A> initializer(Supplier<A> initializer) {
 			return this;
 		}
 
-		@Deprecated
+		@Override
 		public AttachmentRegistry.Builder<A> syncWith(StreamCodec<? super RegistryFriendlyByteBuf, A> packetCodec, AttachmentSyncPredicate syncPredicate) {
 			Objects.requireNonNull(packetCodec, "packet codec cannot be null");
 			Objects.requireNonNull(syncPredicate, "sync predicate cannot be null");
@@ -117,17 +124,42 @@ public AttachmentRegistry.Builder<A> syncWith(StreamCodec<? super RegistryFriend
 			return this;
 		}
 
+		@Override
+		public AttachmentRegistry.Builder<A> syncWith(StreamCodec<? super RegistryFriendlyByteBuf, A> packetCodec, AttachmentSyncPredicate syncPredicate, int maxSyncBytes) {
+			if (maxSyncBytes <= 0) {
+				throw new IllegalArgumentException("max sync bytes must be positive");
+			}
+
+			syncWith(packetCodec, syncPredicate);
+
+			this.maxSyncBytes = maxSyncBytes;
+			return this;
+		}
+
 		@Override
 		public AttachmentType<A> buildAndRegister(Identifier id) {
 			Objects.requireNonNull(id, "identifier cannot be null");
 
-			if (syncPredicate != null && id.toString().length() > AttachmentSync.MAX_IDENTIFIER_SIZE) {
-				throw new IllegalArgumentException(
-						"Identifier length is too long for a synced attachment type (was %d, maximum is %d)".formatted(
-								id.toString().length(),
-								AttachmentSync.MAX_IDENTIFIER_SIZE
-						)
-				);
+			if (syncPredicate != null) {
+				int identifierBytes = ByteBufUtil.utf8MaxBytes(id.toString());
+				int maxPaddingBytes = AttachmentTargetInfo.MAX_SIZE_IN_BYTES + VarInt.getByteSize(identifierBytes) + identifierBytes + 5 * 2;
+
+				if (maxSyncBytes == -1) { // If no custom limit set, then calculate default limit based on id size of the attachment
+					maxSyncBytes = AttachmentSync.INITIAL_MAX_ATTACHMENT_SYNC_PAYLOAD_SIZE - maxPaddingBytes;
+				}
+
+				int maxPayloadBytes = maxSyncBytes + maxPaddingBytes;
+
+				// Prevent overflow
+				if (maxPayloadBytes < 0) {
+					maxPayloadBytes = Integer.MAX_VALUE;
+					maxSyncBytes = Integer.MAX_VALUE - maxPaddingBytes;
+				}
+
+				if (maxPayloadBytes > currentMaxPayloadSize) {
+					currentMaxPayloadSize = maxPayloadBytes;
+					PayloadTypeRegistry.playS2C().modifyLargePayloadMaxSize(AttachmentSyncPayloadS2C.ID, currentMaxPayloadSize);
+				}
 			}
 
 			var attachment = new AttachmentTypeImpl<>(
@@ -136,6 +168,7 @@ public AttachmentType<A> buildAndRegister(Identifier id) {
 					persistenceCodec,
 					packetCodec,
 					syncPredicate,
+					maxSyncBytes,
 					copyOnDeath
 			);
 			register(id, attachment);

fabric-data-attachment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/AttachmentTypeImpl.java

@@ -34,6 +34,7 @@ public record AttachmentTypeImpl<A>(
 		@Nullable Codec<A> persistenceCodec,
 		@Nullable StreamCodec<? super RegistryFriendlyByteBuf, A> packetCodec,
 		@Nullable AttachmentSyncPredicate syncPredicate,
+		int maxSyncBytes,
 		boolean copyOnDeath
 ) implements AttachmentType<A> {
 	@Override

fabric-data-attachment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/sync/AttachmentChange.java

@@ -16,11 +16,7 @@
 
 package net.fabricmc.fabric.impl.attachment.sync;
 
-import java.util.ArrayList;
-import java.util.Comparator;
-import java.util.List;
 import java.util.Objects;
-import java.util.Set;
 
 import io.netty.buffer.Unpooled;
 import org.jspecify.annotations.Nullable;
@@ -35,19 +31,13 @@
 import net.minecraft.network.codec.ByteBufCodecs;
 import net.minecraft.network.codec.StreamCodec;
 import net.minecraft.resources.Identifier;
-import net.minecraft.server.level.ServerPlayer;
 import net.minecraft.world.level.Level;
 
 import net.fabricmc.fabric.api.attachment.v1.AttachmentTarget;
 import net.fabricmc.fabric.api.attachment.v1.AttachmentType;
 import net.fabricmc.fabric.api.networking.v1.PacketByteBufs;
-import net.fabricmc.fabric.api.networking.v1.ServerPlayNetworking;
 import net.fabricmc.fabric.impl.attachment.AttachmentRegistryImpl;
 import net.fabricmc.fabric.impl.attachment.AttachmentTypeImpl;
-import net.fabricmc.fabric.impl.attachment.sync.s2c.AttachmentSyncPayloadS2C;
-import net.fabricmc.fabric.mixin.attachment.ServerboundCustomPayloadPacketAccessor;
-import net.fabricmc.fabric.mixin.attachment.VarIntAccessor;
-import net.fabricmc.fabric.mixin.networking.accessor.ServerCommonPacketListenerImplAccessor;
 
 public record AttachmentChange(AttachmentTargetInfo<?> targetInfo, AttachmentType<?> type, byte[] data) {
 	public static final StreamCodec<FriendlyByteBuf, AttachmentChange> PACKET_CODEC = StreamCodec.composite(
@@ -59,8 +49,6 @@ public record AttachmentChange(AttachmentTargetInfo<?> targetInfo, AttachmentTyp
 			ByteBufCodecs.BYTE_ARRAY, AttachmentChange::data,
 			AttachmentChange::new
 	);
-	private static final int MAX_PADDING_SIZE_IN_BYTES = AttachmentTargetInfo.MAX_SIZE_IN_BYTES + AttachmentSync.MAX_IDENTIFIER_SIZE;
-	private static final int MAX_DATA_SIZE_IN_BYTES = ServerboundCustomPayloadPacketAccessor.getMaxPayloadSize() - MAX_PADDING_SIZE_IN_BYTES;
 
 	@SuppressWarnings("unchecked")
 	public static AttachmentChange create(AttachmentTargetInfo<?> targetInfo, AttachmentType<?> type, @Nullable Object value, RegistryAccess dynamicRegistryManager) {
@@ -77,50 +65,21 @@ public static AttachmentChange create(AttachmentTargetInfo<?> targetInfo, Attach
 			buf.writeBoolean(false);
 		}
 
-		byte[] encoded = buf.array();
+		byte[] encoded = new byte[buf.readableBytes()]; // buf.array() will return the backing array directly, which may contain unused space
+		buf.readBytes(encoded);
+		int maxSyncBytes = ((AttachmentTypeImpl<?>) type).maxSyncBytes();
 
-		if (encoded.length > MAX_DATA_SIZE_IN_BYTES) {
-			throw new IllegalArgumentException("Data for attachment '%s' was too big (%d bytes, over maximum %d)".formatted(
+		if (encoded.length > maxSyncBytes) {
+			throw new IllegalArgumentException("Data for attachment '%s' was too big (%d bytes, over maximum %d). This limit can be configured during attachment registration.".formatted(
 					type.identifier(),
 					encoded.length,
-					MAX_DATA_SIZE_IN_BYTES
+					maxSyncBytes
 			));
 		}
 
 		return new AttachmentChange(targetInfo, type, encoded);
 	}
 
-	public static void partitionAndSendPackets(List<AttachmentChange> changes, ServerPlayer player) {
-		Set<Identifier> supported = ((SupportedAttachmentsClientConnection) ((ServerCommonPacketListenerImplAccessor) player.connection).getConnection())
-				.fabric_getSupportedAttachments();
-		// sort by size to better partition packets
-		changes.sort(Comparator.comparingInt(c -> c.data().length));
-		List<AttachmentChange> packetChanges = new ArrayList<>();
-		int maxVarIntSize = VarIntAccessor.getMaxByteSize();
-		int byteSize = maxVarIntSize;
-
-		for (AttachmentChange change : changes) {
-			if (!supported.contains(change.type.identifier())) {
-				continue;
-			}
-
-			int size = MAX_PADDING_SIZE_IN_BYTES + change.data.length;
-
-			if (byteSize + size > MAX_DATA_SIZE_IN_BYTES) {
-				ServerPlayNetworking.send(player, new AttachmentSyncPayloadS2C(packetChanges));
-				packetChanges.clear();
-				byteSize = maxVarIntSize;
-			}
-
-			packetChanges.add(change);
-			byteSize += size;
-		}
-
-		if (!packetChanges.isEmpty()) {
-			ServerPlayNetworking.send(player, new AttachmentSyncPayloadS2C(packetChanges));
-		}
-	}
-
 	@SuppressWarnings("unchecked")
 	@Nullable
 	public Object decodeValue(RegistryAccess dynamicRegistryManager) {

fabric-data-attachment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/sync/AttachmentSync.java

@@ -22,8 +22,13 @@
 import java.util.function.Consumer;
 import java.util.stream.Collectors;
 
+import io.netty.buffer.ByteBufUtil;
+
 import net.minecraft.network.Connection;
+import net.minecraft.network.VarInt;
 import net.minecraft.network.protocol.Packet;
+import net.minecraft.network.protocol.game.ClientGamePacketListener;
+import net.minecraft.network.protocol.game.ClientboundBundlePacket;
 import net.minecraft.resources.Identifier;
 import net.minecraft.server.level.ServerPlayer;
 import net.minecraft.server.network.ConfigurationTask;
@@ -45,7 +50,16 @@
 import net.fabricmc.fabric.mixin.networking.accessor.ServerCommonPacketListenerImplAccessor;
 
 public class AttachmentSync implements ModInitializer {
-	public static final int MAX_IDENTIFIER_SIZE = 256;
+	public static final int INITIAL_MAX_ATTACHMENT_SYNC_PAYLOAD_SIZE;
+
+	static {
+		int identifierSize = ByteBufUtil.utf8MaxBytes(AttachmentSyncPayloadS2C.ID.toString());
+		// Max vanilla S2C packet size - id size, to ensure no splitting by default
+		INITIAL_MAX_ATTACHMENT_SYNC_PAYLOAD_SIZE = 1024 * 1024 - (VarInt.getByteSize(identifierSize) + identifierSize + 5 * 2);
+
+		// Ensure packet is registered before mods register any large attachments
+		PayloadTypeRegistry.playS2C().registerLarge(AttachmentSyncPayloadS2C.ID, AttachmentSyncPayloadS2C.CODEC, INITIAL_MAX_ATTACHMENT_SYNC_PAYLOAD_SIZE);
+	}
 
 	public static AcceptedAttachmentsPayloadC2S createResponsePayload() {
 		return new AcceptedAttachmentsPayloadC2S(AttachmentRegistryImpl.getSyncableAttachments());
@@ -56,7 +70,23 @@ public static void trySync(AttachmentChange change, ServerPlayer player) {
 				.fabric_getSupportedAttachments();
 
 		if (supported.contains(change.type().identifier())) {
-			ServerPlayNetworking.send(player, new AttachmentSyncPayloadS2C(List.of(change)));
+			ServerPlayNetworking.send(player, new AttachmentSyncPayloadS2C(change));
+		}
+	}
+
+	public static void trySync(List<AttachmentChange> changes, ServerPlayer player) {
+		Set<Identifier> supported = ((SupportedAttachmentsClientConnection) ((ServerCommonPacketListenerImplAccessor) player.connection).getConnection())
+				.fabric_getSupportedAttachments();
+
+		List<Packet<? super ClientGamePacketListener>> syncableChanges = new ArrayList<>();
+		changes.forEach(change -> {
+			if (supported.contains(change.type().identifier())) {
+				syncableChanges.add(ServerPlayNetworking.createS2CPacket(new AttachmentSyncPayloadS2C(change)));
+			}
+		});
+
+		if (!syncableChanges.isEmpty()) {
+			ServerPlayNetworking.getSender(player).sendPacket(new ClientboundBundlePacket(syncableChanges));
 		}
 	}
 
@@ -103,8 +133,6 @@ public void onInitialize() {
 		});
 
 		// Play
-		PayloadTypeRegistry.playS2C().register(AttachmentSyncPayloadS2C.ID, AttachmentSyncPayloadS2C.CODEC);
-
 		ServerPlayerEvents.JOIN.register((player) -> {
 			List<AttachmentChange> changes = new ArrayList<>();
 			// sync world attachments
@@ -113,7 +141,7 @@ public void onInitialize() {
 			((AttachmentTargetImpl) player).fabric_computeInitialSyncChanges(player, changes::add);
 
 			if (!changes.isEmpty()) {
-				AttachmentChange.partitionAndSendPackets(changes, player);
+				trySync(changes, player);
 			}
 		});
 
@@ -124,7 +152,7 @@ public void onInitialize() {
 			((AttachmentTargetImpl) destination).fabric_computeInitialSyncChanges(player, changes::add);
 
 			if (!changes.isEmpty()) {
-				AttachmentChange.partitionAndSendPackets(changes, player);
+				trySync(changes, player);
 			}
 		});
 
@@ -133,7 +161,7 @@ public void onInitialize() {
 			((AttachmentTargetImpl) trackedEntity).fabric_computeInitialSyncChanges(player, changes::add);
 
 			if (!changes.isEmpty()) {
-				AttachmentChange.partitionAndSendPackets(changes, player);
+				trySync(changes, player);
 			}
 		});
 	}

fabric-data-attachment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/sync/s2c/AttachmentSyncPayloadS2C.java

@@ -16,19 +16,16 @@
 
 package net.fabricmc.fabric.impl.attachment.sync.s2c;
 
-import java.util.List;
-
 import net.minecraft.network.FriendlyByteBuf;
-import net.minecraft.network.codec.ByteBufCodecs;
 import net.minecraft.network.codec.StreamCodec;
 import net.minecraft.network.protocol.common.custom.CustomPacketPayload;
 import net.minecraft.resources.Identifier;
 
 import net.fabricmc.fabric.impl.attachment.sync.AttachmentChange;
 
-public record AttachmentSyncPayloadS2C(List<AttachmentChange> attachments) implements CustomPacketPayload {
+public record AttachmentSyncPayloadS2C(AttachmentChange attachment) implements CustomPacketPayload {
 	public static final StreamCodec<FriendlyByteBuf, AttachmentSyncPayloadS2C> CODEC = StreamCodec.composite(
-			AttachmentChange.PACKET_CODEC.apply(ByteBufCodecs.list()), AttachmentSyncPayloadS2C::attachments,
+			AttachmentChange.PACKET_CODEC, AttachmentSyncPayloadS2C::attachment,
 			AttachmentSyncPayloadS2C::new
 	);
 	public static final Identifier PACKET_ID = Identifier.fromNamespaceAndPath("fabric", "attachment_sync_v1");

fabric-data-attachment-api-v1/src/main/java/net/fabricmc/fabric/mixin/attachment/PlayerChunkSenderMixin.java

@@ -32,6 +32,7 @@
 
 import net.fabricmc.fabric.impl.attachment.AttachmentTargetImpl;
 import net.fabricmc.fabric.impl.attachment.sync.AttachmentChange;
+import net.fabricmc.fabric.impl.attachment.sync.AttachmentSync;
 
 @Mixin(PlayerChunkSender.class)
 abstract class PlayerChunkSenderMixin {
@@ -49,7 +50,7 @@ private void sendInitialAttachmentData(ServerGamePacketListenerImpl handler, Ser
 		((AttachmentTargetImpl) chunk).fabric_computeInitialSyncChanges(player, changes::add);
 
 		if (!changes.isEmpty()) {
-			AttachmentChange.partitionAndSendPackets(changes, player);
+			AttachmentSync.trySync(changes, player);
 		}
 	}
 }