Fix #110: Pipes correctly handle item NBT and attachments (#112)

gradle.properties

@@ -19,7 +19,7 @@ curseforge_project = 552758
 modrinth_project = fMpvLrnF
 
 # Dependencies
-neoforge_version=20.4.190
+neoforge_version=20.4.235
 
 #########################################################
 # Parchment                                             #

src/main/java/dev/technici4n/moderndynamics/client/ber/FluidPipeRendering.java

@@ -61,7 +61,7 @@ public static void drawFluidInPipe(PipeBlockEntity pipe, PoseStack ms, MultiBuff
             return;
         }
 
-        int color = renderProps.getTintColor(fluid.fluid().defaultFluidState(), level, pos);
+        int color = renderProps.getTintColor(fluid.getFluid().defaultFluidState(), level, pos);
         float r = ((color >> 16) & 255) / 256f;
         float g = ((color >> 8) & 255) / 256f;
         float b = (color & 255) / 256f;

src/main/java/dev/technici4n/moderndynamics/client/compat/emi/DragDropHandler.java

@@ -48,7 +48,7 @@ public boolean dropStack(Screen gui, EmiIngredient dragged, int mouseX, int mous
         if (gui instanceof ItemAttachedIoScreen ioScreen && ing.getKey() instanceof Item i) {
             for (var s : ioScreen.getMenu().slots) {
                 if (s instanceof ItemConfigSlot slot && slot.isActive() && getSlotBounds(s, ioScreen).contains(mouseX, mouseY)) {
-                    var iv = ItemVariant.of(i, ing.getNbt());
+                    var iv = ItemVariant.of(ing.getItemStack());
                     ioScreen.getMenu().setFilter(slot.getConfigIdx(), iv, true);
                     return true;
                 }

src/main/java/dev/technici4n/moderndynamics/test/ItemTransferTest.java

@@ -0,0 +1,58 @@
+/*
+ * Modern Dynamics
+ * Copyright (C) 2021 shartte & Technici4n
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package dev.technici4n.moderndynamics.test;
+
+import dev.technici4n.moderndynamics.init.MdBlocks;
+import dev.technici4n.moderndynamics.test.framework.MdGameTestHelper;
+import net.minecraft.core.BlockPos;
+import net.minecraft.world.item.ItemStack;
+import net.minecraft.world.item.Items;
+import net.minecraft.world.level.block.Blocks;
+import net.minecraft.world.level.block.entity.ChestBlockEntity;
+import net.minecraft.world.level.block.entity.HopperBlockEntity;
+
+public class ItemTransferTest {
+    @MdGameTest
+    public void testHopperInsertingDamagedItem(MdGameTestHelper helper) {
+        var targetChest = new BlockPos(0, 1, 0);
+        helper.setBlock(targetChest, Blocks.CHEST.defaultBlockState());
+        var chest = (ChestBlockEntity) helper.getBlockEntity(targetChest);
+
+        var pipe = targetChest.east();
+        helper.pipe(pipe, MdBlocks.ITEM_PIPE);
+
+        var hopper = pipe.above();
+        helper.setBlock(hopper, Blocks.HOPPER.defaultBlockState());
+
+        var damagedItem = Items.DIAMOND_PICKAXE.getDefaultInstance();
+        damagedItem.setDamageValue(500);
+        ((HopperBlockEntity) helper.getBlockEntity(hopper)).setItem(0, damagedItem.copy());
+
+        helper.startSequence()
+                .thenWaitUntil(() -> {
+                    if (chest.getItem(0).isEmpty()) {
+                        helper.fail("Expected item in chest", targetChest);
+                    }
+                    if (!ItemStack.matches(damagedItem, chest.getItem(0))) {
+                        helper.fail("Wrong item in chest", targetChest);
+                    }
+                })
+                .thenSucceed();
+    }
+}

src/main/java/dev/technici4n/moderndynamics/test/MdGameTests.java

@@ -31,7 +31,8 @@
 public class MdGameTests {
     private final List<Class<?>> testClasses = List.of(
             FluidTransferTest.class,
-            ItemDistributionTest.class);
+            ItemDistributionTest.class,
+            ItemTransferTest.class);
 
     @GameTestGenerator
     public List<TestFunction> generateTests() {

src/main/java/dev/technici4n/moderndynamics/util/FluidVariant.java

@@ -26,133 +26,108 @@
 import net.minecraft.nbt.CompoundTag;
 import net.minecraft.network.FriendlyByteBuf;
 import net.minecraft.network.chat.Component;
-import net.minecraft.resources.ResourceLocation;
-import net.minecraft.world.level.material.FlowingFluid;
 import net.minecraft.world.level.material.Fluid;
 import net.minecraft.world.level.material.Fluids;
 import net.neoforged.fml.ModList;
 import net.neoforged.neoforge.fluids.FluidStack;
+import org.jetbrains.annotations.ApiStatus;
 import org.jetbrains.annotations.Nullable;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
-public final class FluidVariant {
-    private static final Logger LOG = LoggerFactory.getLogger(FluidVariant.class);
-
-    private final Fluid fluid;
-    private final @Nullable CompoundTag nbt;
-    private final int hashCode;
-
-    private FluidVariant(Fluid fluid, @Nullable CompoundTag nbt) {
-        this.fluid = fluid;
-        this.nbt = nbt != null ? nbt.copy() : null; // defensive copy
-        this.hashCode = Objects.hash(fluid, nbt);
-    }
-
-    public FluidVariant(Fluid fluid) {
-        this(fluid, null);
-    }
-
-    public static FluidVariant blank() {
-        return new FluidVariant(Fluids.EMPTY, null);
-    }
-
-    public static FluidVariant of(Fluid fluid, @Nullable CompoundTag nbt) {
-        Objects.requireNonNull(fluid, "Fluid may not be null.");
-
-        if (!fluid.isSource(fluid.defaultFluidState()) && fluid != Fluids.EMPTY) {
-            // Note: the empty fluid is not still, that's why we check for it specifically.
-
-            if (fluid instanceof FlowingFluid flowable) {
-                // Normalize FlowableFluids to their still variants.
-                fluid = flowable.getSource();
-            } else {
-                // If not a FlowableFluid, we don't know how to convert -> crash.
-                ResourceLocation id = BuiltInRegistries.FLUID.getKey(fluid);
-                throw new IllegalArgumentException("Cannot convert flowing fluid %s (%s) into a still fluid.".formatted(id, fluid));
-            }
-        }
-
-        if (nbt == null || fluid == Fluids.EMPTY) {
-            // Use the cached variant inside the fluid
-            return new FluidVariant(fluid, null); // TODO noTagCache.computeIfAbsent(fluid, f -> new FluidVariant(fluid, null));
-        } else {
-            // TODO explore caching fluid variants for non null tags.
-            return new FluidVariant(fluid, nbt);
-        }
-    }
-
-    public static FluidVariant of(Fluid fluid) {
+/**
+ * An immutable association of a still fluid and an optional NBT tag.
+ *
+ * <p>
+ * Do not extend this class. Use {@link #of(Fluid)} and {@link #of(Fluid, NbtCompound)} to create instances.
+ *
+ * <p>
+ * {@link aztech.modern_industrialization.thirdparty.fabrictransfer.api.client.fluid.FluidVariantRendering} can be used for client-side rendering of
+ * fluid variants.
+ *
+ * <p>
+ * <b>Fluid variants must always be compared with {@code equals}, never by reference!</b>
+ * {@code hashCode} is guaranteed to be correct and constant time independently of the size of the NBT.
+ */
+@ApiStatus.NonExtendable
+public interface FluidVariant extends TransferVariant<Fluid> {
+    /**
+     * Retrieve a blank FluidVariant.
+     */
+    static FluidVariant blank() {
+        return of(Fluids.EMPTY);
+    }
+
+    /**
+     * Retrieve an ItemVariant with the item and tag of a stack.
+     */
+    static FluidVariant of(FluidStack stack) {
+        return of(stack.getFluid(), stack.getTag());
+    }
+
+    /**
+     * Retrieve a FluidVariant with a fluid, and a {@code null} tag.
+     *
+     * <p>
+     * The flowing and still variations of {@linkplain net.minecraft.fluid.FlowableFluid flowable fluids}
+     * are normalized to always refer to the still variant. For example,
+     * {@code FluidVariant.of(Fluids.FLOWING_WATER).getFluid() == Fluids.WATER}.
+     */
+    static FluidVariant of(Fluid fluid) {
         return of(fluid, null);
     }
 
-    public static FluidVariant of(FluidStack resource) {
-        return of(resource.getFluid(), resource.getTag());
-    }
-
-    public CompoundTag toNbt() {
-        CompoundTag result = new CompoundTag();
-        result.putString("fluid", BuiltInRegistries.FLUID.getKey(fluid).toString());
-
-        if (nbt != null) {
-            result.put("tag", nbt.copy());
-        }
-
-        return result;
-    }
-
-    public static FluidVariant fromNbt(CompoundTag compound) {
-        try {
-            Fluid fluid = BuiltInRegistries.FLUID.get(new ResourceLocation(compound.getString("fluid")));
-            CompoundTag nbt = compound.contains("tag") ? compound.getCompound("tag") : null;
-            return of(fluid, nbt);
-        } catch (RuntimeException runtimeException) {
-            LOG.debug("Tried to load an invalid FluidVariant from NBT: {}", compound, runtimeException);
-            return FluidVariant.blank();
-        }
-    }
-
-    public void toPacket(FriendlyByteBuf buf) {
-        if (isBlank()) {
-            buf.writeBoolean(false);
-        } else {
-            buf.writeBoolean(true);
-            buf.writeVarInt(BuiltInRegistries.FLUID.getId(fluid));
-            buf.writeNbt(nbt);
-        }
-    }
-
-    public static FluidVariant fromPacket(FriendlyByteBuf buf) {
-        if (!buf.readBoolean()) {
-            return FluidVariant.blank();
-        } else {
-            Fluid fluid = BuiltInRegistries.FLUID.byId(buf.readVarInt());
-            CompoundTag nbt = buf.readNbt();
-            return of(fluid, nbt);
-        }
+    /**
+     * Retrieve a FluidVariant with a fluid, and an optional tag.
+     *
+     * <p>
+     * The flowing and still variations of {@linkplain net.minecraft.fluid.FlowableFluid flowable fluids}
+     * are normalized to always refer to the still fluid. For example,
+     * {@code FluidVariant.of(Fluids.FLOWING_WATER, nbt).getFluid() == Fluids.WATER}.
+     */
+    static FluidVariant of(Fluid fluid, @Nullable CompoundTag nbt) {
+        return FluidVariantImpl.of(fluid, nbt);
     }
 
-    public boolean isBlank() {
-        return fluid.isSame(Fluids.EMPTY);
+    /**
+     * Return the fluid of this variant.
+     */
+    default Fluid getFluid() {
+        return getObject();
     }
 
-    public boolean matches(FluidStack stack) {
-        return fluid == stack.getFluid() && Objects.equals(nbt, stack.getTag());
+    /**
+     * Create a new fluid stack from this variant.
+     */
+    default FluidStack toStack(int count) {
+        if (isBlank())
+            return FluidStack.EMPTY;
+        FluidStack stack = new FluidStack(getFluid(), count);
+        stack.setTag(copyNbt());
+        return stack;
     }
 
-    public Fluid getFluid() {
-        return fluid();
+    /**
+     * Deserialize a variant from an NBT compound tag, assuming it was serialized using {@link #toNbt}.
+     *
+     * <p>
+     * If an error occurs during deserialization, it will be logged with the DEBUG level, and a blank variant will be returned.
+     */
+    static FluidVariant fromNbt(CompoundTag nbt) {
+        return FluidVariantImpl.fromNbt(nbt);
     }
 
-    public FluidStack toStack(int amount) {
-        return new FluidStack(fluid, amount, nbt != null ? nbt.copy() : null);
+    /**
+     * Read a variant from a packet byte buffer, assuming it was serialized using {@link #toPacket}.
+     */
+    static FluidVariant fromPacket(FriendlyByteBuf buf) {
+        return FluidVariantImpl.fromPacket(buf);
     }
 
-    public List<Component> getTooltip() {
+    // Not in MI variants:
+    default List<Component> getTooltip() {
         var tooltip = new ArrayList<Component>();
         tooltip.add(toStack(1).getDisplayName());
 
-        var modId = BuiltInRegistries.FLUID.getKey(fluid).getNamespace();
+        var modId = BuiltInRegistries.FLUID.getKey(getFluid()).getNamespace();
 
         // Heuristic: If the last line doesn't include the modname, add it ourselves
         var modName = formatModName(modId);
@@ -172,40 +147,7 @@ private static String getModName(String modId) {
                 .orElse(modId);
     }
 
-    public @Nullable CompoundTag getNbt() {
-        return nbt != null ? nbt.copy() : null;
+    default boolean matches(FluidStack stack) {
+        return getFluid() == stack.getFluid() && Objects.equals(getNbt(), stack.getTag());
     }
-
-    public Fluid fluid() {
-        return fluid;
-    }
-
-    public @Nullable CompoundTag nbt() {
-        return nbt;
-    }
-
-    @Override
-    public boolean equals(Object obj) {
-        if (obj == this)
-            return true;
-        if (obj == null || obj.getClass() != this.getClass())
-            return false;
-        var that = (FluidVariant) obj;
-        return hashCode == that.hashCode
-                && Objects.equals(this.fluid, that.fluid)
-                && Objects.equals(this.nbt, that.nbt);
-    }
-
-    @Override
-    public int hashCode() {
-        return hashCode;
-    }
-
-    @Override
-    public String toString() {
-        return "FluidVariant[" +
-                "fluid=" + fluid + ", " +
-                "nbt=" + nbt + ']';
-    }
-
 }