Don't fully translate item data to compare net IDs

Just compare the item mappings of the two Java items. This should shave some NBT and display conversion processing time down.

core/src/main/java/org/geysermc/geyser/inventory/Inventory.java

@@ -35,7 +35,9 @@ import lombok.NonNull;
 import lombok.Setter;
 import lombok.ToString;
 import org.geysermc.geyser.GeyserImpl;
+import org.geysermc.geyser.registry.type.ItemMapping;
 import org.geysermc.geyser.session.GeyserSession;
+import org.geysermc.geyser.translator.inventory.item.ItemTranslator;
 import org.jetbrains.annotations.Range;
 
 import java.util.Arrays;
@@ -136,7 +138,9 @@ public abstract class Inventory {
 
     protected void updateItemNetId(GeyserItemStack oldItem, GeyserItemStack newItem, GeyserSession session) {
         if (!newItem.isEmpty()) {
-            if (newItem.getItemData(session).equals(oldItem.getItemData(session), false, false, false)) {
+            ItemMapping oldMapping = ItemTranslator.getBedrockItemMapping(session, oldItem);
+            ItemMapping newMapping = ItemTranslator.getBedrockItemMapping(session, newItem);
+            if (oldMapping.getBedrockId() == newMapping.getBedrockId()) {
                 newItem.setNetId(oldItem.getNetId());
             } else {
                 newItem.setNetId(session.getNextItemNetId());

core/src/main/java/org/geysermc/geyser/session/cache/LodestoneCache.java

@@ -30,9 +30,6 @@ import com.github.steveice10.opennbt.tag.builtin.IntTag;
 import com.github.steveice10.opennbt.tag.builtin.StringTag;
 import it.unimi.dsi.fastutil.ints.Int2ObjectMap;
 import it.unimi.dsi.fastutil.ints.Int2ObjectOpenHashMap;
-import lombok.AllArgsConstructor;
-import lombok.EqualsAndHashCode;
-import lombok.Getter;
 import org.geysermc.geyser.inventory.GeyserItemStack;
 
 import javax.annotation.Nullable;
@@ -43,7 +40,7 @@ import java.util.WeakHashMap;
  * A temporary cache for lodestone information.
  * Bedrock requests the lodestone position information separately from the item.
  */
-public class LodestoneCache {
+public final class LodestoneCache {
     /**
      * A list of any GeyserItemStacks that are lodestones. Used mainly to minimize Bedrock's "pop-in" effect
      * when a new item has been created; instead we can re-use already existing IDs
@@ -121,8 +118,16 @@ public class LodestoneCache {
     }
 
     public @Nullable LodestonePos getPos(int id) {
-        // We should not need to check the activeLodestones map as Bedrock should already be aware of this ID
+        LodestonePos pos = this.lodestones.remove(id);
-        return this.lodestones.remove(id);
+        if (pos != null) {
+            return pos;
+        }
+        for (LodestonePos activePos : this.activeLodestones.values()) {
+            if (activePos.id == id) {
+                return activePos;
+            }
+        }
+        return null;
     }
 
     public void clear() {
@@ -131,16 +136,7 @@ public class LodestoneCache {
         this.lodestones.clear();
     }
 
-    @Getter
+    public record LodestonePos(int id, int x, int y, int z, String dimension) {
-    @AllArgsConstructor
-    @EqualsAndHashCode
-    public static class LodestonePos {
-        private final int id;
-        private final int x;
-        private final int y;
-        private final int z;
-        private final String dimension;
-
         boolean equals(int x, int y, int z, String dimension) {
             return this.x == x && this.y == y && this.z == z && this.dimension.equals(dimension);
         }

core/src/main/java/org/geysermc/geyser/translator/inventory/item/BannerTranslator.java

@@ -155,7 +155,7 @@ public class BannerTranslator extends ItemTranslator {
     }
 
     @Override
-    public ItemData.Builder translateToBedrock(ItemStack itemStack, ItemMapping mapping, ItemMappings mappings) {
+    protected ItemData.Builder translateToBedrock(ItemStack itemStack, ItemMapping mapping, ItemMappings mappings) {
         if (itemStack.getNbt() == null) {
             return super.translateToBedrock(itemStack, mapping, mappings);
         }
@@ -163,9 +163,7 @@ public class BannerTranslator extends ItemTranslator {
         ItemData.Builder builder = super.translateToBedrock(itemStack, mapping, mappings);
 
         CompoundTag blockEntityTag = itemStack.getNbt().get("BlockEntityTag");
-        if (blockEntityTag != null && blockEntityTag.contains("Patterns")) {
+        if (blockEntityTag != null && blockEntityTag.get("Patterns") instanceof ListTag patterns) {
-            ListTag patterns = blockEntityTag.get("Patterns");
-
             NbtMapBuilder nbtBuilder = builder.build().getTag().toBuilder(); //TODO fix ugly hack
             if (patterns.equals(OMINOUS_BANNER_PATTERN)) {
                 // Remove the current patterns and set the ominous banner type

core/src/main/java/org/geysermc/geyser/translator/inventory/item/CompassTranslator.java

@@ -26,7 +26,9 @@
 package org.geysermc.geyser.translator.inventory.item;
 
 import com.github.steveice10.mc.protocol.data.game.entity.metadata.ItemStack;
-import com.github.steveice10.opennbt.tag.builtin.*;
+import com.github.steveice10.opennbt.tag.builtin.ByteTag;
+import com.github.steveice10.opennbt.tag.builtin.CompoundTag;
+import com.github.steveice10.opennbt.tag.builtin.Tag;
 import com.nukkitx.protocol.bedrock.data.inventory.ItemData;
 import org.geysermc.geyser.network.MinecraftProtocol;
 import org.geysermc.geyser.registry.Registries;
@@ -51,19 +53,30 @@ public class CompassTranslator extends ItemTranslator {
     }
 
     @Override
-    public ItemData.Builder translateToBedrock(ItemStack itemStack, ItemMapping mapping, ItemMappings mappings) {
+    protected ItemData.Builder translateToBedrock(ItemStack itemStack, ItemMapping mapping, ItemMappings mappings) {
-        if (itemStack.getNbt() == null) return super.translateToBedrock(itemStack, mapping, mappings);
+        if (isLodestoneCompass(itemStack.getNbt())) {
-
+            // NBT will be translated in nbt/LodestoneCompassTranslator if applicable
-        Tag lodestoneTag = itemStack.getNbt().get("LodestoneTracked");
+            return super.translateToBedrock(itemStack, mappings.getStoredItems().lodestoneCompass(), mappings);
-        if (lodestoneTag instanceof ByteTag) {
-            // Get the fake lodestonecompass entry
-            mapping = mappings.getStoredItems().lodestoneCompass();
-            // NBT will be translated in nbt/LodestoneCompassTranslator
         }
-
         return super.translateToBedrock(itemStack, mapping, mappings);
     }
 
+    @Override
+    protected ItemMapping getItemMapping(int javaId, CompoundTag nbt, ItemMappings mappings) {
+        if (isLodestoneCompass(nbt)) {
+            return mappings.getStoredItems().lodestoneCompass();
+        }
+        return super.getItemMapping(javaId, nbt, mappings);
+    }
+
+    private boolean isLodestoneCompass(CompoundTag nbt) {
+        if (nbt != null) {
+            Tag lodestoneTag = nbt.get("LodestoneTracked");
+            return lodestoneTag instanceof ByteTag;
+        }
+        return false;
+    }
+
     @Override
     public ItemStack translateToJava(ItemData itemData, ItemMapping mapping, ItemMappings mappings) {
         if (mapping.getBedrockIdentifier().equals("minecraft:lodestone_compass")) {

core/src/main/java/org/geysermc/geyser/translator/inventory/item/ItemTranslator.java

@@ -37,6 +37,7 @@ import it.unimi.dsi.fastutil.ints.Int2ObjectOpenHashMap;
 import net.kyori.adventure.text.Component;
 import net.kyori.adventure.text.format.NamedTextColor;
 import org.geysermc.geyser.GeyserImpl;
+import org.geysermc.geyser.inventory.GeyserItemStack;
 import org.geysermc.geyser.registry.BlockRegistries;
 import org.geysermc.geyser.registry.type.ItemMapping;
 import org.geysermc.geyser.registry.type.ItemMappings;
@@ -157,18 +158,13 @@ public abstract class ItemTranslator {
 
         nbt = translateDisplayProperties(session, nbt, bedrockItem);
         if (session.isAdvancedTooltips()) {
-            nbt = addAdvancedTooltips(nbt, session.getItemMappings().getMapping(stack), session.getLocale());
+            nbt = addAdvancedTooltips(nbt, bedrockItem, session.getLocale());
         }
 
         ItemStack itemStack = new ItemStack(stack.getId(), stack.getAmount(), nbt);
 
-        ItemData.Builder builder;
+        ItemTranslator itemStackTranslator = ITEM_STACK_TRANSLATORS.getOrDefault(bedrockItem.getJavaId(), DEFAULT_TRANSLATOR);
-        ItemTranslator itemStackTranslator = ITEM_STACK_TRANSLATORS.get(bedrockItem.getJavaId());
+        ItemData.Builder builder = itemStackTranslator.translateToBedrock(itemStack, bedrockItem, session.getItemMappings());
-        if (itemStackTranslator != null) {
-            builder = itemStackTranslator.translateToBedrock(itemStack, bedrockItem, session.getItemMappings());
-        } else {
-            builder = DEFAULT_TRANSLATOR.translateToBedrock(itemStack, bedrockItem, session.getItemMappings());
-        }
         if (bedrockItem.isBlock()) {
             builder.blockRuntimeId(bedrockItem.getBedrockBlockId());
         }
@@ -263,6 +259,19 @@ public abstract class ItemTranslator {
         return canModifyBedrock;
     }
 
+    /**
+     * Given an item stack, determine the item mapping that should be applied to Bedrock players.
+     */
+    @Nonnull
+    public static ItemMapping getBedrockItemMapping(GeyserSession session, @Nonnull GeyserItemStack itemStack) {
+        if (itemStack.isEmpty()) {
+            return ItemMapping.AIR;
+        }
+        int javaId = itemStack.getJavaId();
+        return ITEM_STACK_TRANSLATORS.getOrDefault(javaId, DEFAULT_TRANSLATOR)
+                .getItemMapping(javaId, itemStack.getNbt(), session.getItemMappings());
+    }
+
     private static final ItemTranslator DEFAULT_TRANSLATOR = new ItemTranslator() {
         @Override
         public List<ItemMapping> getAppliedItems() {
@@ -270,7 +279,7 @@ public abstract class ItemTranslator {
         }
     };
 
-    public ItemData.Builder translateToBedrock(ItemStack itemStack, ItemMapping mapping, ItemMappings mappings) {
+    protected ItemData.Builder translateToBedrock(ItemStack itemStack, ItemMapping mapping, ItemMappings mappings) {
         if (itemStack == null) {
             // Return, essentially, air
             return ItemData.builder();
@@ -295,6 +304,10 @@ public abstract class ItemTranslator {
 
     public abstract List<ItemMapping> getAppliedItems();
 
+    protected ItemMapping getItemMapping(int javaId, CompoundTag nbt, ItemMappings mappings) {
+        return mappings.getMapping(javaId);
+    }
+
     public NbtMap translateNbtToBedrock(CompoundTag tag) {
         NbtMapBuilder builder = NbtMap.builder();
         if (tag.getValue() != null && !tag.getValue().isEmpty()) {

core/src/main/java/org/geysermc/geyser/translator/inventory/item/PotionTranslator.java

@@ -54,7 +54,7 @@ public class PotionTranslator extends ItemTranslator {
     }
 
     @Override
-    public ItemData.Builder translateToBedrock(ItemStack itemStack, ItemMapping mapping, ItemMappings mappings) {
+    protected ItemData.Builder translateToBedrock(ItemStack itemStack, ItemMapping mapping, ItemMappings mappings) {
         if (itemStack.getNbt() == null) return super.translateToBedrock(itemStack, mapping, mappings);
         Tag potionTag = itemStack.getNbt().get("Potion");
         if (potionTag instanceof StringTag) {

core/src/main/java/org/geysermc/geyser/translator/inventory/item/TippedArrowTranslator.java

@@ -59,7 +59,7 @@ public class TippedArrowTranslator extends ItemTranslator {
     }
 
     @Override
-    public ItemData.Builder translateToBedrock(ItemStack itemStack, ItemMapping mapping, ItemMappings mappings) {
+    protected ItemData.Builder translateToBedrock(ItemStack itemStack, ItemMapping mapping, ItemMappings mappings) {
         if (!mapping.getJavaIdentifier().equals("minecraft:tipped_arrow") || itemStack.getNbt() == null) {
             // We're only concerned about minecraft:arrow when translating Bedrock -> Java
             return super.translateToBedrock(itemStack, mapping, mappings);

core/src/main/java/org/geysermc/geyser/translator/protocol/bedrock/BedrockPositionTrackingDBClientRequestTranslator.java

@@ -58,14 +58,14 @@ public class BedrockPositionTrackingDBClientRequestTranslator extends PacketTran
 
         // Build the NBT data for the update
         NbtMapBuilder builder = NbtMap.builder();
-        builder.putInt("dim", DimensionUtils.javaToBedrock(pos.getDimension()));
+        builder.putInt("dim", DimensionUtils.javaToBedrock(pos.dimension()));
         builder.putString("id", "0x" + String.format("%08X", packet.getTrackingId()));
 
         builder.putByte("version", (byte) 1); // Not sure what this is for
         builder.putByte("status", (byte) 0); // Not sure what this is for
 
         // Build the position for the update
-        builder.putList("pos", NbtType.INT, pos.getX(), pos.getY(), pos.getZ());
+        builder.putList("pos", NbtType.INT, pos.x(), pos.y(), pos.z());
         broadcastPacket.setTag(builder.build());
 
         session.sendUpstreamPacket(broadcastPacket);