diff --git a/DynmapCore/src/main/java/org/dynmap/MapManager.java b/DynmapCore/src/main/java/org/dynmap/MapManager.java index 76a57872e..4ef974363 100644 --- a/DynmapCore/src/main/java/org/dynmap/MapManager.java +++ b/DynmapCore/src/main/java/org/dynmap/MapManager.java @@ -890,11 +890,30 @@ public void run() { } private class CheckWorldTimes implements Runnable { - HashMap last_worldborder = new HashMap(); + private final HashMap last_worldborder = new HashMap(); + + private boolean sameBorder(Polygon border, Polygon previous) { + if (border == null || previous == null) { + return border == previous; + } + if (border.size() != previous.size()) { + return false; + } + for (int i = 0; i < border.size(); i++) { + Polygon.Point2D point = border.getVertex(i); + Polygon.Point2D oldPoint = previous.getVertex(i); + if (point.x != oldPoint.x || point.y != oldPoint.y) { + return false; + } + } + return true; + } + public void run() { Future f = core.getServer().callSyncMethod(new Callable() { public Integer call() throws Exception { long now_nsec = System.nanoTime(); + last_worldborder.keySet().retainAll(worlds); for(DynmapWorld w : worlds) { if(w.isLoaded()) { int new_servertime = (int)(w.getTime() % 24000); @@ -907,14 +926,24 @@ public Integer call() throws Exception { } // Check world border Polygon wb = w.getWorldBorder(); - Polygon oldwb = last_worldborder.get(w.getName()); - if (((wb == null) && (oldwb == null)) || - wb.equals(oldwb)) { // No change - } - else { + Polygon oldwb = last_worldborder.get(w); + if (!sameBorder(wb, oldwb)) { + // Keep a deep snapshot: platforms may reuse mutable polygons/vertices. + Polygon snapshot = null; + if (wb != null) { + snapshot = new Polygon(); + for (int i = 0; i < wb.size(); i++) { + Polygon.Point2D point = wb.getVertex(i); + snapshot.addVertex(point.x, point.y); + } + } core.listenerManager.processWorldEvent(EventType.WORLD_SPAWN_CHANGE, w); + last_worldborder.put(w, snapshot); } } + else { + last_worldborder.remove(w); + } /* Tick invalidated tiles processing */ for(MapTypeState mts : w.mapstate) { mts.tickMapTypeState(now_nsec); diff --git a/DynmapCore/src/main/java/org/dynmap/markers/impl/MarkerImpl.java b/DynmapCore/src/main/java/org/dynmap/markers/impl/MarkerImpl.java index fa325978f..2419e4870 100644 --- a/DynmapCore/src/main/java/org/dynmap/markers/impl/MarkerImpl.java +++ b/DynmapCore/src/main/java/org/dynmap/markers/impl/MarkerImpl.java @@ -226,7 +226,13 @@ public double getZ() { @Override public void setLocation(String worldid, double x, double y, double z) { if(markerset == null) return; + boolean sameWorld = this.world.equals(worldid); + if(sameWorld && this.x == x && this.y == y && this.z == z) return; + if(!sameWorld) { + MarkerAPIImpl.markerUpdated(this, MarkerUpdate.DELETED); + } this.world = worldid; + this.normalized_world = DynmapWorld.normalizeWorldName(worldid); this.x = x; this.y = y; this.z = z; diff --git a/DynmapCore/src/test/java/org/dynmap/WorldBorderUpdateTest.java b/DynmapCore/src/test/java/org/dynmap/WorldBorderUpdateTest.java new file mode 100644 index 000000000..dc7490734 --- /dev/null +++ b/DynmapCore/src/test/java/org/dynmap/WorldBorderUpdateTest.java @@ -0,0 +1,139 @@ +package org.dynmap; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.*; + +import java.lang.reflect.Constructor; +import java.lang.reflect.Field; +import java.util.ArrayList; +import java.util.concurrent.Callable; +import java.util.concurrent.CompletableFuture; + +import org.dynmap.common.DynmapListenerManager; +import org.dynmap.common.DynmapListenerManager.EventType; +import org.dynmap.common.DynmapServerInterface; +import org.dynmap.utils.Polygon; +import org.junit.Before; +import org.junit.Test; + +public class WorldBorderUpdateTest { + private MapManager manager; + private DynmapWorld world; + private DynmapListenerManager listeners; + private Runnable check; + + @Before + public void setUp() throws Exception { + manager = mock(MapManager.class); + manager.worlds = new ArrayList(); + DynmapCore core = mock(DynmapCore.class); + listeners = mock(DynmapListenerManager.class); + core.listenerManager = listeners; + DynmapServerInterface server = mock(DynmapServerInterface.class); + doCallRealMethod().when(core).setServer(server); + core.setServer(server); + when(server.callSyncMethod(any())).thenAnswer(invocation -> { + Callable task = invocation.getArgument(0); + // Let exceptions fail the test instead of being swallowed by the job's logger. + return CompletableFuture.completedFuture(task.call()); + }); + Field coreField = MapManager.class.getDeclaredField("core"); + coreField.setAccessible(true); + coreField.set(manager, core); + Constructor constructor = Class.forName("org.dynmap.MapManager$CheckWorldTimes") + .getDeclaredConstructor(MapManager.class); + constructor.setAccessible(true); + check = (Runnable) constructor.newInstance(manager); + world = newWorld(); + manager.worlds.add(world); + } + + private DynmapWorld newWorld() { + DynmapWorld result = mock(DynmapWorld.class); + result.mapstate = new ArrayList(); + when(result.getName()).thenReturn("world"); + when(result.isLoaded()).thenReturn(true); + return result; + } + + private Polygon border(double offset, double size) { + Polygon result = new Polygon(); + result.addVertex(offset, offset); + result.addVertex(offset + size, offset); + result.addVertex(offset + size, offset + size); + result.addVertex(offset, offset + size); + return result; + } + + @Test + public void equivalentNewPolygonsOnlyNotifyOnce() { + when(world.getWorldBorder()).thenAnswer(invocation -> border(0, 100)); + for (int i = 0; i < 120; i++) check.run(); + verify(listeners, times(1)).processWorldEvent(EventType.WORLD_SPAWN_CHANGE, world); + } + + @Test + public void absentBorderDoesNotNotify() { + check.run(); + check.run(); + verifyNoInteractions(listeners); + } + + @Test + public void additionRemovalResizeAndMoveEachNotifyOnce() { + when(world.getWorldBorder()).thenReturn(null, border(0, 100), border(0, 100), + border(0, 200), border(10, 200), null, null); + for (int i = 0; i < 7; i++) check.run(); + verify(listeners, times(4)).processWorldEvent(EventType.WORLD_SPAWN_CHANGE, world); + } + + @Test + public void mutablePolygonAndVerticesAreSnapshotted() { + Polygon polygon = border(0, 100); + when(world.getWorldBorder()).thenReturn(polygon); + check.run(); + polygon.getVertex(0).x = -10; + check.run(); + check.run(); + polygon.addVertex(0, -10); + check.run(); + verify(listeners, times(3)).processWorldEvent(EventType.WORLD_SPAWN_CHANGE, world); + } + + @Test + public void unloadedWorldIsCheckedAgainWhenReloaded() { + when(world.getWorldBorder()).thenReturn(border(0, 100)); + check.run(); + when(world.isLoaded()).thenReturn(false); + check.run(); + when(world.isLoaded()).thenReturn(true); + check.run(); + verify(listeners, times(2)).processWorldEvent(EventType.WORLD_SPAWN_CHANGE, world); + } + + @Test + public void removedWorldDoesNotKeepItsPreviousBorder() { + when(world.getWorldBorder()).thenReturn(border(0, 100)); + check.run(); + manager.worlds.clear(); + check.run(); + manager.worlds.add(world); + check.run(); + check.run(); + verify(listeners, times(2)).processWorldEvent(EventType.WORLD_SPAWN_CHANGE, world); + } + + @Test + public void replacementWorldWithSameNameIsCheckedAgain() { + when(world.getWorldBorder()).thenReturn(border(0, 100)); + check.run(); + manager.worlds.clear(); + DynmapWorld replacement = newWorld(); + when(replacement.getWorldBorder()).thenReturn(border(0, 100)); + manager.worlds.add(replacement); + check.run(); + check.run(); + verify(listeners).processWorldEvent(EventType.WORLD_SPAWN_CHANGE, world); + verify(listeners).processWorldEvent(EventType.WORLD_SPAWN_CHANGE, replacement); + } +} diff --git a/DynmapCore/src/test/java/org/dynmap/markers/impl/MarkerLocationUpdateTest.java b/DynmapCore/src/test/java/org/dynmap/markers/impl/MarkerLocationUpdateTest.java new file mode 100644 index 000000000..9071fad9a --- /dev/null +++ b/DynmapCore/src/test/java/org/dynmap/markers/impl/MarkerLocationUpdateTest.java @@ -0,0 +1,98 @@ +package org.dynmap.markers.impl; + +import static org.junit.Assert.*; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +import java.lang.reflect.Field; +import java.util.Map; + +import org.dynmap.Client; +import org.dynmap.MapManager; +import org.dynmap.markers.MarkerIcon.MarkerSize; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.InOrder; + +public class MarkerLocationUpdateTest { + private MarkerAPIImpl previousApi; + private MapManager previousManager; + private MarkerAPIImpl api; + private MapManager manager; + private MarkerImpl marker; + + @Before + public void setUp() { + previousApi = MarkerAPIImpl.api; + previousManager = MapManager.mapman; + api = new MarkerAPIImpl(); + MarkerAPIImpl.api = api; + manager = mock(MapManager.class); + MapManager.mapman = manager; + MarkerIconImpl icon = mock(MarkerIconImpl.class); + when(icon.getMarkerIconID()).thenReturn("default"); + when(icon.getMarkerIconSize()).thenReturn(MarkerSize.MARKER_16x16); + MarkerSetImpl set = mock(MarkerSetImpl.class); + when(set.getMarkerSetID()).thenReturn("markers"); + marker = new MarkerImpl("spawn", "Spawn", true, "old/world", 1, 64, 2, icon, true, set); + } + + @After + public void tearDown() { + MarkerAPIImpl.api = previousApi; + MapManager.mapman = previousManager; + } + + private Object field(String name) throws Exception { + Field field = MarkerAPIImpl.class.getDeclaredField(name); + field.setAccessible(true); + return field.get(api); + } + + @Test + public void identicalLocationDoesNotDirtyOrNotify() throws Exception { + for (int i = 0; i < 120; i++) marker.setLocation(new String("old/world"), 1, 64, 2); + verifyNoInteractions(manager); + assertTrue(((Map) field("dirty_worlds")).isEmpty()); + assertEquals(false, field("dirty_markers")); + } + + @Test + public void coordinateChangeNotifiesAndPersists() throws Exception { + marker.setLocation("old/world", 3, 65, 4); + verify(manager).pushUpdate(eq("old-world"), any(Client.Update.class)); + assertTrue(((Map) field("dirty_worlds")).containsKey("old-world")); + assertEquals(true, field("dirty_markers")); + assertEquals(3, marker.getX(), 0); + assertEquals(65, marker.getY(), 0); + assertEquals(4, marker.getZ(), 0); + } + + @Test + public void eachCoordinateCanIndependentlyTriggerAnUpdate() { + marker.setLocation("old/world", 3, 64, 2); + marker.setLocation("old/world", 3, 65, 2); + marker.setLocation("old/world", 3, 65, 4); + marker.setLocation("old/world", 3, 65, 4); + verify(manager, times(3)).pushUpdate(eq("old-world"), any(Client.Update.class)); + } + + @Test + public void worldMoveDeletesOldAndUpdatesNewWorld() throws Exception { + marker.setLocation("new/world", 1, 64, 2); + ArgumentCaptor oldUpdate = ArgumentCaptor.forClass(Client.Update.class); + ArgumentCaptor newUpdate = ArgumentCaptor.forClass(Client.Update.class); + InOrder order = inOrder(manager); + order.verify(manager).pushUpdate(eq("old-world"), oldUpdate.capture()); + order.verify(manager).pushUpdate(eq("new-world"), newUpdate.capture()); + assertEquals("markerdeleted", ((MarkerAPIImpl.MarkerUpdated) oldUpdate.getValue()).msg); + assertEquals("markerupdated", ((MarkerAPIImpl.MarkerUpdated) newUpdate.getValue()).msg); + assertEquals("new-world", marker.getNormalizedWorld()); + Map dirty = (Map) field("dirty_worlds"); + assertTrue(dirty.containsKey("old-world")); + assertTrue(dirty.containsKey("new-world")); + assertEquals(true, field("dirty_markers")); + } +} diff --git a/docs/r2-marker-validation.md b/docs/r2-marker-validation.md new file mode 100644 index 000000000..f2af64632 --- /dev/null +++ b/docs/r2-marker-validation.md @@ -0,0 +1,80 @@ +# R2 marker update regression validation + +Related: #1. Base revision: `93b454efb8802dc7406d6873434f2aeec5c636f4`. + +## What changes + +`CheckWorldTimes` compares border vertices and keeps a deep snapshot after a +successful notification. An equivalent newly allocated polygon does not notify +again. Removing a border notifies once without dereferencing null. Unloaded and +removed worlds lose their snapshots, and a replacement world is tracked separately. + +`MarkerImpl.setLocation` skips an identical world and coordinates. A real coordinate +change still updates the client and persistent markers. A world move first removes +the old client marker, then updates the normalized world and notifies the new world; +both worlds' marker files are invalidated. These changes are separate commits. + +Storage, rendering intervals, visible marker sets, and platform dependencies are +unchanged. This patch does not suppress spawn, border, or WorldGuard markers. + +## Automated checks + +Run with the repository's Gradle wrapper and a supported JDK: + +```text +./gradlew :DynmapCore:test +./gradlew :spigot:build +``` + +The normal settings also configure Fabric and Forge. If unrelated platform setup +blocks a focused core check, an untracked settings file can include just +`DynmapCore` and `DynmapCoreAPI`, with their `projectDir` pointing to the original +directories and an unchanged copy of the root `build.gradle` as its root build. +Do not modify module sources or dependencies to make this check pass. This is a +focused check, not proof of all-platform compatibility. + +The regression tests execute the actual border polling job and marker notification +path. Cases cover equivalent new polygons, absent borders, add/remove/resize/move, +mutable vertices, unload/reload, removal/re-addition, replacement worlds, unchanged +marker locations, individual coordinate changes, persistence, and old/new world +notifications. They restore the marker API and map manager singletons after use. + +Before the fix, five of the initial nine new cases failed. After the fix and two +additional lifecycle/coordinate cases, all 104 core tests passed on Java 21 with +Java 8 source/target compatibility. All-platform builds and live storage behavior +remain separate validation gates. + +## Server comparison + +1. Record the server/Paper/Java version, source revision, candidate version and + SHA-256. Compare the candidate's S3 implementation with the currently installed + artifact so a pre-existing storage customization is not accidentally lost. +2. Start with an isolated test server, fresh disposable world, local-only listeners, + and separate storage. Do not give a test instance the production R2 prefix. +3. Confirm startup, helper selection, bundled dependencies (including Jetty), tiles, + spawn/border markers, normal block updates and zoom. Exercise border addition, + removal, resize and movement, spawn movement and world unload/reload. Test + ordinary markers and WorldGuard in an environment that includes those plugins. +4. Measure a stable border and then real changes using identical intervals and + marker visibility before/after. Compare the marker JSON with only its top-level + `timestamp` removed. A changing timestamp/ETag alone is not a content change. +5. Measure successful `PutObject` counts per marker object at the storage writer or + provider. Origin `LastModified` advancing while normalized content is constant + establishes repeated writes, but sampled timestamps do not provide exact counts. + Cached browser GET responses cannot measure origin writes. +6. Keep dynamic `standalone/dynmap_*.json`, rendered tiles, other markers and reads + separate. This patch is not expected to eliminate legitimate dynamic updates. + +## Production gate and rollback + +A passing core test/build or merged PR is not a production rollout. Before a +production canary, retain the original JAR and configuration in a unique backup +directory, record checksums and ownership, and agree on the selected server and +activation window. Preserve the plugin updater's existing policy until a scoped +pin has been reviewed. Never delete existing map tiles or marker persistence. + +Stage only the candidate JAR for the agreed restart. Verify the running version, +startup logs, public map, marker sets, actual change propagation and storage writes +after activation. If a check fails, restore the original JAR/configuration and +activate it in the agreed window. Do not infer activation from a copied file or +from a successful build. Keep the issue open until the server comparison is complete.