Comparator update order different from vanilla

[2No2Name]

Minecraft Version: 1.21.4

NeoForge Version: 21.4.50-beta

Steps to Reproduce:

1. Place a sunflower (to get the orientation correct)
2. Build the rest (sticky pistons) as in the screenshots
3. Put an item into the chest

Description of issue:
With neoforge:

With vanilla:

Neoforge's comparator update order seems to be different from vanilla

[2No2Name]

Code analysis: Level.java: updateNeighbourForOutputSignal uses Direction.values() instead of Direction.Plane.HORIZONTAL. These have different iteration order for the horizontal directions

[2No2Name]

Seems like this is for a vertical comparator implementation

[sciwhiz12]

After digging through the patch git blame, this change was implemented all the way back in 2016, to fix MinecraftForge/MinecraftForge#2428.

[2No2Name]

The issue also includes the vertical updates existing, which is detectable and different from vanilla. I think in some reasonable setups this might occur accidentally, breaking contraptions. (Comparator update detector above or below some other inventories.)

This setup updates the detector with neoforge, but not in vanilla.

For some reason placing the comparator directly on the hopper doesn't trigger the behavior, I don't understand why.

[2No2Name]

For some reason placing the comparator directly on the hopper doesn't trigger the behavior, I don't understand why.

Ah you are implementing pos.getY() == neighbor.getY() in ComparatorBlock for the first update attempt (1 block range) in

ComparatorBlock:
   @Override
    public void onNeighborChange(BlockState state, net.minecraft.world.level.LevelReader levelReader, BlockPos pos, BlockPos neighbor) {
        if (pos.getY() == neighbor.getY() && levelReader instanceof Level level && !levelReader.isClientSide()) {
            // TODO porting: check this still works as expected
            state.handleNeighborChanged(level, pos, levelReader.getBlockState(neighbor).getBlock(), null, false);
        }
    }

But it is not implemented for the 2 block range update. I suggest removing the check above and instead doing something at the point of sending the comparator updates

[2No2Name]

I suggest if you want to keep the vertical updates, reorder them as in the PR and then instead of blockState.is(Blocks.COMPARATOR) do your blockstate.getWeakChanges(this, blockpos) && (direction.isHorizontal || !blockState.is(Blocks.COMPARATOR))

An alternative might be to implement the second update with the same code path as the first update