Configuration networking improvements (#3832)

* Configuration networking fixes

* Review fixes.

* Update fabric-networking-api-v1/src/testmodClient/java/net/fabricmc/fabric/test/networking/client/configuration/NetworkingConfigurationClientTest.java

Co-authored-by: Octol1ttle <l1ttleofficial@outlook.com>

---------

Co-authored-by: Octol1ttle <l1ttleofficial@outlook.com>

fabric-networking-api-v1/src/client/java/net/fabricmc/fabric/api/client/networking/v1/ClientConfigurationConnectionEvents.java

@@ -30,6 +30,8 @@ public final class ClientConfigurationConnectionEvents {
 	/**
 	 * Event indicating a connection entering the CONFIGURATION state, ready for registering channel handlers.
 	 *
+	 * <p>No packets should be sent when this event is invoked.
+	 *
 	 * @see ClientConfigurationNetworking#registerReceiver(CustomPayload.Id, ClientConfigurationNetworking.ConfigurationPayloadHandler)
 	 */
 	public static final Event<ClientConfigurationConnectionEvents.Init> INIT = EventFactory.createArrayBacked(ClientConfigurationConnectionEvents.Init.class, callbacks -> (handler, client) -> {
@@ -38,14 +40,25 @@ public final class ClientConfigurationConnectionEvents {
 		}
 	});
 
+	/**
+	 * An event called after the connection has been initialized and is ready to start sending and receiving configuration packets.
+	 *
+	 * <p>Packets may be sent during this event.
+	 */
+	public static final Event<ClientConfigurationConnectionEvents.Start> START = EventFactory.createArrayBacked(ClientConfigurationConnectionEvents.Start.class, callbacks -> (handler, client) -> {
+		for (ClientConfigurationConnectionEvents.Start callback : callbacks) {
+			callback.onConfigurationStart(handler, client);
+		}
+	});
+
 	/**
 	 * An event called after the ReadyS2CPacket has been received, just before switching to the PLAY state.
 	 *
 	 * <p>No packets should be sent when this event is invoked.
 	 */
-	public static final Event<ClientConfigurationConnectionEvents.Ready> READY = EventFactory.createArrayBacked(ClientConfigurationConnectionEvents.Ready.class, callbacks -> (handler, client) -> {
-		for (ClientConfigurationConnectionEvents.Ready callback : callbacks) {
-			callback.onConfigurationReady(handler, client);
+	public static final Event<ClientConfigurationConnectionEvents.Complete> COMPLETE = EventFactory.createArrayBacked(ClientConfigurationConnectionEvents.Complete.class, callbacks -> (handler, client) -> {
+		for (ClientConfigurationConnectionEvents.Complete callback : callbacks) {
+			callback.onConfigurationComplete(handler, client);
 		}
 	});
 
@@ -69,12 +82,38 @@ public final class ClientConfigurationConnectionEvents {
 	}
 
 	@FunctionalInterface
-	public interface Ready {
-		void onConfigurationReady(ClientConfigurationNetworkHandler handler, MinecraftClient client);
+	public interface Start {
+		void onConfigurationStart(ClientConfigurationNetworkHandler handler, MinecraftClient client);
+	}
+
+	@FunctionalInterface
+	public interface Complete {
+		void onConfigurationComplete(ClientConfigurationNetworkHandler handler, MinecraftClient client);
 	}
 
 	@FunctionalInterface
 	public interface Disconnect {
 		void onConfigurationDisconnect(ClientConfigurationNetworkHandler handler, MinecraftClient client);
 	}
+
+	// Deprecated:
+
+	/**
+	 * @deprecated replaced by {@link #COMPLETE}
+	 */
+	@Deprecated
+	public static final Event<ClientConfigurationConnectionEvents.Ready> READY = EventFactory.createArrayBacked(ClientConfigurationConnectionEvents.Ready.class, callbacks -> (handler, client) -> {
+		for (ClientConfigurationConnectionEvents.Ready callback : callbacks) {
+			callback.onConfigurationReady(handler, client);
+		}
+	});
+
+	/**
+	 * @deprecated replaced by {@link ClientConfigurationConnectionEvents.Complete}
+	 */
+	@Deprecated
+	@FunctionalInterface
+	public interface Ready {
+		void onConfigurationReady(ClientConfigurationNetworkHandler handler, MinecraftClient client);
+	}
 }

fabric-networking-api-v1/src/client/java/net/fabricmc/fabric/impl/networking/client/ClientConfigurationNetworkAddon.java

@@ -22,6 +22,7 @@ import java.util.Objects;
 import net.minecraft.client.MinecraftClient;
 import net.minecraft.client.network.ClientConfigurationNetworkHandler;
 import net.minecraft.network.NetworkPhase;
+import net.minecraft.network.packet.BrandCustomPayload;
 import net.minecraft.network.packet.CustomPayload;
 import net.minecraft.network.packet.Packet;
 import net.minecraft.util.Identifier;
@@ -39,6 +40,7 @@ import net.fabricmc.fabric.mixin.networking.client.accessor.ClientConfigurationN
 public final class ClientConfigurationNetworkAddon extends ClientCommonNetworkAddon<ClientConfigurationNetworking.ConfigurationPayloadHandler<?>, ClientConfigurationNetworkHandler> {
 	private final ContextImpl context;
 	private boolean sentInitialRegisterPacket;
+	private boolean hasStarted;
 
 	public ClientConfigurationNetworkAddon(ClientConfigurationNetworkHandler handler, MinecraftClient client) {
 		super(ClientNetworkingImpl.CONFIGURATION, ((ClientCommonNetworkHandlerAccessor) handler).getConnection(), "ClientPlayNetworkAddon for " + ((ClientConfigurationNetworkHandlerAccessor) handler).getProfile().getName(), handler, client);
@@ -53,6 +55,12 @@ public final class ClientConfigurationNetworkAddon extends ClientCommonNetworkAd
 		ClientConfigurationConnectionEvents.INIT.invoker().onConfigurationInit(this.handler, this.client);
 	}
 
+	@Override
+	public void onServerReady() {
+		super.onServerReady();
+		invokeStartEvent();
+	}
+
 	@Override
 	protected void receiveRegistration(boolean register, RegistrationPayload payload) {
 		super.receiveRegistration(register, payload);
@@ -60,6 +68,27 @@ public final class ClientConfigurationNetworkAddon extends ClientCommonNetworkAd
 		if (register && !this.sentInitialRegisterPacket) {
 			this.sendInitialChannelRegistrationPacket();
 			this.sentInitialRegisterPacket = true;
+
+			this.onServerReady();
+		}
+	}
+
+	@Override
+	public boolean handle(CustomPayload payload) {
+		boolean result = super.handle(payload);
+
+		if (payload instanceof BrandCustomPayload) {
+			// If we have received this without first receiving the registration packet, its likely a vanilla server.
+			invokeStartEvent();
+		}
+
+		return result;
+	}
+
+	private void invokeStartEvent() {
+		if (!hasStarted) {
+			hasStarted = true;
+			ClientConfigurationConnectionEvents.START.invoker().onConfigurationStart(this.handler, this.client);
 		}
 	}
 
@@ -84,7 +113,8 @@ public final class ClientConfigurationNetworkAddon extends ClientCommonNetworkAd
 		C2SConfigurationChannelEvents.UNREGISTER.invoker().onChannelUnregister(this.handler, this, this.client, ids);
 	}
 
-	public void handleReady() {
+	public void handleComplete() {
+		ClientConfigurationConnectionEvents.COMPLETE.invoker().onConfigurationComplete(this.handler, this.client);
 		ClientConfigurationConnectionEvents.READY.invoker().onConfigurationReady(this.handler, this.client);
 		ClientNetworkingImpl.setClientConfigurationAddon(null);
 	}

fabric-networking-api-v1/src/client/java/net/fabricmc/fabric/impl/networking/client/ClientNetworkingImpl.java

@@ -127,7 +127,6 @@ public final class ClientNetworkingImpl {
 	}
 
 	public static void setClientConfigurationAddon(ClientConfigurationNetworkAddon addon) {
-		assert addon == null || currentPlayAddon == null;
 		currentConfigurationAddon = addon;
 	}
 

fabric-networking-api-v1/src/client/java/net/fabricmc/fabric/mixin/networking/client/ClientCommonNetworkHandlerMixin.java

@@ -16,10 +16,7 @@
 
 package net.fabricmc.fabric.mixin.networking.client;
 
-import org.slf4j.Logger;
-import org.spongepowered.asm.mixin.Final;
 import org.spongepowered.asm.mixin.Mixin;
-import org.spongepowered.asm.mixin.Shadow;
 import org.spongepowered.asm.mixin.injection.At;
 import org.spongepowered.asm.mixin.injection.Inject;
 import org.spongepowered.asm.mixin.injection.callback.CallbackInfo;
@@ -34,10 +31,6 @@ import net.fabricmc.fabric.impl.networking.client.ClientPlayNetworkAddon;
 
 @Mixin(ClientCommonNetworkHandler.class)
 public abstract class ClientCommonNetworkHandlerMixin implements NetworkHandlerExtensions {
-	@Shadow
-	@Final
-	private static Logger LOGGER;
-
 	@Inject(method = "onCustomPayload(Lnet/minecraft/network/packet/s2c/common/CustomPayloadS2CPacket;)V", at = @At("HEAD"), cancellable = true)
 	public void onCustomPayload(CustomPayloadS2CPacket packet, CallbackInfo ci) {
 		final CustomPayload payload = packet.payload();

fabric-networking-api-v1/src/client/java/net/fabricmc/fabric/mixin/networking/client/ClientConfigurationNetworkHandlerMixin.java

@@ -52,8 +52,8 @@ public abstract class ClientConfigurationNetworkHandlerMixin extends ClientCommo
 	}
 
 	@Inject(method = "onReady", at = @At(value = "NEW", target = "(Lnet/minecraft/client/MinecraftClient;Lnet/minecraft/network/ClientConnection;Lnet/minecraft/client/network/ClientConnectionState;)Lnet/minecraft/client/network/ClientPlayNetworkHandler;"))
-	public void onReady(ReadyS2CPacket packet, CallbackInfo ci) {
-		this.addon.handleReady();
+	public void handleComplete(ReadyS2CPacket packet, CallbackInfo ci) {
+		this.addon.handleComplete();
 	}
 
 	@Override

fabric-networking-api-v1/src/client/java/net/fabricmc/fabric/mixin/networking/client/ClientLoginNetworkHandlerMixin.java

@@ -30,7 +30,6 @@ import net.minecraft.network.ClientConnection;
 import net.minecraft.network.packet.s2c.login.LoginQueryRequestS2CPacket;
 
 import net.fabricmc.fabric.impl.networking.NetworkHandlerExtensions;
-import net.fabricmc.fabric.impl.networking.client.ClientConfigurationNetworkAddon;
 import net.fabricmc.fabric.impl.networking.client.ClientLoginNetworkAddon;
 import net.fabricmc.fabric.impl.networking.payload.PacketByteBufLoginQueryRequestPayload;
 
@@ -65,12 +64,6 @@ abstract class ClientLoginNetworkHandlerMixin implements NetworkHandlerExtension
 		}
 	}
 
-	@Inject(method = "onSuccess", at = @At("TAIL"))
-	private void handleConfigurationReady(CallbackInfo ci) {
-		NetworkHandlerExtensions networkHandlerExtensions = (NetworkHandlerExtensions) connection.getPacketListener();
-		((ClientConfigurationNetworkAddon) networkHandlerExtensions.getAddon()).onServerReady();
-	}
-
 	@Override
 	public ClientLoginNetworkAddon getAddon() {
 		return this.addon;

fabric-networking-api-v1/src/testmod/java/net/fabricmc/fabric/test/networking/configuration/NetworkingConfigurationTest.java

@@ -18,15 +18,20 @@ package net.fabricmc.fabric.test.networking.configuration;
 
 import java.util.function.Consumer;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import net.minecraft.network.PacketByteBuf;
 import net.minecraft.network.codec.PacketCodec;
 import net.minecraft.network.packet.CustomPayload;
 import net.minecraft.network.packet.Packet;
+import net.minecraft.server.command.DebugConfigCommand;
 import net.minecraft.server.network.ServerPlayerConfigurationTask;
 import net.minecraft.text.Text;
 import net.minecraft.util.Identifier;
 
 import net.fabricmc.api.ModInitializer;
+import net.fabricmc.fabric.api.command.v2.CommandRegistrationCallback;
 import net.fabricmc.fabric.api.networking.v1.PayloadTypeRegistry;
 import net.fabricmc.fabric.api.networking.v1.ServerConfigurationConnectionEvents;
 import net.fabricmc.fabric.api.networking.v1.ServerConfigurationNetworking;
@@ -36,10 +41,13 @@ import net.fabricmc.fabric.test.networking.NetworkingTestmods;
  * Also see NetworkingConfigurationClientTest.
  */
 public class NetworkingConfigurationTest implements ModInitializer {
+	private static final Logger LOGGER = LoggerFactory.getLogger(NetworkingConfigurationTest.class);
+
 	@Override
 	public void onInitialize() {
 		PayloadTypeRegistry.configurationS2C().register(ConfigurationPacket.ID, ConfigurationPacket.CODEC);
 		PayloadTypeRegistry.configurationC2S().register(ConfigurationCompletePacket.ID, ConfigurationCompletePacket.CODEC);
+		PayloadTypeRegistry.configurationC2S().register(ConfigurationStartPacket.ID, ConfigurationStartPacket.CODEC);
 
 		ServerConfigurationConnectionEvents.CONFIGURE.register((handler, server) -> {
 			// You must check to see if the client can handle your config task
@@ -54,6 +62,13 @@ public class NetworkingConfigurationTest implements ModInitializer {
 		ServerConfigurationNetworking.registerGlobalReceiver(ConfigurationCompletePacket.ID, (packet, context) -> {
 			context.networkHandler().completeTask(TestConfigurationTask.KEY);
 		});
+
+		ServerConfigurationNetworking.registerGlobalReceiver(ConfigurationStartPacket.ID, (packet, context) -> {
+			LOGGER.info("Received configuration start packet from client");
+		});
+
+		// Enable the vanilla debugconfig command
+		CommandRegistrationCallback.EVENT.register((dispatcher, registryAccess, environment) -> DebugConfigCommand.register(dispatcher));
 	}
 
 	public record TestConfigurationTask(String data) implements ServerPlayerConfigurationTask {
@@ -102,4 +117,18 @@ public class NetworkingConfigurationTest implements ModInitializer {
 			return ID;
 		}
 	}
+
+	public static class ConfigurationStartPacket implements CustomPayload {
+		public static final ConfigurationStartPacket INSTANCE = new ConfigurationStartPacket();
+		public static final CustomPayload.Id<ConfigurationStartPacket> ID = new Id<>(Identifier.of(NetworkingTestmods.ID, "configure_start"));
+		public static final PacketCodec<PacketByteBuf, ConfigurationStartPacket> CODEC = PacketCodec.unit(INSTANCE);
+
+		private ConfigurationStartPacket() {
+		}
+
+		@Override
+		public Id<? extends CustomPayload> getId() {
+			return ID;
+		}
+	}
 }

fabric-networking-api-v1/src/testmodClient/java/net/fabricmc/fabric/test/networking/client/configuration/NetworkingConfigurationClientTest.java

@@ -16,11 +16,17 @@
 
 package net.fabricmc.fabric.test.networking.client.configuration;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import net.fabricmc.api.ClientModInitializer;
+import net.fabricmc.fabric.api.client.networking.v1.ClientConfigurationConnectionEvents;
 import net.fabricmc.fabric.api.client.networking.v1.ClientConfigurationNetworking;
 import net.fabricmc.fabric.test.networking.configuration.NetworkingConfigurationTest;
 
 public class NetworkingConfigurationClientTest implements ClientModInitializer {
+	private static final Logger LOGGER = LoggerFactory.getLogger(NetworkingConfigurationTest.class);
+
 	@Override
 	public void onInitializeClient() {
 		ClientConfigurationNetworking.registerGlobalReceiver(NetworkingConfigurationTest.ConfigurationPacket.ID, (packet, context) -> {
@@ -29,5 +35,16 @@ public class NetworkingConfigurationClientTest implements ClientModInitializer {
 			// Respond back to the server that the task is complete
 			context.responseSender().sendPacket(NetworkingConfigurationTest.ConfigurationCompletePacket.INSTANCE);
 		});
+
+		ClientConfigurationConnectionEvents.START.register((handler, client) -> {
+			if (!ClientConfigurationNetworking.canSend(NetworkingConfigurationTest.ConfigurationStartPacket.ID)) {
+				// This isn't fatal as it will happen when connecting to a vanilla server.
+				LOGGER.warn("Not sending configuration start packet; is this a vanilla server?");
+				return;
+			}
+
+			LOGGER.info("Sending configuration start packet to server");
+			ClientConfigurationNetworking.send(NetworkingConfigurationTest.ConfigurationStartPacket.INSTANCE);
+		});
 	}
 }