Scope addon-created BlueMap marker sets to their own worlds

src/main/java/world/bentobox/bentobox/hooks/BlueMapHook.java

@@ -13,6 +13,7 @@
 import org.bukkit.event.EventPriority;
 import org.bukkit.event.Listener;
 import org.eclipse.jdt.annotation.NonNull;
+import org.eclipse.jdt.annotation.Nullable;
 
 import de.bluecolored.bluemap.api.BlueMapAPI;
 import de.bluecolored.bluemap.api.BlueMapMap;
@@ -45,6 +46,14 @@ public class BlueMapHook extends MapHook implements Listener {
      * One marker set per game mode; key is the friendly name of the game mode.
      */
     private final Map<String, MarkerSet> markerSets = new HashMap<>();
+    /**
+     * Worlds each addon-created marker set (see {@link #createMarkerSet(String, String)}) has
+     * markers in, so a set is only attached to the maps of those worlds rather than to every
+     * map on the server. Populated lazily as markers are added, because
+     * {@link MapHook#createMarkerSet(String, String)} carries no world. Game mode marker sets
+     * are not tracked here - they are attached directly in {@link #registerGameMode}.
+     */
+    private final Map<String, Set<World>> markerSetWorlds = new HashMap<>();
 
     public BlueMapHook() {
         super("BlueMap", Material.MAP);
@@ -87,11 +96,13 @@ private void populateAll() {
             gameModeNames.add(addon.getWorldSettings().getFriendlyName());
             registerGameMode(addon);
         });
-        // Re-attach any addon-created marker sets (created via createMarkerSet) to all maps,
-        // since a BlueMap reload drops them from the freshly built maps too
+        // Re-attach any addon-created marker sets (created via createMarkerSet), since a BlueMap
+        // reload drops them from the freshly built maps too. Only to the worlds they actually
+        // have markers in - attaching them to every map put unrelated sets on every world's map.
         markerSets.forEach((id, markerSet) -> {
             if (!gameModeNames.contains(id)) {
-                api.getMaps().forEach(map -> map.getMarkerSets().put(id, markerSet));
+                markerSetWorlds.getOrDefault(id, Set.of())
+                        .forEach(world -> addMarkerSetToWorld(world, id, markerSet));
             }
         });
     }
