Fix screen handler data syncing, also rewrite it while im at it to reduce the number of packets sent.

RebornCore/src/main/java/reborncore/client/screen/builder/BuiltScreenHandler.java

@@ -24,6 +24,8 @@
 
 package reborncore.client.screen.builder;
 
+import it.unimi.dsi.fastutil.ints.Int2ObjectMap;
+import it.unimi.dsi.fastutil.ints.Int2ObjectOpenHashMap;
 import net.minecraft.entity.player.PlayerEntity;
 import net.minecraft.inventory.CraftingInventory;
 import net.minecraft.inventory.Inventory;
@@ -34,17 +36,23 @@ import net.minecraft.screen.ScreenHandlerType;
 import net.minecraft.screen.slot.Slot;
 import net.minecraft.util.math.BlockPos;
 import org.apache.commons.lang3.Range;
-import org.apache.commons.lang3.tuple.MutableTriple;
 import org.apache.commons.lang3.tuple.Pair;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import reborncore.common.blockentity.MachineBaseBlockEntity;
+import reborncore.common.network.ClientBoundPackets;
+import reborncore.common.network.NetworkManager;
 import reborncore.common.util.ItemUtils;
 import reborncore.mixin.common.AccessorScreenHandler;
+import reborncore.mixin.ifaces.ServerPlayerEntityScreenHandler;
 
-import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.function.*;
 
