Maintain object identity in the storage adapter snapshot (#5856)

* Fixes #5849: Maintain object identity in the storage adapter snapshot. * Fixes #5849: Reimplement how we handle transactions / change notifications in the Fabric storage adapter. It now properly maintains referential equality of stacks after aborted transactions.
AppliedEnergistics · Dec 23, 2021 · a0af648 · a0af648
1 parent 57c357c
commit a0af648
Show file tree

Hide file tree

Showing 38 changed files with 284 additions and 143 deletions.
diff --git a/src/main/java/appeng/api/inventories/InternalInventory.java b/src/main/java/appeng/api/inventories/InternalInventory.java
@@ -30,6 +30,8 @@
 
 import com.google.common.base.Preconditions;
 
+import org.jetbrains.annotations.ApiStatus;
+
 import net.fabricmc.fabric.api.transfer.v1.item.ItemStorage;
 import net.fabricmc.fabric.api.transfer.v1.item.ItemVariant;
 import net.fabricmc.fabric.api.transfer.v1.storage.Storage;
@@ -427,4 +429,11 @@ default boolean mayAllowTransfer() {
         return size() > 0;
     }
 
+    /**
+     * Forces a change notification to any listeners for the given slot. For internal use only. This should also cause
+     * the inventory to be saved.
+     */
+    @ApiStatus.Internal
+    default void sendChangeNotification(int slot) {
+    }
 }
diff --git a/src/main/java/appeng/api/inventories/InternalInventoryStorage.java b/src/main/java/appeng/api/inventories/InternalInventoryStorage.java
@@ -1,10 +1,12 @@
 package appeng.api.inventories;
 
-import java.util.ArrayList;
 import java.util.Iterator;
-import java.util.List;
 import java.util.NoSuchElementException;
 
+import javax.annotation.Nullable;
+
+import com.google.common.base.Preconditions;
+
 import net.fabricmc.fabric.api.transfer.v1.item.ItemVariant;
 import net.fabricmc.fabric.api.transfer.v1.storage.Storage;
 import net.fabricmc.fabric.api.transfer.v1.storage.StoragePreconditions;
@@ -15,8 +17,11 @@
 
 import appeng.core.definitions.AEItems;
 
