Vanilla Jukebox bug with Fabric transfer API v1

Question

Vanilla Jukebox bug with Fabric transfer API v1

niklaswimmer opened this issue 10 months ago · 0 comments

niklaswimmer commented 10 months ago

Triggering the bug

Extract the disc from the Jukebox using Fabric's transfer API v1.

The disc is extracted from the Jukebox but the music restarts. The music is now playing and the Jukebox's state is hasRecord:true, but it stores no item (the disc will be wherever you extracted it to and breaking the Jukebox will not drop a disc).

The music will keep on playing for its normal duration and can not be stopped, breaking the Jukebox will not affect the music either (it will keep on playing). The Jukebox's state will remain as hasRecord:true and can be (as far as I found out) not altered anymore, putting a new disc into the Jukebox will not work.

Reproducing the bug

To reproduce, any Fabric mod that has pipes (or something similar) that use the transfer API will work. I tested it with Modern Industrialization's Item Pipes.

Alternatively, if you are in a dev environment, you can apply the following patch to make Vanilla hoppers use the transfer API.

diff --git a/fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/mixin/transfer/HopperBlockEntityMixin.java b/fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/mixin/transfer/HopperBlockEntityMixin.java
index cff062a3..7e2a15ef 100644
--- a/fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/mixin/transfer/HopperBlockEntityMixin.java
+++ b/fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/mixin/transfer/HopperBlockEntityMixin.java
@@ -83,7 +83,7 @@ public class HopperBlockEntityMixin {
        )
        private static void hookExtract(World world, Hopper hopper, CallbackInfoReturnable<Boolean> cir, Inventory inputInventory) {
                // Let vanilla handle the transfer if it found an inventory.
-               if (inputInventory != null) return;
+//             if (inputInventory != null) return;

                // Otherwise inject our transfer logic.
                BlockPos sourcePos = BlockPos.ofFloored(hopper.getHopperX(), hopper.getHopperY() + 1.0D, hopper.getHopperZ());

You will have to wait until the music finishes once, because the Jukebox's redstone signal blocks the hopper while playing. I recommend using the melohi disc in that case, as it is the shortest (1:36) that is actually pleasant to listen to (if you don't care, use 11).

Fixing the bug

Don't know. But it happens because the JukeboxBlockEntity (which implements Container and is therefore discoverable via the transfer API) starts playing the music every time it's setStack method is called. Apparently, if you call extract on a Storage it will end up calling this method. This causes the music to start playing every time an item is extracted. For more information see the stacktraces below. Note that in one extraction the setStack method is called multiple times.

Stacktraces

I obtained those by breakpointing `JukeboxBlockEntity#startPlaying` and getting a thread dump for every hit. They are truncated to start at hopper's `extract` method which then calls into the hopper mixin and then into the transfer API.

first hit

This is the simulated extraction.

java.lang.Thread.State: RUNNABLE
at net.minecraft.block.entity.JukeboxBlockEntity.startPlaying(JukeboxBlockEntity.java:84)
at net.minecraft.block.entity.JukeboxBlockEntity.setStack(JukeboxBlockEntity.java:144)
at net.fabricmc.fabric.impl.transfer.item.InventorySlotWrapper.setStack(InventorySlotWrapper.java:63)
at net.fabricmc.fabric.api.transfer.v1.item.base.SingleStackStorage.createSnapshot(SingleStackStorage.java:159)
at net.fabricmc.fabric.api.transfer.v1.item.base.SingleStackStorage.createSnapshot(SingleStackStorage.java:42)
at net.fabricmc.fabric.api.transfer.v1.transaction.base.SnapshotParticipant.updateSnapshots(SnapshotParticipant.java:102)
at net.fabricmc.fabric.impl.transfer.item.InventorySlotWrapper.updateSnapshots(InventorySlotWrapper.java:128)
at net.fabricmc.fabric.api.transfer.v1.item.base.SingleStackStorage.extract(SingleStackStorage.java:144)
at net.fabricmc.fabric.impl.transfer.item.InventorySlotWrapper.extract(InventorySlotWrapper.java:97)
at net.fabricmc.fabric.impl.transfer.item.InventorySlotWrapper.extract(InventorySlotWrapper.java:40)
at net.fabricmc.fabric.api.transfer.v1.storage.StorageUtil.simulateExtract(StorageUtil.java:162)
at net.fabricmc.fabric.api.transfer.v1.storage.StorageUtil.move(StorageUtil.java:98)
at net.minecraft.block.entity.HopperBlockEntity.handler$zbd000$fabric-transfer-api-v1$hookExtract(HopperBlockEntityMixin.java:93)
at net.minecraft.block.entity.HopperBlockEntity.extract(HopperBlockEntity.java:191)

