Stop death bounce-back from hijacking the drops

src/main/java/world/bentobox/border/listeners/PlayerListener.java

@@ -1,7 +1,9 @@
 package world.bentobox.border.listeners;
 
+import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
@@ -23,6 +25,7 @@
 import org.bukkit.event.entity.EntityDamageEvent.DamageCause;
 import org.bukkit.event.entity.EntityDismountEvent;
 import org.bukkit.event.entity.EntityMountEvent;
+import org.bukkit.event.entity.ItemSpawnEvent;
 import org.bukkit.event.entity.PlayerDeathEvent;
 import org.bukkit.event.player.PlayerDropItemEvent;
 import org.bukkit.event.player.PlayerJoinEvent;
@@ -71,10 +74,18 @@
 public class PlayerListener implements Listener {
 
     private static final Vector XZ = new Vector(1, 0, 1);
+    /** How close to a death spot, in blocks, an item must spawn to count as a death drop. */
+    private static final double DEATH_DROP_RADIUS_SQUARED = 5 * 5D;
     private final Border addon;
     private final Set<UUID> inTeleport;
     private final BorderShower show;
     private final Map<Player, BukkitTask> mountedPlayers = new HashMap<>();
+    /** Death spots whose drops the server has yet to spawn. Entries last one tick. */
+    private final List<DeathDrops> pendingDeathDrops = new ArrayList<>();
+
+    /** A death whose drops will spawn this tick, and the island they must stay on. */
+    private record DeathDrops(Location location, Island island) {
+    }
 
     /**
      * Constructs a new PlayerListener.
@@ -500,21 +511,52 @@ && isOn(event.getPlayer())
     }
 
     /**
-     * Bounces items back to inside the barrier if dropped when a player dies
+     * Remembers where a player died so that the items the server is about to drop there can
+     * be bounced back inside the barrier.
+     * <p>
+     * The drops list must not be touched here: taking the items out of the event and dropping
+     * them by hand breaks every plugin that stores death drops (death chests, graves, keep
+     * inventory). Instead this runs at MONITOR, after those plugins have decided what happens
+     * to the items, and only notes the death spot. Whatever the server then actually drops is
+     * picked up in {@link #onDeathDropSpawn(ItemSpawnEvent)} and tracked from there.
      *
      * @param event event
      */
-    @EventHandler(priority = EventPriority.NORMAL, ignoreCancelled = true)
+    @EventHandler(priority = EventPriority.MONITOR)
     public void onPlayerDeath(PlayerDeathEvent event) {
-        if (addon.getSettings().isBounceBack()
-                && addon.inGameWorld(event.getPlayer().getWorld())
-                && isOn(event.getPlayer())) {
-            Location loc = event.getPlayer().getLocation();
-            addon.getIslands().getIslandAt(Objects.requireNonNull(loc)).ifPresent(is -> {
-                event.getDrops().forEach(item -> trackItem(event.getPlayer().getWorld().dropItemNaturally(loc, item), is));
-                event.getDrops().clear(); // We handled them
-            });
+        if (!addon.getSettings().isBounceBack()
+                || event.getDrops().isEmpty()
+                || !addon.inGameWorld(event.getPlayer().getWorld())
+                || !isOn(event.getPlayer())) {
+            return;
+        }
+        Location loc = event.getPlayer().getLocation();
+        addon.getIslands().getIslandAt(Objects.requireNonNull(loc)).ifPresent(is -> {
+            DeathDrops deathDrops = new DeathDrops(loc, is);
+            pendingDeathDrops.add(deathDrops);
+            // The server spawns the drops in this tick, straight after the event returns
+            Bukkit.getScheduler().runTask(addon.getPlugin(), () -> pendingDeathDrops.remove(deathDrops));
+        });
+    }
+
+    /**
+     * Tracks death drops as the server spawns them, so they cannot leave the island's
+     * protection zone. Items spawning this tick near a recorded death spot are the drops the
+     * server kept after every plugin had its say on the death event.
+     *
+     * @param event event
+     */
+    @EventHandler(priority = EventPriority.NORMAL, ignoreCancelled = true)
+    public void onDeathDropSpawn(ItemSpawnEvent event) {
+        if (pendingDeathDrops.isEmpty()) {
+            return;
         }
+        Location loc = event.getLocation();
+        pendingDeathDrops.stream()
+                .filter(dd -> Objects.equals(loc.getWorld(), dd.location().getWorld()))
+                .filter(dd -> loc.distanceSquared(dd.location()) <= DEATH_DROP_RADIUS_SQUARED)
+                .findFirst()
+                .ifPresent(dd -> trackItem(event.getEntity(), dd.island()));
     }
 
     /**

src/test/java/world/bentobox/border/listeners/PlayerListenerTest.java

@@ -1,5 +1,6 @@
 package world.bentobox.border.listeners;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -11,6 +12,7 @@
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
 import java.util.UUID;
@@ -19,8 +21,12 @@
 import org.bukkit.Bukkit;
 import org.bukkit.GameMode;
 import org.bukkit.Location;
+import org.bukkit.entity.Item;
 import org.bukkit.entity.Player;
 import org.bukkit.entity.Vehicle;
+import org.bukkit.event.entity.ItemSpawnEvent;
+import org.bukkit.event.entity.PlayerDeathEvent;
+import org.bukkit.inventory.ItemStack;
 import org.bukkit.event.player.PlayerJoinEvent;
 import org.bukkit.event.player.PlayerMoveEvent;
 import org.bukkit.event.player.PlayerQuitEvent;
@@ -74,6 +80,16 @@ class PlayerListenerTest extends CommonTestSetup {
     private Vehicle vehicle;
     @Mock
     private GameModeAddon gma;
+    @Mock
+    private PlayerDeathEvent deathEvent;
+    @Mock
+    private ItemSpawnEvent itemSpawnEvent;
+    @Mock
+    private Item item;
+    @Mock
+    private ItemStack itemStack;
+    @Mock
+    private Location farAway;
 
     private MockedStatic<User> mockedUser;
 
@@ -464,4 +480,84 @@ void testOnProtectionRangeChange() {
         verify(show).showBorder(player, island);
     }
 
+    /**
+     * Test method for {@link world.bentobox.border.listeners.PlayerListener#onPlayerDeath(org.bukkit.event.entity.PlayerDeathEvent)}.
+     * The drops must be left in the event for death chest plugins and the server to handle.
+     */
+    @Test
+    void testOnPlayerDeathLeavesDropsAlone() {
+        List<ItemStack> drops = new ArrayList<>();
+        drops.add(itemStack);
+        when(deathEvent.getPlayer()).thenReturn(player);
+        when(deathEvent.getDrops()).thenReturn(drops);
+        pl.onPlayerDeath(deathEvent);
+        assertEquals(1, drops.size());
+        verify(world, never()).dropItemNaturally(any(Location.class), any(ItemStack.class));
+        // The death spot is remembered, and forgotten again next tick
+        verify(sch).runTask(eq(plugin), any(Runnable.class));
+    }
+
+    /**
+     * Test method for {@link world.bentobox.border.listeners.PlayerListener#onDeathDropSpawn(org.bukkit.event.entity.ItemSpawnEvent)}.
+     * An item spawning at a fresh death spot is a death drop and gets the bounce-back tracking.
+     */
+    @Test
+    void testDeathDropsAreTrackedWhenTheySpawn() {
+        List<ItemStack> drops = new ArrayList<>();
+        drops.add(itemStack);
+        when(deathEvent.getPlayer()).thenReturn(player);
+        when(deathEvent.getDrops()).thenReturn(drops);
+        pl.onPlayerDeath(deathEvent);
+        when(itemSpawnEvent.getLocation()).thenReturn(to);
+        when(itemSpawnEvent.getEntity()).thenReturn(item);
+        pl.onDeathDropSpawn(itemSpawnEvent);
+        verify(sch).runTaskTimer(eq(plugin), any(Runnable.class), eq(1L), eq(1L));
+    }
+
+    /**
+     * Test method for {@link world.bentobox.border.listeners.PlayerListener#onDeathDropSpawn(org.bukkit.event.entity.ItemSpawnEvent)}.
+     */
+    @Test
+    void testItemSpawnWithNoRecentDeathIsIgnored() {
+        pl.onDeathDropSpawn(itemSpawnEvent);
+        verify(sch, never()).runTaskTimer(any(), any(Runnable.class), eq(1L), eq(1L));
+    }
+
+    /**
+     * Test method for {@link world.bentobox.border.listeners.PlayerListener#onDeathDropSpawn(org.bukkit.event.entity.ItemSpawnEvent)}.
+     */
+    @Test
+    void testItemSpawnFarFromDeathIsNotTracked() {
+        List<ItemStack> drops = new ArrayList<>();
+        drops.add(itemStack);
+        when(deathEvent.getPlayer()).thenReturn(player);
+        when(deathEvent.getDrops()).thenReturn(drops);
+        pl.onPlayerDeath(deathEvent);
+        when(farAway.getWorld()).thenReturn(world);
+        when(farAway.distanceSquared(to)).thenReturn(100.0);
+        when(itemSpawnEvent.getLocation()).thenReturn(farAway);
+        pl.onDeathDropSpawn(itemSpawnEvent);
+        verify(sch, never()).runTaskTimer(any(), any(Runnable.class), eq(1L), eq(1L));
+    }
+
+    /**
+     * Test method for {@link world.bentobox.border.listeners.PlayerListener#onPlayerDeath(org.bukkit.event.entity.PlayerDeathEvent)}.
+     */
+    @Test
+    void testOnPlayerDeathBounceBackDisabled() {
+        settings.setBounceBack(false);
+        pl.onPlayerDeath(deathEvent);
+        verify(sch, never()).runTask(any(), any(Runnable.class));
+    }
+
+    /**
+     * Test method for {@link world.bentobox.border.listeners.PlayerListener#onPlayerDeath(org.bukkit.event.entity.PlayerDeathEvent)}.
+     */
+    @Test
+    void testOnPlayerDeathNoDrops() {
+        when(deathEvent.getDrops()).thenReturn(new ArrayList<>());
+        pl.onPlayerDeath(deathEvent);
+        verify(sch, never()).runTask(any(), any(Runnable.class));
+    }
+
 }