From b3de41d3876d03a2ac4ac9f0fa0d9c3ca51bde90 Mon Sep 17 00:00:00 2001 From: XxFran10xX <318299142+XxFran10xX@users.noreply.github.com> Date: Fri, 30 Oct 2026 10:56:13 +0100 Subject: [PATCH 1/3] fix: stop leaking label displays and clean up the leftovers Label TextDisplays were spawned persistent and tracked only by an in-memory UUID, so every restart or chunk unload/reload saved the old display with the chunk and spawned a new one next to it. Main has about 3,900 stacked copies around the trial rooms. - Spawn label displays non-persistent and tag them with trialrooms:label. - Never spawn while the label's chunk is unloaded, and only create a display while a player is within the 96-block view range (tick() keeps the same show/hide behaviour and update cadence). - Before spawning, remove a lost TrialRooms display at the same position. - New LabelCleanup listener: on EntitiesLoadEvent (and once at enable for chunks already loaded) remove TextDisplays sitting exactly on a live label position that no label owns and that are either tagged or legacy (persistent, empty PDC, no scoreboard tags). Logs one line per chunk. Co-Authored-By: Claude Opus 5.5 --- .../tfminecraft/trialrooms/TrialRooms.java | 5 + .../environment/spawner/Hologram.java | 155 ++++++--- .../environment/spawner/LabelCleanup.java | 132 ++++++++ .../tfminecraft/trialrooms/LifecycleTest.java | 6 +- .../environment/spawner/HologramTest.java | 301 ++++++++++++------ .../environment/spawner/LabelCleanupTest.java | 191 +++++++++++ 6 files changed, 635 insertions(+), 155 deletions(-) create mode 100644 src/main/java/net/tfminecraft/trialrooms/environment/spawner/LabelCleanup.java create mode 100644 src/test/java/net/tfminecraft/trialrooms/environment/spawner/LabelCleanupTest.java diff --git a/src/main/java/net/tfminecraft/trialrooms/TrialRooms.java b/src/main/java/net/tfminecraft/trialrooms/TrialRooms.java index fbd699b..2c43b78 100644 --- a/src/main/java/net/tfminecraft/trialrooms/TrialRooms.java +++ b/src/main/java/net/tfminecraft/trialrooms/TrialRooms.java @@ -7,6 +7,7 @@ import org.bukkit.plugin.java.JavaPlugin; import net.tfminecraft.trialrooms.environment.entrance.ui.EntranceEditor; +import net.tfminecraft.trialrooms.environment.spawner.LabelCleanup; import net.tfminecraft.trialrooms.environment.spawner.ui.ActiveSpawnerEditor; import net.tfminecraft.trialrooms.loader.ConfigLoader; import net.tfminecraft.trialrooms.loader.SpawnerLoader; @@ -46,6 +47,8 @@ public void onEnable() { Bukkit.getPluginManager().registerEvents(EntranceManager.get(), this); Bukkit.getPluginManager().registerEvents(new ConversionDeathGuard(), this); Bukkit.getPluginManager().registerEvents(new PlayerManager(TrialRooms.getInstance()), this); + // Removes label displays left behind by older versions when their chunk loads. + Bukkit.getPluginManager().registerEvents(new LabelCleanup(), this); // Load DTOs @@ -54,6 +57,8 @@ public void onEnable() { List chests = Database.loadChests(); spawnerManager.hydrateFrom(spawners); ChestManager.get().hydrateFrom(chests); + // Chunks already loaded before the listener existed (spawn chunks, /reload). + LabelCleanup.sweepLoadedChunks(); persistenceReady = true; spawnerManager.start(); getLogger().info("TrialRooms Plugin Enabled!"); diff --git a/src/main/java/net/tfminecraft/trialrooms/environment/spawner/Hologram.java b/src/main/java/net/tfminecraft/trialrooms/environment/spawner/Hologram.java index 8a28994..f58c820 100644 --- a/src/main/java/net/tfminecraft/trialrooms/environment/spawner/Hologram.java +++ b/src/main/java/net/tfminecraft/trialrooms/environment/spawner/Hologram.java @@ -2,15 +2,36 @@ import org.bukkit.Bukkit; import org.bukkit.Location; +import org.bukkit.NamespacedKey; import org.bukkit.World; import org.bukkit.entity.Entity; import org.bukkit.entity.TextDisplay; +import org.bukkit.persistence.PersistentDataType; import org.bukkit.plugin.java.JavaPlugin; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Set; import java.util.UUID; +import java.util.WeakHashMap; import java.util.function.Supplier; +/** + * A floating TextDisplay label above a TrialRooms block. + * + *

