Fix attachment sync when changing world + migrate to client gametest

fabric-data-attachment-api-v1/build.gradle

@@ -11,5 +11,6 @@ testDependencies(project, [
 	':fabric-lifecycle-events-v1',
 	':fabric-biome-api-v1',
 	':fabric-command-api-v2',
-	':fabric-rendering-v1'
+	':fabric-rendering-v1',
+	':fabric-client-gametest-api-v1'
 ])

fabric-data-attachment-api-v1/src/client/java/net/fabricmc/fabric/mixin/attachment/client/ClientPlayNetworkHandlerMixin.java

@@ -21,7 +21,9 @@
 import com.llamalad7.mixinextras.sugar.Local;
 import org.spongepowered.asm.mixin.Mixin;
 import org.spongepowered.asm.mixin.injection.At;
+import org.spongepowered.asm.mixin.injection.Slice;
 
+import net.minecraft.client.MinecraftClient;
 import net.minecraft.client.network.ClientPlayNetworkHandler;
 import net.minecraft.client.network.ClientPlayerEntity;
 import net.minecraft.network.packet.s2c.play.PlayerRespawnS2CPacket;
@@ -32,13 +34,19 @@
 abstract class ClientPlayNetworkHandlerMixin {
 	@WrapOperation(
 			method = "onPlayerRespawn",
-			at = @At(value = "INVOKE", target = "Lnet/minecraft/client/network/ClientPlayerEntity;init()V")
+			at = @At(
+					value = "FIELD",
+					target = "Lnet/minecraft/client/MinecraftClient;player:Lnet/minecraft/client/network/ClientPlayerEntity;"
+			),
+			slice = @Slice(
+					from = @At(value = "INVOKE", target = "Lnet/minecraft/client/network/ClientPlayNetworkHandler;startWorldLoading(Lnet/minecraft/client/network/ClientPlayerEntity;Lnet/minecraft/client/world/ClientWorld;Lnet/minecraft/client/gui/screen/DownloadingTerrainScreen$WorldEntryReason;)V")
+			)
 	)
-	private void copyAttachmentsOnClientRespawn(ClientPlayerEntity newPlayer, Operation<Void> init, PlayerRespawnS2CPacket packet, @Local(ordinal = 0) ClientPlayerEntity oldPlayer) {
+	private void copyAttachmentsOnClientRespawn(MinecraftClient client, ClientPlayerEntity newPlayer, Operation<Void> init, PlayerRespawnS2CPacket packet, @Local(ordinal = 0) ClientPlayerEntity oldPlayer) {
 		/*
 		 * The KEEP_ATTRIBUTES flag is not set on a death respawn, and set in all other cases
 		 */
 		AttachmentTargetImpl.transfer(oldPlayer, newPlayer, !packet.hasFlag(PlayerRespawnS2CPacket.KEEP_ATTRIBUTES));
-		init.call(newPlayer);
+		init.call(client, newPlayer);
 	}
 }

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

@@ -29,6 +29,7 @@
 import net.minecraft.util.Identifier;
 
 import net.fabricmc.api.ModInitializer;
+import net.fabricmc.fabric.api.entity.event.v1.ServerEntityWorldChangeEvents;
 import net.fabricmc.fabric.api.networking.v1.EntityTrackingEvents;
 import net.fabricmc.fabric.api.networking.v1.PayloadTypeRegistry;
 import net.fabricmc.fabric.api.networking.v1.ServerConfigurationConnectionEvents;
@@ -114,6 +115,17 @@ public void onInitialize() {
 			}
 		});
 
