Don't propagate redstone when blink/label changes

Historically, computers tracked whether any world-visible state
(on/off/blinking, label and redstone outputs) had changed with a single
"has changed" flag. While this is simple to use, this has the curious
side effect of that term.setCursorBlink() or os.setComputerLabel() would
cause a block update!

This isn't really a problem in practice - it just means slightly more
block updates. However, the redstone propagation sometimes causes the
computer to invalidate/recheck peripherals, which masks several other
(yet unfixed) bugs.

projects/common/src/main/java/dan200/computercraft/shared/computer/blocks/AbstractComputerBlockEntity.java

@@ -113,16 +113,13 @@ public abstract class AbstractComputerBlockEntity extends BlockEntity implements
         return InteractionResult.PASS;
     }
 
-    public void neighborChanged(BlockPos neighbour) {
-        updateInputAt(neighbour);
-    }
-
     protected void serverTick() {
         if (getLevel().isClientSide) return;
         if (computerID < 0 && !startOn) return; // Don't tick if we don't need a computer!
 
         var computer = createServerComputer();
 
+        // Update any peripherals that have changed.
         if (invalidSides != 0) {
             for (var direction : DirectionUtil.FACINGS) {
                 if ((invalidSides & (1 << direction.ordinal())) != 0) refreshPeripheral(computer, direction);
@@ -139,16 +136,30 @@ public abstract class AbstractComputerBlockEntity extends BlockEntity implements
 
         fresh = false;
         computerID = computer.getID();
-        label = computer.getLabel();
-        on = computer.isOn();
 
-        // Update the block state if needed. We don't fire a block update intentionally,
-        // as this only really is needed on the client side.
+        // If the on state has changed, mark as as dirty.
+        var newOn = computer.isOn();
+        if (on != newOn) {
+            on = newOn;
+            setChanged();
+        }
+
+        // If the label has changed, mark as dirty and sync to client.
+        var newLabel = computer.getLabel();
+        if (!Objects.equals(label, newLabel)) {
+            label = newLabel;
+            BlockEntityHelpers.updateBlock(this);
+        }
+
+        // Update the block state if needed.
         updateBlockState(computer.getState());
 
-        // TODO: This should ideally be split up into label/id/on (which should save NBT and sync to client) and
-        //  redstone (which should update outputs)
-        if (computer.hasOutputChanged()) updateOutput();
+        var changes = computer.pollAndResetChanges();
+        if (changes != 0) {
+            for (var direction : DirectionUtil.FACINGS) {
+                if ((changes & (1 << remapToLocalSide(direction).ordinal())) != 0) updateRedstoneTo(direction);
+            }
+        }
     }
 
     protected abstract void updateBlockState(ComputerState newState);
@@ -198,11 +209,15 @@ public abstract class AbstractComputerBlockEntity extends BlockEntity implements
         return localSide;
     }
 
-    private void updateRedstoneInputs(ServerComputer computer) {
-        var pos = getBlockPos();
-        for (var dir : DirectionUtil.FACINGS) updateRedstoneInput(computer, dir, pos.relative(dir));
-    }
-
+    /**
+     * Update the redstone input on a particular side.
+     * <p>
+     * This is called <em>immediately</em> when a neighbouring block changes (see {@link #neighborChanged(BlockPos)}).
+     *
+     * @param computer  The current server computer.
+     * @param dir       The direction to update in.
+     * @param targetPos The position of the adjacent block, equal to {@code getBlockPos().offset(dir)}.
+     */
     private void updateRedstoneInput(ServerComputer computer, Direction dir, BlockPos targetPos) {
         var offsetSide = dir.getOpposite();
         var localDir = remapToLocalSide(dir);
@@ -211,6 +226,15 @@ public abstract class AbstractComputerBlockEntity extends BlockEntity implements
         computer.setBundledRedstoneInput(localDir, BundledRedstone.getOutput(getLevel(), targetPos, offsetSide));
     }
 
+    /**
+     * Update the peripheral on a particular side.
+     * <p>
+     * This is called from {@link #serverTick()}, after a peripheral has been marked as invalid (such as in
+     * {@link #neighborChanged(BlockPos)})
+     *
+     * @param computer The current server computer.
+     * @param dir      The direction to update in.
+     */
     private void refreshPeripheral(ServerComputer computer, Direction dir) {
         invalidSides &= ~(1 << dir.ordinal());
 
@@ -243,7 +267,18 @@ public abstract class AbstractComputerBlockEntity extends BlockEntity implements
         }
     }
 
