neoforged / NeoForge

Neo Modding API for Minecraft, based on Forge
https://projects.neoforged.net/neoforged/neoforge
Other
1.1k stars 161 forks source link

Make the `AnvilUpdateEvent` fire after computing the vanilla result #736

Open Shadows-of-Fire opened 4 months ago

Shadows-of-Fire commented 4 months ago

A common pattern I see in usage of the AnvilUpdateEvent is adjusting what vanilla would do in certain scenarios. With the current implementation, this requires copying the entire vanilla code path and recomputing the output, then modifying it.

If we fire the event after the vanilla result is computed, with access to the vanilla result, we can better permit this usecase.

Shadows-of-Fire commented 4 months ago

Note this is sort of annoying to solve - the ideal would be to move the event firing to the end of AnvilMenu#createResult, but that method has three early return statements that cannot trivially be bypassed.

It may be possible to wrap the entire method in a lambda or delegate it to another "fake" method (we do this in some other places) like this:

@ -0,0 +1,31 @@
diff --git a/patches/net/minecraft/world/inventory/AnvilMenu.java.patch b/patches/net/minecraft/world/inventory/AnvilMenu.java.patch
index 2c73a1207..9af5b5bf0 100644
--- a/patches/net/minecraft/world/inventory/AnvilMenu.java.patch
+++ b/patches/net/minecraft/world/inventory/AnvilMenu.java.patch
@@ -18,13 +18,24 @@
                  BlockState blockstate1 = AnvilBlock.damage(blockstate);
                  if (blockstate1 == null) {
                      p_150479_.removeBlock(p_150480_, false);
-@@ -124,8 +_,11 @@
+@@ -110,6 +_,11 @@
+ 
+     @Override
+     public void createResult() {
++        createVanillaResult();
++        net.neoforged.neoforge.common.CommonHooks.onAnvilChange(this, inputSlots, resultSlots, itemName, this.cost, this.player);        
++    }
++        
++    public void createVanillaResult() {
+         ItemStack itemstack = this.inputSlots.getItem(0);
+         this.cost.set(1);
+         int i = 0;
+@@ -124,8 +_,10 @@
              Map<Enchantment, Integer> map = EnchantmentHelper.getEnchantments(itemstack1);
              j += itemstack.getBaseRepairCost() + (itemstack2.isEmpty() ? 0 : itemstack2.getBaseRepairCost());
              this.repairItemCountCost = 0;
 +            boolean flag = false;
 +
-+            if (!net.neoforged.neoforge.common.CommonHooks.onAnvilChange(this, itemstack, itemstack2, resultSlots, itemName, j, this.player)) return;
              if (!itemstack2.isEmpty()) {
 -                boolean flag = itemstack2.is(Items.ENCHANTED_BOOK) && !EnchantedBookItem.getEnchantments(itemstack2).isEmpty();
 +                flag = itemstack2.getItem() == Items.ENCHANTED_BOOK && !EnchantedBookItem.getEnchantments(itemstack2).isEmpty();
Fuzss commented 4 months ago

Would love to see this change being made. Every time I've used this event so far it would have always been better to already have the vanilla result computed for applying some adjustments.

When this is changed please also look at GrindstoneEvent.OnPlaceItem which is basically the same event for grindstones, which also currently runs before the vanilla result is computed.