Redstone comparators reading inventory changes do not always cause updates to nearby redstone components

[2No2Name]

Minecraft Version: 1.21.3

NeoForge Version: 21.3.11-beta

Logs: No log needed

Steps to Reproduce:

1. Build this
2. Put 1 item into the chest (e.g. a chest item). Put 5 items into the hopper (e.g. hopper item, stack size 1, hopper item is stackable to 64)
3. Activate and deactivate the lever

Description of issue:
The behavior with neoforge is different from vanilla behavior, breaking certain redstone contraptions. Neoforge redstone comparators seem to only send updates to nearby redstone components when their signal strength actually changes. In vanilla, each inventory update causes the comparator to send updates. Even a player clicking into an empty slot does.

In the setup, the deviation can be observed during step 3:
The piston does not extend or retract when neoforge is installed.
When using vanilla, the piston extends and retracts together with the lever power.

[sciwhiz12]

Confirmed on 21.4.13-beta (in NeoDev).

As I understand it, the problem here is not that redstone comparators in NeoForge only send updates when the signal strength changes, but that the signal strength actually changes for a very brief moment due to the design of Minecraft's inventories.

The actual cause of the problem

HopperBlockEntity#tryTakeInItemFromSlot is the method called by the hopper to attempt taking out an item from the source container (as seen in HopperBlockEntity#suckInItems). Because Minecraft containers (the technical term for the inventory of blocks and entities, aside from the player's) do not have a 'simulate' system, the aforementioned method works by actually extracting an item from the container, and then attempting to insert the extracted item into the hopper. If the extracted item could not be inserted into the hopper, then the extraction is effectively reversed by setting the contents of the slot in the source container back to the original item.

However, when the source container--in this case, a chest--is called to extract an item, and later to reset the slot in order to 'reverse' the extract, it also calls BlockEntity#setChanged() on itself. That call causes the comparator analog output to be updated (and neighbors to be notified of the change).

As you can infer then, because both the extraction and its reversal cause the chest's analog output to be updated, there is a brief window where the analog output dips down to 0 (because the one item in the chest is extracted) and then goes back up to 1 (because the extraction is reversed). That change in analog output is what triggers the comparator to update, thus causing the reported behavior.

Indeed, this can be confirmed if you insert another item in the chest. By having two items in the chest, the analog output of the chest stays at 1 even when the first item is extracted and un-extracted by the hopper, thus causing the piston to not flip in sync with the lever.

You can confirm this even more if you put exactly enough items in the chest such that taking out one item causes the analog output to change one level. For example, putting a stack and 60 (totalling 124 items) of items in the chest also shows the same behavior as one item (as seen via the piston and lever). That is because a stack and 60 (124) produces an analog signal of 2, while a stack and 59 (123) produces an analog signal of 1.

Why NeoForge doesn't trigger the problem

NeoForge doesn't trigger the reported behavior because it uses its own item extraction method that respects the IItemHandler capability, as a substitute for vanilla's Container system. That capability is able to simulate an extraction, which means the chest only ever becomes empty when it actually does an extraction--it does not do the same "extract, then un-extract later if unable to insert into hopper" manner as vanilla's containers.

The only way I was able to 'reintroduce' the reported behavior was to manually extract and un-extract an item in VanillaInventoryCodeHooks#extractHook:

@@ -18,6 +18,7 @@ import net.minecraft.world.level.Level;
 import net.minecraft.world.level.block.DropperBlock;
 import net.minecraft.world.level.block.HopperBlock;
 import net.minecraft.world.level.block.entity.BlockEntity;
+import net.minecraft.world.level.block.entity.ChestBlockEntity;
 import net.minecraft.world.level.block.entity.CrafterBlockEntity;
 import net.minecraft.world.level.block.entity.DispenserBlockEntity;
 import net.minecraft.world.level.block.entity.Hopper;
