Repository: pajlads/DinkPlugin
Pull request #171 (2023-02-18 04:03:40)

fix(kc): allow ticks between duration and boss name
Since Grotesque Guardians has a long death animation, kill count message can be sent seconds after the fight duration message was sent. As a result, the webhook message did not acknowledge the personal best time (discovered by @Felanbird). Now, we hold onto fight duration data for up to 10 ticks so that it can be associated with the delayed kill count message (but we don't apply this timer in the converse direction to avoid delayed messages for other bosses if time information was not sent/parseable). 


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index ea348ba..454c1eb 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 - Minor: Include pet acquisition milestone in webhook metadata from clan message. (#169)
 - Minor: Add setting to skip death notifications with low value of items lost. (#166)
+- Bugfix: Associate fight duration to delayed kill count messages. (#171)
 - Bugfix: Fire skill notification when jumping over a level that matches the configured interval. (#164)
 - Dev: Upgrade Gradle from v7.6 to v8.0. (#167, #170)
 - Dev: Improve thread-safety of level notifier. (#165)
diff --git a/src/main/java/dinkplugin/notifiers/KillCountNotifier.java b/src/main/java/dinkplugin/notifiers/KillCountNotifier.java
index 58c8a5a..2fd6692 100644
--- a/src/main/java/dinkplugin/notifiers/KillCountNotifier.java
+++ b/src/main/java/dinkplugin/notifiers/KillCountNotifier.java
@@ -21,6 +21,8 @@ import java.util.Arrays;
 import java.util.Collections;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -38,7 +40,17 @@ public class KillCountNotifier extends BaseNotifier {
     private static final Pattern SECONDARY_REGEX = Pattern.compile("Your (?:completed|subdued) (?<key>.+) count is: (?<value>\\d+)\\b");
     private static final Pattern TIME_REGEX = Pattern.compile("(?:Duration|time|Subdued in):? (?<time>[\\d:]+(.\\d+)?)\\.?", Pattern.CASE_INSENSITIVE);
 
-    private volatile BossNotificationData data = null;
+    /**
+     * The maximum number of ticks to hold onto a fight duration without a corresponding boss name.
+     * <p>
+     * Note: unlike other notifiers, this is applied asymmetrically
+     * (i.e., we do not wait for fight duration if only boss name was received on the tick)
+     */
+    @VisibleForTesting
+    static final int MAX_BAD_TICKS = 10;
+
+    private final AtomicInteger badTicks = new AtomicInteger();
+    private final AtomicReference<BossNotificationData> data = new AtomicReference<>();
 
     @Override
     public boolean isEnabled() {
@@ -51,7 +63,8 @@ public class KillCountNotifier extends BaseNotifier {
     }
 
     public void reset() {
-        this.data = null;
+        this.data.set(null);
+        this.badTicks.set(0);
     }
 
     public void onGameMessage(String message) {
@@ -66,10 +79,17 @@ public class KillCountNotifier extends BaseNotifier {
     }
 
     public void onTick() {
+        BossNotificationData data = this.data.get();
         if (data != null) {
-            // all data must be sent on the same tick to be included
-            handleKill(data);
-            reset();
+            if (data.getBoss() != null) {
+                // once boss name has arrived, we notify at tick end (even if duration hasn't arrived)
+                handleKill(data);
+                reset();
+            } else if (badTicks.incrementAndGet() > MAX_BAD_TICKS) {
+                // after receiving fight duration, allow up to 10 ticks for boss name to arrive.
+                // if boss name doesn't arrive in time, reset (to avoid stale data contaminating later notifications)
+                reset();
+            }
         }
     }
 
@@ -129,20 +149,22 @@ public class KillCountNotifier extends BaseNotifier {
     }
 
     private void updateData(BossNotificationData updated) {
-        if (data == null) {
-            this.data = updated;
-        } else {
-            // Boss data and timing are sent in separate messages
-            // where the order of the messages differs depending on the boss.
-            // Here, we update data without setting any not-null values back to null.
-            this.data = new BossNotificationData(
-                defaultIfNull(updated.getBoss(), data.getBoss()),
-                defaultIfNull(updated.getCount(), data.getCount()),
-                defaultIfNull(updated.getGameMessage(), data.getGameMessage()),
-                defaultIfNull(updated.getTime(), data.getTime()),
-                defaultIfNull(updated.isPersonalBest(), data.isPersonalBest())
-            );
-        }
+        data.getAndUpdate(old -> {
+            if (old == null) {
+                return updated;
+            } else {
+                // Boss data and timing are sent in separate messages
+                // where the order of the messages differs depending on the boss.
+                // Here, we update data without setting any not-null values back to null.
+                return new BossNotificationData(
+                    defaultIfNull(updated.getBoss(), old.getBoss()),
+                    defaultIfNull(updated.getCount(), old.getCount()),
+                    defaultIfNull(updated.getGameMessage(), old.getGameMessage()),
+                    defaultIfNull(updated.getTime(), old.getTime()),
+                    defaultIfNull(updated.isPersonalBest(), old.isPersonalBest())
+                );
+            }
+        });
     }
 
     private static Optional<BossNotificationData> parse(String message) {


Test patch:
diff --git a/src/test/java/dinkplugin/notifiers/KillCountNotifierTest.java b/src/test/java/dinkplugin/notifiers/KillCountNotifierTest.java
index 4ce71bd..128530c 100644
--- a/src/test/java/dinkplugin/notifiers/KillCountNotifierTest.java
+++ b/src/test/java/dinkplugin/notifiers/KillCountNotifierTest.java
@@ -12,6 +12,7 @@ import org.junit.jupiter.api.Test;
 import org.mockito.InjectMocks;
 
 import java.time.Duration;
+import java.util.stream.IntStream;
 
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.mockito.ArgumentMatchers.any;
@@ -154,6 +155,64 @@ class KillCountNotifierTest extends MockedNotifierTest {
         assertDoesNotThrow(() -> RuneLiteAPI.GSON.toJson(body));
     }
 
+    @Test
+    void testNotifyPbDelayed() {
+        // more config
+        when(config.killCountInterval()).thenReturn(99);
+
+        // fire events
+        notifier.onGameMessage("Fight duration: 1:54.00 (new personal best)");
+        IntStream.range(0, KillCountNotifier.MAX_BAD_TICKS - 1).forEach(i -> notifier.onTick());
+        String gameMessage = "Your Grotesque Guardians kill count is: 79.";
+        notifier.onGameMessage(gameMessage);
+        notifier.onTick();
+
+        // check notification
+        NotificationBody<BossNotificationData> body = NotificationBody.<BossNotificationData>builder()
+            .text(PLAYER_NAME + " has defeated Grotesque Guardians with a new personal best time of 01:54.00 and a completion count of 79")
+            .extra(new BossNotificationData("Grotesque Guardians", 79, gameMessage, Duration.ofMinutes(1).plusSeconds(54), true))
+            .playerName(PLAYER_NAME)
+            .type(NotificationType.KILL_COUNT)
+            .build();
+
+        verify(messageHandler).createMessage(
+            PRIMARY_WEBHOOK_URL,
+            true,
+            body
+        );
+
+        assertDoesNotThrow(() -> RuneLiteAPI.GSON.toJson(body));
+    }
+
+    @Test
+    void testNotifyExtremeDelayMissingPb() {
+        // more config
+        when(config.killCountInterval()).thenReturn(1);
+
+        // fire events
+        notifier.onGameMessage("Fight duration: 1:54.00 (new personal best)");
+        IntStream.range(0, KillCountNotifier.MAX_BAD_TICKS + 1).forEach(i -> notifier.onTick());
+        String gameMessage = "Your Grotesque Guardians kill count is: 80.";
+        notifier.onGameMessage(gameMessage);
+        notifier.onTick();
+
+        // check notification
+        NotificationBody<BossNotificationData> body = NotificationBody.<BossNotificationData>builder()
+            .text(PLAYER_NAME + " has defeated Grotesque Guardians with a completion count of 80")
+            .extra(new BossNotificationData("Grotesque Guardians", 80, gameMessage, null, null))
+            .playerName(PLAYER_NAME)
+            .type(NotificationType.KILL_COUNT)
+            .build();
+
+        verify(messageHandler).createMessage(
+            PRIMARY_WEBHOOK_URL,
+            true,
+            body
+        );
+
+        assertDoesNotThrow(() -> RuneLiteAPI.GSON.toJson(body));
+    }
+
     @Test
     void testNotifyPbLong() {
         // more config