From 8998c219a1cc4861e6938e18d7fca014e30ee0c7 Mon Sep 17 00:00:00 2001 From: Dennis C Date: Mon, 25 Dec 2023 20:47:13 +0100 Subject: [PATCH 1/3] [1.20.4] Reimplement ID offset on modded EntityDataSerializers (#425) This reimplements the ID offset previously applied to modded `EntityDataSerializer`s which was lost during the registry rework. Previously this was achieved by setting a minimum ID on the registry. Due to the modified vanilla registries not supporting that, the IDs of modded serializers now also start at 0 which leads to the client using the vanilla serializer corresponding to the ID of the modded one and potentially blowing up while deserializing the data item, i.e. by reading past the buffer's length. --- .../syncher/EntityDataSerializers.java.patch | 17 ++- .../neoforge/common/CommonHooks.java | 9 +- .../entity/EntityDataSerializerTest.java | 107 ++++++++++++++++++ 3 files changed, 129 insertions(+), 4 deletions(-) create mode 100644 tests/src/main/java/net/neoforged/neoforge/debug/entity/EntityDataSerializerTest.java diff --git a/patches/net/minecraft/network/syncher/EntityDataSerializers.java.patch b/patches/net/minecraft/network/syncher/EntityDataSerializers.java.patch index 4e23daad93..7812a36787 100644 --- a/patches/net/minecraft/network/syncher/EntityDataSerializers.java.patch +++ b/patches/net/minecraft/network/syncher/EntityDataSerializers.java.patch @@ -1,11 +1,24 @@ --- a/net/minecraft/network/syncher/EntityDataSerializers.java +++ b/net/minecraft/network/syncher/EntityDataSerializers.java -@@ -157,16 +_,16 @@ +@@ -156,17 +_,29 @@ + FriendlyByteBuf::writeQuaternion, FriendlyByteBuf::readQuaternion ); ++ private static final org.slf4j.Logger LOGGER = com.mojang.logging.LogUtils.getLogger(); ++ private static final StackWalker STACK_WALKER = StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE); ++ /** ++ * @deprecated NeoForge: Use {@link net.neoforged.neoforge.registries.NeoForgeRegistries#ENTITY_DATA_SERIALIZERS} instead ++ */ ++ @Deprecated public static void registerSerializer(EntityDataSerializer p_135051_) { - SERIALIZERS.add(p_135051_); -+ if (SERIALIZERS.add(p_135051_) >= 256) throw new RuntimeException("Vanilla DataSerializer ID limit exceeded"); ++ if (!STACK_WALKER.getCallerClass().equals(EntityDataSerializers.class)) { ++ // TODO 1.20.5: throw an exception in dev instead ++ LOGGER.error("Modded EntityDataSerializers must be registered to NeoForgeRegistries.ENTITY_DATA_SERIALIZERS instead to prevent ID mismatches between client and server!", new Throwable()); ++ } ++ ++ if (SERIALIZERS.add(p_135051_) >= net.neoforged.neoforge.common.CommonHooks.VANILLA_SERIALIZER_LIMIT) ++ throw new RuntimeException("Vanilla EntityDataSerializer ID limit exceeded"); } @Nullable diff --git a/src/main/java/net/neoforged/neoforge/common/CommonHooks.java b/src/main/java/net/neoforged/neoforge/common/CommonHooks.java index 31c45580a7..fb5435ec2f 100644 --- a/src/main/java/net/neoforged/neoforge/common/CommonHooks.java +++ b/src/main/java/net/neoforged/neoforge/common/CommonHooks.java @@ -895,11 +895,13 @@ public static boolean hasNoElements(Ingredient ingredient) { @Deprecated(forRemoval = true, since = "1.20.1") // Tags use a codec now This was never used in 1.20 public static void deserializeTagAdditions(List list, JsonObject json, List allList) {} + public static final int VANILLA_SERIALIZER_LIMIT = 256; + @Nullable public static EntityDataSerializer getSerializer(int id, CrudeIncrementalIntIdentityHashBiMap> vanilla) { EntityDataSerializer serializer = vanilla.byId(id); if (serializer == null) { - return NeoForgeRegistries.ENTITY_DATA_SERIALIZERS.byId(id); + return NeoForgeRegistries.ENTITY_DATA_SERIALIZERS.byId(id - VANILLA_SERIALIZER_LIMIT); } return serializer; } @@ -907,7 +909,10 @@ public static EntityDataSerializer getSerializer(int id, CrudeIncrementalIntI public static int getSerializerId(EntityDataSerializer serializer, CrudeIncrementalIntIdentityHashBiMap> vanilla) { int id = vanilla.getId(serializer); if (id < 0) { - return NeoForgeRegistries.ENTITY_DATA_SERIALIZERS.getId(serializer); + id = NeoForgeRegistries.ENTITY_DATA_SERIALIZERS.getId(serializer); + if (id >= 0) { + return id + VANILLA_SERIALIZER_LIMIT; + } } return id; } diff --git a/tests/src/main/java/net/neoforged/neoforge/debug/entity/EntityDataSerializerTest.java b/tests/src/main/java/net/neoforged/neoforge/debug/entity/EntityDataSerializerTest.java new file mode 100644 index 0000000000..43a2b63efc --- /dev/null +++ b/tests/src/main/java/net/neoforged/neoforge/debug/entity/EntityDataSerializerTest.java @@ -0,0 +1,107 @@ +/* + * Copyright (c) NeoForged and contributors + * SPDX-License-Identifier: LGPL-2.1-only + */ + +package net.neoforged.neoforge.debug.entity; + +import io.netty.buffer.Unpooled; +import net.minecraft.client.renderer.entity.EntityRenderer; +import net.minecraft.client.renderer.entity.EntityRendererProvider; +import net.minecraft.gametest.framework.GameTest; +import net.minecraft.nbt.CompoundTag; +import net.minecraft.network.FriendlyByteBuf; +import net.minecraft.network.protocol.game.ClientboundSetEntityDataPacket; +import net.minecraft.network.syncher.EntityDataAccessor; +import net.minecraft.network.syncher.EntityDataSerializer; +import net.minecraft.network.syncher.SynchedEntityData; +import net.minecraft.resources.ResourceLocation; +import net.minecraft.world.entity.Entity; +import net.minecraft.world.entity.EntityType; +import net.minecraft.world.entity.MobCategory; +import net.minecraft.world.level.Level; +import net.neoforged.neoforge.common.CommonHooks; +import net.neoforged.neoforge.registries.DeferredHolder; +import net.neoforged.neoforge.registries.NeoForgeRegistries; +import net.neoforged.testframework.DynamicTest; +import net.neoforged.testframework.TestFramework; +import net.neoforged.testframework.annotation.ForEachTest; +import net.neoforged.testframework.annotation.OnInit; +import net.neoforged.testframework.annotation.TestHolder; +import net.neoforged.testframework.gametest.EmptyTemplate; +import net.neoforged.testframework.registration.RegistrationHelper; + +@ForEachTest(groups = EntityDataSerializerTest.GROUP) +public class EntityDataSerializerTest { + public static final String GROUP = "level.entity.data_serializer"; + + private static final RegistrationHelper REG_HELPER = RegistrationHelper.create("neotests_custom_entity_data_serializer"); + private static final DeferredHolder, EntityDataSerializer> TEST_SERIALIZER = REG_HELPER + .registrar(NeoForgeRegistries.Keys.ENTITY_DATA_SERIALIZERS) + .register("test_serializer", () -> EntityDataSerializer.simple((buf, b) -> buf.writeByte(b), FriendlyByteBuf::readByte)); + + @OnInit + static void register(final TestFramework framework) { + REG_HELPER.register(framework.modEventBus()); + } + + @GameTest + @EmptyTemplate(floor = true) + @TestHolder(description = "Tests if custom EntityDataSerializers are properly handled") + static void customEntityDataSerializer(final DynamicTest test, final RegistrationHelper reg) { + var testEntity = reg.entityTypes().registerType("serializer_test_entity", () -> EntityType.Builder.of(TestEntity::new, MobCategory.CREATURE) + .sized(1, 1)).withRenderer(() -> TestEntityRenderer::new); + + test.onGameTest(helper -> { + var entity = helper.spawn(testEntity.get(), 1, 2, 1); + var items = entity.getEntityData().packDirty(); + if (items == null) { + helper.fail("Expected dirty entity data, got none"); + return; + } + var pkt = new ClientboundSetEntityDataPacket(entity.getId(), items); + FriendlyByteBuf buf = new FriendlyByteBuf(Unpooled.buffer()); + pkt.write(buf); + helper.assertTrue(buf.readVarInt() == entity.getId(), "Entity ID didn't match"); // Drop entity ID + buf.readByte(); // Drop item ID + int expectedId = NeoForgeRegistries.ENTITY_DATA_SERIALIZERS.getId(TEST_SERIALIZER.get()) + CommonHooks.VANILLA_SERIALIZER_LIMIT; + helper.assertTrue(buf.readVarInt() == expectedId, "Serializer ID didn't match"); + buf.readByte(); // Drop data + buf.readByte(); // Drop EOF marker + helper.assertTrue(buf.readableBytes() == 0, "Buffer not empty"); + + helper.succeed(); + }); + } + + private static class TestEntity extends Entity { + private static final EntityDataAccessor DATA_TEST_VALUE = SynchedEntityData.defineId(TestEntity.class, TEST_SERIALIZER.value()); + + public TestEntity(EntityType entityType, Level level) { + super(entityType, level); + entityData.set(DATA_TEST_VALUE, (byte) 1); + } + + @Override + protected void defineSynchedData() { + entityData.define(DATA_TEST_VALUE, (byte) 0); + } + + @Override + protected void readAdditionalSaveData(CompoundTag tag) {} + + @Override + protected void addAdditionalSaveData(CompoundTag tag) {} + } + + private static class TestEntityRenderer extends EntityRenderer { + public TestEntityRenderer(EntityRendererProvider.Context context) { + super(context); + } + + @Override + public ResourceLocation getTextureLocation(TestEntity entity) { + return null; + } + } +} From 4e5aa14612fe12c5c1b8f8fd04fe4b318a19500f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20K=C3=A4mpf?= <48529807+PlatinPython@users.noreply.github.com> Date: Tue, 26 Dec 2023 14:04:08 +0100 Subject: [PATCH 2/3] Bump CoreMods version. (#426) Update CoreMods version to 6.0.4 which uses the new SPI artifact. This fixes the duplicate SPI dependencies which caused a `NoClassDefFoundError` in certain situations. --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index a105049e37..0690c7bccf 100644 --- a/gradle.properties +++ b/gradle.properties @@ -14,7 +14,7 @@ neoform_version=20231207.154220 mergetool_version=2.0.0 accesstransformers_version=10.0.1 -coremods_version=6.0.2 +coremods_version=6.0.4 eventbus_version=7.2.0 modlauncher_version=10.0.9 securejarhandler_version=2.1.24 From 97e83fae7cb310ca224635566510ab36e3ff9871 Mon Sep 17 00:00:00 2001 From: Technici4n <13494793+Technici4n@users.noreply.github.com> Date: Tue, 26 Dec 2023 17:53:52 +0100 Subject: [PATCH 3/3] Remove conditional recipe serializer, use regular conditions instead (#427) --- .../item/crafting/RecipeManager.java.patch | 2 +- .../neoforge/common/NeoForgeMod.java | 6 --- .../common/crafting/ConditionalRecipe.java | 38 ----------------- .../common/crafting/CraftingHelper.java | 41 ------------------- 4 files changed, 1 insertion(+), 86 deletions(-) delete mode 100644 src/main/java/net/neoforged/neoforge/common/crafting/ConditionalRecipe.java diff --git a/patches/net/minecraft/world/item/crafting/RecipeManager.java.patch b/patches/net/minecraft/world/item/crafting/RecipeManager.java.patch index 5e4817ff50..d656135e56 100644 --- a/patches/net/minecraft/world/item/crafting/RecipeManager.java.patch +++ b/patches/net/minecraft/world/item/crafting/RecipeManager.java.patch @@ -34,7 +34,7 @@ + + public static Optional> fromJson(ResourceLocation p_44046_, JsonObject p_44047_, com.mojang.serialization.DynamicOps jsonElementOps) { + Optional> recipe = net.neoforged.neoforge.common.conditions.ICondition.getWithWithConditionsCodec(net.neoforged.neoforge.common.util.NeoForgeExtraCodecs.CONDITIONAL_RECIPE_CODEC, jsonElementOps, p_44047_); -+ return recipe.filter(r -> r != net.neoforged.neoforge.common.crafting.CraftingHelper.EMPTY_RECIPE).map(r -> new RecipeHolder<>(p_44046_, r)); ++ return recipe.map(r -> new RecipeHolder<>(p_44046_, r)); } public void replaceRecipes(Iterable> p_44025_) { diff --git a/src/main/java/net/neoforged/neoforge/common/NeoForgeMod.java b/src/main/java/net/neoforged/neoforge/common/NeoForgeMod.java index 93f8ac8f14..61794f6780 100644 --- a/src/main/java/net/neoforged/neoforge/common/NeoForgeMod.java +++ b/src/main/java/net/neoforged/neoforge/common/NeoForgeMod.java @@ -53,7 +53,6 @@ import net.minecraft.world.item.ItemDisplayContext; import net.minecraft.world.item.Items; import net.minecraft.world.item.crafting.Ingredient; -import net.minecraft.world.item.crafting.RecipeSerializer; import net.minecraft.world.level.BlockAndTintGetter; import net.minecraft.world.level.BlockGetter; import net.minecraft.world.level.GameRules; @@ -94,7 +93,6 @@ import net.neoforged.neoforge.common.conditions.TagEmptyCondition; import net.neoforged.neoforge.common.conditions.TrueCondition; import net.neoforged.neoforge.common.crafting.CompoundIngredient; -import net.neoforged.neoforge.common.crafting.ConditionalRecipe; import net.neoforged.neoforge.common.crafting.DifferenceIngredient; import net.neoforged.neoforge.common.crafting.IngredientType; import net.neoforged.neoforge.common.crafting.IntersectionIngredient; @@ -287,9 +285,6 @@ public class NeoForgeMod { public static final DeferredHolder, IngredientType> DIFFERENCE_INGREDIENT_TYPE = INGREDIENT_TYPES.register("difference", () -> new IngredientType<>(DifferenceIngredient.CODEC, DifferenceIngredient.CODEC_NONEMPTY)); public static final DeferredHolder, IngredientType> INTERSECTION_INGREDIENT_TYPE = INGREDIENT_TYPES.register("intersection", () -> new IngredientType<>(IntersectionIngredient.CODEC, IntersectionIngredient.CODEC_NONEMPTY)); - private static final DeferredRegister> RECIPE_SERIALIZERS = DeferredRegister.create(Registries.RECIPE_SERIALIZER, "neoforge"); - public static final DeferredHolder, ConditionalRecipe> CONDITIONAL_RECIPE = RECIPE_SERIALIZERS.register("conditional", ConditionalRecipe::new); - private static final DeferredRegister> CONDITION_CODECS = DeferredRegister.create(NeoForgeRegistries.Keys.CONDITION_CODECS, "neoforge"); public static final DeferredHolder, Codec> AND_CONDITION = CONDITION_CODECS.register("and", () -> AndCondition.CODEC); public static final DeferredHolder, Codec> FALSE_CONDITION = CONDITION_CODECS.register("false", () -> FalseCondition.CODEC); @@ -487,7 +482,6 @@ public NeoForgeMod(IEventBus modEventBus, Dist dist) { VANILLA_INGREDIENT_TYPES.register(modEventBus); INGREDIENT_TYPES.register(modEventBus); CONDITION_CODECS.register(modEventBus); - RECIPE_SERIALIZERS.register(modEventBus); NeoForge.EVENT_BUS.addListener(this::serverStopping); ModLoadingContext.get().registerConfig(ModConfig.Type.CLIENT, NeoForgeConfig.clientSpec); ModLoadingContext.get().registerConfig(ModConfig.Type.SERVER, NeoForgeConfig.serverSpec); diff --git a/src/main/java/net/neoforged/neoforge/common/crafting/ConditionalRecipe.java b/src/main/java/net/neoforged/neoforge/common/crafting/ConditionalRecipe.java deleted file mode 100644 index 73f4b5b348..0000000000 --- a/src/main/java/net/neoforged/neoforge/common/crafting/ConditionalRecipe.java +++ /dev/null @@ -1,38 +0,0 @@ -/* - * Copyright (c) NeoForged and contributors - * SPDX-License-Identifier: LGPL-2.1-only - */ - -package net.neoforged.neoforge.common.crafting; - -import com.mojang.serialization.Codec; -import java.util.List; -import java.util.Optional; -import java.util.function.Function; -import net.minecraft.network.FriendlyByteBuf; -import net.minecraft.world.item.crafting.Recipe; -import net.minecraft.world.item.crafting.RecipeSerializer; -import net.neoforged.neoforge.common.conditions.WithConditions; -import net.neoforged.neoforge.common.util.NeoForgeExtraCodecs; - -public class ConditionalRecipe> implements RecipeSerializer { - public static final Codec> CONDITIONAL_RECIPES_CODEC = NeoForgeExtraCodecs.CONDITIONAL_RECIPE_CODEC.listOf().fieldOf("recipes").codec() - .xmap(optionals -> optionals.stream().filter(Optional::isPresent).findFirst().flatMap(Function.identity()).>map(WithConditions::carrier).orElse(CraftingHelper.EMPTY_RECIPE), - r -> List.of(Optional.of(new WithConditions<>(r)))); - - @Override - public Codec codec() { - return (Codec) CONDITIONAL_RECIPES_CODEC; - } - - // Should never get here as it's a wrapper - @Override - public T fromNetwork(FriendlyByteBuf p_44106_) { - throw new UnsupportedOperationException("Attempted to read conditional recipe from network; this is a wrapper class!"); - } - - @Override - public void toNetwork(FriendlyByteBuf p_44101_, T p_44102_) { - - } -} diff --git a/src/main/java/net/neoforged/neoforge/common/crafting/CraftingHelper.java b/src/main/java/net/neoforged/neoforge/common/crafting/CraftingHelper.java index 4e55844bac..f6ed9b4cb6 100644 --- a/src/main/java/net/neoforged/neoforge/common/crafting/CraftingHelper.java +++ b/src/main/java/net/neoforged/neoforge/common/crafting/CraftingHelper.java @@ -5,24 +5,16 @@ package net.neoforged.neoforge.common.crafting; -import com.google.gson.Gson; -import com.google.gson.GsonBuilder; import com.mojang.datafixers.util.Either; import com.mojang.serialization.Codec; import com.mojang.serialization.DataResult; import java.util.function.Function; -import net.minecraft.core.RegistryAccess; import net.minecraft.core.registries.BuiltInRegistries; import net.minecraft.nbt.CompoundTag; import net.minecraft.nbt.TagParser; import net.minecraft.util.ExtraCodecs; -import net.minecraft.world.Container; import net.minecraft.world.item.ItemStack; import net.minecraft.world.item.crafting.Ingredient; -import net.minecraft.world.item.crafting.Recipe; -import net.minecraft.world.item.crafting.RecipeSerializer; -import net.minecraft.world.item.crafting.RecipeType; -import net.minecraft.world.level.Level; import net.neoforged.neoforge.common.NeoForgeMod; import net.neoforged.neoforge.common.util.NeoForgeExtraCodecs; import net.neoforged.neoforge.registries.NeoForgeRegistries; @@ -38,39 +30,6 @@ public class CraftingHelper { private static final Logger LOGGER = LogManager.getLogger(); @SuppressWarnings("unused") private static final Marker CRAFTHELPER = MarkerManager.getMarker("CRAFTHELPER"); - private static Gson GSON = new GsonBuilder().setPrettyPrinting().disableHtmlEscaping().create(); - public static final Recipe EMPTY_RECIPE = new Recipe<>() { - @Override - public boolean matches(Container p_44002_, Level p_44003_) { - return false; - } - - @Override - public ItemStack assemble(Container p_44001_, RegistryAccess p_267165_) { - return ItemStack.EMPTY; - } - - @Override - public boolean canCraftInDimensions(int p_43999_, int p_44000_) { - return false; - } - - @Override - public ItemStack getResultItem(RegistryAccess p_267052_) { - return ItemStack.EMPTY; - } - - @Override - public RecipeSerializer getSerializer() { - throw new UnsupportedOperationException("Empty recipe has no serializer"); - } - - @Override - public RecipeType getType() { - throw new UnsupportedOperationException("Empty recipe has no type"); - } - }; - public static final Codec TAG_CODEC = ExtraCodecs.withAlternative(TagParser.AS_CODEC, net.minecraft.nbt.CompoundTag.CODEC); @ApiStatus.Internal