@@ -41,6 +42,10 @@ public class VanillaInventoryCodeHooks {
         return getSourceItemHandler(level, dest)
                 .map(itemHandlerResult -> {
                     IItemHandler handler = itemHandlerResult.getKey();
+                    if (itemHandlerResult.getValue() instanceof ChestBlockEntity entity) {
+                        ItemStack stack = entity.removeItem(0, 1);
+                        entity.setItem(0, stack);
+                    }

                     for (int i = 0; i < handler.getSlots(); i++) {
                         ItemStack extractItem = handler.extractItem(i, 1, true);

By inserting that code, which does the same "extraction then un-extraction" routine by vanilla containers, we cause the analog output to change as stated above and the reported behavior to be reintroduced.

Potential solution

As a potential solution, we could adapt a solution conceptually similar to the above: modify VanillaInventoryCodeHooks#extractHook such that for source objects which implement Container (as does all vanilla containers, like chests) and for the first slot it encounters with an item, it will do an extract and un-extraction to mimic vanilla's own behavior in that regard.

That way, any source object that implements both the IItemHandler capability and the Container interface (such as vanilla blocks) would trigger the reported behavior. Those which only implement the capability would not trigger the reported behavior, and those which only implement the interface would trigger the reported behavior (as the rest of the code in HopperBlockEntity#suckInItems would apply).

An example of the potential solution follows:

@@ -42,9 +43,27 @@ public class VanillaInventoryCodeHooks {
                 .map(itemHandlerResult -> {
                     IItemHandler handler = itemHandlerResult.getKey();

+                    boolean vanillaExtracted = false;
                     for (int i = 0; i < handler.getSlots(); i++) {
                         ItemStack extractItem = handler.extractItem(i, 1, true);
                         if (!extractItem.isEmpty()) {
+                            // Mimic HopperBlockEntity#tryTakeInItemFromSlot's behavior of extracting then un-extracting from a slot, to fix #1750
+                            // Only do this once per extract operation, which should be enough to mimic the needed behavior
+                            if (!vanillaExtracted && itemHandlerResult.getValue() instanceof Container container) {      
+                                ItemStack originalStack = container.getItem(i);
+                                int originalCount = originalStack.getCount();
+                                // #removeItem returns a new stack with the requested count, and shrinks the old existing stack
+                                container.removeItem(i, 1);
+
+                                // Grow the existing stack back to its original count
+                                originalStack.setCount(originalCount);
+                                // If the existing stack was emptied, reset the stack back into its slot
+                                if (originalCount == 1) {
+                                    container.setItem(i, originalStack);
+                                }
+                                vanillaExtracted = true;
+                            }
+
                             for (int j = 0; j < dest.getContainerSize(); j++) {
                                 ItemStack destStack = dest.getItem(j);
                                 if (dest.canPlaceItem(j, extractItem) && (destStack.isEmpty() || destStack.getCount() < d
estStack.getMaxStackSize() && destStack.getCount() < dest.getMaxStackSize() && ItemStack.isSameItemSameComponents(extractItem, destStack))) {

[2No2Name]

The potential solution would still be different from vanilla, as the reaction of comparators can differ depending on whether the signal strength of the inventory is reduced by the item extraction attempt. In Lithium's hopper optimizations, I defined the relevant different comparator update patterns that are detectable.

• NO_UPDATE: example: empty inventory, inventory that does not send comparator updates on hopper pull attempt

• UPDATE: example: inventory with items, but removing any single item does not change the signal strength

• DECREMENT_UPDATE_INCREMENT_UPDATE: example: inventory with items, but removing the first item reduces the signal strength

• UPDATE_DECREMENT_UPDATE_INCREMENT_UPDATE: example: inventory with items, removing the first item does not reduce the signal strength, but there is another item that will reduce the signal strength when removed

The original report (with one item in the chest) should give the update pattern DECREMENT_UPDATE_INCREMENT_UPDATE.

I define two types of comparator updates: Update with signal strength change and update without signal strength change. Vanilla sends up to 3*Number of slots updates per run of the hopper item extraction code, so there is a high incentive to reduce it. But some properties are detectable. There are two types of comparator update detector in the game. One detects only the updates that happen with a signal strength change. The other one detects all updates (both with and without signal strength change). There is currently no known way to build a detector that does not react to the first update its type can detect, meaning that multiple updates in a row can be summarized into one, yielding the definition of the above 4 comparator update patterns which can be distinguished.

I will post a few screenshots of the setups detecting the difference in a while

[2No2Name]

General comparator update detector:
Block update detector:

I placed some redstone torches, wire and redstone lamps to visualize the activations:
Clicking in an empty barrel slot:

NO_UPDATE: Empty barrel, no activations:

UPDATE: No signal strength change when removing one item:

DECREMENT_UPDATE_INCREMENT_UPDATE: Signal strength change when removing one item from the first non-empty stack:

UPDATE_DECREMENT_UPDATE_INCREMENT_UPDATE: Signal strength change when removing one item, but not from the first non-empty stack:

Now you might ask how a player could detect the difference between the comparator update patterns DECREMENT_UPDATE_INCREMENT_UPDATE and UPDATE_DECREMENT_UPDATE_INCREMENT_UPDATE. The answer to that is that the barrel sends out the comparator updates in a deterministic order (Direction.NORTH, Direction.EAST, Direction.SOUTH, Direction.WEST). When placing the general detector SOUTH of the barrel and the block update detector NORTH, the order of which of the detector activates first shows the difference of these two comparator update patterns.
Will insert a screenshot shortly

[2No2Name]

The two update detectors are hooked up to pistons that retract the same block. The piston that successfully pulls the block was triggered to retract first.

Note that this is a manual setup, the lever that is putting the piston into a budded state is operated manually here. The noteblock is triggered shortly before taking the screenshots. The player is looking towards west, the general comparator update detector is south of the chest/barrel and the block update detector is north. If there was no detectable difference between the update patterns, the block update detector should activate first.

DECREMENT_UPDATE_INCREMENT_UPDATE:

UPDATE_DECREMENT_UPDATE_INCREMENT_UPDATE: The general detector is south, so updated after the block update detector. But the block update detector cannot detect the first update of the UPDATE_DECREMENT_UPDATE_INCREMENT_UPDATE, since it does not include a signal strength change. Thus the general comparator update detector retracts the piston first.

[2No2Name]

I believe the mod loader should not change vanilla's behavior. The complexity of emulating these vanilla comparator update patterns probably doesn't belong into a mod loader either. I propose to always send all comparator updates that vanilla sends