-class InternalInventoryStorage extends SnapshotParticipant<List<ItemStack>> implements Storage<ItemVariant> {
+class InternalInventoryStorage extends SnapshotParticipant<InternalInventoryStorage.Snapshot>
+        implements Storage<ItemVariant> {
     private final InternalInventory inventory;
+    @Nullable
+    private Snapshot lastReleasedSnapshot;
 
     public InternalInventoryStorage(InternalInventory inventory) {
         this.inventory = inventory;
@@ -82,7 +87,6 @@ public long extract(ItemVariant resource, long maxAmount, TransactionContext tra
                         return 0;
                     }
 
-                    // TODO FABRIC 117: DISGUSTING. Must update snapshot only for this slot.
                     updateSnapshots(transaction);
 
                     return inventory.extractItem(currentSlot, (int) Math.min(Integer.MAX_VALUE, maxAmount), false)
@@ -113,19 +117,64 @@ public long getCapacity() {
     }
 
     @Override
-    protected List<ItemStack> createSnapshot() {
-        // TODO FABRIC 117: DISGUSTING.
-        List<ItemStack> snapshot = new ArrayList<>(inventory.size());
+    protected Snapshot createSnapshot() {
+        Snapshot snapshot;
+        if (this.lastReleasedSnapshot != null && this.lastReleasedSnapshot.items.length == inventory.size()) {
+            snapshot = this.lastReleasedSnapshot;
+            this.lastReleasedSnapshot = null;
+        } else {
+            snapshot = new Snapshot();
+        }
+
         for (int i = 0; i < inventory.size(); i++) {
-            snapshot.add(inventory.getStackInSlot(i).copy());
+            var stack = inventory.getStackInSlot(i);
+            snapshot.items[i] = stack;
+            snapshot.counts[i] = stack.getCount();
         }
         return snapshot;
     }
 
     @Override
-    protected void readSnapshot(List<ItemStack> snapshot) {
-        for (int i = 0; i < snapshot.size(); i++) {
-            inventory.setItemDirect(i, snapshot.get(i));
+    protected void readSnapshot(Snapshot snapshot) {
+        var items = snapshot.items;
+        var counts = snapshot.counts;
+        for (int i = 0; i < items.length; i++) {
+            var stack = items[i];
+            // Restore the previous count as well, the inventory might mutate the stack count for extract/insert
+            // We do not restore NBT since the Storage API does not give access to the original NBT and the inventory
+            // doesn't mutate it itself
+            if (stack.getCount() != counts[i]) {
+                stack.setCount(counts[i]);
+            }
+            inventory.setItemDirect(i, stack);
+        }
+    }
+
+    @Override
+    protected void releaseSnapshot(Snapshot snapshot) {
+        this.lastReleasedSnapshot = snapshot;
+    }
+
+    public class Snapshot {
+        ItemStack[] items;
+        int[] counts;
+
+        public Snapshot() {
+            this.items = new ItemStack[inventory.size()];
+            this.counts = new int[inventory.size()];
+        }
+    }
+
+    @Override
+    protected void onFinalCommit() {
+        // Diff the last snapshot against the inventory to collect change notifications
+        Preconditions.checkState(lastReleasedSnapshot != null, "There should have been at least one snapshot");
+
+        for (int i = 0; i < lastReleasedSnapshot.items.length; i++) {
+            var current = inventory.getStackInSlot(i);
+            if (current != lastReleasedSnapshot.items[i] || current.getCount() != lastReleasedSnapshot.counts[i]) {
+                inventory.sendChangeNotification(i);
+            }
         }
     }
 }
diff --git a/src/main/java/appeng/api/inventories/SubInventoryProxy.java b/src/main/java/appeng/api/inventories/SubInventoryProxy.java
@@ -84,9 +84,12 @@ public ItemStack insertItem(int slot, ItemStack stack, boolean simulate) {
     }
 
     @Override
-
     public ItemStack extractItem(int slot, int amount, boolean simulate) {
         return delegate.extractItem(translateSlot(slot), amount, simulate);
     }
 
+    @Override
+    public void sendChangeNotification(int slot) {
+        delegate.sendChangeNotification(translateSlot(slot));
+    }
 }
diff --git a/src/main/java/appeng/blockentity/AEBaseInvBlockEntity.java b/src/main/java/appeng/blockentity/AEBaseInvBlockEntity.java
@@ -84,8 +84,7 @@ public void getDrops(Level level, BlockPos pos, List<ItemStack> drops) {
     }
 
     @Override
-    public abstract void onChangeInventory(InternalInventory inv, int slot, ItemStack removed,
-            ItemStack added);
+    public abstract void onChangeInventory(InternalInventory inv, int slot);
 
     public InternalInventory getExposedInventoryForSide(Direction side) {
         return this.getInternalInventory();

diff --git a/src/main/java/appeng/blockentity/crafting/MolecularAssemblerBlockEntity.java b/src/main/java/appeng/blockentity/crafting/MolecularAssemblerBlockEntity.java
@@ -354,8 +354,7 @@ public InternalInventory getExposedInventoryForSide(Direction side) {
     }
 
     @Override
-    public void onChangeInventory(InternalInventory inv, int slot,
-            ItemStack removed, ItemStack added) {
+    public void onChangeInventory(InternalInventory inv, int slot) {
         if (inv == this.gridInv || inv == this.patternInv) {
             this.recalculatePlan();
         }

diff --git a/src/main/java/appeng/blockentity/inventory/AppEngCellInventory.java b/src/main/java/appeng/blockentity/inventory/AppEngCellInventory.java
@@ -29,20 +29,14 @@
 public class AppEngCellInventory extends BaseInternalInventory {
     private final AppEngInternalInventory inv;
     private final StorageCell[] handlerForSlot;
-    /**
-     * Remembers for which itemstack the handler in handlerForSlot[] was queried.
-     */
-    private final ItemStack[] handlerStackForSlot;
 
     public AppEngCellInventory(InternalInventoryHost host, int slots) {
         this.inv = new AppEngInternalInventory(host, slots, 1);
         this.handlerForSlot = new StorageCell[slots];
-        this.handlerStackForSlot = new ItemStack[slots];
     }
 
     public void setHandler(int slot, StorageCell handler) {
         this.handlerForSlot[slot] = handler;
-        this.handlerStackForSlot[slot] = inv.getStackInSlot(slot);
     }
 
     public void setFilter(IAEItemFilter filter) {
@@ -53,7 +47,6 @@ public void setFilter(IAEItemFilter filter) {
     public void setItemDirect(int slot, ItemStack stack) {
         this.persist(slot);
         this.inv.setItemDirect(slot, stack);
-        this.cleanup(slot);
     }
 
     @Override
@@ -70,17 +63,13 @@ public ItemStack getStackInSlot(int slot) {
     @Override
     public ItemStack insertItem(int slot, ItemStack stack, boolean simulate) {
         this.persist(slot);
-        final ItemStack ret = this.inv.insertItem(slot, stack, simulate);
-        this.cleanup(slot);
-        return ret;
+        return this.inv.insertItem(slot, stack, simulate);
     }
 
     @Override
     public ItemStack extractItem(int slot, int amount, boolean simulate) {
         this.persist(slot);
-        final ItemStack ret = this.inv.extractItem(slot, amount, simulate);
-        this.cleanup(slot);
-        return ret;
+        return this.inv.extractItem(slot, amount, simulate);
     }
 
     @Override
@@ -105,9 +94,8 @@ private void persist(int slot) {
         }
     }
 
-    private void cleanup(int slot) {
-        if (this.handlerForSlot[slot] != null && this.handlerStackForSlot[slot] != this.inv.getStackInSlot(slot)) {
-            this.handlerForSlot[slot] = null;
-        }
+    @Override
+    public void sendChangeNotification(int slot) {
+        inv.sendChangeNotification(slot);
     }
 }
diff --git a/src/main/java/appeng/blockentity/misc/CellWorkbenchBlockEntity.java b/src/main/java/appeng/blockentity/misc/CellWorkbenchBlockEntity.java
@@ -102,8 +102,7 @@ public InternalInventory getSubInventory(ResourceLocation id) {
     }
 
     @Override
-    public void onChangeInventory(InternalInventory inv, int slot, ItemStack removedStack,
-            ItemStack newStack) {
+    public void onChangeInventory(InternalInventory inv, int slot) {
         if (inv == this.cell && !this.locked) {
             this.locked = true;
             try {

diff --git a/src/main/java/appeng/blockentity/misc/ChargerBlockEntity.java b/src/main/java/appeng/blockentity/misc/ChargerBlockEntity.java
@@ -118,8 +118,7 @@ public InternalInventory getInternalInventory() {
     }
 
     @Override
-    public void onChangeInventory(InternalInventory inv, int slot,
-            ItemStack removed, ItemStack added) {
+    public void onChangeInventory(InternalInventory inv, int slot) {
         getMainNode().ifPresent((grid, node) -> {
             grid.getTickManager().wakeDevice(node);
         });

diff --git a/src/main/java/appeng/blockentity/misc/CondenserBlockEntity.java b/src/main/java/appeng/blockentity/misc/CondenserBlockEntity.java
@@ -157,8 +157,7 @@ public InternalInventory getInternalInventory() {
     }
 
     @Override
-    public void onChangeInventory(InternalInventory inv, int slot,
-            ItemStack removed, ItemStack added) {
+    public void onChangeInventory(InternalInventory inv, int slot) {
     }
 
     @Override

diff --git a/src/main/java/appeng/blockentity/misc/InscriberBlockEntity.java b/src/main/java/appeng/blockentity/misc/InscriberBlockEntity.java
@@ -184,8 +184,7 @@ public InternalInventory getInternalInventory() {
     }
 
     @Override
-    public void onChangeInventory(InternalInventory inv, int slot,
-            ItemStack removed, ItemStack added) {
+    public void onChangeInventory(InternalInventory inv, int slot) {
         if (slot == 0) {
             this.setProcessingTime(0);
         }

diff --git a/src/main/java/appeng/blockentity/misc/SecurityStationBlockEntity.java b/src/main/java/appeng/blockentity/misc/SecurityStationBlockEntity.java
@@ -94,8 +94,7 @@ public SecurityStationBlockEntity(BlockEntityType<?> blockEntityType, BlockPos p
     }
 
     @Override
-    public void onChangeInventory(InternalInventory inv, int slot,
-            ItemStack removedStack, ItemStack newStack) {
+    public void onChangeInventory(InternalInventory inv, int slot) {
 
     }
 

diff --git a/src/main/java/appeng/blockentity/misc/VibrationChamberBlockEntity.java b/src/main/java/appeng/blockentity/misc/VibrationChamberBlockEntity.java
@@ -113,8 +113,7 @@ public InternalInventory getInternalInventory() {
     }
 
     @Override
-    public void onChangeInventory(InternalInventory inv, int slot,
-            ItemStack removed, ItemStack added) {
+    public void onChangeInventory(InternalInventory inv, int slot) {
         if (this.getBurnTime() <= 0 && this.canEatFuel()) {
             getMainNode().ifPresent((grid, node) -> {
                 grid.getTickManager().wakeDevice(node);

diff --git a/src/main/java/appeng/blockentity/networking/ControllerBlockEntity.java b/src/main/java/appeng/blockentity/networking/ControllerBlockEntity.java
@@ -20,7 +20,6 @@
 
 import net.minecraft.core.BlockPos;
 import net.minecraft.core.Direction;
-import net.minecraft.world.item.ItemStack;
 import net.minecraft.world.level.block.entity.BlockEntityType;
 import net.minecraft.world.level.block.state.BlockState;
 
@@ -134,8 +133,7 @@ public InternalInventory getInternalInventory() {
     }
 
     @Override
-    public void onChangeInventory(InternalInventory inv, int slot,
-            ItemStack removed, ItemStack added) {
+    public void onChangeInventory(InternalInventory inv, int slot) {
     }
 
     /**

diff --git a/src/main/java/appeng/blockentity/networking/EnergyAcceptorBlockEntity.java b/src/main/java/appeng/blockentity/networking/EnergyAcceptorBlockEntity.java
@@ -20,7 +20,6 @@
 
 import net.minecraft.core.BlockPos;
 import net.minecraft.core.Direction;
-import net.minecraft.world.item.ItemStack;
 import net.minecraft.world.level.block.entity.BlockEntityType;
 import net.minecraft.world.level.block.state.BlockState;
 
@@ -68,8 +67,7 @@ public InternalInventory getInternalInventory() {
     }
 
     @Override
-    public void onChangeInventory(InternalInventory inv, int slot,
-            ItemStack removed, ItemStack added) {
+    public void onChangeInventory(InternalInventory inv, int slot) {
 
     }
 }
diff --git a/src/main/java/appeng/blockentity/networking/WirelessBlockEntity.java b/src/main/java/appeng/blockentity/networking/WirelessBlockEntity.java
@@ -111,8 +111,7 @@ public InternalInventory getInternalInventory() {
     }
 
     @Override
-    public void onChangeInventory(InternalInventory inv, int slot,
-            ItemStack removed, ItemStack added) {
+    public void onChangeInventory(InternalInventory inv, int slot) {
         // :P
     }
 

diff --git a/src/main/java/appeng/blockentity/qnb/QuantumBridgeBlockEntity.java b/src/main/java/appeng/blockentity/qnb/QuantumBridgeBlockEntity.java
@@ -104,8 +104,7 @@ public InternalInventory getInternalInventory() {
     }
 
     @Override
-    public void onChangeInventory(InternalInventory inv, int slot,
-            ItemStack removed, ItemStack added) {
+    public void onChangeInventory(InternalInventory inv, int slot) {
         if (this.cluster != null) {
             this.cluster.updateStatus(true);
         }

diff --git a/src/main/java/appeng/blockentity/spatial/SpatialIOPortBlockEntity.java b/src/main/java/appeng/blockentity/spatial/SpatialIOPortBlockEntity.java
@@ -165,8 +165,7 @@ public InternalInventory getInternalInventory() {
     }
 
     @Override
-    public void onChangeInventory(InternalInventory inv, int slot,
-            ItemStack removed, ItemStack added) {
+    public void onChangeInventory(InternalInventory inv, int slot) {
 
     }
 

diff --git a/src/main/java/appeng/blockentity/storage/ChestBlockEntity.java b/src/main/java/appeng/blockentity/storage/ChestBlockEntity.java
@@ -389,8 +389,7 @@ public InternalInventory getInternalInventory() {
     }
 
     @Override
-    public void onChangeInventory(InternalInventory inv, int slot,
-            ItemStack removed, ItemStack added) {
+    public void onChangeInventory(InternalInventory inv, int slot) {
         if (inv == this.cellInventory) {
             this.cellHandler = null;
             this.isCached = false; // recalculate the storage cell.
@@ -403,7 +402,7 @@ public void onChangeInventory(InternalInventory inv, int slot,
                 this.markForUpdate();
             }
         }
-        if (inv == this.inputInventory && !added.isEmpty()) {
+        if (inv == this.inputInventory && !inv.getStackInSlot(slot).isEmpty()) {
             this.tryToStoreContents();
         }
     }

diff --git a/src/main/java/appeng/blockentity/storage/DriveBlockEntity.java b/src/main/java/appeng/blockentity/storage/DriveBlockEntity.java
@@ -290,8 +290,7 @@ public InternalInventory getInternalInventory() {
     }
 
     @Override
-    public void onChangeInventory(InternalInventory inv, int slot,
-            ItemStack removed, ItemStack added) {
+    public void onChangeInventory(InternalInventory inv, int slot) {
         if (this.isCached) {
             this.isCached = false; // recalculate the storage cell.
             this.updateState();
@@ -317,6 +316,7 @@ private void updateState() {
     // Returns idle power draw of slot
     private double updateStateForSlot(int slot) {
         this.invBySlot[slot] = null;
+        this.inv.setHandler(slot, null);
 
         var is = this.inv.getStackInSlot(slot);
         if (!is.isEmpty()) {

diff --git a/src/main/java/appeng/blockentity/storage/IOPortBlockEntity.java b/src/main/java/appeng/blockentity/storage/IOPortBlockEntity.java
@@ -206,8 +206,7 @@ public InternalInventory getInternalInventory() {
     }
 
     @Override
-    public void onChangeInventory(InternalInventory inv, int slot,
-            ItemStack removed, ItemStack added) {
+    public void onChangeInventory(InternalInventory inv, int slot) {
         if (this.inputCells == inv) {
             this.updateTask();
         }