Ensure the terminal exists when creating a monitor peripheral

Previously we had the invariant that if we had a server monitor, we also
had a terminal. When a monitor shrank into a place, we deleted the
monitor, and then recreated it when a peripheral was requested.

As of ab785a0906 this has changed
slightly, and we now just delete the terminal (keeping the ServerMonitor
around). However, we didn't adjust the peripheral code accordingly,
meaning we didn't recreate the /terminal/ when a peripheral was
requested.

The fix for this is very simple - most of the rest of this commit is
some additional code for ensuring monitor invariants hold, so we can
write tests with a little more confidence.

I'm not 100% sold on this approach. It's tricky having a double layer of
nullable state (ServerMonitor, and then the terminal). However, I think
this is reasonable - the ServerMonitor is a reference to the multiblock,
and the Terminal is part of the multiblock's state.

Even after all the refactors, monitor code is still nastier than I'd
like :/.

Fixes #1608

projects/common/src/main/java/dan200/computercraft/shared/peripheral/monitor/MonitorBlockEntity.java

@@ -157,7 +157,6 @@ public class MonitorBlockEntity extends BlockEntity {
         if (xIndex == 0 && yIndex == 0) {
             // If we're the origin, set up the new monitor
             serverMonitor = new ServerMonitor(advanced, this);
-            serverMonitor.rebuild();
 
             // And propagate it to child monitors
             for (var x = 0; x < width; x++) {
@@ -178,6 +177,11 @@ public class MonitorBlockEntity extends BlockEntity {
         }
     }
 
+    private void createServerTerminal() {
+        var monitor = createServerMonitor();
+        if (monitor != null && monitor.getTerminal() == null) monitor.rebuild();
+    }
+
     @Nullable
     public ClientMonitor getClientMonitor() {
         if (clientMonitor != null) return clientMonitor;
@@ -377,6 +381,8 @@ public class MonitorBlockEntity extends BlockEntity {
                 BlockEntityHelpers.updateBlock(monitor);
             }
         }
+
+        assertInvariant();
     }
 
     void updateNeighborsDeferred() {
@@ -487,9 +493,10 @@ public class MonitorBlockEntity extends BlockEntity {
     }
 
     public IPeripheral peripheral() {
-        createServerMonitor();
+        createServerTerminal();
-        if (peripheral != null) return peripheral;
+        var peripheral = this.peripheral != null ? this.peripheral : (this.peripheral = new MonitorPeripheral(this));
-        return peripheral = new MonitorPeripheral(this);
+        assertInvariant();
+        return peripheral;
     }
 
     void addComputer(IComputerAccess computer) {
@@ -528,4 +535,85 @@ public class MonitorBlockEntity extends BlockEntity {
             Math.max(startPos.getZ(), endPos.getZ()) + 1
         );
     }
+
+    /**
+     * Assert all {@linkplain #checkInvariants() monitor invariants} hold.
+     */
+    private void assertInvariant() {
+        assert checkInvariants() : "Monitor invariants failed. See logs.";
+    }
+
+    /**
+     * Check various invariants about this monitor multiblock. This is only called when assertions are enabled, so
+     * will be skipped outside of tests.
+     *
+     * @return Whether all invariants passed.
+     */
+    private boolean checkInvariants() {
+        LOG.debug("Checking monitor invariants at {}", getBlockPos());
+
+        var okay = true;
+
+        if (width <= 0 || height <= 0) {
+            okay = false;
+            LOG.error("Monitor {} has non-positive of {}x{}", getBlockPos(), width, height);
+        }
+
+        var hasPeripheral = false;
+        var origin = getOrigin().getMonitor();
+        var serverMonitor = origin != null ? origin.serverMonitor : this.serverMonitor;
+        for (var x = 0; x < width; x++) {
+            for (var y = 0; y < height; y++) {
+                var monitor = getLoadedMonitor(x, y).getMonitor();
+                if (monitor == null) continue;
+
+                hasPeripheral |= monitor.peripheral != null;
+
+                if (monitor.serverMonitor != null && monitor.serverMonitor != serverMonitor) {
+                    okay = false;
+                    LOG.error(
+                        "Monitor {} expected to be have serverMonitor={}, but was {}",
+                        monitor.getBlockPos(), serverMonitor, monitor.serverMonitor
+                    );
+                }
+
+                if (monitor.xIndex != x || monitor.yIndex != y) {
+                    okay = false;
+                    LOG.error(
+                        "Monitor {} expected to be at {},{}, but believes it is {},{}",
+                        monitor.getBlockPos(), x, y, monitor.xIndex, monitor.yIndex
+                    );
+                }
+
+                if (monitor.width != width || monitor.height != height) {
+                    okay = false;
+                    LOG.error(
+                        "Monitor {} expected to be size {},{}, but believes it is {},{}",
+                        monitor.getBlockPos(), width, height, monitor.width, monitor.height
+                    );
+                }
+
+                var expectedState = getBlockState().setValue(MonitorBlock.STATE, MonitorEdgeState.fromConnections(
+                    y < height - 1, y > 0, x > 0, x < width - 1
+                ));
+                if (monitor.getBlockState() != expectedState) {
+                    okay = false;
+                    LOG.error(
+                        "Monitor {} expected to have state {}, but has state {}",
+                        monitor.getBlockState(), expectedState, monitor.getBlockState()
+                    );
+                }
+            }
+        }
+
+        if (hasPeripheral != (serverMonitor != null && serverMonitor.getTerminal() != null)) {
+            okay = false;
+            LOG.error(
+                "Peripheral is {}, but serverMonitor={} and serverMonitor.terminal={}",
+                hasPeripheral, serverMonitor, serverMonitor == null ? null : serverMonitor.getTerminal()
+            );
+        }
+
+        return okay;
+    }
 }

projects/common/src/testMod/kotlin/dan200/computercraft/gametest/Monitor_Test.kt

@@ -17,7 +17,10 @@ import net.minecraft.gametest.framework.GameTestGenerator
 import net.minecraft.gametest.framework.GameTestHelper
 import net.minecraft.gametest.framework.TestFunction
 import net.minecraft.nbt.CompoundTag
+import net.minecraft.world.entity.EntityType
+import net.minecraft.world.item.ItemStack
 import net.minecraft.world.level.block.Blocks
+import org.junit.jupiter.api.Assertions.*
 import java.util.*
 
 class Monitor_Test {
@@ -61,6 +64,45 @@ class Monitor_Test {
         }
     }
 
+    /**
+     * When a monitor is destroyed and then replaced, the terminal is recreated.
+     */
+    @GameTest
+    fun Creates_terminal(helper: GameTestHelper) = helper.sequence {
+        fun monitorAt(x: Int) =
+            helper.getBlockEntity(BlockPos(x, 2, 2), ModRegistry.BlockEntities.MONITOR_ADVANCED.get())
+
+        thenExecute {
+            for (i in 1..3) {
+                assertNull(monitorAt(i).cachedServerMonitor, "Monitor $i starts with no ServerMonitor")
+            }
+
+            monitorAt(2).peripheral()
+            assertNotNull(monitorAt(1).cachedServerMonitor?.terminal, "Creating a peripheral creates a terminal")
+
+            // Then remove the middle monitor and check it splits into two.
+            helper.setBlock(BlockPos(2, 2, 2), Blocks.AIR.defaultBlockState())
+
+            assertNotNull(monitorAt(3).cachedServerMonitor, "Origin retains its monitor")
+            assertNull(monitorAt(3).cachedServerMonitor!!.terminal, "Origin deletes the terminal")
+            assertNotEquals(monitorAt(1).cachedServerMonitor, monitorAt(3).cachedServerMonitor, "Monitors are different")
+
+            // Then set the monitor, check it rejoins and recreates the terminal.
+            val pos = BlockPos(2, 2, 2)
+            helper.setBlock(pos, ModRegistry.Blocks.MONITOR_ADVANCED.get())
+            ModRegistry.Blocks.MONITOR_ADVANCED.get().setPlacedBy(
+                helper.level,
+                helper.absolutePos(pos),
+                helper.getBlockState(pos),
+                EntityType.COW.create(helper.level),
+                ItemStack.EMPTY,
+            )
+            monitorAt(2).peripheral()
+
+            assertNotNull(monitorAt(1).cachedServerMonitor?.terminal, "Recreates the terminal")
+        }
+    }
+
     /**
      * Test monitors render correctly
      */

projects/common/src/testMod/resources/data/cctest/structures/monitor_test.creates_terminal.snbt

@@ -0,0 +1,139 @@
+{
+    DataVersion: 3465,
+    size: [5, 5, 5],
+    data: [
+        {pos: [0, 0, 0], state: "minecraft:polished_andesite"},
+        {pos: [0, 0, 1], state: "minecraft:polished_andesite"},
+        {pos: [0, 0, 2], state: "minecraft:polished_andesite"},
+        {pos: [0, 0, 3], state: "minecraft:polished_andesite"},
+        {pos: [0, 0, 4], state: "minecraft:polished_andesite"},
+        {pos: [1, 0, 0], state: "minecraft:polished_andesite"},
+        {pos: [1, 0, 1], state: "minecraft:polished_andesite"},
+        {pos: [1, 0, 2], state: "minecraft:polished_andesite"},
+        {pos: [1, 0, 3], state: "minecraft:polished_andesite"},
+        {pos: [1, 0, 4], state: "minecraft:polished_andesite"},
+        {pos: [2, 0, 0], state: "minecraft:polished_andesite"},
+        {pos: [2, 0, 1], state: "minecraft:polished_andesite"},
+        {pos: [2, 0, 2], state: "minecraft:polished_andesite"},
+        {pos: [2, 0, 3], state: "minecraft:polished_andesite"},
+        {pos: [2, 0, 4], state: "minecraft:polished_andesite"},
+        {pos: [3, 0, 0], state: "minecraft:polished_andesite"},
+        {pos: [3, 0, 1], state: "minecraft:polished_andesite"},
+        {pos: [3, 0, 2], state: "minecraft:polished_andesite"},
+        {pos: [3, 0, 3], state: "minecraft:polished_andesite"},
+        {pos: [3, 0, 4], state: "minecraft:polished_andesite"},
+        {pos: [4, 0, 0], state: "minecraft:polished_andesite"},
+        {pos: [4, 0, 1], state: "minecraft:polished_andesite"},
+        {pos: [4, 0, 2], state: "minecraft:polished_andesite"},
+        {pos: [4, 0, 3], state: "minecraft:polished_andesite"},
+        {pos: [4, 0, 4], state: "minecraft:polished_andesite"},
+        {pos: [0, 1, 0], state: "minecraft:air"},
+        {pos: [0, 1, 1], state: "minecraft:air"},
+        {pos: [0, 1, 2], state: "minecraft:air"},
+        {pos: [0, 1, 3], state: "minecraft:air"},
+        {pos: [0, 1, 4], state: "minecraft:air"},
+        {pos: [1, 1, 0], state: "minecraft:air"},
+        {pos: [1, 1, 1], state: "minecraft:air"},
+        {pos: [1, 1, 2], state: "computercraft:monitor_advanced{facing:north,orientation:north,state:l}", nbt: {Height: 1, Width: 3, XIndex: 2, YIndex: 0, id: "computercraft:monitor_advanced"}},
+        {pos: [1, 1, 3], state: "minecraft:air"},
+        {pos: [1, 1, 4], state: "minecraft:air"},
+        {pos: [2, 1, 0], state: "minecraft:air"},
+        {pos: [2, 1, 1], state: "minecraft:air"},
+        {pos: [2, 1, 2], state: "computercraft:monitor_advanced{facing:north,orientation:north,state:lr}", nbt: {Height: 1, Width: 3, XIndex: 1, YIndex: 0, id: "computercraft:monitor_advanced"}},
+        {pos: [2, 1, 3], state: "minecraft:air"},
+        {pos: [2, 1, 4], state: "minecraft:air"},
+        {pos: [3, 1, 0], state: "minecraft:air"},
+        {pos: [3, 1, 1], state: "minecraft:air"},
+        {pos: [3, 1, 2], state: "computercraft:monitor_advanced{facing:north,orientation:north,state:r}", nbt: {Height: 1, Width: 3, XIndex: 0, YIndex: 0, id: "computercraft:monitor_advanced"}},
+        {pos: [3, 1, 3], state: "minecraft:air"},
+        {pos: [3, 1, 4], state: "minecraft:air"},
+        {pos: [4, 1, 0], state: "minecraft:air"},
+        {pos: [4, 1, 1], state: "minecraft:air"},
+        {pos: [4, 1, 2], state: "minecraft:air"},
+        {pos: [4, 1, 3], state: "minecraft:air"},
+        {pos: [4, 1, 4], state: "minecraft:air"},
+        {pos: [0, 3, 0], state: "minecraft:air"},
+        {pos: [0, 2, 1], state: "minecraft:air"},
+        {pos: [0, 2, 2], state: "minecraft:air"},
+        {pos: [0, 2, 3], state: "minecraft:air"},
+        {pos: [0, 2, 4], state: "minecraft:air"},
+        {pos: [1, 2, 0], state: "minecraft:air"},
+        {pos: [1, 2, 1], state: "minecraft:air"},
+        {pos: [1, 2, 2], state: "minecraft:air"},
+        {pos: [1, 2, 3], state: "minecraft:air"},
+        {pos: [1, 2, 4], state: "minecraft:air"},
+        {pos: [2, 2, 0], state: "minecraft:air"},
+        {pos: [2, 2, 1], state: "minecraft:air"},
+        {pos: [2, 2, 2], state: "minecraft:air"},
+        {pos: [2, 2, 3], state: "minecraft:air"},
+        {pos: [2, 2, 4], state: "minecraft:air"},
+        {pos: [3, 2, 0], state: "minecraft:air"},
+        {pos: [3, 2, 1], state: "minecraft:air"},
+        {pos: [3, 2, 2], state: "minecraft:air"},
+        {pos: [3, 2, 3], state: "minecraft:air"},
+        {pos: [3, 2, 4], state: "minecraft:air"},
+        {pos: [4, 2, 0], state: "minecraft:air"},
+        {pos: [4, 2, 1], state: "minecraft:air"},
+        {pos: [4, 2, 2], state: "minecraft:air"},
+        {pos: [4, 2, 3], state: "minecraft:air"},
+        {pos: [4, 2, 4], state: "minecraft:air"},
+        {pos: [0, 3, 0], state: "minecraft:air"},
+        {pos: [0, 3, 1], state: "minecraft:air"},
+        {pos: [0, 3, 2], state: "minecraft:air"},
+        {pos: [0, 3, 3], state: "minecraft:air"},
+        {pos: [0, 3, 4], state: "minecraft:air"},
+        {pos: [1, 3, 0], state: "minecraft:air"},
+        {pos: [1, 3, 1], state: "minecraft:air"},
+        {pos: [1, 3, 2], state: "minecraft:air"},
+        {pos: [1, 3, 3], state: "minecraft:air"},
+        {pos: [1, 3, 4], state: "minecraft:air"},
+        {pos: [2, 3, 0], state: "minecraft:air"},
+        {pos: [2, 3, 1], state: "minecraft:air"},
+        {pos: [2, 3, 2], state: "minecraft:air"},
+        {pos: [2, 3, 3], state: "minecraft:air"},
+        {pos: [2, 3, 4], state: "minecraft:air"},
+        {pos: [3, 3, 0], state: "minecraft:air"},
+        {pos: [3, 3, 1], state: "minecraft:air"},
+        {pos: [3, 3, 2], state: "minecraft:air"},
+        {pos: [3, 3, 3], state: "minecraft:air"},
+        {pos: [3, 3, 4], state: "minecraft:air"},
+        {pos: [4, 3, 0], state: "minecraft:air"},
+        {pos: [4, 3, 1], state: "minecraft:air"},
+        {pos: [4, 3, 2], state: "minecraft:air"},
+        {pos: [4, 3, 3], state: "minecraft:air"},
+        {pos: [4, 3, 4], state: "minecraft:air"},
+        {pos: [0, 4, 0], state: "minecraft:air"},
+        {pos: [0, 4, 1], state: "minecraft:air"},
+        {pos: [0, 4, 2], state: "minecraft:air"},
+        {pos: [0, 4, 3], state: "minecraft:air"},
+        {pos: [0, 4, 4], state: "minecraft:air"},
+        {pos: [1, 4, 0], state: "minecraft:air"},
+        {pos: [1, 4, 1], state: "minecraft:air"},
+        {pos: [1, 4, 2], state: "minecraft:air"},
+        {pos: [1, 4, 3], state: "minecraft:air"},
+        {pos: [1, 4, 4], state: "minecraft:air"},
+        {pos: [2, 4, 0], state: "minecraft:air"},
+        {pos: [2, 4, 1], state: "minecraft:air"},
+        {pos: [2, 4, 2], state: "minecraft:air"},
+        {pos: [2, 4, 3], state: "minecraft:air"},
+        {pos: [2, 4, 4], state: "minecraft:air"},
+        {pos: [3, 4, 0], state: "minecraft:air"},
+        {pos: [3, 4, 1], state: "minecraft:air"},
+        {pos: [3, 4, 2], state: "minecraft:air"},
+        {pos: [3, 4, 3], state: "minecraft:air"},
+        {pos: [3, 4, 4], state: "minecraft:air"},
+        {pos: [4, 4, 0], state: "minecraft:air"},
+        {pos: [4, 4, 1], state: "minecraft:air"},
+        {pos: [4, 4, 2], state: "minecraft:air"},
+        {pos: [4, 4, 3], state: "minecraft:air"},
+        {pos: [4, 4, 4], state: "minecraft:air"}
+    ],
+    entities: [],
+    palette: [
+        "minecraft:polished_andesite",
+        "minecraft:air",
+        "computercraft:monitor_advanced{facing:north,orientation:north,state:l}",
+        "computercraft:monitor_advanced{facing:north,orientation:north,state:lr}",
+        "computercraft:monitor_advanced{facing:north,orientation:north,state:r}"
+    ]
+}

projects/fabric/build.gradle.kts

@@ -150,6 +150,8 @@ loom {
 
             // Load cctest last, so it can override resources. This bypasses Fabric's shuffling of mods
             property("fabric.debug.loadLate", "cctest")
+
+            vmArg("-ea")
         }
 
         val testClient by registering {

projects/forge/build.gradle.kts

@@ -102,6 +102,7 @@ minecraft {
             configureForGameTest()
 
             property("forge.logging.console.level", "info")
+            jvmArg("-ea")
         }
     }
 }