The display is a runtime-only entity: it is never saved with the chunk and is + * recreated by {@link #tick()} while a player is within view range. Every display is + * tagged with {@link #LABEL_KEY} so leftovers can be recognised by {@link LabelCleanup}. + */ public class Hologram { + /** PDC marker on every label display spawned by TrialRooms. */ + public static final NamespacedKey LABEL_KEY = new NamespacedKey("trialrooms", "label"); + + /** Every live hologram, so the cleanup knows label positions and current displays. */ + private static final Set REGISTRY = Collections.newSetFromMap(new WeakHashMap<>()); + private final Supplier locationSupplier; // where to place it private final Supplier nameSupplier; // what to display @@ -23,41 +44,25 @@ public class Hologram { public Hologram(Supplier locationSupplier, Supplier nameSupplier) { this.locationSupplier = locationSupplier; this.nameSupplier = nameSupplier; + REGISTRY.add(this); } public void setYOffset(double yOffset) { this.yOffset = yOffset; } + /** Shows or updates the label now if a player can see it; otherwise {@link #tick()} shows it later. */ public void spawn(JavaPlugin plugin) { - runSync(plugin, () -> { - TextDisplay td = getOrCreateDisplay(); - if (td == null) return; - Location pos = position(); - if (pos != null) td.teleport(pos); - applyText(td); - }); + runSync(plugin, this::update); } + /** Updates text and position of the label, creating it only if a player can see it. */ public void refresh(JavaPlugin plugin) { - runSync(plugin, () -> { - TextDisplay td = findDisplay(); - if (td == null || td.isDead()) { - td = getOrCreateDisplay(); - if (td == null) return; - } - Location pos = position(); - if (pos != null) td.teleport(pos); - applyText(td); - }); + runSync(plugin, this::update); } public void remove(JavaPlugin plugin) { - runSync(plugin, () -> { - TextDisplay td = findDisplay(); - if (td != null && !td.isDead()) td.remove(); - entityId = null; - }); + runSync(plugin, this::removeCurrent); } /** Distance-based lifecycle (spawn near players, despawn when nobody nearby). */ @@ -66,48 +71,86 @@ public void tick() { if (pos == null) return; TextDisplay td = findDisplay(); - - boolean anyNearby = false; - for (var p : Bukkit.getOnlinePlayers()) { - if (p.getWorld() == pos.getWorld() && p.getLocation().distanceSquared(pos) <= VIEW_RADIUS_SQ) { - anyNearby = true; break; - } - } - - if (anyNearby) { - if (td == null || td.isDead()) { - spawn(plugin()); + if (anyPlayerNear(pos) && isChunkLoaded(pos)) { + if (td == null) { + create(pos); } else { // keep text up to date in case suppliers change frequently applyText(td); } } else if (td != null) { - remove(plugin()); + removeCurrent(); } } - // ===== Internals ===== + // ===== Registry (used by LabelCleanup) ===== + + /** Current label positions of all live holograms (null positions skipped). */ + static List labelPositions() { + List out = new ArrayList<>(); + for (Hologram h : new ArrayList<>(REGISTRY)) { + Location pos; + try { + pos = h.position(); + } catch (IllegalArgumentException worldUnloaded) { + continue; // Location#getWorld throws once its world was unloaded + } + if (pos != null) out.add(pos); + } + return out; + } - private TextDisplay getOrCreateDisplay() { - Location base = locationSupplier.get(); - if (base == null) return null; - World world = base.getWorld(); - if (world == null) return null; + /** UUIDs of the displays the live holograms currently own. */ + static Set ownedDisplayIds() { + Set out = new HashSet<>(); + for (Hologram h : new ArrayList<>(REGISTRY)) { + if (h.entityId != null) out.add(h.entityId); + } + return out; + } - TextDisplay existing = findDisplay(); - if (existing != null && !existing.isDead()) return existing; + // ===== Internals ===== + private void update() { Location pos = position(); + if (pos == null) return; + if (!isChunkLoaded(pos)) { + removeCurrent(); + return; + } + + TextDisplay td = findDisplay(); + if (td == null) { + if (!anyPlayerNear(pos)) return; + create(pos); + return; + } + td.teleport(pos); + applyText(td); + } + + private void removeCurrent() { + TextDisplay td = findDisplay(); + if (td != null) td.remove(); + entityId = null; + } + + private TextDisplay create(Location pos) { + World world = pos.getWorld(); + // A display we lost track of (for example after a plugin reload) must not stay next to the new one. + LabelCleanup.removeLeftovers(world, pos.getBlockX() >> 4, pos.getBlockZ() >> 4, + world.getNearbyEntitiesByType(TextDisplay.class, pos, LabelCleanup.SEARCH_RADIUS)); + TextDisplay spawned = world.spawn(pos, TextDisplay.class, td -> { td.setInvulnerable(true); - td.setPersistent(true); + // Runtime-only: tick() recreates it, so it must never be saved with the chunk. + td.setPersistent(false); td.setGravity(false); + td.getPersistentDataContainer().set(LABEL_KEY, PersistentDataType.BYTE, (byte) 1); // Nice defaults (safe across recent Paper/Bukkit versions): try { td.setBillboard(org.bukkit.entity.Display.Billboard.CENTER); } catch (Throwable ignored) {} try { td.setShadowed(true); } catch (Throwable ignored) {} try { td.setSeeThrough(false); } catch (Throwable ignored) {} - // optional: remove the dark background if your server build supports it - // try { td.setBackgroundColor(org.joml.Vector4f...); } catch (Throwable ignored) {} applyText(td); }); entityId = spawned.getUniqueId(); @@ -117,15 +160,28 @@ private TextDisplay getOrCreateDisplay() { private TextDisplay findDisplay() { if (entityId == null) return null; Entity e = Bukkit.getEntity(entityId); - return (e instanceof TextDisplay td) ? td : null; + return (e instanceof TextDisplay td && !td.isDead()) ? td : null; } private Location position() { Location base = locationSupplier.get(); - if (base == null) return null; + if (base == null || base.getWorld() == null) return null; return base.clone().add(0.5, yOffset, 0.5); } + private static boolean isChunkLoaded(Location pos) { + return pos.getWorld().isChunkLoaded(pos.getBlockX() >> 4, pos.getBlockZ() >> 4); + } + + private static boolean anyPlayerNear(Location pos) { + for (var p : Bukkit.getOnlinePlayers()) { + if (p.getWorld() == pos.getWorld() && p.getLocation().distanceSquared(pos) <= VIEW_RADIUS_SQ) { + return true; + } + } + return false; + } + private void applyText(TextDisplay td) { String raw = nameSupplier.get(); if (raw == null) raw = ""; @@ -136,9 +192,4 @@ private void runSync(JavaPlugin plugin, Runnable r) { if (Bukkit.isPrimaryThread()) r.run(); else Bukkit.getScheduler().runTask(plugin, r); } - - private JavaPlugin plugin() { - // Replace with your plugin singleton if you like; this avoids passing it around for tick() - return net.tfminecraft.trialrooms.TrialRooms.getInstance(); - } } diff --git a/src/main/java/net/tfminecraft/trialrooms/environment/spawner/LabelCleanup.java b/src/main/java/net/tfminecraft/trialrooms/environment/spawner/LabelCleanup.java new file mode 100644 index 0000000..1add6b9 --- /dev/null +++ b/src/main/java/net/tfminecraft/trialrooms/environment/spawner/LabelCleanup.java @@ -0,0 +1,132 @@ +package net.tfminecraft.trialrooms.environment.spawner; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; +import java.util.UUID; + +import org.bukkit.Chunk; +import org.bukkit.Location; +import org.bukkit.World; +import org.bukkit.entity.Entity; +import org.bukkit.entity.TextDisplay; +import org.bukkit.event.EventHandler; +import org.bukkit.event.Listener; +import org.bukkit.event.world.EntitiesLoadEvent; +import org.bukkit.persistence.PersistentDataContainer; +import org.bukkit.persistence.PersistentDataType; + +import net.tfminecraft.trialrooms.TrialRooms; + +/** + * Removes TrialRooms label displays that are not the current one of their label. + * + *

Older versions saved every label display with the chunk and spawned a new one after + * each restart or chunk reload, so thousands of identical copies piled up. A display is + * only removed when all of these hold: + *

+ */ +public final class LabelCleanup implements Listener { + + private record ChunkKey(World world, int x, int z) {} + + /** Largest coordinate difference still counted as "the label position". */ + static final double POSITION_EPSILON = 1.0e-3; + /** Radius of the entity lookup around a label before it spawns its display. */ + static final double SEARCH_RADIUS = 0.25; + + @EventHandler + public void onEntitiesLoad(EntitiesLoadEvent e) { + Chunk chunk = e.getChunk(); + removeLeftovers(chunk.getWorld(), chunk.getX(), chunk.getZ(), e.getEntities()); + } + + /** + * Cleans the chunks of all live labels whose entities are already loaded. + * Chunks that load later are handled by {@link #onEntitiesLoad(EntitiesLoadEvent)}. + * + * @return the number of displays removed + */ + public static int sweepLoadedChunks() { + Set chunks = new LinkedHashSet<>(); + for (Location pos : Hologram.labelPositions()) { + chunks.add(new ChunkKey(pos.getWorld(), pos.getBlockX() >> 4, pos.getBlockZ() >> 4)); + } + int removed = 0; + for (ChunkKey key : chunks) { + if (!key.world().isChunkLoaded(key.x(), key.z())) continue; + Chunk chunk = key.world().getChunkAt(key.x(), key.z()); + if (!chunk.isEntitiesLoaded()) continue; + removed += removeLeftovers(key.world(), key.x(), key.z(), List.of(chunk.getEntities())); + } + return removed; + } + + /** + * Removes leftover label displays among {@code entities} for the labels in chunk + * ({@code cx}, {@code cz}) of {@code world}, logging one line when any are removed. + * + * @return the number of displays removed + */ + static int removeLeftovers(World world, int cx, int cz, Collection entities) { + List displays = new ArrayList<>(); + for (Entity entity : entities) { + if (entity instanceof TextDisplay td) displays.add(td); + } + if (displays.isEmpty()) return 0; + + List labels = new ArrayList<>(); + for (Location pos : Hologram.labelPositions()) { + if (world.equals(pos.getWorld()) && (pos.getBlockX() >> 4) == cx && (pos.getBlockZ() >> 4) == cz) { + labels.add(pos); + } + } + if (labels.isEmpty()) return 0; + + Set owned = Hologram.ownedDisplayIds(); + int removed = 0; + for (TextDisplay td : displays) { + if (owned.contains(td.getUniqueId())) continue; + if (!isLeftoverLabel(td, labels)) continue; + td.remove(); + removed++; + } + if (removed > 0) { + TrialRooms.getInstance().getLogger().info("Removed " + removed + " leftover label displays at " + + world.getName() + " chunk " + cx + "," + cz + " (blocks " + (cx << 4) + "," + (cz << 4) + ")"); + } + return removed; + } + + /** Whether {@code td} is a TrialRooms label display sitting on one of {@code labels}. */ + static boolean isLeftoverLabel(TextDisplay td, List labels) { + if (td.isDead() || td.getVehicle() != null) return false; + PersistentDataContainer pdc = td.getPersistentDataContainer(); + if (!pdc.has(Hologram.LABEL_KEY, PersistentDataType.BYTE) && !isLegacyLabel(td, pdc)) return false; + Location at = td.getLocation(); + for (Location label : labels) { + if (samePosition(at, label)) return true; + } + return false; + } + + private static boolean isLegacyLabel(TextDisplay td, PersistentDataContainer pdc) { + return td.isPersistent() && pdc.isEmpty() && td.getScoreboardTags().isEmpty(); + } + + static boolean samePosition(Location a, Location b) { + return a.getWorld() != null && a.getWorld().equals(b.getWorld()) + && Math.abs(a.getX() - b.getX()) <= POSITION_EPSILON + && Math.abs(a.getY() - b.getY()) <= POSITION_EPSILON + && Math.abs(a.getZ() - b.getZ()) <= POSITION_EPSILON; + } +} diff --git a/src/test/java/net/tfminecraft/trialrooms/LifecycleTest.java b/src/test/java/net/tfminecraft/trialrooms/LifecycleTest.java index 2c25bca..5c37b13 100644 --- a/src/test/java/net/tfminecraft/trialrooms/LifecycleTest.java +++ b/src/test/java/net/tfminecraft/trialrooms/LifecycleTest.java @@ -4,6 +4,7 @@ import static org.mockito.Mockito.*; import java.util.*; +import net.tfminecraft.trialrooms.environment.spawner.LabelCleanup; import net.tfminecraft.trialrooms.loader.*; import net.tfminecraft.trialrooms.manager.*; import net.tfminecraft.trialrooms.persist.Database; @@ -19,13 +20,16 @@ void pluginInitializesHandlersLoadersPersistenceAndShutdown() { var tl = mockConstruction(TableLoader.class); var db = mockStatic(Database.class); var chests = mockStatic(ChestManager.class); - var entrances = mockStatic(EntranceManager.class)) { + var entrances = mockStatic(EntranceManager.class); + var cleanup = mockStatic(LabelCleanup.class)) { var cm = mock(ChestManager.class); var em = mock(EntranceManager.class); chests.when(ChestManager::get).thenReturn(cm); entrances.when(EntranceManager::get).thenReturn(em); singleton.when(TrialRooms::getInstance).thenCallRealMethod(); var loaded = MockBukkit.load(TrialRooms.class); + // Leftover label displays in chunks that loaded before the plugin are swept once. + cleanup.verify(LabelCleanup::sweepLoadedChunks); assertSame(loaded, TrialRooms.getInstance()); assertSame(spawners.constructed().getFirst(), loaded.getSpawnerManager()); verify(cm).hydrateFrom(List.of()); diff --git a/src/test/java/net/tfminecraft/trialrooms/environment/spawner/HologramTest.java b/src/test/java/net/tfminecraft/trialrooms/environment/spawner/HologramTest.java index 4ff1034..cd145f0 100644 --- a/src/test/java/net/tfminecraft/trialrooms/environment/spawner/HologramTest.java +++ b/src/test/java/net/tfminecraft/trialrooms/environment/spawner/HologramTest.java @@ -9,128 +9,225 @@ import net.tfminecraft.trialrooms.TrialTestSupport; import org.bukkit.*; import org.bukkit.entity.*; +import org.bukkit.persistence.PersistentDataContainer; +import org.bukkit.persistence.PersistentDataType; import org.junit.jupiter.api.*; +import org.mockito.MockedStatic; class HologramTest extends TrialTestSupport { - @Test - void displayLifecycleUpdatesTextAndRemovesOutsideViewRange() { - World world = mock(World.class); - var loc = new Location(world, 1, 64, 3); - var current = new AtomicReference(loc); - var text = new AtomicReference("hello"); - var display = mock(TextDisplay.class); - var id = UUID.randomUUID(); + World world; + TextDisplay display; + PersistentDataContainer pdc; + UUID id; + Player player; + MockedStatic bukkit; + + @BeforeEach + void setUpWorld() { + world = mock(World.class); + when(world.isChunkLoaded(anyInt(), anyInt())).thenReturn(true); + display = mock(TextDisplay.class); + pdc = mock(PersistentDataContainer.class); + id = UUID.randomUUID(); when(display.getUniqueId()).thenReturn(id); + when(display.getPersistentDataContainer()).thenReturn(pdc); when(world.spawn(any(Location.class), eq(TextDisplay.class), any(Consumer.class))) .thenAnswer( a -> { ((Consumer) a.getArgument(2)).accept(display); return display; }); - try (var b = mockStatic(Bukkit.class, CALLS_REAL_METHODS)) { - b.when(() -> Bukkit.getEntity(id)).thenReturn(display); - var h = new Hologram(current::get, text::get); - h.setYOffset(2); - h.spawn(plugin); - verify(display).setInvulnerable(true); - verify(display).setPersistent(true); - verify(display).setGravity(false); - verify(display, atLeastOnce()).setText("hello"); - verify(display).teleport(loc.clone().add(.5, 2, .5)); - h.spawn(plugin); - verify(world, times(1)) - .spawn(any(Location.class), eq(TextDisplay.class), any(Consumer.class)); - text.set(null); - h.refresh(plugin); - verify(display).setText(""); - var p = mock(Player.class); - when(p.getWorld()).thenReturn(world); - when(p.getLocation()).thenReturn(loc); - b.when(Bukkit::getOnlinePlayers).thenReturn(List.of(p)); - h.tick(); - when(display.isDead()).thenReturn(true); - h.tick(); - h.refresh(plugin); - when(display.isDead()).thenReturn(false); - when(p.getLocation()).thenReturn(loc.clone().add(200, 0, 0)); - h.tick(); - verify(display).remove(); - h.tick(); - when(p.getWorld()).thenReturn(mock(World.class)); - h.tick(); - current.set(null); - h.tick(); - h.spawn(plugin); - h.refresh(plugin); - h.remove(plugin); - current.set(new Location(null, 0, 0, 0)); - h.spawn(plugin); - h.refresh(plugin); - } + player = mock(Player.class); + when(player.getWorld()).thenReturn(world); + bukkit = mockStatic(Bukkit.class, CALLS_REAL_METHODS); + bukkit.when(() -> Bukkit.getEntity(id)).thenReturn(display); + bukkit.when(Bukkit::getOnlinePlayers).thenReturn(List.of(player)); + } + + @AfterEach + void closeBukkit() { + bukkit.close(); + // Holograms stay registered until collected; make their world look unloaded to later tests. + reset(world); + } + + private void spawns(int times) { + verify(world, times(times)) + .spawn(any(Location.class), eq(TextDisplay.class), any(Consumer.class)); } @Test - void absentDisplayAndCompatibilityFailuresAreHandled() { - World world = mock(World.class); + void spawnCreatesOneTaggedRuntimeOnlyDisplayAndUpdatesIt() { + var loc = new Location(world, 1, 64, 3); + var text = new AtomicReference("hello"); + when(player.getLocation()).thenReturn(loc); + var h = new Hologram(() -> loc, text::get); + h.setYOffset(2); + var pos = loc.clone().add(.5, 2, .5); + + h.spawn(plugin); + spawns(1); + verify(world).spawn(eq(pos), eq(TextDisplay.class), any(Consumer.class)); + verify(display).setInvulnerable(true); + verify(display).setPersistent(false); + verify(display, never()).setPersistent(true); + verify(display).setGravity(false); + verify(pdc).set(Hologram.LABEL_KEY, PersistentDataType.BYTE, (byte) 1); + verify(display).setBillboard(Display.Billboard.CENTER); + verify(display).setText("hello"); + // Before spawning it looks for a lost display at the exact label position. + verify(world).getNearbyEntitiesByType(TextDisplay.class, pos, LabelCleanup.SEARCH_RADIUS); + + // A second spawn, refresh and tick reuse the display. + h.spawn(plugin); + text.set(null); + h.refresh(plugin); + h.tick(); + spawns(1); + verify(display, times(2)).teleport(pos); + verify(display, times(2)).setText(""); + assertTrue(Hologram.ownedDisplayIds().contains(id)); + assertTrue(Hologram.labelPositions().contains(pos)); + + h.remove(plugin); + verify(display).remove(); + assertFalse(Hologram.ownedDisplayIds().contains(id)); + } + + @Test + void displayIsOnlyCreatedWhileAPlayerIsInViewRangeAndTheChunkIsLoaded() { var loc = new Location(world, 0, 64, 0); - var display = mock(TextDisplay.class); - var id = UUID.randomUUID(); - when(display.getUniqueId()).thenReturn(id); + var h = new Hologram(() -> loc, () -> "text"); + + // Nobody in range: spawn/refresh/tick never create (and tick has nothing to remove). + when(player.getLocation()).thenReturn(loc.clone().add(200, 0, 0)); + h.spawn(plugin); + h.refresh(plugin); + h.tick(); + when(player.getWorld()).thenReturn(mock(World.class)); + when(player.getLocation()).thenReturn(loc); + h.tick(); + spawns(0); + + // Player near but the label's chunk is not loaded. + when(player.getWorld()).thenReturn(world); + when(world.isChunkLoaded(0, 0)).thenReturn(false); + h.spawn(plugin); + h.tick(); + spawns(0); + + // Chunk loaded: tick creates it, then updates it while the player stays. + when(world.isChunkLoaded(0, 0)).thenReturn(true); + h.tick(); + spawns(1); + h.tick(); + spawns(1); + verify(display, atLeast(2)).setText("text"); + + // Player leaves: removed; next tick does nothing more. + when(player.getLocation()).thenReturn(loc.clone().add(200, 0, 0)); + h.tick(); + verify(display).remove(); + bukkit.when(() -> Bukkit.getEntity(id)).thenReturn(null); + h.tick(); + verify(display, times(1)).remove(); + + // Player back while an (unexpected) display exists but the chunk unloaded: removed. + bukkit.when(() -> Bukkit.getEntity(id)).thenReturn(display); + when(player.getLocation()).thenReturn(loc); + h.tick(); + spawns(2); + when(world.isChunkLoaded(0, 0)).thenReturn(false); + h.tick(); + verify(display, times(2)).remove(); + h.tick(); + h.refresh(plugin); + verify(display, times(2)).remove(); + } + + @Test + void lostOrDeadDisplaysAreReplacedAndUnloadedLabelsAreDropped() { + var loc = new Location(world, 0, 64, 0); + when(player.getLocation()).thenReturn(loc); + var h = new Hologram(() -> loc, () -> "text"); + h.tick(); + spawns(1); + // Display unloaded with its chunk (UUID no longer resolves): one replacement. + bukkit.when(() -> Bukkit.getEntity(id)).thenReturn(null); + h.refresh(plugin); + spawns(2); + // The UUID resolves to something that is not a TextDisplay. + bukkit.when(() -> Bukkit.getEntity(id)).thenReturn(mock(Entity.class)); + h.tick(); + spawns(3); + // Dead display is replaced, never removed again. + bukkit.when(() -> Bukkit.getEntity(id)).thenReturn(display); + when(display.isDead()).thenReturn(true); + h.tick(); + spawns(4); + h.remove(plugin); + verify(display, never()).remove(); + // Chunk unloaded: refresh drops the reference without spawning. + when(display.isDead()).thenReturn(false); + when(world.isChunkLoaded(0, 0)).thenReturn(false); + h.refresh(plugin); + spawns(4); + } + + @Test + void missingLocationsAndWorldsAreIgnored() { + var current = new AtomicReference(null); + var h = new Hologram(current::get, () -> "text"); + h.tick(); + h.spawn(plugin); + h.refresh(plugin); + h.remove(plugin); + current.set(new Location(null, 0, 0, 0)); + h.tick(); + h.spawn(plugin); + assertFalse(Hologram.labelPositions().contains(null)); + spawns(0); + var unloaded = new Hologram(() -> { + throw new IllegalArgumentException("World unloaded"); + }, () -> "text"); + assertDoesNotThrow(Hologram::labelPositions); + unloaded.setYOffset(1); + } + + @Test + void compatibilityFailuresAndOffThreadCallsAreHandled() { + var loc = new Location(world, 0, 64, 0); + when(player.getLocation()).thenReturn(loc); doThrow(new UnsupportedOperationException()).when(display).setBillboard(any()); doThrow(new UnsupportedOperationException()).when(display).setShadowed(true); doThrow(new UnsupportedOperationException()).when(display).setSeeThrough(false); - when(world.spawn(any(Location.class), eq(TextDisplay.class), any(Consumer.class))) - .thenAnswer( - a -> { - ((Consumer) a.getArgument(2)).accept(display); - return display; - }); - try (var b = mockStatic(Bukkit.class, CALLS_REAL_METHODS)) { - var h = new Hologram(() -> loc, () -> "text"); - h.refresh(plugin); - b.when(() -> Bukkit.getEntity(id)).thenReturn(mock(Entity.class)); - h.refresh(plugin); - b.when(() -> Bukkit.getEntity(id)).thenReturn(display); - when(display.isDead()).thenReturn(true); - h.remove(plugin); - verify(display, never()).remove(); - b.when(Bukkit::isPrimaryThread).thenReturn(false); - h.spawn(plugin); - b.when(Bukkit::isPrimaryThread).thenReturn(true); - server.getScheduler().performOneTick(); - verify(world, atLeast(3)) - .spawn(any(Location.class), eq(TextDisplay.class), any(Consumer.class)); - } + var h = new Hologram(() -> loc, () -> "text"); + bukkit.when(Bukkit::isPrimaryThread).thenReturn(false); + h.spawn(plugin); + spawns(0); + bukkit.when(Bukkit::isPrimaryThread).thenReturn(true); + server.getScheduler().performOneTick(); + spawns(1); + verify(display).setText("text"); } @Test - void disappearingLocationBetweenDisplayLookupAndTeleportIsSafe() { - World world = mock(World.class); + void spawningRemovesALostDisplayAtTheSamePosition() { var loc = new Location(world, 0, 64, 0); - var display = mock(TextDisplay.class); - var id = UUID.randomUUID(); - when(display.getUniqueId()).thenReturn(id); - when(world.spawn(any(Location.class), eq(TextDisplay.class), any(Consumer.class))) - .thenAnswer( - a -> { - ((Consumer) a.getArgument(2)).accept(display); - return display; - }); - var calls = new java.util.concurrent.atomic.AtomicInteger(); - try (var b = mockStatic(Bukkit.class, CALLS_REAL_METHODS)) { - var h = new Hologram(() -> calls.incrementAndGet() <= 2 ? loc : null, () -> "text"); - h.spawn(plugin); - verify(display, never()).teleport(any(Location.class)); - b.when(() -> Bukkit.getEntity(id)).thenReturn(display); - h.refresh(plugin); - verify(display, never()).teleport(any(Location.class)); - var p = mock(Player.class); - when(p.getWorld()).thenReturn(world); - when(p.getLocation()).thenReturn(loc); - b.when(Bukkit::getOnlinePlayers).thenReturn(List.of(p)); - new Hologram(() -> loc, () -> "new").tick(); - verify(world, times(2)) - .spawn(any(Location.class), eq(TextDisplay.class), any(Consumer.class)); - } + var pos = loc.clone().add(.5, 1, .5); + when(player.getLocation()).thenReturn(loc); + var lost = mock(TextDisplay.class); + var lostPdc = mock(PersistentDataContainer.class); + when(lost.getUniqueId()).thenReturn(UUID.randomUUID()); + when(lost.getPersistentDataContainer()).thenReturn(lostPdc); + when(lostPdc.has(Hologram.LABEL_KEY, PersistentDataType.BYTE)).thenReturn(true); + when(lost.getLocation()).thenReturn(pos.clone()); + doReturn(List.of(lost)) + .when(world) + .getNearbyEntitiesByType(TextDisplay.class, pos, LabelCleanup.SEARCH_RADIUS); + var h = new Hologram(() -> loc, () -> "text"); + h.tick(); + verify(lost).remove(); + spawns(1); } } diff --git a/src/test/java/net/tfminecraft/trialrooms/environment/spawner/LabelCleanupTest.java b/src/test/java/net/tfminecraft/trialrooms/environment/spawner/LabelCleanupTest.java new file mode 100644 index 0000000..9e56d49 --- /dev/null +++ b/src/test/java/net/tfminecraft/trialrooms/environment/spawner/LabelCleanupTest.java @@ -0,0 +1,191 @@ +package net.tfminecraft.trialrooms.environment.spawner; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.*; + +import java.util.*; +import java.util.function.Consumer; +import net.tfminecraft.trialrooms.TrialTestSupport; +import org.bukkit.*; +import org.bukkit.entity.*; +import org.bukkit.event.world.EntitiesLoadEvent; +import org.bukkit.persistence.PersistentDataContainer; +import org.bukkit.persistence.PersistentDataType; +import org.junit.jupiter.api.*; + +class LabelCleanupTest extends TrialTestSupport { + World world; + World otherWorld; + // Spawner block at (4126, 278, 2533): level label at y+1.0, status label at y+1.5. + Location block; + Location levelPos; + Location statusPos; + Hologram level; + Hologram status; + final int cx = 4126 >> 4; + final int cz = 2533 >> 4; + + @BeforeEach + void labels() { + world = mock(World.class); + otherWorld = mock(World.class); + when(world.getName()).thenReturn("world"); + block = new Location(world, 4126, 278, 2533); + levelPos = block.clone().add(.5, 1, .5); + statusPos = block.clone().add(.5, 1.5, .5); + level = new Hologram(() -> block, () -> "[LVL: 1]"); + status = new Hologram(() -> block, () -> "Cooldown 18m 10s"); + status.setYOffset(1.5); + } + + @AfterEach + void unloadWorld() { + // Holograms stay registered until collected; make their world look unloaded to later tests. + reset(world); + } + + /** A display loaded from disk the way older versions saved it. */ + private TextDisplay legacy(Location at) { + var td = mock(TextDisplay.class); + var pdc = mock(PersistentDataContainer.class); + when(td.getUniqueId()).thenReturn(UUID.randomUUID()); + when(td.getPersistentDataContainer()).thenReturn(pdc); + when(td.getLocation()).thenReturn(at.clone()); + when(td.isPersistent()).thenReturn(true); + when(pdc.isEmpty()).thenReturn(true); + when(td.getScoreboardTags()).thenReturn(Set.of()); + return td; + } + + private TextDisplay tagged(Location at) { + var td = legacy(at); + when(td.isPersistent()).thenReturn(false); + when(td.getPersistentDataContainer().isEmpty()).thenReturn(false); + when(td.getPersistentDataContainer().has(Hologram.LABEL_KEY, PersistentDataType.BYTE)) + .thenReturn(true); + return td; + } + + private Chunk chunk(int x, int z) { + var chunk = mock(Chunk.class); + when(chunk.getWorld()).thenReturn(world); + when(chunk.getX()).thenReturn(x); + when(chunk.getZ()).thenReturn(z); + return chunk; + } + + @Test + void entitiesLoadRemovesEveryStackedLegacyCopyAtTheLabelPositions() { + var copies = new ArrayList(); + for (int i = 0; i < 659; i++) { + copies.add(legacy(levelPos)); + copies.add(legacy(statusPos)); + } + copies.add(mock(Zombie.class)); + new LabelCleanup().onEntitiesLoad(new EntitiesLoadEvent(chunk(cx, cz), copies)); + for (var e : copies) if (e instanceof TextDisplay td) verify(td).remove(); + } + + @Test + void otherPluginsDisplaysAndTheCurrentLabelAreNeverRemoved() { + var player = mock(Player.class); + when(player.getWorld()).thenReturn(world); + when(player.getLocation()).thenReturn(block); + when(world.isChunkLoaded(anyInt(), anyInt())).thenReturn(true); + var current = tagged(levelPos); + var currentId = UUID.randomUUID(); + when(current.getUniqueId()).thenReturn(currentId); + when(world.spawn(any(Location.class), eq(TextDisplay.class), any(Consumer.class))) + .thenAnswer( + a -> { + ((Consumer) a.getArgument(2)).accept(current); + return current; + }); + try (var b = mockStatic(Bukkit.class, CALLS_REAL_METHODS)) { + b.when(Bukkit::getOnlinePlayers).thenReturn(List.of(player)); + b.when(() -> Bukkit.getEntity(currentId)).thenReturn(current); + level.tick(); + } + assertTrue(Hologram.ownedDisplayIds().contains(currentId)); + + var withOtherPdc = legacy(levelPos); + when(withOtherPdc.getPersistentDataContainer().isEmpty()).thenReturn(false); + var withTag = legacy(levelPos); + when(withTag.getScoreboardTags()).thenReturn(Set.of("speech")); + var runtimeOnly = legacy(levelPos); + when(runtimeOnly.isPersistent()).thenReturn(false); + var riding = legacy(levelPos); + when(riding.getVehicle()).thenReturn(mock(Player.class)); + var dead = legacy(levelPos); + when(dead.isDead()).thenReturn(true); + var offX = legacy(levelPos.clone().add(.01, 0, 0)); + var offY = legacy(levelPos.clone().add(0, .01, 0)); + var offZ = legacy(levelPos.clone().add(0, 0, .01)); + var between = legacy(levelPos.clone().add(0, .25, 0)); + var otherWorldCopy = legacy(new Location(otherWorld, 4126.5, 279, 2533.5)); + var noWorld = legacy(new Location(null, 4126.5, 279, 2533.5)); + var keep = + List.of( + current, withOtherPdc, withTag, runtimeOnly, riding, dead, offX, offY, offZ, between, + otherWorldCopy, noWorld); + + assertEquals(0, LabelCleanup.removeLeftovers(world, cx, cz, keep)); + for (var e : keep) verify(e, never()).remove(); + + // Tiny floating point noise still counts as the label position. + var noisy = legacy(levelPos.clone().add(1e-6, -1e-6, 1e-6)); + var lostTagged = tagged(statusPos); + assertEquals(2, LabelCleanup.removeLeftovers(world, cx, cz, List.of(current, noisy, lostTagged))); + verify(noisy).remove(); + verify(lostTagged).remove(); + verify(current, never()).remove(); + } + + @Test + void chunksWithoutLabelsOrDisplaysAreSkippedCheaply() { + var copy = legacy(levelPos); + assertEquals(0, LabelCleanup.removeLeftovers(world, cx, cz, List.of(mock(Zombie.class)))); + assertEquals(0, LabelCleanup.removeLeftovers(world, cx, cz, List.of())); + // Same entity list, but the event's chunk/world holds no TrialRooms label. + assertEquals(0, LabelCleanup.removeLeftovers(world, cx + 1, cz, List.of(copy))); + assertEquals(0, LabelCleanup.removeLeftovers(world, cx, cz + 1, List.of(copy))); + assertEquals(0, LabelCleanup.removeLeftovers(otherWorld, cx, cz, List.of(copy))); + verify(copy, never()).remove(); + verify(copy, never()).getPersistentDataContainer(); + } + + @Test + void enableSweepCleansOnlyLoadedLabelChunksWithLoadedEntities() { + var farBlock = new Location(world, 100, 64, 100); + var far = new Hologram(() -> farBlock, () -> "far"); + var waitingBlock = new Location(world, 200, 64, 200); + var waiting = new Hologram(() -> waitingBlock, () -> "waiting"); + var loaded = chunk(cx, cz); + var stillLoading = chunk(200 >> 4, 200 >> 4); + var copy = legacy(levelPos); + var copy2 = legacy(statusPos); + when(loaded.isEntitiesLoaded()).thenReturn(true); + when(loaded.getEntities()).thenReturn(new Entity[] {copy, copy2}); + when(world.isChunkLoaded(cx, cz)).thenReturn(true); + when(world.isChunkLoaded(200 >> 4, 200 >> 4)).thenReturn(true); + when(world.getChunkAt(cx, cz)).thenReturn(loaded); + when(world.getChunkAt(200 >> 4, 200 >> 4)).thenReturn(stillLoading); + + assertEquals(2, LabelCleanup.sweepLoadedChunks()); + verify(copy).remove(); + verify(copy2).remove(); + // Both labels share one chunk: it is read once. + verify(loaded, times(1)).getEntities(); + verify(stillLoading, never()).getEntities(); + verify(world, never()).getChunkAt(100 >> 4, 100 >> 4); + assertNotNull(far); + assertNotNull(waiting); + } + + @Test + void samePositionNeedsTheSameWorld() { + assertTrue(LabelCleanup.samePosition(levelPos, levelPos.clone())); + assertFalse(LabelCleanup.samePosition(levelPos, new Location(otherWorld, 4126.5, 279, 2533.5))); + assertFalse(LabelCleanup.samePosition(new Location(null, 4126.5, 279, 2533.5), levelPos)); + } +} From a7a15d10a06e37977d1b3475954def1236ea29dc Mon Sep 17 00:00:00 2001 From: XxFran10xX <318299142+XxFran10xX@users.noreply.github.com> Date: Fri, 30 Oct 2026 19:02:51 +0100 Subject: [PATCH 2/3] fix: address review of the label leak cleanup - Deregister labels whose spawner, chest or entrance is deleted (Hologram.dispose), so their old spot no longer counts as a live label. - tick() teleports an existing display when its label moved. - position() treats a label in an unloaded world as having no position, so one bad label no longer breaks a whole tick loop or cleanup pass. - The startup sweep reads the labels once and groups them by chunk. - Untagged legacy displays are only removed when they look exactly like old TrialRooms labels (centre billboard, shadowed, not see-through, no default background) and show a text that label produces: the spawner level/status forms, the entrance "Right-Click with a ... to enter", or the current text for chest and exit labels. Co-Authored-By: Claude Opus 5.5 --- .../environment/chest/LootChest.java | 7 ++ .../environment/entrance/Entrance.java | 10 +- .../environment/spawner/ActiveSpawner.java | 9 +- .../environment/spawner/Hologram.java | 60 +++++++-- .../environment/spawner/LabelCleanup.java | 83 ++++++++---- .../environment/spawner/LegacyLabelTexts.java | 31 +++++ .../trialrooms/manager/ChestManager.java | 2 +- .../trialrooms/manager/EntranceManager.java | 4 +- .../trialrooms/EntranceManagerTest.java | 1 + .../trialrooms/EnvironmentTest.java | 8 ++ .../environment/spawner/HologramTest.java | 68 ++++++++-- .../environment/spawner/LabelCleanupTest.java | 119 ++++++++++++++---- 12 files changed, 328 insertions(+), 74 deletions(-) create mode 100644 src/main/java/net/tfminecraft/trialrooms/environment/spawner/LegacyLabelTexts.java diff --git a/src/main/java/net/tfminecraft/trialrooms/environment/chest/LootChest.java b/src/main/java/net/tfminecraft/trialrooms/environment/chest/LootChest.java index 5925565..4ab2e0a 100644 --- a/src/main/java/net/tfminecraft/trialrooms/environment/chest/LootChest.java +++ b/src/main/java/net/tfminecraft/trialrooms/environment/chest/LootChest.java @@ -148,6 +148,13 @@ public void removeHologram() { } catch (Exception ignored) {} } + /** The chest was deleted: remove its label for good. */ + public void disposeHologram() { + try { + hologram.dispose(TrialRooms.getInstance()); + } catch (Exception ignored) {} + } + private Location normalize(Location in) { if (in == null) return null; return new Location(in.getWorld(), in.getBlockX(), in.getBlockY(), in.getBlockZ()); diff --git a/src/main/java/net/tfminecraft/trialrooms/environment/entrance/Entrance.java b/src/main/java/net/tfminecraft/trialrooms/environment/entrance/Entrance.java index e5adacc..0e5d169 100644 --- a/src/main/java/net/tfminecraft/trialrooms/environment/entrance/Entrance.java +++ b/src/main/java/net/tfminecraft/trialrooms/environment/entrance/Entrance.java @@ -3,6 +3,7 @@ import net.tfminecraft.tlibs.TLibs; import net.tfminecraft.trialrooms.TrialRooms; import net.tfminecraft.trialrooms.environment.spawner.Hologram; +import net.tfminecraft.trialrooms.environment.spawner.LegacyLabelTexts; import net.tfminecraft.trialrooms.persist.Database; import org.bukkit.ChatColor; @@ -39,7 +40,8 @@ public Entrance(Location entranceLodestone) { this.entranceHolo = new Hologram( () -> this.entranceLoc, - this::buildEntranceText + this::buildEntranceText, + LegacyLabelTexts::entrance ); this.entranceHolo.setYOffset(1.55); @@ -89,6 +91,12 @@ public void removeHolograms(JavaPlugin plugin) { try { exitHolo.remove(plugin); } catch (Exception ignored) {} } + /** The entrance was deleted: remove its labels for good. */ + public void disposeHolograms(JavaPlugin plugin) { + try { entranceHolo.dispose(plugin); } catch (Exception ignored) {} + try { exitHolo.dispose(plugin); } catch (Exception ignored) {} + } + public void refreshHolograms(JavaPlugin plugin) { // Cheap refresh (safe even if not spawned) try { entranceHolo.refresh(plugin); } catch (Exception ignored) {} diff --git a/src/main/java/net/tfminecraft/trialrooms/environment/spawner/ActiveSpawner.java b/src/main/java/net/tfminecraft/trialrooms/environment/spawner/ActiveSpawner.java index f30cf09..792acd1 100644 --- a/src/main/java/net/tfminecraft/trialrooms/environment/spawner/ActiveSpawner.java +++ b/src/main/java/net/tfminecraft/trialrooms/environment/spawner/ActiveSpawner.java @@ -108,11 +108,13 @@ public ActiveSpawner(Location loc, Spawner spawner) { this.levelHologram = new Hologram( () -> this.loc, - this::getLevelFormatted + this::getLevelFormatted, + LegacyLabelTexts::spawnerLevel ); this.statusHologram = new Hologram( () -> this.loc, - this::getStatusText + this::getStatusText, + LegacyLabelTexts::spawnerStatus ); this.statusHologram.setYOffset(1.5); @@ -190,7 +192,8 @@ public void destroy() { } killActiveEnemiesSilently(); // << add this destroyed = true; - removeHolograms(TrialRooms.getInstance()); + levelHologram.dispose(TrialRooms.getInstance()); + statusHologram.dispose(TrialRooms.getInstance()); } // ---------- Ticking ---------- diff --git a/src/main/java/net/tfminecraft/trialrooms/environment/spawner/Hologram.java b/src/main/java/net/tfminecraft/trialrooms/environment/spawner/Hologram.java index f58c820..16fc570 100644 --- a/src/main/java/net/tfminecraft/trialrooms/environment/spawner/Hologram.java +++ b/src/main/java/net/tfminecraft/trialrooms/environment/spawner/Hologram.java @@ -16,7 +16,9 @@ import java.util.Set; import java.util.UUID; import java.util.WeakHashMap; +import java.util.function.Predicate; import java.util.function.Supplier; +import java.util.regex.Pattern; /** * A floating TextDisplay label above a TrialRooms block. @@ -34,6 +36,8 @@ public class Hologram { private final Supplier locationSupplier; // where to place it private final Supplier nameSupplier; // what to display + // recognises this label's plain text on displays saved by older versions + private final Predicate legacyText; private UUID entityId; private double yOffset = 1.0; // height above block center @@ -41,9 +45,25 @@ public class Hologram { // view radius squared (occlusion is handled by the engine; we keep radius for perf) private static final double VIEW_RADIUS_SQ = 96 * 96; + private static final Pattern COLOR_CODES = Pattern.compile("(?i)§[0-9A-FK-ORX]"); + + /** A live label: where it sits and which plain texts an old saved copy of it may show. */ + record Label(Location position, Predicate legacyText) {} + + /** A label whose old saved copies show exactly the text it shows now. */ public Hologram(Supplier locationSupplier, Supplier nameSupplier) { + this(locationSupplier, nameSupplier, null); + } + + /** + * @param legacyText matches the plain (uncoloured) text an old saved copy of this label + * may show; null means "exactly the current text" + */ + public Hologram(Supplier locationSupplier, Supplier nameSupplier, + Predicate legacyText) { this.locationSupplier = locationSupplier; this.nameSupplier = nameSupplier; + this.legacyText = legacyText != null ? legacyText : this::showsCurrentText; REGISTRY.add(this); } @@ -65,6 +85,12 @@ public void remove(JavaPlugin plugin) { runSync(plugin, this::removeCurrent); } + /** Permanently retires the label: removes its display and stops LabelCleanup treating its spot as live. */ + public void dispose(JavaPlugin plugin) { + REGISTRY.remove(this); + remove(plugin); + } + /** Distance-based lifecycle (spawn near players, despawn when nobody nearby). */ public void tick() { Location pos = position(); @@ -75,7 +101,8 @@ public void tick() { if (td == null) { create(pos); } else { - // keep text up to date in case suppliers change frequently + // follow a moved label, and keep text up to date in case suppliers change frequently + if (!LabelCleanup.samePosition(td.getLocation(), pos)) td.teleport(pos); applyText(td); } } else if (td != null) { @@ -85,21 +112,21 @@ public void tick() { // ===== Registry (used by LabelCleanup) ===== - /** Current label positions of all live holograms (null positions skipped). */ - static List labelPositions() { - List out = new ArrayList<>(); + /** All live labels with a position (labels without one, or in an unloaded world, are skipped). */ + static List