@@ -123,6 +134,9 @@ public void registerGameMode(@NonNull GameModeAddon addon) {
     }
 
     private void addMarkerSetToWorld(World world, String markerSetId, MarkerSet markerSet) {
+        if (api == null || world == null) {
+            return;
+        }
         api.getWorld(world).ifPresent(bmWorld -> {
 
             for (BlueMapMap map : bmWorld.getMaps()) {
@@ -132,6 +146,25 @@ private void addMarkerSetToWorld(World world, String markerSetId, MarkerSet mark
         });
     }
 
+    /**
+     * Records that an addon-created marker set has markers in the given world and attaches the
+     * set to that world's maps. Called as markers are added, since
+     * {@link MapHook#createMarkerSet(String, String)} has no world parameter to scope by.
+     * No-op for unknown marker set IDs or a null world.
+     * @param markerSetId the marker set ID
+     * @param world the world a marker was just added in
+     */
+    private void trackAndAttach(String markerSetId, World world) {
+        MarkerSet markerSet = markerSets.get(markerSetId);
+        if (markerSet == null || world == null) {
+            return;
+        }
+        // Track regardless of whether BlueMap is loaded, so populateAll() can attach it later
+        markerSetWorlds.computeIfAbsent(markerSetId, k -> new HashSet<>()).add(world);
+        // Attaching is an idempotent put, so this is safe to repeat
+        addMarkerSetToWorld(world, markerSetId, markerSet);
+    }
+
     private void setMarker(MarkerSet markerSet, Island island) {
         Settings settings = plugin.getSettings();
         String label = getIslandLabel(island);
@@ -229,9 +262,13 @@ private void remove(String islandUniqueId, GameModeAddon addon) {
 
     /**
      * Returns the BlueMapAPI instance for addons to create custom markers directly.
-     * @return the BlueMapAPI instance
+     * <p>
+     * This is {@code null} until BlueMap has finished loading, and again while BlueMap is
+     * reloading, so callers must null-check it. Prefer the {@link MapHook} methods, which
+     * handle the API being unavailable and re-attach marker sets after a BlueMap reload.
+     * @return the BlueMapAPI instance, or {@code null} if BlueMap is not currently loaded
      */
-    @NonNull
+    @Nullable
     public BlueMapAPI getBlueMapAPI() {
         return api;
     }
@@ -249,15 +286,22 @@ public MarkerSet getMarkerSet(@NonNull GameModeAddon addon) {
 
     @Override
     public void createMarkerSet(@NonNull String id, @NonNull String label) {
-        MarkerSet markerSet = markerSets.computeIfAbsent(id,
+        // The set is only created here. It is attached to a world's maps when the first marker
+        // for that world is added - MapHook#createMarkerSet has no world to scope by, and
+        // attaching to every map put e.g. a warps set on unrelated game modes' maps.
+        markerSets.computeIfAbsent(id,
                 k -> MarkerSet.builder().label(label).toggleable(true).defaultHidden(false).build());
-        api.getMaps().forEach(map -> map.getMarkerSets().put(id, markerSet));
     }
 
     @Override
     public void removeMarkerSet(@NonNull String id) {
         markerSets.remove(id);
-        api.getMaps().forEach(map -> map.getMarkerSets().remove(id));
+        Set<World> worlds = markerSetWorlds.remove(id);
+        if (api == null || worlds == null) {
+            return;
+        }
+        worlds.forEach(world -> api.getWorld(world)
+                .ifPresent(bmWorld -> bmWorld.getMaps().forEach(map -> map.getMarkerSets().remove(id))));
     }
 
     @Override
@@ -277,6 +321,7 @@ public void addPointMarker(@NonNull String markerSetId, @NonNull String markerId
             POIMarker marker = POIMarker.builder().label(label).listed(true).defaultIcon()
                     .position(location.getX(), location.getY(), location.getZ()).build();
             markerSet.put(markerId, marker);
+            trackAndAttach(markerSetId, location.getWorld());
         }
     }
 
@@ -299,6 +344,7 @@ public void addAreaMarker(@NonNull String markerSetId, @NonNull String markerId,
                     .lineColor(toBlueMapColor(lineColor)).fillColor(toBlueMapColor(fillColor)).lineWidth(lineWidth)
                     .build();
             markerSet.put(markerId, area);
+            trackAndAttach(markerSetId, world);
         }
     }
 
@@ -317,6 +363,7 @@ public void addPolygonMarker(@NonNull String markerSetId, @NonNull String marker
                     .lineColor(toBlueMapColor(lineColor)).fillColor(toBlueMapColor(fillColor)).lineWidth(lineWidth)
                     .build();
             markerSet.put(markerId, area);
+            trackAndAttach(markerSetId, world);
         }
     }
 

src/test/java/world/bentobox/bentobox/hooks/BlueMapHookTest.java

@@ -14,6 +14,7 @@
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import java.awt.Color;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
@@ -553,13 +554,117 @@ void testGetMarkerSetNotRegistered() {
     }
 
     @Test
-    void testCreateMarkerSet() {
-        when(blueMapAPI.getMaps()).thenReturn(List.of(blueMapMap));
+    void testCreateMarkerSetNotAttachedUntilMarkerAdded() {
         hook.hook();
         simulateBlueMapEnable();
         hook.createMarkerSet("warps.markers", "Warps");
-        // Should be attached to the BlueMap map
+        // Creating a set alone says nothing about which world it belongs to, so it must not be
+        // attached to any map yet
+        assertFalse(mapMarkerSets.containsKey("warps.markers"));
+    }
+
+    @Test
+    void testAddPointMarkerAttachesSetToItsWorld() {
+        hook.hook();
+        simulateBlueMapEnable();
+        hook.createMarkerSet("warps.markers", "Warps");
+        hook.addPointMarker("warps.markers", "warp1", "Warp One", locationIn(overWorld), "default");
+
         assertTrue(mapMarkerSets.containsKey("warps.markers"));
         assertEquals("Warps", mapMarkerSets.get("warps.markers").getLabel());
+        assertNotNull(mapMarkerSets.get("warps.markers").get("warp1"));
+    }
+
+    @Test
+    void testAddPointMarkerDoesNotAttachSetToUnrelatedWorld() {
+        // A second world with its own BlueMap map, unrelated to where the marker is added
+        World otherWorld = mock(World.class);
+        BlueMapWorld otherBMWorld = mock(BlueMapWorld.class);
+        BlueMapMap otherMap = mock(BlueMapMap.class);
+        Map<String, MarkerSet> otherMapMarkerSets = new HashMap<>();
+        when(blueMapAPI.getWorld(otherWorld)).thenReturn(Optional.of(otherBMWorld));
+        when(otherBMWorld.getMaps()).thenReturn(List.of(otherMap));
+        when(otherMap.getMarkerSets()).thenReturn(otherMapMarkerSets);
+        when(blueMapAPI.getMaps()).thenReturn(List.of(blueMapMap, otherMap));
+
+        hook.hook();
+        simulateBlueMapEnable();
+        hook.createMarkerSet("warps.markers", "Warps");
+        hook.addPointMarker("warps.markers", "warp1", "Warp One", locationIn(overWorld), "default");
+
+        assertTrue(mapMarkerSets.containsKey("warps.markers"));
+        // The unrelated world's map must not pick up the set
+        assertFalse(otherMapMarkerSets.containsKey("warps.markers"));
+    }
+
+    @Test
+    void testAddAreaMarkerAttachesSetToItsWorld() {
+        hook.hook();
+        simulateBlueMapEnable();
+        hook.createMarkerSet("regions", "Regions");
+        hook.addAreaMarker("regions", "r1", "Region One", overWorld, 0, 0, 10, 10, Color.RED, Color.BLUE, 2);
+
+        assertTrue(mapMarkerSets.containsKey("regions"));
+        assertNotNull(mapMarkerSets.get("regions").get("r1"));
+    }
+
+    @Test
+    void testAddPolygonMarkerAttachesSetToItsWorld() {
+        hook.hook();
+        simulateBlueMapEnable();
+        hook.createMarkerSet("regions", "Regions");
+        hook.addPolygonMarker("regions", "p1", "Poly One", overWorld, new double[] { 0, 10, 10 },
+                new double[] { 0, 0, 10 }, Color.RED, Color.BLUE, 2);
+
+        assertTrue(mapMarkerSets.containsKey("regions"));
+        assertNotNull(mapMarkerSets.get("regions").get("p1"));
+    }
+
+    @Test
+    void testRemoveMarkerSetDetachesFromItsWorld() {
+        hook.hook();
+        simulateBlueMapEnable();
+        hook.createMarkerSet("warps.markers", "Warps");
+        hook.addPointMarker("warps.markers", "warp1", "Warp One", locationIn(overWorld), "default");
+        assertTrue(mapMarkerSets.containsKey("warps.markers"));
+
+        hook.removeMarkerSet("warps.markers");
+        assertFalse(mapMarkerSets.containsKey("warps.markers"));
+    }
+
+    @Test
+    void testAddonMarkerSetReattachedToItsWorldAfterBlueMapReload() {
+        hook.hook();
+        simulateBlueMapEnable();
+        hook.createMarkerSet("warps.markers", "Warps");
+        hook.addPointMarker("warps.markers", "warp1", "Warp One", locationIn(overWorld), "default");
+
+        // A BlueMap reload discards the maps and their marker sets
+        simulateBlueMapDisable();
+        mapMarkerSets.clear();
+        simulateBlueMapEnable();
+
+        assertTrue(mapMarkerSets.containsKey("warps.markers"));
+        assertNotNull(mapMarkerSets.get("warps.markers").get("warp1"));
+    }
+
+    @Test
+    void testCreateMarkerSetBeforeBlueMapLoadsDoesNotThrow() {
+        hook.hook();
+        // BlueMap not loaded yet - api is null
+        hook.createMarkerSet("warps.markers", "Warps");
+        hook.addPointMarker("warps.markers", "warp1", "Warp One", locationIn(overWorld), "default");
+        hook.removeMarkerSet("warps.markers");
+        assertNull(hook.getBlueMapAPI());
+    }
+
+    /** A mocked Location in the given world; only world and coordinates are used by the hook. */
+    private Location locationIn(World world) {
+        Location loc = mock(Location.class);
+        when(loc.getWorld()).thenReturn(world);
+        when(loc.getX()).thenReturn(0.0);
+        when(loc.getY()).thenReturn(64.0);
+        when(loc.getZ()).thenReturn(0.0);
+        return loc;
     }
 }