second hit

This is the rollback of the simulated transaction.

at net.minecraft.block.entity.JukeboxBlockEntity.startPlaying(JukeboxBlockEntity.java:84)
at net.minecraft.block.entity.JukeboxBlockEntity.setStack(JukeboxBlockEntity.java:144)
at net.fabricmc.fabric.impl.transfer.item.InventorySlotWrapper.setStack(InventorySlotWrapper.java:63)
at net.fabricmc.fabric.api.transfer.v1.item.base.SingleStackStorage.readSnapshot(SingleStackStorage.java:165)
at net.fabricmc.fabric.api.transfer.v1.item.base.SingleStackStorage.readSnapshot(SingleStackStorage.java:42)
at net.fabricmc.fabric.api.transfer.v1.transaction.base.SnapshotParticipant.onClose(SnapshotParticipant.java:117)
at net.fabricmc.fabric.impl.transfer.transaction.TransactionManagerImpl$TransactionImpl.close(TransactionManagerImpl.java:138)
at net.fabricmc.fabric.impl.transfer.transaction.TransactionManagerImpl$TransactionImpl.abort(TransactionManagerImpl.java:181)
at net.fabricmc.fabric.impl.transfer.transaction.TransactionManagerImpl$TransactionImpl.close(TransactionManagerImpl.java:192)
at net.fabricmc.fabric.api.transfer.v1.storage.StorageUtil.simulateExtract(StorageUtil.java:163)
at net.fabricmc.fabric.api.transfer.v1.storage.StorageUtil.move(StorageUtil.java:98)
at net.minecraft.block.entity.HopperBlockEntity.handler$zbd000$fabric-transfer-api-v1$hookExtract(HopperBlockEntityMixin.java:93)
at net.minecraft.block.entity.HopperBlockEntity.extract(HopperBlockEntity.java:191)

third hit

This is the actual transaction.

at net.minecraft.block.entity.JukeboxBlockEntity.startPlaying(JukeboxBlockEntity.java:84)
at net.minecraft.block.entity.JukeboxBlockEntity.setStack(JukeboxBlockEntity.java:144)
at net.fabricmc.fabric.impl.transfer.item.InventorySlotWrapper.setStack(InventorySlotWrapper.java:63)
at net.fabricmc.fabric.api.transfer.v1.item.base.SingleStackStorage.createSnapshot(SingleStackStorage.java:159)
at net.fabricmc.fabric.api.transfer.v1.item.base.SingleStackStorage.createSnapshot(SingleStackStorage.java:42)
at net.fabricmc.fabric.api.transfer.v1.transaction.base.SnapshotParticipant.updateSnapshots(SnapshotParticipant.java:102)
at net.fabricmc.fabric.impl.transfer.item.InventorySlotWrapper.updateSnapshots(InventorySlotWrapper.java:128)
at net.fabricmc.fabric.api.transfer.v1.item.base.SingleStackStorage.extract(SingleStackStorage.java:144)
at net.fabricmc.fabric.impl.transfer.item.InventorySlotWrapper.extract(InventorySlotWrapper.java:97)
at net.fabricmc.fabric.impl.transfer.item.InventorySlotWrapper.extract(InventorySlotWrapper.java:40)
at net.fabricmc.fabric.api.transfer.v1.storage.StorageUtil.move(StorageUtil.java:105)
at net.minecraft.block.entity.HopperBlockEntity.handler$zbd000$fabric-transfer-api-v1$hookExtract(HopperBlockEntityMixin.java:93)
at net.minecraft.block.entity.HopperBlockEntity.extract(HopperBlockEntity.java:191)

I guess the real problem here is that the transfer API assumes setStack has no side effects. I am not familiar with Fabric at all tbh, so I will leave finding a solution to this problem to others.

Share to