-    private void updateInputAt(BlockPos neighbour) {
+    /**
+     * Called when a neighbour block changes.
+     * <p>
+     * This finds the side the neighbour block is on, and updates the inputs accordingly.
+     * <p>
+     * We do <strong>NOT</strong> update the peripheral immediately. Blocks and block entities are sometimes
+     * inconsistent at the point where an update is received, and so we instead just mark that side as dirty (see
+     * {@link #invalidSides}) and refresh it {@linkplain #serverTick() next tick}.
+     *
+     * @param neighbour The position of the neighbour block.
+     */
+    public void neighborChanged(BlockPos neighbour) {
         var computer = getServerComputer();
         if (computer == null) return;
 
@@ -258,22 +293,28 @@ public abstract class AbstractComputerBlockEntity extends BlockEntity implements
 
         // If the position is not any adjacent one, update all inputs. This is pretty terrible, but some redstone mods
         // handle this incorrectly.
-        updateRedstoneInputs(computer);
+        for (var dir : DirectionUtil.FACINGS) updateRedstoneInput(computer, dir, getBlockPos().relative(dir));
         invalidSides = (1 << 6) - 1; // Mark all peripherals as dirty.
     }
 
     /**
-     * Update the block's state and propagate redstone output.
+     * Update outputs in a specific direction.
+     *
+     * @param direction The direction to propagate outputs in.
      */
-    public void updateOutput() {
-        BlockEntityHelpers.updateBlock(this);
-        for (var dir : DirectionUtil.FACINGS) RedstoneUtil.propagateRedstoneOutput(getLevel(), getBlockPos(), dir);
+    protected void updateRedstoneTo(Direction direction) {
+        RedstoneUtil.propagateRedstoneOutput(getLevel(), getBlockPos(), direction);
 
         var computer = getServerComputer();
-        if (computer != null) updateRedstoneInputs(computer);
+        if (computer != null) updateRedstoneInput(computer, direction, getBlockPos().relative(direction));
     }
 
-    protected abstract ServerComputer createComputer(int id);
+    /**
+     * Update all redstone outputs.
+     */
+    public void updateRedstone() {
+        for (var dir : DirectionUtil.FACINGS) updateRedstoneTo(dir);
+    }
 
     @Override
     public final int getComputerID() {
@@ -331,6 +372,8 @@ public abstract class AbstractComputerBlockEntity extends BlockEntity implements
         return computer;
     }
 
+    protected abstract ServerComputer createComputer(int id);
+
     @Nullable
     public ServerComputer getServerComputer() {
         return getLevel().isClientSide || getLevel().getServer() == null ? null : ServerContext.get(getLevel().getServer()).registry().get(instanceID);

projects/common/src/main/java/dan200/computercraft/shared/computer/blocks/ComputerBlockEntity.java

@@ -51,7 +51,7 @@ public class ComputerBlockEntity extends AbstractComputerBlockEntity {
     protected void updateBlockState(ComputerState newState) {
         var existing = getBlockState();
         if (existing.getValue(ComputerBlock.STATE) != newState) {
-            getLevel().setBlock(getBlockPos(), existing.setValue(ComputerBlock.STATE, newState), 3);
+            getLevel().setBlock(getBlockPos(), existing.setValue(ComputerBlock.STATE, newState), ComputerBlock.UPDATE_CLIENTS);
         }
     }
 

projects/common/src/main/java/dan200/computercraft/shared/computer/core/ServerComputer.java

@@ -42,7 +42,6 @@ public class ServerComputer implements InputHandler, ComputerEnvironment {
     private final NetworkedTerminal terminal;
     private final AtomicBoolean terminalChanged = new AtomicBoolean(false);
 
-    private boolean changedLastFrame;
     private int ticksSincePing;
 
     public ServerComputer(
@@ -96,10 +95,7 @@ public class ServerComputer implements InputHandler, ComputerEnvironment {
 
     public void tickServer() {
         ticksSincePing++;
-
         computer.tick();
-
-        changedLastFrame = computer.pollAndResetChanged();
         if (terminalChanged.getAndSet(false)) onTerminalChanged();
     }
 
@@ -119,8 +115,8 @@ public class ServerComputer implements InputHandler, ComputerEnvironment {
         return ticksSincePing > 100;
     }
 
-    public boolean hasOutputChanged() {
-        return changedLastFrame;
+    public int pollAndResetChanges() {
+        return computer.pollAndResetChanges();
     }
 
     public int register() {
@@ -167,7 +163,7 @@ public class ServerComputer implements InputHandler, ComputerEnvironment {
     }
 
     public ComputerState getState() {
-        if (!isOn()) return ComputerState.OFF;
+        if (!computer.isOn()) return ComputerState.OFF;
         return computer.isBlinking() ? ComputerState.BLINKING : ComputerState.ON;
     }
 

projects/common/src/main/java/dan200/computercraft/shared/peripheral/modem/wired/CableBlock.java

@@ -127,7 +127,7 @@ public class CableBlock extends Block implements SimpleWaterloggedBlock, EntityB
             item = new ItemStack(ModRegistry.Items.CABLE.get());
         }
 
-        world.setBlock(pos, correctConnections(world, pos, newState), 3);
+        world.setBlockAndUpdate(pos, correctConnections(world, pos, newState));
 
         cable.modemChanged();
         cable.connectionsChanged();

projects/common/src/main/java/dan200/computercraft/shared/peripheral/modem/wired/CableBlockItem.java

@@ -30,7 +30,7 @@ public abstract class CableBlockItem extends BlockItem {
         // TODO: Check entity collision.
         if (!state.canSurvive(world, pos)) return false;
 
-        world.setBlock(pos, state, 3);
+        world.setBlockAndUpdate(pos, state);
         var soundType = state.getBlock().getSoundType(state);
         world.playSound(null, pos, soundType.getPlaceSound(), SoundSource.BLOCKS, (soundType.getVolume() + 1.0F) / 2.0F, soundType.getPitch() * 0.8F);
 

projects/common/src/main/java/dan200/computercraft/shared/pocket/core/PocketServerComputer.java

@@ -11,6 +11,7 @@ import dan200.computercraft.api.upgrades.UpgradeData;
 import dan200.computercraft.core.computer.ComputerSide;
 import dan200.computercraft.shared.common.IColouredItem;
 import dan200.computercraft.shared.computer.core.ComputerFamily;
+import dan200.computercraft.shared.computer.core.ComputerState;
 import dan200.computercraft.shared.computer.core.ServerComputer;
 import dan200.computercraft.shared.config.Config;
 import dan200.computercraft.shared.network.client.PocketComputerDataMessage;
@@ -37,7 +38,10 @@ public class PocketServerComputer extends ServerComputer implements IPocketAcces
     private ItemStack stack = ItemStack.EMPTY;
 
     private int lightColour = -1;
-    private boolean lightChanged = false;
+
+    // The state the previous tick, used to determine if the state needs to be sent to the client.
+    private int oldLightColour = -1;
+    private @Nullable ComputerState oldComputerState;
 
     private final Set<ServerPlayer> tracking = new HashSet<>();
 
@@ -82,10 +86,7 @@ public class PocketServerComputer extends ServerComputer implements IPocketAcces
     @Override
     public void setLight(int colour) {
         if (colour < 0 || colour > 0xFFFFFF) colour = -1;
-
-        if (lightColour == colour) return;
         lightColour = colour;
-        lightChanged = true;
     }
 
     @Override
@@ -156,9 +157,11 @@ public class PocketServerComputer extends ServerComputer implements IPocketAcces
         tracking.removeIf(player -> !player.isAlive() || player.level() != getLevel());
 
         // And now find any new players, add them to the tracking list, and broadcast state where appropriate.
-        var sendState = hasOutputChanged() || lightChanged;
-        lightChanged = false;
-        if (sendState) {
+        var state = getState();
+        if (oldLightColour != lightColour || oldComputerState != state) {
+            oldComputerState = state;
+            oldLightColour = lightColour;
+
             // Broadcast the state to all players
             tracking.addAll(getLevel().players());
             ServerNetworking.sendToPlayers(new PocketComputerDataMessage(this, false), tracking);

projects/common/src/main/java/dan200/computercraft/shared/turtle/blocks/TurtleBlockEntity.java

@@ -186,7 +186,7 @@ public class TurtleBlockEntity extends AbstractComputerBlockEntity implements Ba
         if (dir.getAxis() == Direction.Axis.Y) dir = Direction.NORTH;
         level.setBlockAndUpdate(worldPosition, getBlockState().setValue(TurtleBlock.FACING, dir));
 
-        updateOutput();
+        updateRedstone();
         updateInputsImmediately();
 
         onTileEntityChange();

projects/common/src/main/java/dan200/computercraft/shared/turtle/core/TurtleBrain.java

@@ -296,7 +296,7 @@ public class TurtleBrain implements TurtleAccessInternal {
                         oldWorld.removeBlock(oldPos, false);
 
                         // Make sure everybody knows about it
-                        newTurtle.updateOutput();
+                        newTurtle.updateRedstone();
                         newTurtle.updateInputsImmediately();
                         return true;
                     }

projects/core/src/main/java/dan200/computercraft/core/computer/Computer.java

@@ -15,8 +15,7 @@ import dan200.computercraft.core.filesystem.FileSystem;
 import dan200.computercraft.core.terminal.Terminal;
 
 import javax.annotation.Nullable;
-import java.util.Objects;
-import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicLong;
 
 /**
@@ -54,9 +53,9 @@ public class Computer {
     private final AtomicLong lastTaskId = new AtomicLong();
 
     // Additional state about the computer and its environment.
-    private boolean blinking = false;
     private final Environment internalEnvironment;
-    private final AtomicBoolean externalOutputChanged = new AtomicBoolean();
+
+    private final AtomicInteger externalOutputChanges = new AtomicInteger();
 
     private boolean startRequested;
     private int ticksSinceStart = -1;
@@ -140,10 +139,7 @@ public class Computer {
     }
 
     public void setLabel(@Nullable String label) {
-        if (!Objects.equals(label, this.label)) {
-            this.label = label;
-            externalOutputChanged.set(true);
-        }
+        this.label = label;
     }
 
     public void tick() {
@@ -164,28 +160,24 @@ public class Computer {
         internalEnvironment.tick();
 
         // Propagate the environment's output to the world.
-        if (internalEnvironment.updateOutput()) externalOutputChanged.set(true);
-
-        // Set output changed if the terminal has changed from blinking to not
-        var blinking = terminal.getCursorBlink() &&
-            terminal.getCursorX() >= 0 && terminal.getCursorX() < terminal.getWidth() &&
-            terminal.getCursorY() >= 0 && terminal.getCursorY() < terminal.getHeight();
-        if (blinking != this.blinking) {
-            this.blinking = blinking;
-            externalOutputChanged.set(true);
-        }
+        externalOutputChanges.accumulateAndGet(internalEnvironment.updateOutput(), (x, y) -> x | y);
     }
 
-    void markChanged() {
-        externalOutputChanged.set(true);
-    }
-
-    public boolean pollAndResetChanged() {
-        return externalOutputChanged.getAndSet(false);
+    /**
+     * Get a bitmask returning which sides on the computer have changed, resetting the internal state.
+     *
+     * @return What sides on the computer have changed.
+     */
+    public int pollAndResetChanges() {
+        return externalOutputChanges.getAndSet(0);
     }
 
     public boolean isBlinking() {
-        return isOn() && blinking;
+        if (!isOn() || !terminal.getCursorBlink()) return false;
+
+        var cursorX = terminal.getCursorX();
+        var cursorY = terminal.getCursorY();
+        return cursorX >= 0 && cursorX < terminal.getWidth() && cursorY >= 0 && cursorY < terminal.getHeight();
     }
 
     public void addApi(ILuaAPI api) {

projects/core/src/main/java/dan200/computercraft/core/computer/ComputerExecutor.java

@@ -411,7 +411,6 @@ final class ComputerExecutor implements ComputerScheduler.Worker {
 
             // Initialisation has finished, so let's mark ourselves as on.
             isOn = true;
-            computer.markChanged();
         } finally {
             isOnLock.unlock();
         }
@@ -446,7 +445,6 @@ final class ComputerExecutor implements ComputerScheduler.Worker {
             }
 
             computer.getEnvironment().resetOutput();
-            computer.markChanged();
         } finally {
             isOnLock.unlock();
         }

projects/core/src/main/java/dan200/computercraft/core/computer/Environment.java

@@ -222,22 +222,22 @@ public final class Environment implements IAPIEnvironment {
      *
      * @return If the outputs have changed.
      */
-    boolean updateOutput() {
+    int updateOutput() {
         // Mark output as changed if the internal redstone has changed
         synchronized (internalOutput) {
-            if (!internalOutputChanged) return false;
+            if (!internalOutputChanged) return 0;
 
-            var changed = false;
+            var changed = 0;
 
             for (var i = 0; i < ComputerSide.COUNT; i++) {
                 if (externalOutput[i] != internalOutput[i]) {
                     externalOutput[i] = internalOutput[i];
-                    changed = true;
+                    changed |= 1 << i;
                 }
 
                 if (externalBundledOutput[i] != internalBundledOutput[i]) {
                     externalBundledOutput[i] = internalBundledOutput[i];
-                    changed = true;
+                    changed |= 1 << i;
                 }
             }
 

projects/web/src/main/java/cc/tweaked/web/EmulatedComputer.java

@@ -30,6 +30,7 @@ import org.teavm.jso.typedarrays.ArrayBuffer;
 import javax.annotation.Nullable;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
+import java.util.Objects;
 
 /**
  * Manages the core lifecycle of an emulated {@link Computer}.
@@ -47,6 +48,9 @@ class EmulatedComputer implements ComputerEnvironment, ComputerHandle {
     private boolean disposed = false;
     private final MemoryMount mount = new MemoryMount();
 
+    private @Nullable String oldLabel;
+    private boolean oldOn;
+
     EmulatedComputer(ComputerContext context, ComputerDisplay computerAccess) {
         this.computerAccess = computerAccess;
         this.computer = new Computer(context, this, terminal, 0);
@@ -68,8 +72,10 @@ class EmulatedComputer implements ComputerEnvironment, ComputerHandle {
             LOG.error("Error when ticking computer", e);
         }
 
-        if (computer.pollAndResetChanged()) {
-            computerAccess.setState(computer.getLabel(), computer.isOn());
+        var newLabel = computer.getLabel();
+        var newOn = computer.isOn();
+        if (!Objects.equals(oldLabel, newLabel) || oldOn != newOn) {
+            computerAccess.setState(oldLabel = newLabel, oldOn = newOn);
         }
 
         for (var side : SIDES) {