+		ServerEntityWorldChangeEvents.AFTER_PLAYER_CHANGE_WORLD.register((player, origin, destination) -> {
+			// sync new world's attachments
+			// no conflict with previous one because the client world is recreated every time
+			List<AttachmentChange> changes = new ArrayList<>();
+			((AttachmentTargetImpl) destination).fabric_computeInitialSyncChanges(player, changes::add);
+
+			if (!changes.isEmpty()) {
+				AttachmentChange.partitionAndSendPackets(changes, player);
+			}
+		});
+
 		EntityTrackingEvents.START_TRACKING.register((trackedEntity, player) -> {
 			List<AttachmentChange> changes = new ArrayList<>();
 			((AttachmentTargetImpl) trackedEntity).fabric_computeInitialSyncChanges(player, changes::add);

fabric-data-attachment-api-v1/src/testmod/java/net/fabricmc/fabric/test/attachment/AttachmentTestMod.java

@@ -16,58 +16,26 @@
 
 package net.fabricmc.fabric.test.attachment;
 
-import static net.minecraft.server.command.CommandManager.argument;
-import static net.minecraft.server.command.CommandManager.literal;
-
-import java.io.File;
-import java.io.IOException;
-
-import com.mojang.brigadier.builder.LiteralArgumentBuilder;
-import com.mojang.brigadier.context.CommandContext;
-import com.mojang.brigadier.exceptions.CommandSyntaxException;
-import com.mojang.brigadier.exceptions.SimpleCommandExceptionType;
 import com.mojang.serialization.Codec;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import net.minecraft.block.entity.BlockEntity;
-import net.minecraft.command.argument.BlockPosArgumentType;
-import net.minecraft.command.argument.ColumnPosArgumentType;
-import net.minecraft.command.argument.EntityArgumentType;
-import net.minecraft.entity.EquipmentSlot;
 import net.minecraft.item.ItemStack;
 import net.minecraft.network.codec.PacketCodecs;
 import net.minecraft.registry.Registries;
 import net.minecraft.registry.Registry;
 import net.minecraft.registry.RegistryKey;
 import net.minecraft.registry.RegistryKeys;
-import net.minecraft.server.command.ServerCommandSource;
-import net.minecraft.server.network.ServerPlayerEntity;
-import net.minecraft.server.world.ServerWorld;
-import net.minecraft.text.Text;
 import net.minecraft.util.Identifier;
-import net.minecraft.util.WorldSavePath;
-import net.minecraft.util.math.ChunkPos;
-import net.minecraft.util.math.ColumnPos;
-import net.minecraft.world.chunk.Chunk;
-import net.minecraft.world.chunk.ChunkStatus;
-import net.minecraft.world.chunk.ProtoChunk;
-import net.minecraft.world.chunk.WorldChunk;
-import net.minecraft.world.chunk.WrapperProtoChunk;
 import net.minecraft.world.gen.GenerationStep;
 import net.minecraft.world.gen.feature.DefaultFeatureConfig;
 
 import net.fabricmc.api.ModInitializer;
 import net.fabricmc.fabric.api.attachment.v1.AttachmentRegistry;
 import net.fabricmc.fabric.api.attachment.v1.AttachmentSyncPredicate;
-import net.fabricmc.fabric.api.attachment.v1.AttachmentTarget;
 import net.fabricmc.fabric.api.attachment.v1.AttachmentType;
 import net.fabricmc.fabric.api.biome.v1.BiomeModifications;
 import net.fabricmc.fabric.api.biome.v1.BiomeSelectors;
-import net.fabricmc.fabric.api.command.v2.CommandRegistrationCallback;
-import net.fabricmc.fabric.api.event.lifecycle.v1.ServerChunkEvents;
-import net.fabricmc.fabric.api.event.lifecycle.v1.ServerEntityEvents;
-import net.fabricmc.fabric.api.event.lifecycle.v1.ServerLifecycleEvents;
 
 public class AttachmentTestMod implements ModInitializer {
 	public static final String MOD_ID = "fabric-data-attachment-api-v1-testmod";
@@ -101,7 +69,7 @@ public class AttachmentTestMod implements ModInitializer {
 					.syncWith(PacketCodecs.BOOLEAN, AttachmentSyncPredicate.allButTarget())
 	);
 	public static final AttachmentType<Boolean> SYNCED_CREATIVE_ONLY = AttachmentRegistry.create(
-			Identifier.of(MOD_ID, "synced_custom"),
+			Identifier.of(MOD_ID, "synced_creative"),
 			builder -> builder
 					.initializer(() -> false)
 					.persistent(Codec.BOOL)
@@ -114,12 +82,6 @@ public class AttachmentTestMod implements ModInitializer {
 					.persistent(ItemStack.CODEC)
 					.syncWith(ItemStack.OPTIONAL_PACKET_CODEC, AttachmentSyncPredicate.all())
 	);
-	public static final SimpleCommandExceptionType TARGET_NOT_FOUND = new SimpleCommandExceptionType(Text.literal("Target not found"));
-
-	public static final ChunkPos FAR_CHUNK_POS = new ChunkPos(300, 0);
-
-	private boolean serverStarted = false;
-	public static boolean featurePlaced = false;
 
 	@Override
 	public void onInitialize() {
@@ -130,137 +92,5 @@ public void onInitialize() {
 				GenerationStep.Feature.VEGETAL_DECORATION,
 				RegistryKey.of(RegistryKeys.PLACED_FEATURE, Identifier.of(MOD_ID, "set_attachment"))
 		);
-
-		ServerLifecycleEvents.SERVER_STARTED.register(server -> {
-			File saveRoot = server.getSavePath(WorldSavePath.ROOT).toFile();
-			File markerFile = new File(saveRoot, MOD_ID + "_MARKER");
-			boolean firstLaunch;
-
-			try {
-				firstLaunch = markerFile.createNewFile();
-			} catch (IOException e) {
-				throw new RuntimeException(e);
-			}
-
-			ServerWorld overworld = server.getOverworld();
-			WorldChunk chunk = overworld.getChunk(0, 0);
-
-			if (firstLaunch) {
-				LOGGER.info("First launch, testing attachment by feature");
-
-				if (featurePlaced) {
-					if (!"feature".equals(chunk.getAttached(FEATURE_ATTACHMENT))) {
-						throw new AssertionError("Feature did not write attachment to ProtoChunk");
-					}
-				} else {
-					LOGGER.warn("Feature not placed, could not test writing during worldgen");
-				}
-
-				LOGGER.info("setting up persistent attachments");
-
-				overworld.setAttached(PERSISTENT, "world_data");
-
-				chunk.setAttached(PERSISTENT, "chunk_data");
-				chunk.setAttached(SYNCED_WITH_ALL, true);
-
-				ProtoChunk protoChunk = (ProtoChunk) overworld.getChunkManager().getChunk(FAR_CHUNK_POS.x, FAR_CHUNK_POS.z, ChunkStatus.STRUCTURE_STARTS, true);
-				protoChunk.setAttached(PERSISTENT, "protochunk_data");
-			} else {
-				LOGGER.info("Second launch, testing persistent attachments");
-
-				if (!"world_data".equals(overworld.getAttached(PERSISTENT))) throw new AssertionError("World attachment did not persist");
-				if (!"chunk_data".equals(chunk.getAttached(PERSISTENT))) throw new AssertionError("WorldChunk attachment did not persist");
-
-				WrapperProtoChunk wrapperProtoChunk = (WrapperProtoChunk) overworld.getChunkManager().getChunk(0, 0, ChunkStatus.EMPTY, true);
-				if (!"chunk_data".equals(wrapperProtoChunk.getAttached(PERSISTENT))) throw new AssertionError("Attachment is not accessible through WrapperProtoChunk");
-
-				Chunk farChunk = overworld.getChunkManager().getChunk(FAR_CHUNK_POS.x, FAR_CHUNK_POS.z, ChunkStatus.EMPTY, true);
-
-				if (farChunk instanceof WrapperProtoChunk) {
-					LOGGER.warn("Far chunk already generated, can't test persistence in ProtoChunk.");
-				} else {
-					if (!"protochunk_data".equals(farChunk.getAttached(PERSISTENT))) throw new AssertionError("ProtoChunk attachment did not persist");
-				}
-			}
-
-			serverStarted = true;
-		});
-
-		// Testing hint: load far chunk by running /tp @s 4800 ~ 0
-		ServerChunkEvents.CHUNK_LOAD.register(((world, chunk) -> {
-			if (!chunk.getPos().equals(FAR_CHUNK_POS)) return;
-
-			if (!serverStarted) {
-				LOGGER.warn("Chunk {} loaded before server started, can't test transfer of attachments to WorldChunk", FAR_CHUNK_POS);
-				return;
-			}
-
-			LOGGER.info("Loaded chunk {}, testing transfer of attachments to WorldChunk", FAR_CHUNK_POS);
-
-			if (!"protochunk_data".equals(chunk.getAttached(PERSISTENT))) throw new AssertionError("ProtoChunk attachment was not transfered to WorldChunk");
-		}));
-
-		CommandRegistrationCallback.EVENT.register((dispatcher, registryAccess, environment) -> dispatcher.register(
-				literal("attachment")
-						.then(buildCommandForKind("all", "all", SYNCED_WITH_ALL))
-						.then(buildCommandForKind("self_only", "only self", SYNCED_WITH_TARGET))
-						.then(buildCommandForKind("others_only", "all but self", SYNCED_EXCEPT_TARGET))
-						.then(buildCommandForKind("creative_only", "creative players only", SYNCED_CREATIVE_ONLY))
-		));
-
-		ServerEntityEvents.EQUIPMENT_CHANGE.register((livingEntity, equipmentSlot, previousStack, currentStack) -> {
-			if (equipmentSlot == EquipmentSlot.HEAD && livingEntity instanceof ServerPlayerEntity player) {
-				player.setAttached(SYNCED_ITEM, currentStack);
-			}
-		});
-	}
-
-	private static LiteralArgumentBuilder<ServerCommandSource> buildCommandForKind(String id, String syncedWith, AttachmentType<Boolean> type) {
-		return literal(id).executes(context -> updateAttachmentFor(
-				context.getSource().getPlayerOrThrow(),
-				type,
-				context,
-				"Set self flag (synced with %s) to %%s".formatted(syncedWith)
-		)).then(
-				argument("target", EntityArgumentType.entity()).executes(context -> updateAttachmentFor(
-						EntityArgumentType.getEntity(context, "target"),
-						type,
-						context,
-						"Set entity flag (synced with %s) to %%s".formatted(syncedWith)
-				))
-		).then(argument("pos", BlockPosArgumentType.blockPos()).executes(context -> {
-			BlockEntity be = context.getSource().getWorld().getBlockEntity(BlockPosArgumentType.getBlockPos(context, "pos"));
-
-			if (be == null) {
-				throw TARGET_NOT_FOUND.create();
-			}
-
-			return updateAttachmentFor(
-					be,
-					type,
-					context,
-					"Set block entity flag (synced with %s) to %%s".formatted(syncedWith)
-			);
-		})).then(argument("chunkPos", ColumnPosArgumentType.columnPos()).executes(context -> {
-			ColumnPos pos = ColumnPosArgumentType.getColumnPos(context, "chunkpos");
-			return updateAttachmentFor(
-					context.getSource().getWorld().getChunk(pos.x(), pos.z(), ChunkStatus.STRUCTURE_STARTS, true),
-					type,
-					context,
-					"Set chunk flag (synced with %s) to %%s".formatted(syncedWith)
-			);
-		})).then(literal("world").executes(context -> updateAttachmentFor(
-				context.getSource().getWorld(),
-				type,
-				context,
-				"Set world flag (synced with %s) to %%s".formatted(syncedWith)
-		)));
-	}
-
-	private static int updateAttachmentFor(AttachmentTarget target, AttachmentType<Boolean> attachment, CommandContext<ServerCommandSource> context, String messageFormat) throws CommandSyntaxException {
-		boolean current = target.getAttachedOrElse(attachment, false);
-		target.setAttached(attachment, !current);
-		context.getSource().sendFeedback(() -> Text.literal(messageFormat.formatted(!current)), false);
-		return 1;
 	}
 }

fabric-data-attachment-api-v1/src/testmod/java/net/fabricmc/fabric/test/attachment/SetAttachmentFeature.java

@@ -27,6 +27,8 @@
 import net.minecraft.world.gen.feature.util.FeatureContext;
 
 public class SetAttachmentFeature extends Feature<DefaultFeatureConfig> {
+	public static boolean featurePlaced;
+
 	public SetAttachmentFeature(Codec<DefaultFeatureConfig> codec) {
 		super(codec);
 	}
@@ -36,13 +38,13 @@ public boolean generate(FeatureContext<DefaultFeatureConfig> context) {
 		Chunk chunk = context.getWorld().getChunk(context.getOrigin());
 
 		if (chunk.getPos().equals(new ChunkPos(0, 0))) {
-			AttachmentTestMod.featurePlaced = true;
+			featurePlaced = true;
 
 			if (!(chunk instanceof ProtoChunk) || chunk instanceof WrapperProtoChunk) {
 				AttachmentTestMod.LOGGER.warn("Feature not attaching to ProtoChunk");
 			}
 
-			chunk.setAttached(AttachmentTestMod.FEATURE_ATTACHMENT, "feature");
+			chunk.setAttached(AttachmentTestMod.FEATURE_ATTACHMENT, "feature_data");
 		}
 
 		return true;

fabric-data-attachment-api-v1/src/testmod/resources/fabric.mod.json

@@ -15,19 +15,16 @@
     "main": [
       "net.fabricmc.fabric.test.attachment.AttachmentTestMod"
     ],
-    "client": [
-      "net.fabricmc.fabric.test.attachment.client.AttachmentTestModClient"
-    ],
     "fabric-gametest": [
       "net.fabricmc.fabric.test.attachment.gametest.AttachmentCopyTests",
       "net.fabricmc.fabric.test.attachment.gametest.BlockEntityTests"
+    ],
+    "fabric-client-gametest": [
+      "net.fabricmc.fabric.test.attachment.client.gametest.PersistenceGametest",
+      "net.fabricmc.fabric.test.attachment.client.gametest.SyncGametest"
     ]
   },
   "mixins": [
-    "fabric-data-attachment-api-v1-testmod.mixins.json",
-    {
-      "config": "fabric-data-attachment-api-v1-testmod.client.mixins.json",
-      "environment": "client"
-    }
+    "fabric-data-attachment-api-v1-testmod.mixins.json"
   ]
 }