From 93ad40efbbf7b573c5ad48e7d54d215d9cc19200 Mon Sep 17 00:00:00 2001 From: Jonathan Coates Date: Mon, 9 Oct 2023 22:03:43 +0100 Subject: [PATCH] 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 ab785a090698e6972caac95691393428c6f8370b 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 --- .../monitor/MonitorBlockEntity.java | 96 +++++++++++- .../computercraft/gametest/Monitor_Test.kt | 42 ++++++ .../monitor_test.creates_terminal.snbt | 139 ++++++++++++++++++ projects/fabric/build.gradle.kts | 2 + projects/forge/build.gradle.kts | 1 + 5 files changed, 276 insertions(+), 4 deletions(-) create mode 100644 projects/common/src/testMod/resources/data/cctest/structures/monitor_test.creates_terminal.snbt diff --git a/projects/common/src/main/java/dan200/computercraft/shared/peripheral/monitor/MonitorBlockEntity.java b/projects/common/src/main/java/dan200/computercraft/shared/peripheral/monitor/MonitorBlockEntity.java index fbd451bf5..a08feac5a 100644 --- a/projects/common/src/main/java/dan200/computercraft/shared/peripheral/monitor/MonitorBlockEntity.java +++ b/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(); - if (peripheral != null) return peripheral; - return peripheral = new MonitorPeripheral(this); + createServerTerminal(); + var peripheral = this.peripheral != null ? this.peripheral : (this.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; + } } diff --git a/projects/common/src/testMod/kotlin/dan200/computercraft/gametest/Monitor_Test.kt b/projects/common/src/testMod/kotlin/dan200/computercraft/gametest/Monitor_Test.kt index 4e9c54bed..2433a5893 100644 --- a/projects/common/src/testMod/kotlin/dan200/computercraft/gametest/Monitor_Test.kt +++ b/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 */ diff --git a/projects/common/src/testMod/resources/data/cctest/structures/monitor_test.creates_terminal.snbt b/projects/common/src/testMod/resources/data/cctest/structures/monitor_test.creates_terminal.snbt new file mode 100644 index 000000000..83f4fcea8 --- /dev/null +++ b/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}" + ] +} diff --git a/projects/fabric/build.gradle.kts b/projects/fabric/build.gradle.kts index 3a737ca38..9e81e6a99 100644 --- a/projects/fabric/build.gradle.kts +++ b/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 { diff --git a/projects/forge/build.gradle.kts b/projects/forge/build.gradle.kts index bb80450e8..ac6882a3e 100644 --- a/projects/forge/build.gradle.kts +++ b/projects/forge/build.gradle.kts @@ -102,6 +102,7 @@ minecraft { configureForGameTest() property("forge.logging.console.level", "info") + jvmArg("-ea") } } }