Fix mutating ItemData NBT (#1762)

* Fix mutating ItemData NBT

Fix for a very old bug where we were overwriting NBT in stored `ItemData` every time we called `GT_OreDictUnificator.get()` or `GT_OreDictUnificator.get_nocopy()`

Maybe fixes:
 * https://github.com/GTNewHorizons/GT-New-Horizons-Modpack/issues/12678

Please review this carefully, since it's very difficult to properly test changes to this part of the codebase.

In `get_nocopy()`, since we now make a copy, I was able to set stack size too. Also I removed `assert rStack != null` since it is already checked by `GT_Utility.isStackInvalid()`.

In `isInputStackEqual()`, I just deleted `rStack.setTagCompound()` since the only reference to `rStack` is in `GT_Utility.areStacksEqual()`, which has `aIgnoreNBT == true` so setting NBT does nothing.

* Don't copy if no NBT is present

1 files changed, 29 insertions, 9 deletions

diff --git a/src/main/java/gregtech/api/util/GT_OreDictUnificator.java b/src/main/java/gregtech/api/util/GT_OreDictUnificator.java
index a0422a5a0b..47f70a9c35 100644
--- a/src/main/java/gregtech/api/util/GT_OreDictUnificator.java
+++ b/src/main/java/gregtech/api/util/GT_OreDictUnificator.java
@@ -187,12 +187,15 @@ public class GT_OreDictUnificator {
             tPrefixMaterial.mUnificationTarget = sName2StackMap.get(tPrefixMaterial.toString());
         ItemStack rStack = tPrefixMaterial.mUnificationTarget;
         if (GT_Utility.isStackInvalid(rStack)) return GT_Utility.copyOrNull(aStack);
-        rStack.setTagCompound(aStack.getTagCompound());
+        ItemStack newStack;
         if (unsafe) {
-            return GT_Utility.copyAmountUnsafe(aStack.stackSize, rStack);
+            newStack = GT_Utility.copyAmountUnsafe(aStack.stackSize, rStack);
         } else {
-            return GT_Utility.copyAmount(aStack.stackSize, rStack);
+            newStack = GT_Utility.copyAmount(aStack.stackSize, rStack);
         }
+        // NBT is assigned by reference here, so mutating it may have unexpected side effects.
+        newStack.setTagCompound(aStack.getTagCompound());
+        return newStack;
     }
 
     /**
@@ -210,7 +213,6 @@ public class GT_OreDictUnificator {
     static ItemStack get_nocopy(boolean aUseBlackList, ItemStack aStack) {
         if (GT_Utility.isStackInvalid(aStack)) return null;
         ItemData tPrefixMaterial = getAssociation(aStack);
-        ItemStack rStack = null;
         if (tPrefixMaterial == null || !tPrefixMaterial.hasValidPrefixMaterialData()
                 || (aUseBlackList && tPrefixMaterial.mBlackListed))
             return aStack;
@@ -220,11 +222,30 @@ public class GT_OreDictUnificator {
         }
         if (tPrefixMaterial.mUnificationTarget == null)
             tPrefixMaterial.mUnificationTarget = sName2StackMap.get(tPrefixMaterial.toString());
-        rStack = tPrefixMaterial.mUnificationTarget;
+        ItemStack rStack = tPrefixMaterial.mUnificationTarget;
         if (GT_Utility.isStackInvalid(rStack)) return aStack;
-        assert rStack != null;
-        rStack.setTagCompound(aStack.getTagCompound());
-        return rStack;
+
+        // Yes, == and not .equals().
+        // This check is primarily intended to optimize for the case where both rStack and aStack
+        // do not have NBT, and so we would be comparing null == null.
+        //
+        // Even if aStack and rStack may have equal NBT, we prefer to do an inexpensive
+        // new ItemStack() over the potentially expensive NBTTagCompound.equals().
+        if (aStack.getTagCompound() == rStack.getTagCompound()) {
+            // Warning: rStack's stack size may not be equal to aStack's stack size.
+            return rStack;
+        }
+
+        // Okay, okay, I lied, we actually do need to make a copy.
+        // This is to fix a long-standing bug where we were mutating NBT directly on rStack,
+        // which had unexpected and unpredictable ripple effects.
+        //
+        // We will do some custom copying here, to avoid ItemStack.copy(),
+        // which calls the potentially expensive NBTTagCompound.copy()
+        // NBT is assigned by reference here, so mutating it may have unexpected side effects.
+        ItemStack newStack = new ItemStack(rStack.getItem(), aStack.stackSize, Items.feather.getDamage(rStack));
+        newStack.setTagCompound(aStack.getTagCompound());
+        return newStack;
     }
 
     /**
@@ -252,7 +273,6 @@ public class GT_OreDictUnificator {
         rStack = tPrefixMaterial.mUnificationTarget;
         if (GT_Utility.isStackInvalid(rStack))
             return !alreadyCompared && GT_Utility.areStacksEqual(aStack, unified_tStack, true);
-        rStack.setTagCompound(aStack.getTagCompound());
         return GT_Utility.areStacksEqual(rStack, unified_tStack, true);
     }