-public class BuiltScreenHandler extends ScreenHandler implements ExtendedScreenHandlerListener {
+public class BuiltScreenHandler extends ScreenHandler {
+	private static final Logger LOGGER = LogManager.getLogger();
 
 	private final String name;
 
@@ -52,9 +60,10 @@ public class BuiltScreenHandler extends ScreenHandler implements ExtendedScreenH
 	private final List<Range<Integer>> playerSlotRanges;
 	private final List<Range<Integer>> blockEntitySlotRanges;
 
-	private final ArrayList<MutableTriple<IntSupplier, IntConsumer, Short>> shortValues;
+	// Holds the syncpair along with the last value
-	private final ArrayList<MutableTriple<IntSupplier, IntConsumer, Integer>> integerValues;
+	private final Map<SyncPair, Object> syncPairCache = new HashMap<>();
-	private final ArrayList<MutableTriple<Supplier, Consumer, Object>> objectValues;
+	private final Int2ObjectMap<SyncPair> syncPairIdLookup = new Int2ObjectOpenHashMap<>();
+
 	private List<Consumer<CraftingInventory>> craftEvents;
 	private Integer[] integerParts;
 
@@ -71,36 +80,17 @@ public class BuiltScreenHandler extends ScreenHandler implements ExtendedScreenH
 		this.playerSlotRanges = playerSlotRange;
 		this.blockEntitySlotRanges = blockEntitySlotRange;
 
-		this.shortValues = new ArrayList<>();
-		this.integerValues = new ArrayList<>();
-		this.objectValues = new ArrayList<>();
-
 		this.blockEntity = blockEntity;
 	}
 
-	public void addShortSync(final List<Pair<IntSupplier, IntConsumer>> syncables) {
+	public void addObjectSync(final List<Pair<Supplier<?>, Consumer<?>>> syncables) {
-
+		for (final Pair<Supplier<?>, Consumer<?>> syncable : syncables) {
-		for (final Pair<IntSupplier, IntConsumer> syncable : syncables) {
+			// Add a new sync pair to the cache with a null value
-			this.shortValues.add(MutableTriple.of(syncable.getLeft(), syncable.getRight(), (short) 0));
+			int id = syncPairCache.size() + 1;
+			var syncPair = new SyncPair(syncable.getLeft(), syncable.getRight(), id);
+			this.syncPairCache.put(syncPair, null);
+			this.syncPairIdLookup.put(id, syncPair);
 		}
-		this.shortValues.trimToSize();
-	}
-
-	public void addIntegerSync(final List<Pair<IntSupplier, IntConsumer>> syncables) {
-
-		for (final Pair<IntSupplier, IntConsumer> syncable : syncables) {
-			this.integerValues.add(MutableTriple.of(syncable.getLeft(), syncable.getRight(), 0));
-		}
-		this.integerValues.trimToSize();
-		this.integerParts = new Integer[this.integerValues.size()];
-	}
-
-	public void addObjectSync(final List<Pair<Supplier, Consumer>> syncables) {
-
-		for (final Pair<Supplier, Consumer> syncable : syncables) {
-			this.objectValues.add(MutableTriple.of(syncable.getLeft(), syncable.getRight(), null));
-		}
-		this.objectValues.trimToSize();
 	}
 
 	public void addCraftEvents(final List<Consumer<CraftingInventory>> craftEvents) {
@@ -124,44 +114,7 @@ public class BuiltScreenHandler extends ScreenHandler implements ExtendedScreenH
 		super.sendContentUpdates();
 
 		for (final ScreenHandlerListener listener : ((AccessorScreenHandler) (this)).getListeners()) {
-
+			sendContentUpdatePacketToListener(listener);
-			int i = 0;
-			if (!this.shortValues.isEmpty()) {
-				for (final MutableTriple<IntSupplier, IntConsumer, Short> value : this.shortValues) {
-					final short supplied = (short) value.getLeft().getAsInt();
-					if (supplied != value.getRight()) {
-
-						listener.onPropertyUpdate(this, i, supplied);
-						value.setRight(supplied);
-					}
-					i++;
-				}
-			}
-
-			if (!this.integerValues.isEmpty()) {
-				for (final MutableTriple<IntSupplier, IntConsumer, Integer> value : this.integerValues) {
-					final int supplied = value.getLeft().getAsInt();
-					if (supplied != value.getRight()) {
-
-						listener.onPropertyUpdate(this, i, supplied >> 16);
-						listener.onPropertyUpdate(this, i + 1, (short) (supplied & 0xFFFF));
-						value.setRight(supplied);
-					}
-					i += 2;
-				}
-			}
-
-			if (!this.objectValues.isEmpty()) {
-				int objects = 0;
-				for (final MutableTriple<Supplier, Consumer, Object> value : this.objectValues) {
-					final Object supplied = value.getLeft().get();
-					if (supplied != value.getRight()) {
-						sendObject(listener, this, objects, supplied);
-						value.setRight(supplied);
-					}
-					objects++;
-				}
-			}
 		}
 	}
 
@@ -169,57 +122,45 @@ public class BuiltScreenHandler extends ScreenHandler implements ExtendedScreenH
 	public void addListener(final ScreenHandlerListener listener) {
 		super.addListener(listener);
 
-		int i = 0;
+		sendContentUpdatePacketToListener(listener);
-		if (!this.shortValues.isEmpty()) {
-			for (final MutableTriple<IntSupplier, IntConsumer, Short> value : this.shortValues) {
-				final short supplied = (short) value.getLeft().getAsInt();
-
-				listener.onPropertyUpdate(this, i, supplied);
-				value.setRight(supplied);
-				i++;
-			}
 	}
 
-		if (!this.integerValues.isEmpty()) {
+	private void sendContentUpdatePacketToListener(final ScreenHandlerListener listener) {
-			for (final MutableTriple<IntSupplier, IntConsumer, Integer> value : this.integerValues) {
+		Int2ObjectMap<Object> updatedValues = new Int2ObjectOpenHashMap<>();
-				final int supplied = value.getLeft().getAsInt();
 
-				listener.onPropertyUpdate(this, i, supplied >> 16);
+		this.syncPairCache.replaceAll((syncPair, cached) -> {
-				listener.onPropertyUpdate(this, i + 1, (short) (supplied & 0xFFFF));
+			final Object value = syncPair.supplier().get();
-				value.setRight(supplied);
+
-				i += 2;
+			if (value != cached) {
+				updatedValues.put(syncPair.id, value);
+				return value;
 			}
+			return null;
+		});
+
+		sendUpdatedValues(listener, this, updatedValues);
 	}
 
-		if (!this.objectValues.isEmpty()) {
+	public void handleUpdateValues(Int2ObjectMap<Object> updatedValues) {
-			int objects = 0;
+		updatedValues.int2ObjectEntrySet().forEach(entry -> {
-			for (final MutableTriple<Supplier, Consumer, Object> value : this.objectValues) {
+			SyncPair syncPair = syncPairIdLookup.get(entry.getIntKey());
-				final Object supplied = value.getLeft();
+			if (syncPair == null) {
-				sendObject(listener, this, objects, ((Supplier) supplied).get());
+				LOGGER.warn("Unknown sync pair id: " + entry.getIntKey());
-				value.setRight(supplied);
+				return;
-				objects++;
-			}
-		}
 			}
 
-	@Override
+			// TODO check the object type here?
-	public void handleObject(int var, Object value) {
+			syncPair.consumer().accept(entry.getValue());
-		this.objectValues.get(var).getMiddle().accept(value);
+		});
 	}
 
-	@Override
+	private void sendUpdatedValues(ScreenHandlerListener screenHandlerListener, ScreenHandler screenHandler, Int2ObjectMap<Object> updatedValues) {
-	public void setProperty(int id, int value) {
+		if (updatedValues.isEmpty()) {
-		if (id < this.shortValues.size()) {
+			return;
-			this.shortValues.get(id).getMiddle().accept((short) value);
-			this.shortValues.get(id).setRight((short) value);
-		} else if (id - this.shortValues.size() < this.integerValues.size() * 2) {
-
-			if ((id - this.shortValues.size()) % 2 == 0) {
-				this.integerParts[(id - this.shortValues.size()) / 2] = value;
-			} else {
-				this.integerValues.get((id - this.shortValues.size()) / 2).getMiddle().accept(
-						(this.integerParts[(id - this.shortValues.size()) / 2] & 0xFFFF) << 16 | value & 0xFFFF);
 		}
+
+		if (screenHandlerListener instanceof ServerPlayerEntityScreenHandler serverPlayerEntityScreenHandler) {
+			NetworkManager.sendToPlayer(ClientBoundPackets.createPacketSendObject(screenHandler, updatedValues), serverPlayerEntityScreenHandler.rc_getServerPlayerEntity());
 		}
 	}
 
@@ -370,4 +311,8 @@ public class BuiltScreenHandler extends ScreenHandler implements ExtendedScreenH
 	public ScreenHandlerType<BuiltScreenHandler> getType() {
 		return type;
 	}
+
+	private record SyncPair(Supplier supplier, Consumer consumer, int id) {
+
+	}
 }

RebornCore/src/main/java/reborncore/client/screen/builder/ExtendedScreenHandlerListener.java

@@ -1,44 +0,0 @@
-/*
- * This file is part of RebornCore, licensed under the MIT License (MIT).
- *
- * Copyright (c) 2021 TeamReborn
- *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to deal
- * in the Software without restriction, including without limitation the rights
- * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- * copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in all
- * copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
- * SOFTWARE.
- */
-
-package reborncore.client.screen.builder;
-
-import net.minecraft.screen.ScreenHandler;
-import net.minecraft.screen.ScreenHandlerListener;
-import net.minecraft.server.network.ServerPlayerEntity;
-import reborncore.common.network.ClientBoundPackets;
-import reborncore.common.network.NetworkManager;
-
-public interface ExtendedScreenHandlerListener {
-
-	default void sendObject(ScreenHandlerListener screenHandlerListener, ScreenHandler screenHandler, int var, Object value) {
-		if (screenHandlerListener instanceof ServerPlayerEntity) {
-			NetworkManager.sendToPlayer(ClientBoundPackets.createPacketSendObject(var, value, screenHandler), (ServerPlayerEntity) screenHandlerListener);
-		}
-	}
-
-	default void handleObject(int var, Object value) {
-
-	}
-}

RebornCore/src/main/java/reborncore/client/screen/builder/ScreenHandlerBuilder.java

@@ -47,7 +47,7 @@ public class ScreenHandlerBuilder {
 	final List<Slot> slots;
 	final List<Range<Integer>> playerInventoryRanges, blockEntityInventoryRanges;
 
-	final List<Pair<Supplier, Consumer>> objectValues;
+	final List<Pair<Supplier<?>, Consumer<?>>> objectValues;
 
 	final List<Consumer<CraftingInventory>> craftEvents;
 

RebornCore/src/main/java/reborncore/client/screen/builder/Syncable.java

@@ -32,6 +32,6 @@ import java.util.function.Supplier;
 
 public interface Syncable {
 
-	void getSyncPair(List<Pair<Supplier, Consumer>> pairList);
+	void getSyncPair(List<Pair<Supplier<?>, Consumer<?>>> pairList);
 
 }

RebornCore/src/main/java/reborncore/common/blockentity/RedstoneConfiguration.java

@@ -177,7 +177,7 @@ public class RedstoneConfiguration implements NBTSerializable, Syncable {
 	}
 
 	@Override
-	public void getSyncPair(List<Pair<Supplier, Consumer>> pairList) {
+	public void getSyncPair(List<Pair<Supplier<?>, Consumer<?>>> pairList) {
 		pairList.add(Pair.of(this::write, (Consumer<NbtCompound>) this::read));
 	}
 

RebornCore/src/main/java/reborncore/common/network/ClientBoundPacketHandlers.java

@@ -24,6 +24,8 @@
 
 package reborncore.common.network;
 
+import it.unimi.dsi.fastutil.ints.Int2ObjectMap;
+import it.unimi.dsi.fastutil.ints.Int2ObjectOpenHashMap;
 import net.fabricmc.api.EnvType;
 import net.fabricmc.api.Environment;
 import net.minecraft.block.entity.BlockEntity;
@@ -35,9 +37,11 @@ import net.minecraft.screen.ScreenHandler;
 import net.minecraft.util.Identifier;
 import net.minecraft.util.math.BlockPos;
 import net.minecraft.world.World;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import reborncore.RebornCore;
 import reborncore.client.ClientChunkManager;
-import reborncore.client.screen.builder.ExtendedScreenHandlerListener;
+import reborncore.client.screen.builder.BuiltScreenHandler;
 import reborncore.common.blockentity.FluidConfiguration;
 import reborncore.common.blockentity.MachineBaseBlockEntity;
 import reborncore.common.blockentity.SlotConfiguration;
@@ -47,6 +51,7 @@ import java.util.List;
 
 @Environment(EnvType.CLIENT)
 public class ClientBoundPacketHandlers {
+	private static final Logger LOGGER = LogManager.getLogger();
 
 	public static void init() {
 		NetworkManager.registerClientBoundHandler(new Identifier("reborncore", "custom_description"), (client, handler, packetBuffer, responseSender) -> {
@@ -103,15 +108,30 @@ public class ClientBoundPacketHandlers {
 		});
 
 		NetworkManager.registerClientBoundHandler(new Identifier("reborncore", "send_object"), (client, handler, packetBuffer, responseSender) -> {
+			int size = packetBuffer.readInt();
+			ExtendedPacketBuffer epb = new ExtendedPacketBuffer(packetBuffer);
+			Int2ObjectMap<Object> updatedValues = new Int2ObjectOpenHashMap<>();
+
+			for (int i = 0; i < size; i++) {
 				int id = packetBuffer.readInt();
-			Object value = new ExtendedPacketBuffer(packetBuffer).readObject();
+				Object value =  epb.readObject();
-			String container = packetBuffer.readString(packetBuffer.readInt());
+				updatedValues.put(id, value);
+			}
+
+			String name = packetBuffer.readString(packetBuffer.readInt());
+
 			client.execute(() -> {
 				Screen gui = MinecraftClient.getInstance().currentScreen;
-				if (gui instanceof HandledScreen) {
+				if (gui instanceof HandledScreen handledScreen) {
-					ScreenHandler screenHandler = ((HandledScreen) gui).getScreenHandler();
+					ScreenHandler screenHandler = handledScreen.getScreenHandler();
-					if (screenHandler instanceof ExtendedScreenHandlerListener) {
+					if (screenHandler instanceof BuiltScreenHandler builtScreenHandler) {
-						((ExtendedScreenHandlerListener) screenHandler).handleObject(id, value);
+						String shName = screenHandler.getClass().getName();
+						if (!shName.equals(name)) {
+							LOGGER.warn("Received packet for {} but screen handler {} is open!", name, shName);
+							return;
+						}
+
+						builtScreenHandler.handleUpdateValues(updatedValues);
 					}
 				}
 			});

RebornCore/src/main/java/reborncore/common/network/ClientBoundPackets.java

@@ -24,6 +24,7 @@
 
 package reborncore.common.network;
 
+import it.unimi.dsi.fastutil.ints.Int2ObjectMap;
 import net.minecraft.block.entity.BlockEntity;
 import net.minecraft.nbt.NbtCompound;
 import net.minecraft.screen.ScreenHandler;
@@ -62,10 +63,13 @@ public class ClientBoundPackets {
 		});
 	}
 
-	public static IdentifiedPacket createPacketSendObject(int id, Object value, ScreenHandler screenHandler) {
+	public static IdentifiedPacket createPacketSendObject(ScreenHandler screenHandler, Int2ObjectMap<Object> updatedValues) {
 		return NetworkManager.createClientBoundPacket(new Identifier("reborncore", "send_object"), packetBuffer -> {
-			packetBuffer.writeInt(id);
+			packetBuffer.writeInt(updatedValues.size());
-			packetBuffer.writeObject(value);
+			updatedValues.forEach((integer, o) -> {
+				packetBuffer.writeInt(integer);
+				packetBuffer.writeObject(o);
+			});
 			packetBuffer.writeInt(screenHandler.getClass().getName().length());
 			packetBuffer.writeString(screenHandler.getClass().getName());
 		});

RebornCore/src/main/java/reborncore/common/util/Tank.java

@@ -133,7 +133,7 @@ public class Tank implements GenericFluidContainer<Direction>, Syncable {
 	}
 
 	@Override
-	public void getSyncPair(List<Pair<Supplier, Consumer>> pairList) {
+	public void getSyncPair(List<Pair<Supplier<?>, Consumer<?>>> pairList) {
 		pairList.add(Pair.of(() -> Registry.FLUID.getId(fluidInstance.getFluid()).toString(), (Consumer<String>) o -> fluidInstance.setFluid(Registry.FLUID.get(new Identifier(o)))));
 		pairList.add(Pair.of(() -> fluidInstance.getAmount(), o -> fluidInstance.setAmount((FluidValue) o)));
 	}

RebornCore/src/main/java/reborncore/mixin/common/MixinServerPlayerEntity.java

@@ -0,0 +1,18 @@
+package reborncore.mixin.common;
+
+import net.minecraft.server.network.ServerPlayerEntity;
+import org.spongepowered.asm.mixin.Mixin;
+import org.spongepowered.asm.mixin.Shadow;
+import reborncore.mixin.ifaces.ServerPlayerEntityScreenHandler;
+
+@Mixin(targets = "net.minecraft.server.network.ServerPlayerEntity$2")
+public class MixinServerPlayerEntity implements ServerPlayerEntityScreenHandler {
+
+	@Shadow
+	ServerPlayerEntity field_29183;
+
+	@Override
+	public ServerPlayerEntity rc_getServerPlayerEntity() {
+		return field_29183;
+	}
+}

RebornCore/src/main/java/reborncore/mixin/ifaces/ServerPlayerEntityScreenHandler.java

@@ -0,0 +1,8 @@
+package reborncore.mixin.ifaces;
+
+import net.minecraft.server.network.ServerPlayerEntity;
+
+public interface ServerPlayerEntityScreenHandler {
+
+	ServerPlayerEntity rc_getServerPlayerEntity();
+}

RebornCore/src/main/resources/reborncore.common.mixins.json

@@ -15,7 +15,8 @@
     "MixinItemStack",
     "MixinLivingEntity",
     "MixinPlayerEntity",
-    "MixinRecipeManager"
+    "MixinRecipeManager",
+	"MixinServerPlayerEntity"
   ],
   "injectors": {
     "defaultRequire": 1