Repository: pajlads/DinkPlugin (java)

Issue:
As a JSON/extra consumer I would like to see the name of the 'killing' monster on death events
Is it possible to capture/emit the name of the event or monster which killed the user similar to how the pvp username is emitted on death?

Pull request description:
feat: allow death attribution to NPCs
Closes #189


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 65c94ac..2b53d30 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,6 @@
 ## Unreleased
 
+- Minor: Include NPC killer in death notification metadata. (#207)
 - Minor: Add advanced setting to hide chat when screenshotting. (#208)
 - Minor: Rescale screenshots to comply with Discord's 8MB size limit. (#200)
 - Minor: Include combat level in skill notifications. (#203)
diff --git a/README.md b/README.md
index 3cf1b9c..4004c37 100644
--- a/README.md
+++ b/README.md
@@ -127,7 +127,7 @@ Lastly, Dink makes exceptions for Inferno and TzHaar Fight Cave; deaths in these
 **Note**: If *Distinguish PvP deaths* is disabled, the message content will be the non-PvP version.
 
 <details>
-  <summary>JSON for non-PvP death:</summary>
+  <summary>JSON for non-combat death:</summary>
 
 ```json5
 {
@@ -159,7 +159,33 @@ Lastly, Dink makes exceptions for Inferno and TzHaar Fight Cave; deaths in these
   "extra": {
     "valueLost": 300,
     "isPvp": true,
-    "pker": "%PKER%",
+    "killerName": "%PKER%",
+    "keptItems": [],
+    "lostItems": [
+      {
+        "id": 314,
+        "quantity": 100,
+        "priceEach": 3,
+        "name": "Feather"
+      }
+    ]
+  },
+  "type": "DEATH"
+}
+```
+</details>
+
+<details>
+  <summary>JSON for NPC scenarios:</summary>
+
+```json5
+{
+  "content": "%USERNAME% has died...",
+  "extra": {
+    "valueLost": 300,
+    "isPvp": false,
+    "killerName": "%NPC%",
+    "killerNpcId": 69, 
     "keptItems": [],
     "lostItems": [
       {
diff --git a/src/main/java/dinkplugin/notifiers/DeathNotifier.java b/src/main/java/dinkplugin/notifiers/DeathNotifier.java
index f9507c3..915501c 100644
--- a/src/main/java/dinkplugin/notifiers/DeathNotifier.java
+++ b/src/main/java/dinkplugin/notifiers/DeathNotifier.java
@@ -13,6 +13,9 @@ import net.runelite.api.Actor;
 import net.runelite.api.Client;
 import net.runelite.api.Item;
 import net.runelite.api.ItemID;
+import net.runelite.api.NPC;
+import net.runelite.api.NPCComposition;
+import net.runelite.api.ParamID;
 import net.runelite.api.Player;
 import net.runelite.api.Prayer;
 import net.runelite.api.Varbits;
@@ -20,6 +23,9 @@ import net.runelite.api.events.ActorDeath;
 import net.runelite.api.events.InteractingChanged;
 import net.runelite.api.vars.AccountType;
 import net.runelite.client.game.ItemManager;
+import net.runelite.client.game.NPCManager;
+import org.apache.commons.lang3.ArrayUtils;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.tuple.Pair;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -29,11 +35,13 @@ import javax.inject.Inject;
 import javax.inject.Singleton;
 import java.lang.ref.WeakReference;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
 import java.util.Optional;
+import java.util.function.BiFunction;
 import java.util.function.BiPredicate;
 import java.util.function.Function;
 import java.util.function.Predicate;
@@ -43,11 +51,23 @@ import java.util.stream.Collectors;
 @Singleton
 public class DeathNotifier extends BaseNotifier {
 
+    private static final String ATTACK_OPTION = "Attack";
+
     /**
      * Checks whether the actor is alive and interacting with the specified player.
      */
     private static final BiPredicate<Player, Actor> INTERACTING;
 
+    /**
+     * Checks whether a NPC is a valid candidate to be our killer.
+     */
+    private static final Predicate<NPCComposition> NPC_VALID;
+
+    /**
+     * Orders NPCs by their likelihood of being our killer.
+     */
+    private static final BiFunction<NPCManager, Player, Comparator<NPC>> NPC_COMPARATOR;
+
     /**
      * Orders actors by their likelihood of being the killer of the specified player.
      */
@@ -56,6 +76,9 @@ public class DeathNotifier extends BaseNotifier {
     @Inject
     private ItemManager itemManager;
 
+    @Inject
+    private NPCManager npcManager;
+
     /**
      * Tracks the last {@link Actor} our local player interacted with,
      * for the purposes of attributing deaths to particular {@link Player}'s.
@@ -116,7 +139,9 @@ public class DeathNotifier extends BaseNotifier {
 
         Actor killer = identifyKiller();
         boolean pk = killer instanceof Player;
-        String notifyMessage = buildMessage(killer, losePrice, pk);
+        boolean npc = killer instanceof NPC;
+        String killerName = killer != null ? StringUtils.defaultIfEmpty(killer.getName(), "?") : null;
+        String notifyMessage = buildMessage(killerName, losePrice, pk, npc);
 
         List<SerializedItemStack> lostStacks = getStacks(itemManager, lostItems, true);
         List<SerializedItemStack> keptStacks = getStacks(itemManager, keptItems, false);
@@ -136,7 +161,9 @@ public class DeathNotifier extends BaseNotifier {
         DeathNotificationData extra = new DeathNotificationData(
             losePrice,
             pk,
-            pk ? killer.getName() : null,
+            pk ? killerName : null,
+            killerName,
+            npc ? ((NPC) killer).getId() : null,
             keptStacks,
             lostStacks
         );
@@ -149,7 +176,7 @@ public class DeathNotifier extends BaseNotifier {
             .build());
     }
 
-    private String buildMessage(Actor killer, long losePrice, boolean pk) {
+    private String buildMessage(String killer, long losePrice, boolean pk, boolean npc) {
         boolean pvp = pk && config.deathNotifPvpEnabled();
         String template;
         if (pvp)
@@ -160,7 +187,9 @@ public class DeathNotifier extends BaseNotifier {
             .replace("%USERNAME%", Utils.getPlayerName(client))
             .replace("%VALUELOST%", String.valueOf(losePrice));
         if (pvp) {
-            notifyMessage = notifyMessage.replace("%PKER%", String.valueOf(killer.getName()));
+            notifyMessage = notifyMessage.replace("%PKER%", killer);
+        } else if (npc) {
+            notifyMessage = notifyMessage.replace("%NPC%", killer);
         }
         return notifyMessage;
     }
@@ -208,7 +237,12 @@ public class DeathNotifier extends BaseNotifier {
                 return pker.get();
         }
 
-        return null;
+        // otherwise search through NPCs interacting with us
+        return Arrays.stream(client.getCachedNPCs())
+            .filter(interacting)
+            .filter(npc -> NPC_VALID.test(npc.getTransformedComposition()))
+            .min(NPC_COMPARATOR.apply(npcManager, localPlayer)) // O(n)
+            .orElse(null);
     }
 
     /**
@@ -226,6 +260,12 @@ public class DeathNotifier extends BaseNotifier {
             return pvpEnabled && !other.isClanMember() && !other.isFriend() && !other.isFriendsChatMember();
         }
 
+        if (actor instanceof NPC) {
+            NPCComposition npc = ((NPC) actor).getTransformedComposition();
+            return NPC_VALID.test(npc) && ArrayUtils.contains(npc.getActions(), ATTACK_OPTION);
+        }
+
+        log.warn("Encountered unknown type of Actor; was neither Player nor NPC!");
         return false;
     }
 
@@ -314,6 +354,31 @@ public class DeathNotifier extends BaseNotifier {
     static {
         INTERACTING = (localPlayer, a) -> a != null && !a.isDead() && a.getInteracting() == localPlayer;
 
+        NPC_VALID = comp -> comp != null && comp.isInteractible() && !comp.isFollower() && comp.getCombatLevel() > 0;
+
+        NPC_COMPARATOR = (npcManager, localPlayer) -> Comparator
+            .comparing(
+                NPC::getTransformedComposition,
+                Comparator.nullsFirst(
+                    Comparator
+                        .comparing(
+                            (NPCComposition comp) -> comp.getStringValue(ParamID.NPC_HP_NAME),
+                            Comparator.comparing(StringUtils::isNotEmpty) // prefer has name in hit points UI
+                        )
+                        .thenComparing(comp -> ArrayUtils.contains(comp.getActions(), ATTACK_OPTION)) // prefer explicitly attackable
+                        .thenComparingInt(NPCComposition::getCombatLevel) // prefer high level
+                        .thenComparingInt(NPCComposition::getSize) // prefer large
+                        .thenComparing(NPCComposition::isMinimapVisible) // prefer visible on minimap
+                        .thenComparing(
+                            Comparator.nullsFirst(
+                                Comparator.comparing(comp -> npcManager.getHealth(comp.getId())) // prefer high max health
+                            )
+                        )
+                )
+            )
+            .thenComparingInt(p -> -localPlayer.getLocalLocation().distanceTo(p.getLocalLocation())) // prefer nearby
+            .reversed(); // for consistency with PK_COMPARATOR such that Stream#min should be used in #identifyKiller
+
         PK_COMPARATOR = localPlayer -> Comparator
             .comparing(Player::isClanMember) // prefer not in clan
             .thenComparing(Player::isFriend) // prefer not friend
diff --git a/src/main/java/dinkplugin/notifiers/data/DeathNotificationData.java b/src/main/java/dinkplugin/notifiers/data/DeathNotificationData.java
index ee2c71d..a8774b2 100644
--- a/src/main/java/dinkplugin/notifiers/data/DeathNotificationData.java
+++ b/src/main/java/dinkplugin/notifiers/data/DeathNotificationData.java
@@ -2,15 +2,36 @@ package dinkplugin.notifiers.data;
 
 import lombok.EqualsAndHashCode;
 import lombok.Value;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
 
-import java.util.List;
+import java.util.Collection;
 
 @Value
 @EqualsAndHashCode(callSuper = false)
 public class DeathNotificationData extends NotificationData {
-    Long valueLost;
+
+    long valueLost;
+
     boolean isPvp;
+
+    /**
+     * @deprecated in favor of {@link #getKillerName()}
+     */
+    @Nullable
+    @Deprecated
     String pker;
-    List<SerializedItemStack> keptItems;
-    List<SerializedItemStack> lostItems;
+
+    @Nullable
+    String killerName;
+
+    @Nullable
+    Integer killerNpcId;
+
+    @NotNull
+    Collection<SerializedItemStack> keptItems;
+
+    @NotNull
+    Collection<SerializedItemStack> lostItems;
+
 }


Test patch:
diff --git a/src/test/java/dinkplugin/notifiers/DeathNotifierTest.java b/src/test/java/dinkplugin/notifiers/DeathNotifierTest.java
index 8bb21cf..329e9d8 100644
--- a/src/test/java/dinkplugin/notifiers/DeathNotifierTest.java
+++ b/src/test/java/dinkplugin/notifiers/DeathNotifierTest.java
@@ -10,9 +10,14 @@ import net.runelite.api.InventoryID;
 import net.runelite.api.Item;
 import net.runelite.api.ItemContainer;
 import net.runelite.api.ItemID;
+import net.runelite.api.NPC;
+import net.runelite.api.NPCComposition;
+import net.runelite.api.NpcID;
+import net.runelite.api.ParamID;
 import net.runelite.api.Player;
 import net.runelite.api.Prayer;
 import net.runelite.api.Varbits;
+import net.runelite.api.coords.LocalPoint;
 import net.runelite.api.coords.WorldPoint;
 import net.runelite.api.events.ActorDeath;
 import net.runelite.api.events.InteractingChanged;
@@ -26,6 +31,7 @@ import java.util.List;
 
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
@@ -59,8 +65,10 @@ class DeathNotifierTest extends MockedNotifierTest {
         // init client mocks
         when(client.getVarbitValue(Varbits.IN_WILDERNESS)).thenReturn(1);
         when(client.getPlayers()).thenReturn(Collections.emptyList());
+        when(client.getCachedNPCs()).thenReturn(new NPC[0]);
         WorldPoint location = new WorldPoint(0, 0, 0);
         when(localPlayer.getWorldLocation()).thenReturn(location);
+        when(localPlayer.getLocalLocation()).thenReturn(new LocalPoint(0, 0));
 
         // init item mocks
         mockItem(ItemID.RUBY, RUBY_PRICE, "Ruby");
@@ -68,6 +76,9 @@ class DeathNotifierTest extends MockedNotifierTest {
         mockItem(ItemID.OPAL, OPAL_PRICE, "Opal");
         mockItem(ItemID.COAL, COAL_PRICE, "Coal");
         mockItem(ItemID.TUNA, TUNA_PRICE, "Tuna");
+
+        // init npc mocks
+        when(npcManager.getHealth(anyInt())).thenReturn(50);
     }
 
     @Test
@@ -81,7 +92,7 @@ class DeathNotifierTest extends MockedNotifierTest {
             false,
             NotificationBody.builder()
                 .text(String.format("%s has died, losing %d gp", PLAYER_NAME, 0))
-                .extra(new DeathNotificationData(0L, false, null, Collections.emptyList(), Collections.emptyList()))
+                .extra(new DeathNotificationData(0L, false, null, null, null, Collections.emptyList(), Collections.emptyList()))
                 .type(NotificationType.DEATH)
                 .build()
         );
@@ -126,7 +137,7 @@ class DeathNotifierTest extends MockedNotifierTest {
             false,
             NotificationBody.builder()
                 .text(String.format("%s has died, losing %d gp", PLAYER_NAME, TUNA_PRICE))
-                .extra(new DeathNotificationData((long) TUNA_PRICE, false, null, kept, lost))
+                .extra(new DeathNotificationData(TUNA_PRICE, false, null, null, null, kept, lost))
                 .type(NotificationType.DEATH)
                 .embeds(embeds)
                 .build()
@@ -151,7 +162,7 @@ class DeathNotifierTest extends MockedNotifierTest {
             false,
             NotificationBody.builder()
                 .text(String.format("%s has been PKed by %s for %d gp", PLAYER_NAME, pker, 0))
-                .extra(new DeathNotificationData(0L, true, pker, Collections.emptyList(), Collections.emptyList()))
+                .extra(new DeathNotificationData(0L, true, pker, pker, null, Collections.emptyList(), Collections.emptyList()))
                 .type(NotificationType.DEATH)
                 .build()
         );
@@ -178,7 +189,7 @@ class DeathNotifierTest extends MockedNotifierTest {
             false,
             NotificationBody.builder()
                 .text(String.format("%s has been PKed by %s for %d gp", PLAYER_NAME, pker, 0))
-                .extra(new DeathNotificationData(0L, true, pker, Collections.emptyList(), Collections.emptyList()))
+                .extra(new DeathNotificationData(0L, true, pker, pker, null, Collections.emptyList(), Collections.emptyList()))
                 .type(NotificationType.DEATH)
                 .build()
         );
@@ -202,7 +213,49 @@ class DeathNotifierTest extends MockedNotifierTest {
             false,
             NotificationBody.builder()
                 .text(String.format("%s has died, losing %d gp", PLAYER_NAME, 0))
-                .extra(new DeathNotificationData(0L, false, null, Collections.emptyList(), Collections.emptyList()))
+                .extra(new DeathNotificationData(0L, false, null, null, null, Collections.emptyList(), Collections.emptyList()))
+                .type(NotificationType.DEATH)
+                .build()
+        );
+    }
+
+    @Test
+    void testNotifyNpc() {
+        // init mocks
+        String name = "Guard";
+        NPC other = mock(NPC.class);
+        when(other.getName()).thenReturn(name);
+        when(other.getId()).thenReturn(NpcID.GUARD);
+        when(other.isDead()).thenReturn(false);
+        when(other.getCombatLevel()).thenReturn(21);
+        when(other.getInteracting()).thenReturn(localPlayer);
+        when(other.getLocalLocation()).thenReturn(new LocalPoint(1, 1));
+
+        NPCComposition comp = mock(NPCComposition.class);
+        when(other.getTransformedComposition()).thenReturn(comp);
+        when(comp.isInteractible()).thenReturn(true);
+        when(comp.isFollower()).thenReturn(false);
+        when(comp.getSize()).thenReturn(1);
+        when(comp.isMinimapVisible()).thenReturn(true);
+        when(comp.getId()).thenReturn(NpcID.GUARD);
+        when(comp.getName()).thenReturn(name);
+        when(comp.getStringValue(ParamID.NPC_HP_NAME)).thenReturn(name);
+        when(comp.getCombatLevel()).thenReturn(21);
+        when(comp.getActions()).thenReturn(new String[] { "Pickpocket", "Attack", "Examine" });
+
+        when(npcManager.getHealth(NpcID.GUARD)).thenReturn(22);
+        when(client.getCachedNPCs()).thenReturn(new NPC[] { other });
+
+        // fire event
+        plugin.onActorDeath(new ActorDeath(localPlayer));
+
+        // verify notification
+        verify(messageHandler).createMessage(
+            PRIMARY_WEBHOOK_URL,
+            false,
+            NotificationBody.builder()
+                .text(String.format("%s has died, losing %d gp", PLAYER_NAME, 0))
+                .extra(new DeathNotificationData(0L, false, null, name, NpcID.GUARD, Collections.emptyList(), Collections.emptyList()))
                 .type(NotificationType.DEATH)
                 .build()
         );
diff --git a/src/test/java/dinkplugin/notifiers/MockedNotifierTest.java b/src/test/java/dinkplugin/notifiers/MockedNotifierTest.java
index 5d19d1f..d8dfd91 100644
--- a/src/test/java/dinkplugin/notifiers/MockedNotifierTest.java
+++ b/src/test/java/dinkplugin/notifiers/MockedNotifierTest.java
@@ -21,6 +21,7 @@ import net.runelite.client.chat.ChatMessageManager;
 import net.runelite.client.config.ConfigDescriptor;
 import net.runelite.client.config.ConfigManager;
 import net.runelite.client.game.ItemManager;
+import net.runelite.client.game.NPCManager;
 import net.runelite.client.ui.DrawManager;
 import net.runelite.http.api.RuneLiteAPI;
 import okhttp3.OkHttpClient;
@@ -73,6 +74,9 @@ abstract class MockedNotifierTest extends MockedTestBase {
     @Bind
     protected ItemManager itemManager = Mockito.mock(ItemManager.class);
 
+    @Bind
+    protected NPCManager npcManager = Mockito.mock(NPCManager.class);
+
     @Bind
     protected DinkPlugin plugin = Mockito.spy(DinkPlugin.class);