diff --git a/plans/chunk-load-sign-reconciliation-plan.md b/plans/chunk-load-sign-reconciliation-plan.md new file mode 100644 index 0000000..1d5f818 --- /dev/null +++ b/plans/chunk-load-sign-reconciliation-plan.md @@ -0,0 +1,142 @@ +# Chunk-load sign reconciliation + +## Context + +Addresses GitHub issue #110, the follow-up `plans/region-sharded-sign-persistence-plan.md` and +`plans/sign-storage-refactor-options.md` both flagged but deferred: nothing today detects a sign that vanished while +its chunk was unloaded (external region-file deletion/regen, backup restore, manual NBT surgery). The removal path +that exists (`AbstractBlockInject` mixin on `BlockBehaviour.affectNeighborsAfterRemoval`) only fires for an in-game +block change on a loaded chunk — it can't see a sign that disappeared by any other means. `SignManager`'s cache +keeps that entry and its BlueMap marker forever. + +`region-sharded-sign-persistence-plan.md` set up on-disk storage sharded by 512-block region specifically so this +feature would have something cheap to query, but explicitly left `SignManager`'s in-memory `ConcurrentMap` untouched, deferring "a region-indexed in-memory view" to whichever feature actually consumes it. +`sign-storage-refactor-options.md` rejected sharding that map speculatively for the same reason — no caller yet. This +plan is that caller. + +Fabric API already exposes the event needed, no new dependency: `ServerChunkEvents.CHUNK_LOAD` +(`fabric-lifecycle-events-v1`, part of `fabric_api_version` already in `gradle.properties`) — +`Load.onChunkLoad(ServerLevel level, LevelChunk chunk, boolean generated)`, fired once a chunk is already loaded into +a `ServerLevel`. + +**Performance constraint**: chunk load fires constantly — spawn-chunk keep-alive, players walking, world +pre-generation. The design below must not scan the full sign cache per event. + +## Goal + +On every chunk load, cheaply find which cached signs belong to that chunk, check each still has a live +`SignBlockEntity` at its position, and remove (via the existing `SignManager.remove`, unchanged) any that don't. + +## Design + +### `SignChunkKey` (new, plain Java, unit-testable) + +`record SignChunkKey(String parentMap, int chunkX, int chunkZ)`, in `core.signs` (a runtime lookup concern, not +persistence — kept separate from `core.signs.persistence.SignRegionKey`, which is 512-block/32-chunk file-layout +math and stays untouched). `forEntryKey(SignEntryKey key)` computes `Math.floorDiv(x, 16)`/`Math.floorDiv(z, 16)` — +plain per-chunk coordinates, `floorDiv` for the same negative-coordinate reason `SignRegionKey` already documents. + +### `SignChunkIndex` (new, plain Java, unit-testable) + +Wraps `ConcurrentHashMap>` (values are `ConcurrentHashMap.newKeySet()`), in +`core.signs`: +- `add(SignEntryKey key)` — `computeIfAbsent(SignChunkKey.forEntryKey(key), ...).add(key)`. +- `remove(SignEntryKey key)` — drops `key` from its chunk's set; if the set is now empty, removes the chunk entry + too, so emptied-out areas don't leak map entries over a long-running server. +- `keysInChunk(String parentMap, int chunkX, int chunkZ)` — returns a snapshot `List` (empty list if + none tracked), the query the chunk-load handler uses. +- `clear()`. + +Fully independent of `SignManager`/BlueMap types — testable the same way as `SignRegionKeyTest`/ +`SignRegionPartitionerTest`. + +### `SignManager` changes + +- New field: `private final SignChunkIndex chunkIndex = new SignChunkIndex();`. +- Every place `signCache.put(key, entry)` establishes a *new* key (the add branch in `addOrUpdateSign`) also calls + `chunkIndex.add(key)`. `removeByKey` also calls `chunkIndex.remove(key)`. The update branch (existing entry, same + key, text/prefix change) doesn't touch `chunkIndex` — position is immutable once a `SignEntry` exists. +- `reloadSigns()` calls `chunkIndex.clear()` alongside `signCache.clear()` (the replay loop repopulates both via the + same add path). +- New method: `static List getKeysInChunk(String parentMap, int chunkX, int chunkZ)` → delegates to + `chunkIndex.keysInChunk(...)`. Pure data query, no BlueMap/game types — but `SignManager` itself stays outside unit + test coverage regardless (its constructor still builds a `BlueMapAPIConnector`), consistent with its current status + in `testing.md`. + +### Mod-level chunk-load handler (game-coupled, `BlueMapSignMarkersMod`) + +Register alongside the existing three hooks: `ServerChunkEvents.CHUNK_LOAD.register(this::onChunkLoad);`. + +```java +private void onChunkLoad(ServerLevel level, LevelChunk chunk, boolean generated) { + var parentMap = SignHelper.getSignParentMap(level); + var chunkPos = chunk.getPos(); + + for (var key : SignManager.getKeysInChunk(parentMap, chunkPos.x(), chunkPos.z())) { + if (!(chunk.getBlockEntity(new BlockPos(key.x(), key.y(), key.z())) instanceof SignBlockEntity)) { + LOGGER.info("Removing stale sign marker at {} - no sign block found on chunk load " + + "(external deletion/regen?)", key); + SignManager.remove(key); + } + } +} +``` + +`chunk.getBlockEntity(pos)` is used directly (not `level.getBlockEntity(pos)`) since the event already hands us the +loaded chunk — a direct lookup into its already-deserialized block-entity map, no re-resolution through the level's +chunk manager. + +**Deliberately no special case for `generated == true`.** That flag also fires when a chunk's saved data is missing +and Minecraft regenerates it fresh — exactly the "region file deleted externally" scenario this feature targets. +Skipping reconciliation there would silently defeat the main use case. There's no performance reason to skip it +either: the `chunkIndex` lookup is a single hashmap `get` returning an empty list for the overwhelming majority of +chunk loads (no signs ever tracked there), so cost is the same whether or not the chunk is newly generated. + +## Performance + +- Cost per chunk load: one `ConcurrentHashMap.get` (via `keysInChunk`) — O(1), returns empty for nearly every chunk. +- Only chunks that do have tracked signs (a tiny fraction in practice) pay any further cost, bounded by the number of + signs in that one 16x16 chunk (typically 0-2), each a single `getBlockEntity` call already backed by a hashmap the + chunk itself maintains — no scanning, no I/O, no BlueMap API calls unless a removal is actually dispatched. +- No new threading: runs synchronously on the same thread the existing `BLOCK_ENTITY_LOAD` handler already runs on; + `SignManager.remove`'s dispatch to BlueMap still goes through the existing async `ReactiveQueue`. + +## Changes (files) + +1. **New** `core/signs/SignChunkKey.java` — record + `forEntryKey`. +2. **New** `core/signs/SignChunkIndex.java` — the add/remove/keysInChunk/clear wrapper described above. +3. **`core/signs/SignManager.java`** — add `chunkIndex` field, wire it into the add/remove paths and + `reloadSigns()`, add `getKeysInChunk` static method. +4. **`BlueMapSignMarkersMod.java`** — register `ServerChunkEvents.CHUNK_LOAD`, add `onChunkLoad` handler. +5. **New tests**: + - `src/test/java/.../core/signs/SignChunkKeyTest.java` — `floorDiv` chunk assignment, negative coordinates, + boundary blocks (15/16), mirroring `SignRegionKeyTest`'s pattern. + - `src/test/java/.../core/signs/SignChunkIndexTest.java` — add/query round-trip, multiple signs in one chunk, + signs in different chunks/dimensions stay isolated, `remove` drops the chunk entry once its set empties, + `clear` resets everything. + +No `SignFileVersions` bump, no persisted-schema change — this is runtime-cache-only. No `mod_version` bump either: +the last release hasn't shipped yet, so this folds into the same in-progress `26.2-0.17.0`. + +## Out of scope + +- A config toggle to disable reconciliation — not requested, and there's no existing feature-flag mechanism in this + mod to extend. +- Reconciling on chunk *unload* — not requested. +- Any change to `SignRegionKey`/region-sharded on-disk storage — persistence layout is unrelated to this in-memory + lookup index. + +## Verification + +- `./gradlew test` — new `SignChunkKeyTest`/`SignChunkIndexTest` pass alongside the existing suite. +- `./gradlew build` — full build still succeeds. +- Manual, via `./gradlew runServer` (this handler touches live Minecraft/Fabric types, no automated coverage + possible per `testing.md`): + 1. Place a sign, confirm its marker appears in BlueMap. Restart the server, confirm the marker is unchanged and no + spurious remove/re-add happens (watch the log for the new INFO line — it should **not** appear). + 2. Place a sign, stop the server, then remove the physical block without going through the mod (e.g. delete that + chunk's region `.mca` file to force a regen, or edit the block out with an NBT editor). Restart, walk into that + chunk, confirm: the INFO log line appears, and the stale marker disappears from BlueMap. + 3. Repeat with signs in two different dimensions (overworld + nether) at the same x/z to confirm dimension + isolation — loading one dimension's chunk must not touch the other's sign at the same coordinates. diff --git a/plans/sign-storage-refactor-options.md b/plans/sign-storage-refactor-options.md new file mode 100644 index 0000000..4fb38f5 --- /dev/null +++ b/plans/sign-storage-refactor-options.md @@ -0,0 +1,77 @@ +# In-memory sign storage: refactor options + +## Context + +`SignManager` (`src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignManager.java`, `signCache` field) holds +the only in-memory copy of known signs: a single flat `ConcurrentHashMap` (`signCache`). Each +`SignEntry` (`core/signs/SignEntry.java`) carries `playerId` plus parsed front/back text +(`SignLinesParseResult`: prefix/label/detail, `core/signs/SignLinesParseResult.java`). BlueMap itself only ever +receives label/detail via dispatched `MarkerAction`s — it is not queried back for state. + +This doc answers two questions raised during review: is the current layout a space/time problem, and does it make +sense to keep a full original copy of each sign in memory at all given markers already live in BlueMap. + +## Performance analysis + +**Time.** Hot-path operations (`addOrUpdateSign`, `removeByKey`, both driven by mixin injects on player edit / block +removal, and `BLOCK_ENTITY_LOAD`) are `ConcurrentHashMap` get/put/remove — O(1). `getAllSigns()` copies the whole map +into an `ArrayList` — O(n), but its only callers are `saveSigns` (server stop) and `reloadSigns()` (BlueMap reset), +both rare, whole-lifecycle events, not per-sign operations. No time bottleneck exists at any realistic scale. + +**Space.** Per sign: a `SignEntryKey` (3 ints + a `parentMap` String) stored twice — once as the map key, once again +inside `SignEntry.key()` — plus `playerId` and two `SignLinesParseResult` records (3 Strings each). Call it +~150-250 bytes of JVM overhead per entry plus string data. Even 10,000 signs (far beyond what a real server +accumulates — that's more player-placed signs than most maps have blocks-of-interest) is a few MB. Not a real +problem. + +## Does the original copy need to exist? + +Yes. BlueMap is a downstream projection of this cache, not a queryable store the mod can read back from. The cache +is the only source of truth for three things a marker-only view can't provide: + +1. **Diffing.** `isTextDifferent(...)` compares label/detail against the cached entry before dispatching an update, + so a player re-saving identical text doesn't spam BlueMap's marker store or connected web clients with a no-op + update. +2. **Reset replay.** `reset()` → `reloadSigns()` clears and rebuilds the whole cache when BlueMap itself resets. + There is no "ask BlueMap for current markers" path — the mod must be able to resend everything from its own + state. +3. **Persistence.** `signs.json` (region-sharded per + `plans/region-sharded-sign-persistence-plan.md`) is written from `SignManager.getAll()`. Nothing about BlueMap + survives a server restart for this mod's purposes; this cache is what does. + +Removing the original copy isn't on the table. The only real question is whether its *layout* is worth changing. + +## Options + +**1. Do nothing — keep the flat `ConcurrentHashMap`** +- Pros: simplest, already O(1) on the hot path, proven correct, zero migration risk. +- Cons: `SignEntryKey` duplicated (map key + `SignEntry.key()` field); `SignEntryHelper.getPrefix`/`getLabel`/ + `getDetail` recompute from raw parse results on every call site instead of being cached once. + +**2. Cache derived fields (prefix/label/detail) on the entry instead of recomputing via `SignEntryHelper` each call** +- Pros: cuts redundant string work in `addOrUpdateSign`, which currently calls `getPrefix`/`getLabel`/`getDetail` + up to ~3 times per invocation across both the new and existing entry. +- Cons: real code churn for a CPU micro-optimization on a low-frequency hot path (player sign edits, not a + tight loop); `signs.json`'s `SignEntryV2` schema still needs the raw front/back split for persistence regardless, + so this only saves cycles, not memory or file format. + +**3. Shard the in-memory map by dimension (`Map>`), mirroring the + region-sharded on-disk format from #109** +- Pros: conceptually matches the disk layout; would let reset/save scope to a single dimension if that ever became + a requirement. +- Cons: adds a second map hop to every lookup for a capability nothing currently uses — `reset()` and save/load are + whole-world operations today, not per-dimension. `plans/region-sharded-sign-persistence-plan.md` explicitly chose + to leave the in-memory map as-is for this reason, deferring a region-indexed in-memory view to whichever future + feature (chunk-reset reconciliation) actually consumes it. Speculative generality with no current caller. + +**4. Bit-pack `x/y/z` into a primitive `long` key (per dimension) instead of a boxed `SignEntryKey` record** +- Pros: only matters at very large scale (100k+ signs) — shaves allocation/lookup overhead. +- Cons: loses `SignEntryKey`'s readable `equals`/`toString`; needs careful encoding for Minecraft's y-range + (-64..320); meaningful complexity for a scale this mod will not hit in practice. + +## Recommendation + +Option 1. At realistic Minecraft server sign counts this isn't a storage problem — it's a working, correct, +O(1)-on-the-hot-path cache doing exactly the job the architecture needs (diff, replay, persist). Option 2 is worth +revisiting only if profiling ever shows `addOrUpdateSign` as hot; options 3 and 4 solve problems this project does +not have. diff --git a/src/main/java/com/tpwalke2/bluemapsignmarkers/BlueMapSignMarkersMod.java b/src/main/java/com/tpwalke2/bluemapsignmarkers/BlueMapSignMarkersMod.java index 3ce6f6e..5003ee6 100644 --- a/src/main/java/com/tpwalke2/bluemapsignmarkers/BlueMapSignMarkersMod.java +++ b/src/main/java/com/tpwalke2/bluemapsignmarkers/BlueMapSignMarkersMod.java @@ -5,22 +5,30 @@ import com.tpwalke2.bluemapsignmarkers.core.signs.persistence.SignProvider; import net.fabricmc.api.DedicatedServerModInitializer; import net.fabricmc.fabric.api.event.lifecycle.v1.ServerBlockEntityEvents; +import net.fabricmc.fabric.api.event.lifecycle.v1.ServerChunkEvents; import net.fabricmc.fabric.api.event.lifecycle.v1.ServerLifecycleEvents; +import net.minecraft.core.BlockPos; import net.minecraft.server.MinecraftServer; import net.minecraft.server.level.ServerLevel; import net.minecraft.world.level.block.entity.BlockEntity; import net.minecraft.world.level.block.entity.SignBlockEntity; +import net.minecraft.world.level.chunk.LevelChunk; import net.minecraft.world.level.storage.LevelResource; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.nio.file.Path; public class BlueMapSignMarkersMod implements DedicatedServerModInitializer, ServerPathProvider { + private static final Logger LOGGER = LoggerFactory.getLogger(Constants.MOD_ID); + @Override public void onInitializeServer() { ServerLifecycleEvents.SERVER_STARTING.register(this::onServerStarting); ServerLifecycleEvents.SERVER_STOPPING.register(this::onServerStopping); ServerBlockEntityEvents.BLOCK_ENTITY_LOAD.register(this::onBlockEntityLoad); + ServerChunkEvents.CHUNK_LOAD.register(this::onChunkLoad); } private void onServerStarting(MinecraftServer server) { @@ -58,4 +66,21 @@ private void onBlockEntityLoad(BlockEntity blockEntity, ServerLevel world) { SignManager.addOrUpdate(SignHelper.createSignEntry(castBlockEntity, "unknown")); } + + // No special case for a newly-generated chunk (generated == true): that flag also fires when a chunk's saved + // data is missing and Minecraft regenerates it fresh - exactly the "region file deleted externally" case this + // reconciliation targets. Skipping it there would defeat the main use case, and there's no perf reason to: + // getKeysInChunk is a single hashmap lookup, so the cost is the same either way. + private void onChunkLoad(ServerLevel level, LevelChunk chunk, boolean generated) { + var parentMap = SignHelper.getSignParentMap(level); + var chunkPos = chunk.getPos(); + + for (var key : SignManager.getKeysInChunk(parentMap, chunkPos.x(), chunkPos.z())) { + if (!(chunk.getBlockEntity(new BlockPos(key.x(), key.y(), key.z())) instanceof SignBlockEntity)) { + LOGGER.info("Removing stale sign marker at {} - no sign block found on chunk load " + + "(external deletion/regen?)", key); + SignManager.remove(key); + } + } + } } diff --git a/src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignChunkIndex.java b/src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignChunkIndex.java new file mode 100644 index 0000000..30d7aa2 --- /dev/null +++ b/src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignChunkIndex.java @@ -0,0 +1,40 @@ +package com.tpwalke2.bluemapsignmarkers.core.signs; + +import java.util.ArrayList; +import java.util.List; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; + +// Secondary lookup alongside SignManager's flat sign cache: which sign keys fall in a given chunk, so chunk-load +// reconciliation can ask "signs known here?" in O(1) instead of scanning every cached sign. +public class SignChunkIndex { + private final ConcurrentHashMap> signsByChunk = new ConcurrentHashMap<>(); + + // add/remove mutate the per-chunk set inside the map's own compute-family remapping function (not via a + // fetch-then-mutate-outside-the-lock pattern) so the "does this chunk still have entries" decision and the + // mutation happen as one atomic step - ConcurrentHashMap serializes compute/computeIfPresent/computeIfAbsent + // calls against each other for the same key, so a concurrent add can never resurrect a set remove just deleted. + public void add(SignEntryKey key) { + signsByChunk.compute(SignChunkKey.forEntryKey(key), (unusedKey, keysInChunk) -> { + var result = keysInChunk != null ? keysInChunk : ConcurrentHashMap.newKeySet(); + result.add(key); + return result; + }); + } + + public void remove(SignEntryKey key) { + signsByChunk.computeIfPresent(SignChunkKey.forEntryKey(key), (unusedKey, keysInChunk) -> { + keysInChunk.remove(key); + return keysInChunk.isEmpty() ? null : keysInChunk; + }); + } + + public List keysInChunk(String parentMap, int chunkX, int chunkZ) { + var keysInChunk = signsByChunk.get(new SignChunkKey(parentMap, chunkX, chunkZ)); + return keysInChunk == null ? List.of() : new ArrayList<>(keysInChunk); + } + + public void clear() { + signsByChunk.clear(); + } +} diff --git a/src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignChunkKey.java b/src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignChunkKey.java new file mode 100644 index 0000000..818a213 --- /dev/null +++ b/src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignChunkKey.java @@ -0,0 +1,14 @@ +package com.tpwalke2.bluemapsignmarkers.core.signs; + +public record SignChunkKey(String parentMap, int chunkX, int chunkZ) { + // Vanilla chunk size in blocks - distinct from and finer-grained than SignRegionKey's 512-block + // (32-chunk) region math, which is a persistence file-layout concern, not a runtime lookup one. + private static final int CHUNK_SIZE_BLOCKS = 16; + + public static SignChunkKey forEntryKey(SignEntryKey key) { + return new SignChunkKey( + key.parentMap(), + Math.floorDiv(key.x(), CHUNK_SIZE_BLOCKS), + Math.floorDiv(key.z(), CHUNK_SIZE_BLOCKS)); + } +} diff --git a/src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignManager.java b/src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignManager.java index 727069d..0d18259 100644 --- a/src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignManager.java +++ b/src/main/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignManager.java @@ -53,9 +53,14 @@ public static void stop() { getInstance().shutdown(); } + public static List getKeysInChunk(String parentMap, int chunkX, int chunkZ) { + return getInstance().chunkIndex.keysInChunk(parentMap, chunkX, chunkZ); + } + private final BlueMapAPIConnector blueMapAPIConnector; private final ActionFactory actionFactory; private final ConcurrentMap signCache = new ConcurrentHashMap<>(); + private final SignChunkIndex chunkIndex = new SignChunkIndex(); private final Map prefixGroupMap; private SignManager() { @@ -88,6 +93,7 @@ private void reloadSigns() { LOGGER.info("Reloading all signs..."); var existingSigns = getAllSigns(); signCache.clear(); + chunkIndex.clear(); for (SignEntry signEntry : existingSigns) { addOrUpdateSign(signEntry); } @@ -110,6 +116,7 @@ private void addOrUpdateSign(SignEntry signEntry) { if (shouldAddPOIMarker(existing, isPOIMarker)) { LOGGER.debug("Adding POI marker: {}", signEntry); signCache.put(key, signEntry); + chunkIndex.add(key); blueMapAPIConnector.dispatch( actionFactory.createAddPOIAction( key.x(), @@ -203,6 +210,8 @@ private void removeByKey(SignEntryKey key) { return; } + chunkIndex.remove(key); + var prefix = SignEntryHelper.getPrefix(removed); if (prefix == null) { diff --git a/src/test/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignChunkIndexTest.java b/src/test/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignChunkIndexTest.java new file mode 100644 index 0000000..5da358e --- /dev/null +++ b/src/test/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignChunkIndexTest.java @@ -0,0 +1,175 @@ +package com.tpwalke2.bluemapsignmarkers.core.signs; + +import org.junit.jupiter.api.Test; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class SignChunkIndexTest { + + @Test + void keysInChunkReturnsEmptyWhenNothingTracked() { + var index = new SignChunkIndex(); + + assertEquals(0, index.keysInChunk("minecraft:overworld", 0, 0).size()); + } + + @Test + void addMakesKeyFindableByItsChunk() { + var index = new SignChunkIndex(); + var key = new SignEntryKey(1, 64, 1, "minecraft:overworld"); + + index.add(key); + + assertEquals(1, index.keysInChunk("minecraft:overworld", 0, 0).size()); + assertTrue(index.keysInChunk("minecraft:overworld", 0, 0).contains(key)); + } + + @Test + void addTracksMultipleKeysInTheSameChunk() { + var index = new SignChunkIndex(); + var first = new SignEntryKey(1, 64, 1, "minecraft:overworld"); + var second = new SignEntryKey(2, 65, 2, "minecraft:overworld"); + + index.add(first); + index.add(second); + + var keys = index.keysInChunk("minecraft:overworld", 0, 0); + assertEquals(2, keys.size()); + assertTrue(keys.contains(first)); + assertTrue(keys.contains(second)); + } + + @Test + void keysInDifferentChunksStayIsolated() { + var index = new SignChunkIndex(); + var nearby = new SignEntryKey(1, 64, 1, "minecraft:overworld"); + var farAway = new SignEntryKey(600, 64, 600, "minecraft:overworld"); + + index.add(nearby); + index.add(farAway); + + var nearbyChunk = index.keysInChunk("minecraft:overworld", 0, 0); + var farAwayChunk = index.keysInChunk("minecraft:overworld", 37, 37); + assertEquals(1, nearbyChunk.size()); + assertTrue(nearbyChunk.contains(nearby)); + assertEquals(1, farAwayChunk.size()); + assertTrue(farAwayChunk.contains(farAway)); + } + + @Test + void keysInDifferentDimensionsAtTheSameCoordinatesStayIsolated() { + var index = new SignChunkIndex(); + var overworldKey = new SignEntryKey(1, 64, 1, "minecraft:overworld"); + var netherKey = new SignEntryKey(1, 64, 1, "minecraft:the_nether"); + + index.add(overworldKey); + index.add(netherKey); + + assertEquals(1, index.keysInChunk("minecraft:overworld", 0, 0).size()); + assertEquals(1, index.keysInChunk("minecraft:the_nether", 0, 0).size()); + } + + @Test + void removeDropsKeyFromItsChunk() { + var index = new SignChunkIndex(); + var key = new SignEntryKey(1, 64, 1, "minecraft:overworld"); + index.add(key); + + index.remove(key); + + assertEquals(0, index.keysInChunk("minecraft:overworld", 0, 0).size()); + } + + @Test + void removeLeavesOtherKeysInTheSameChunkUntouched() { + var index = new SignChunkIndex(); + var first = new SignEntryKey(1, 64, 1, "minecraft:overworld"); + var second = new SignEntryKey(2, 65, 2, "minecraft:overworld"); + index.add(first); + index.add(second); + + index.remove(first); + + var keys = index.keysInChunk("minecraft:overworld", 0, 0); + assertEquals(1, keys.size()); + assertTrue(keys.contains(second)); + } + + @Test + void removeOfUntrackedKeyIsANoOp() { + var index = new SignChunkIndex(); + var key = new SignEntryKey(1, 64, 1, "minecraft:overworld"); + + index.remove(key); + + assertEquals(0, index.keysInChunk("minecraft:overworld", 0, 0).size()); + } + + @Test + void clearRemovesEverything() { + var index = new SignChunkIndex(); + index.add(new SignEntryKey(1, 64, 1, "minecraft:overworld")); + index.add(new SignEntryKey(600, 64, 600, "minecraft:overworld")); + + index.clear(); + + assertEquals(0, index.keysInChunk("minecraft:overworld", 0, 0).size()); + assertEquals(0, index.keysInChunk("minecraft:overworld", 37, 37).size()); + } + + // Regression test for a race where removing the last key from a chunk could delete the chunk's map entry + // after another thread had already re-added a different key to that same (still-live) Set instance, orphaning + // the newly-added key. Runs many iterations with both operations started simultaneously (via a latch) to + // exercise every possible interleaving between the remove-to-empty and the concurrent add. + @Test + void concurrentAddDuringRemovalOfTheLastKeyIsNeverLost() throws Exception { + var chunkA = new SignEntryKey(1, 64, 1, "minecraft:overworld"); + var chunkB = new SignEntryKey(2, 65, 2, "minecraft:overworld"); + + var executor = Executors.newFixedThreadPool(2); + try { + for (int i = 0; i < 3000; i++) { + var index = new SignChunkIndex(); + index.add(chunkA); + + var ready = new CountDownLatch(2); + var go = new CountDownLatch(1); + + var removeTask = executor.submit(() -> { + ready.countDown(); + awaitUninterruptibly(go); + index.remove(chunkA); + }); + var addTask = executor.submit(() -> { + ready.countDown(); + awaitUninterruptibly(go); + index.add(chunkB); + }); + + assertTrue(ready.await(5, TimeUnit.SECONDS), "iteration " + i + ": a task failed to start in time"); + go.countDown(); + removeTask.get(5, TimeUnit.SECONDS); + addTask.get(5, TimeUnit.SECONDS); + + var keys = index.keysInChunk("minecraft:overworld", 0, 0); + assertTrue(keys.contains(chunkB), "iteration " + i + ": concurrently-added key was lost"); + } + } finally { + executor.shutdownNow(); + } + } + + private static void awaitUninterruptibly(CountDownLatch latch) { + try { + latch.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } +} diff --git a/src/test/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignChunkKeyTest.java b/src/test/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignChunkKeyTest.java new file mode 100644 index 0000000..38738a4 --- /dev/null +++ b/src/test/java/com/tpwalke2/bluemapsignmarkers/core/signs/SignChunkKeyTest.java @@ -0,0 +1,43 @@ +package com.tpwalke2.bluemapsignmarkers.core.signs; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class SignChunkKeyTest { + + @Test + void forEntryKeyAssignsChunkZeroAtOrigin() { + var key = SignChunkKey.forEntryKey(new SignEntryKey(0, 64, 0, "minecraft:overworld")); + + assertEquals(new SignChunkKey("minecraft:overworld", 0, 0), key); + } + + @Test + void forEntryKeyUsesFloorDivisionForNegativeCoordinates() { + var key = SignChunkKey.forEntryKey(new SignEntryKey(-1, 64, -1, "minecraft:overworld")); + + // Truncating division would put x=-1 in the same chunk as x=0 (both round toward zero); floorDiv + // correctly puts it in the chunk to the west/north instead. + assertEquals(-1, key.chunkX()); + assertEquals(-1, key.chunkZ()); + } + + @Test + void forEntryKeySplitsAtChunkBoundary() { + var lastBlockOfChunkZero = SignChunkKey.forEntryKey(new SignEntryKey(15, 64, 15, "minecraft:overworld")); + var firstBlockOfChunkOne = SignChunkKey.forEntryKey(new SignEntryKey(16, 64, 16, "minecraft:overworld")); + + assertEquals(0, lastBlockOfChunkZero.chunkX()); + assertEquals(0, lastBlockOfChunkZero.chunkZ()); + assertEquals(1, firstBlockOfChunkOne.chunkX()); + assertEquals(1, firstBlockOfChunkOne.chunkZ()); + } + + @Test + void forEntryKeyKeepsParentMapUnchanged() { + var key = SignChunkKey.forEntryKey(new SignEntryKey(0, 64, 0, "minecraft:the_nether")); + + assertEquals("minecraft:the_nether", key.parentMap()); + } +}