perf: refresh pickup lore only on items that can carry it, and set only changed slots

src/main/java/net/tfminecraft/magic/artifact/aura/VesselLore.java

@@ -1,47 +1,97 @@
 package net.tfminecraft.magic.artifact.aura;
 
+import org.bukkit.Bukkit;
 import org.bukkit.entity.Player;
 import org.bukkit.inventory.ItemStack;
 import org.bukkit.inventory.PlayerInventory;
+import org.bukkit.inventory.meta.ItemMeta;
+import org.bukkit.persistence.PersistentDataContainer;
 
+import net.tfminecraft.magic.artifact.ArtifactKeys;
 import net.tfminecraft.magic.artifact.ArtifactLore;
 import net.tfminecraft.magic.charge.ChargeIds;
+import net.tfminecraft.magic.charge.ChargeKeys;
 import net.tfminecraft.magic.charge.ChargeLore;
+import net.tfminecraft.magic.charge.ChargeRegistry;
 
 /** Sends an item to whichever lore writer owns it. */
 public final class VesselLore {
 
     private VesselLore() {}
 
     public static ItemStack updateItem(ItemStack stack) {
-        if (stack == null || stack.getType().isAir()) {
+        if (stack == null || stack.getType().isAir() || !mayHaveVesselLore(stack)) {
             return stack;
         }
+        return write(stack);
+    }
+
+    private static ItemStack write(ItemStack stack) {
         if (ChargeIds.isCharge(stack)) {
             return ChargeLore.updateItem(stack);
         }
         return ArtifactLore.updateItem(stack);
     }
 
+    /**
+     * False only for an item neither lore writer touches, found from one copy of its meta. Both writers skip an item
+     * without meta (hasItemMeta). A charge has its tier key or matches a charge item (ChargeIds.isCharge); an artifact
+     * has aura data (AuraData.fromPersistentData) or aura lines its lore can be read back from, and each of those has
+     * a "/" (ArtifactLore.readAura). For anything else both writers leave the item exactly as it was, so it isn't
+     * read again for each of them.
+     */
+    // Keep the existing legacy text representation, formatting, and exact-string comparisons.
+    @SuppressWarnings("deprecation")
+    static boolean mayHaveVesselLore(ItemStack stack) {
+        ItemMeta meta = stack.getItemMeta();
+        if (Bukkit.getItemFactory().equals(meta, null)) {
+            return false;
+        }
+        PersistentDataContainer pdc = meta.getPersistentDataContainer();
+        if (pdc.has(ArtifactKeys.auraCap()) || pdc.has(ArtifactKeys.auraFill()) || pdc.has(ArtifactKeys.auraData())
+                || pdc.has(ChargeKeys.chargeTier()) || ChargeRegistry.match(stack) != null) {
+            return true;
+        }
+        if (!meta.hasLore()) {
+            return false;
+        }
+        for (String line : meta.getLore()) {
+            if (line.indexOf('/') >= 0) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    /**
+     * Rewrites the vessel lore of every item the player carries. Only slots whose item the writers changed are set
+     * again; setting an unchanged item gave the same inventory and only resent the slot to the client.
+     */
     public static void updateInventory(Player player) {
         if (player == null) {
             return;
         }
         PlayerInventory inventory = player.getInventory();
         ItemStack[] storage = inventory.getStorageContents();
-        for (int i = 0; i < storage.length; i++) {
-            storage[i] = updateItem(storage[i]);
-        }
-        inventory.setStorageContents(storage);
         ItemStack[] armor = inventory.getArmorContents();
-        for (int i = 0; i < armor.length; i++) {
-            armor[i] = updateItem(armor[i]);
-        }
-        inventory.setArmorContents(armor);
-        ItemStack[] extra = inventory.getExtraContents();
-        for (int i = 0; i < extra.length; i++) {
-            extra[i] = updateItem(extra[i]);
+        refresh(inventory, storage, 0);
+        refresh(inventory, armor, storage.length);
+        refresh(inventory, inventory.getExtraContents(), storage.length + armor.length);
+    }
+
+    /** Refreshes items, which fill the inventory's slots from first on (storage, then armor, then off hand). */
+    private static void refresh(PlayerInventory inventory, ItemStack[] items, int first) {
+        for (int i = 0; i < items.length; i++) {
+            ItemStack item = items[i];
+            if (item == null || item.getType().isAir() || !mayHaveVesselLore(item)) {
+                continue;
+            }
+            // A copy from before: the writers change the item in place, and it may be the inventory's own.
+            ItemStack before = item.clone();
+            ItemStack after = write(item);
+            if (!after.equals(before)) {
+                inventory.setItem(first + i, after);
+            }
         }
-        inventory.setExtraContents(extra);
     }
 }

src/test/java/net/tfminecraft/magic/artifact/aura/VesselLoreTest.java

@@ -0,0 +1,168 @@
+package net.tfminecraft.magic.artifact.aura;
+
+import static org.junit.jupiter.api.Assertions.*;
+import static org.mockito.ArgumentMatchers.*;
+import static org.mockito.Mockito.*;
+
+import java.util.List;
+import net.tfminecraft.magic.Cache;
+import net.tfminecraft.magic.Magic;
+import net.tfminecraft.magic.artifact.Artifact;
+import net.tfminecraft.magic.artifact.ArtifactIds;
+import net.tfminecraft.magic.artifact.ArtifactKeys;
+import net.tfminecraft.magic.artifact.ArtifactLore;
+import net.tfminecraft.magic.charge.ChargeIds;
+import net.tfminecraft.magic.charge.ChargeDef;
+import net.tfminecraft.magic.charge.ChargeKeys;
+import net.tfminecraft.magic.charge.ChargeLore;
+import net.tfminecraft.magic.charge.ChargeRegistry;
+import net.tfminecraft.magic.charge.TierBands;
+import net.tfminecraft.magic.model.ElementDef;
+import net.tfminecraft.magic.registry.ElementRegistry;
+import org.bukkit.Bukkit;
+import org.bukkit.Material;
+import org.bukkit.NamespacedKey;
+import org.bukkit.configuration.file.YamlConfiguration;
+import org.bukkit.entity.Player;
+import org.bukkit.inventory.ItemFactory;
+import org.bukkit.inventory.ItemStack;
+import org.bukkit.inventory.PlayerInventory;
+import org.bukkit.persistence.PersistentDataType;
+import org.junit.jupiter.api.*;
+import org.mockbukkit.mockbukkit.MockBukkit;
+
+class VesselLoreTest {
+
+  @BeforeEach
+  void setup() {
+    MockBukkit.mock();
+    Magic.plugin = mock(Magic.class);
+    when(Magic.plugin.namespace()).thenReturn("magic");
+    ChargeRegistry.clear();
+    TierBands.clear();
+    TierBands.register("default", 1, 5);
+    ElementRegistry.clear();
+    Cache.artifactGlint = true;
+  }
+
+  @AfterEach
+  void cleanup() {
+    Magic.plugin = null;
+    MockBukkit.unmock();
+    ChargeRegistry.clear();
+    ElementRegistry.clear();
+    TierBands.clear();
+  }
+
+  static ItemStack withLore(String... lines) {
+    var item = new ItemStack(Material.STONE);
+    var meta = item.getItemMeta();
+    meta.setLore(List.of(lines));
+    item.setItemMeta(meta);
+    return item;
+  }
+
+  static ItemStack withKey(NamespacedKey key) {
+    var item = new ItemStack(Material.STONE);
+    var meta = item.getItemMeta();
+    meta.getPersistentDataContainer().set(key, PersistentDataType.INTEGER, 1);
+    item.setItemMeta(meta);
+    return item;
+  }
+
+  @Test
+  void onlyItemsWithVesselKeysAChargeMatchOrSlashedLoreAreLookedAt() {
+    assertFalse(VesselLore.mayHaveVesselLore(new ItemStack(Material.STONE)));
+    var named = new ItemStack(Material.STONE);
+    var meta = named.getItemMeta();
+    meta.setDisplayName("Named stone");
+    named.setItemMeta(meta);
+    assertFalse(VesselLore.mayHaveVesselLore(named));
+    assertFalse(VesselLore.mayHaveVesselLore(withLore("Only lore", "A sword, well kept")));
+    assertTrue(VesselLore.mayHaveVesselLore(withLore("Only lore", "Fire 3 / 10")));
+    assertTrue(VesselLore.mayHaveVesselLore(withKey(ArtifactKeys.auraCap())));
+    assertTrue(VesselLore.mayHaveVesselLore(withKey(ArtifactKeys.auraFill())));
+    assertTrue(VesselLore.mayHaveVesselLore(withKey(ArtifactKeys.auraData())));
+    assertTrue(VesselLore.mayHaveVesselLore(withKey(ChargeKeys.chargeTier())));
+    var shard = new ItemStack(Material.AMETHYST_SHARD);
+    var shardMeta = shard.getItemMeta();
+    shardMeta.setDisplayName("Charge");
+    shard.setItemMeta(shardMeta);
+    try (var registry = mockStatic(ChargeRegistry.class)) {
+      registry.when(() -> ChargeRegistry.match(shard)).thenReturn(new ChargeDef(1, "v.AMETHYST_SHARD", 20));
+      assertTrue(VesselLore.mayHaveVesselLore(shard));
+    }
+  }
+
+  @Test
+  void itemsNoWriterTouchesComeBackUnreadAndUnchanged() {
+    var plain = withLore("Only lore");
+    var before = plain.clone();
+    try (var ids = mockStatic(ChargeIds.class);
+        var charge = mockStatic(ChargeLore.class);
+        var artifact = mockStatic(ArtifactLore.class)) {
+      assertSame(plain, VesselLore.updateItem(plain));
+      ids.verifyNoInteractions();
+      charge.verifyNoInteractions();
+      artifact.verifyNoInteractions();
+      // Lore an artifact could be read back from still goes to the artifact writer.
+      var slashed = withLore("Ten / twenty");
+      artifact.when(() -> ArtifactLore.updateItem(slashed)).thenReturn(slashed);
+      assertSame(slashed, VesselLore.updateItem(slashed));
+      artifact.verify(() -> ArtifactLore.updateItem(slashed));
+    }
+    assertEquals(before, plain);
+  }
+
+  @Test
+  void itemsWithoutMetaAreSkippedLikeBothWritersSkipThem() {
+    // On Paper the item factory calls an item without meta equal to no meta at all; MockBukkit's never does.
+    var factory = mock(ItemFactory.class);
+    when(factory.equals(any(), isNull())).thenReturn(true);
+    try (var bukkit = mockStatic(Bukkit.class, CALLS_REAL_METHODS)) {
+      bukkit.when(Bukkit::getItemFactory).thenReturn(factory);
+      assertFalse(VesselLore.mayHaveVesselLore(withKey(ChargeKeys.chargeTier())));
+    }
+  }
+
+  @Test
+  void inventoryRefreshSetsOnlyTheSlotsWhoseItemChanged() {
+    var blank = withKey(ChargeKeys.chargeTier());
+    var current = withKey(ChargeKeys.chargeTier());
+    ChargeLore.apply(current);
+    var player = mock(Player.class);
+    var inventory = mock(PlayerInventory.class);
+    when(player.getInventory()).thenReturn(inventory);
+    when(inventory.getStorageContents())
+        .thenReturn(new ItemStack[] {withLore("Only lore"), null, new ItemStack(Material.AIR), blank});
+    when(inventory.getArmorContents()).thenReturn(new ItemStack[] {null, current.clone()});
+    var offHand = withKey(ChargeKeys.chargeTier());
+    when(inventory.getExtraContents()).thenReturn(new ItemStack[] {offHand});
+    VesselLore.updateInventory(player);
+    verify(inventory).setItem(3, blank);
+    verify(inventory).setItem(6, offHand);
+    verify(inventory, times(2)).setItem(anyInt(), any());
+    assertEquals(current, blank, "the blank charge got its lore");
+  }
+
+  @Test
+  void anArtifactInTheInventoryGetsItsAuraLoreAndOnlyItsSlotIsSet() {
+    ElementRegistry.register(new ElementDef("fire", new YamlConfiguration()));
+    var artifact = new ItemStack(Material.STONE);
+    ArtifactIds.writeNew(artifact);
+    var aura = Artifact.create();
+    aura.setCap("fire", 10);
+    aura.setFill("fire", 5);
+    aura.persistPdc(artifact);
+    var player = mock(Player.class);
+    var inventory = mock(PlayerInventory.class);
+    when(player.getInventory()).thenReturn(inventory);
+    when(inventory.getStorageContents()).thenReturn(new ItemStack[] {new ItemStack(Material.DIRT), artifact});
+    when(inventory.getArmorContents()).thenReturn(new ItemStack[0]);
+    when(inventory.getExtraContents()).thenReturn(new ItemStack[0]);
+    VesselLore.updateInventory(player);
+    verify(inventory).setItem(1, artifact);
+    verify(inventory, times(1)).setItem(anyInt(), any());
+    assertTrue(artifact.getItemMeta().getLore().stream().anyMatch(line -> line.contains("/")));
+  }
+}