GeyserMC · Camotoy · May 9, 2021 · May 9, 2021
diff --git a/connector/src/main/java/org/geysermc/connector/entity/ItemFrameEntity.java b/connector/src/main/java/org/geysermc/connector/entity/ItemFrameEntity.java
@@ -35,6 +35,7 @@
 import com.nukkitx.protocol.bedrock.data.inventory.ItemData;
 import com.nukkitx.protocol.bedrock.packet.BlockEntityDataPacket;
 import com.nukkitx.protocol.bedrock.packet.UpdateBlockPacket;
+import lombok.Getter;
 import org.geysermc.connector.entity.type.EntityType;
 import org.geysermc.connector.network.session.GeyserSession;
 import org.geysermc.connector.network.translators.item.ItemEntry;
@@ -69,6 +70,11 @@ public class ItemFrameEntity extends Entity {
      * Cached item frame's Bedrock compound tag.
      */
     private NbtMap cachedTag;
+    /**
+     * The item currently in the item frame. Used for block picking.
+     */
+    @Getter
+    private ItemStack heldItem = null;
 
     public ItemFrameEntity(long entityId, long geyserId, EntityType entityType, Vector3f position, Vector3f motion, Vector3f rotation, HangingDirection direction) {
         super(entityId, geyserId, entityType, position, motion, rotation);
@@ -87,7 +93,8 @@ public void spawnEntity(GeyserSession session) {
         bedrockRuntimeId = session.getBlockTranslator().getItemFrame(blockBuilder.build());
         bedrockPosition = Vector3i.from(position.getFloorX(), position.getFloorY(), position.getFloorZ());
 
-        session.getItemFrameCache().put(bedrockPosition, entityId);
+        session.getItemFrameCache().put(bedrockPosition, this);
+
         // Delay is required, or else loading in frames on chunk load is sketchy at best
         session.getConnector().getGeneralThreadPool().schedule(() -> {
             updateBlock(session);
@@ -99,13 +106,14 @@ public void spawnEntity(GeyserSession session) {
     @Override
     public void updateBedrockMetadata(EntityMetadata entityMetadata, GeyserSession session) {
         if (entityMetadata.getId() == 7 && entityMetadata.getValue() != null) {
-            ItemData itemData = ItemTranslator.translateToBedrock(session, (ItemStack) entityMetadata.getValue());
+            this.heldItem = (ItemStack) entityMetadata.getValue();
+            ItemData itemData = ItemTranslator.translateToBedrock(session, heldItem);
             ItemEntry itemEntry = ItemRegistry.getItem((ItemStack) entityMetadata.getValue());
             NbtMapBuilder builder = NbtMap.builder();
 
             builder.putByte("Count", (byte) itemData.getCount());
             if (itemData.getTag() != null) {
-                builder.put("tag", itemData.getTag().toBuilder().build());
+                builder.put("tag", itemData.getTag());
             }
             builder.putShort("Damage", (short) itemData.getDamage());
             builder.putString("Name", itemEntry.getBedrockIdentifier());
@@ -146,7 +154,9 @@ public boolean despawnEntity(GeyserSession session) {
         updateBlockPacket.getFlags().add(UpdateBlockPacket.Flag.NETWORK);
         updateBlockPacket.getFlags().add(UpdateBlockPacket.Flag.NEIGHBORS);
         session.sendUpstreamPacket(updateBlockPacket);
-        session.getItemFrameCache().remove(position, entityId);
+
+        session.getItemFrameCache().remove(bedrockPosition, this);
+
         valid = false;
         return true;
     }
@@ -192,16 +202,7 @@ public void updateBlock(GeyserSession session) {
      * @param session GeyserSession.
      * @return Java entity ID or -1 if not found.
      */
-    public static long getItemFrameEntityId(GeyserSession session, Vector3i position) {
-        return session.getItemFrameCache().getOrDefault(position, -1);
-    }
-
-    /**
-     * Force-remove from the position-to-ID map so it doesn't cause conflicts.
-     * @param session GeyserSession.
-     * @param position position of the removed item frame.
-     */
-    public static void removePosition(GeyserSession session, Vector3i position) {
-        session.getItemFrameCache().remove(position);
+    public static ItemFrameEntity getItemFrameEntity(GeyserSession session, Vector3i position) {
+        return session.getItemFrameCache().get(position);
     }
 }
diff --git a/connector/src/main/java/org/geysermc/connector/network/session/GeyserSession.java b/connector/src/main/java/org/geysermc/connector/network/session/GeyserSession.java
@@ -66,10 +66,7 @@
 import it.unimi.dsi.fastutil.longs.Long2ObjectMap;
 import it.unimi.dsi.fastutil.longs.Long2ObjectMaps;
 import it.unimi.dsi.fastutil.longs.Long2ObjectOpenHashMap;
-import it.unimi.dsi.fastutil.objects.Object2LongMap;
-import it.unimi.dsi.fastutil.objects.Object2LongOpenHashMap;
-import it.unimi.dsi.fastutil.objects.ObjectIterator;
-import it.unimi.dsi.fastutil.objects.ObjectOpenHashSet;
+import it.unimi.dsi.fastutil.objects.*;
 import lombok.AccessLevel;
 import lombok.Getter;
 import lombok.NonNull;
@@ -81,6 +78,7 @@
 import org.geysermc.connector.common.AuthType;
 import org.geysermc.connector.configuration.EmoteOffhandWorkaroundOption;
 import org.geysermc.connector.entity.Entity;
+import org.geysermc.connector.entity.ItemFrameEntity;
 import org.geysermc.connector.entity.Tickable;
 import org.geysermc.connector.entity.attribute.Attribute;
 import org.geysermc.connector.entity.attribute.AttributeType;
@@ -189,10 +187,10 @@ public class GeyserSession implements CommandSender {
     private final Long2ObjectMap<ClientboundMapItemDataPacket> storedMaps = Long2ObjectMaps.synchronize(new Long2ObjectOpenHashMap<>());
 
     /**
-     * A map of Vector3i positions to Java entity IDs.
+     * A map of Vector3i positions to Java entities.
      * Used for translating Bedrock block actions to Java entity actions.
      */
-    private final Object2LongMap<Vector3i> itemFrameCache = new Object2LongOpenHashMap<>();
+    private final Map<Vector3i, ItemFrameEntity> itemFrameCache = new Object2ObjectOpenHashMap<>();
 
     /**
      * Stores a list of all lectern locations and their block entity tags.

diff --git a/...org/geysermc/connector/network/translators/bedrock/BedrockBlockPickRequestTranslator.java b/...org/geysermc/connector/network/translators/bedrock/BedrockBlockPickRequestTranslator.java
@@ -27,6 +27,7 @@
 
 import com.nukkitx.math.vector.Vector3i;
 import com.nukkitx.protocol.bedrock.packet.BlockPickRequestPacket;
+import org.geysermc.connector.entity.ItemFrameEntity;
 import org.geysermc.connector.network.session.GeyserSession;
 import org.geysermc.connector.network.translators.PacketTranslator;
 import org.geysermc.connector.network.translators.Translator;
@@ -43,6 +44,18 @@ public void translate(BlockPickRequestPacket packet, GeyserSession session) {
 
         // Block is air - chunk caching is probably off
         if (blockToPick == BlockTranslator.JAVA_AIR_ID) {
+            // Check for an item frame since the client thinks that's a block when it's an entity in Java
+            ItemFrameEntity entity = ItemFrameEntity.getItemFrameEntity(session, packet.getBlockPosition());
+            if (entity != null) {
+                // Check to see if the item frame has an item in it first
+                if (entity.getHeldItem() != null && entity.getHeldItem().getId() != 0) {
+                    // Grab the item in the frame
+                    InventoryUtils.findOrCreateItem(session, entity.getHeldItem());
+                } else {
+                    // Grab the frame as the item
+                    InventoryUtils.findOrCreateItem(session, "minecraft:item_frame");
+                }
+            }
             return;
         }
 

diff --git a/...rg/geysermc/connector/network/translators/bedrock/BedrockEntityPickRequestTranslator.java b/...rg/geysermc/connector/network/translators/bedrock/BedrockEntityPickRequestTranslator.java
@@ -94,7 +94,7 @@ public void translate(EntityPickRequestPacket packet, GeyserSession session) {
                 break;
             case ARMOR_STAND:
             case END_CRYSTAL:
-            case ITEM_FRAME:
+            //case ITEM_FRAME: Not an entity in Bedrock Edition
             case MINECART:
             case PAINTING:
                 // No spawn egg, just an item

diff --git a/...geysermc/connector/network/translators/bedrock/BedrockInventoryTransactionTranslator.java b/...geysermc/connector/network/translators/bedrock/BedrockInventoryTransactionTranslator.java
@@ -126,16 +126,19 @@ public void translate(InventoryTransactionPacket packet, GeyserSession session)
                         }
 
                         // Bedrock sends block interact code for a Java entity so we send entity code back to Java
-                        if (session.getBlockTranslator().isItemFrame(packet.getBlockRuntimeId()) &&
-                                session.getEntityCache().getEntityByJavaId(ItemFrameEntity.getItemFrameEntityId(session, packet.getBlockPosition())) != null) {
-                            Vector3f vector = packet.getClickPosition();
-                            ClientPlayerInteractEntityPacket interactPacket = new ClientPlayerInteractEntityPacket((int) ItemFrameEntity.getItemFrameEntityId(session, packet.getBlockPosition()),
-                                    InteractAction.INTERACT, Hand.MAIN_HAND, session.isSneaking());
-                            ClientPlayerInteractEntityPacket interactAtPacket = new ClientPlayerInteractEntityPacket((int) ItemFrameEntity.getItemFrameEntityId(session, packet.getBlockPosition()),
-                                    InteractAction.INTERACT_AT, vector.getX(), vector.getY(), vector.getZ(), Hand.MAIN_HAND, session.isSneaking());
-                            session.sendDownstreamPacket(interactPacket);
-                            session.sendDownstreamPacket(interactAtPacket);
-                            break;
+                        if (session.getBlockTranslator().isItemFrame(packet.getBlockRuntimeId())) {
+                            Entity itemFrameEntity = ItemFrameEntity.getItemFrameEntity(session, packet.getBlockPosition());
+                            if (itemFrameEntity != null) {
+                                int entityId = (int) itemFrameEntity.getEntityId();
+                                Vector3f vector = packet.getClickPosition();
+                                ClientPlayerInteractEntityPacket interactPacket = new ClientPlayerInteractEntityPacket(entityId,
+                                        InteractAction.INTERACT, Hand.MAIN_HAND, session.isSneaking());
+                                ClientPlayerInteractEntityPacket interactAtPacket = new ClientPlayerInteractEntityPacket(entityId,
+                                        InteractAction.INTERACT_AT, vector.getX(), vector.getY(), vector.getZ(), Hand.MAIN_HAND, session.isSneaking());
+                                session.sendDownstreamPacket(interactPacket);
+                                session.sendDownstreamPacket(interactAtPacket);
+                                break;
+                            }
                         }
 
                         Vector3i blockPos = BlockUtils.getBlockPosition(packet.getBlockPosition(), packet.getBlockFace());
@@ -279,9 +282,10 @@ else if (packet.getItemInHand() != null && ItemRegistry.BUCKETS.contains(packet.
                         session.sendUpstreamPacket(blockBreakPacket);
                         session.setBreakingBlock(BlockTranslator.JAVA_AIR_ID);
 
-                        long frameEntityId = ItemFrameEntity.getItemFrameEntityId(session, packet.getBlockPosition());
-                        if (frameEntityId != -1 && session.getEntityCache().getEntityByJavaId(frameEntityId) != null) {
-                            ClientPlayerInteractEntityPacket attackPacket = new ClientPlayerInteractEntityPacket((int) frameEntityId, InteractAction.ATTACK, session.isSneaking());
+                        Entity itemFrameEntity = ItemFrameEntity.getItemFrameEntity(session, packet.getBlockPosition());
+                        if (itemFrameEntity != null) {
+                            ClientPlayerInteractEntityPacket attackPacket = new ClientPlayerInteractEntityPacket((int) itemFrameEntity.getEntityId(),
+                                    InteractAction.ATTACK, session.isSneaking());
                             session.sendDownstreamPacket(attackPacket);
                             break;
                         }

diff --git a/...rg/geysermc/connector/network/translators/bedrock/BedrockItemFrameDropItemTranslator.java b/...rg/geysermc/connector/network/translators/bedrock/BedrockItemFrameDropItemTranslator.java
@@ -29,6 +29,7 @@
 import com.github.steveice10.mc.protocol.data.game.entity.player.InteractAction;
 import com.github.steveice10.mc.protocol.packet.ingame.client.player.ClientPlayerInteractEntityPacket;
 import com.nukkitx.protocol.bedrock.packet.ItemFrameDropItemPacket;
+import org.geysermc.connector.entity.Entity;
 import org.geysermc.connector.entity.ItemFrameEntity;
 import org.geysermc.connector.network.session.GeyserSession;
 import org.geysermc.connector.network.translators.PacketTranslator;
@@ -44,9 +45,11 @@ public class BedrockItemFrameDropItemTranslator extends PacketTranslator<ItemFra
 
     @Override
     public void translate(ItemFrameDropItemPacket packet, GeyserSession session) {
-        ClientPlayerInteractEntityPacket interactPacket = new ClientPlayerInteractEntityPacket((int) ItemFrameEntity.getItemFrameEntityId(session, packet.getBlockPosition()),
-                InteractAction.ATTACK, Hand.MAIN_HAND, session.isSneaking());
-        session.sendDownstreamPacket(interactPacket);
+        Entity entity = ItemFrameEntity.getItemFrameEntity(session, packet.getBlockPosition());
+        if (entity != null) {
+            ClientPlayerInteractEntityPacket interactPacket = new ClientPlayerInteractEntityPacket((int) entity.getEntityId(),
+                    InteractAction.ATTACK, Hand.MAIN_HAND, session.isSneaking());
+            session.sendDownstreamPacket(interactPacket);
+        }
     }
-
 }
diff --git a/...geysermc/connector/network/translators/bedrock/entity/player/BedrockActionTranslator.java b/...geysermc/connector/network/translators/bedrock/entity/player/BedrockActionTranslator.java
@@ -216,9 +216,9 @@ public void translate(PlayerActionPacket packet, GeyserSession session) {
                 if (session.getGameMode() != GameMode.CREATIVE) {
                     // As of 1.16.210: item frame items are taken out here.
                     // Survival also sends START_BREAK, but by attaching our process here adventure mode also works
-                    long entityId = ItemFrameEntity.getItemFrameEntityId(session, packet.getBlockPosition());
-                    if (entityId != -1) {
-                        ClientPlayerInteractEntityPacket interactPacket = new ClientPlayerInteractEntityPacket((int) entityId,
+                    Entity itemFrameEntity = ItemFrameEntity.getItemFrameEntity(session, packet.getBlockPosition());
+                    if (itemFrameEntity != null) {
+                        ClientPlayerInteractEntityPacket interactPacket = new ClientPlayerInteractEntityPacket((int) itemFrameEntity.getEntityId(),
                                 InteractAction.ATTACK, Hand.MAIN_HAND, session.isSneaking());
                         session.sendDownstreamPacket(interactPacket);
                         break;

diff --git a/connector/src/main/java/org/geysermc/connector/utils/ChunkUtils.java b/connector/src/main/java/org/geysermc/connector/utils/ChunkUtils.java
@@ -47,7 +47,6 @@
 import lombok.Data;
 import lombok.experimental.UtilityClass;
 import org.geysermc.connector.GeyserConnector;
-import org.geysermc.connector.entity.Entity;
 import org.geysermc.connector.entity.ItemFrameEntity;
 import org.geysermc.connector.entity.player.SkullPlayerEntity;
 import org.geysermc.connector.network.session.GeyserSession;
@@ -329,21 +328,13 @@ public static void updateBlock(GeyserSession session, int blockState, Position p
      */
     public static void updateBlock(GeyserSession session, int blockState, Vector3i position) {
         // Checks for item frames so they aren't tripped up and removed
-        long frameEntityId = ItemFrameEntity.getItemFrameEntityId(session, position);
-        if (frameEntityId != -1) {
-            // TODO: Very occasionally the item frame doesn't sync up when destroyed
-            Entity entity = session.getEntityCache().getEntityByJavaId(frameEntityId);
-            if (blockState == JAVA_AIR_ID && entity != null) { // Item frame is still present and no block overrides that; refresh it
-                ((ItemFrameEntity) entity).updateBlock(session);
+        ItemFrameEntity itemFrameEntity = ItemFrameEntity.getItemFrameEntity(session, position);
+        if (itemFrameEntity != null) {
+            if (blockState == JAVA_AIR_ID) { // Item frame is still present and no block overrides that; refresh it
+                itemFrameEntity.updateBlock(session);
                 return;
             }
-
-            // Otherwise the item frame is gone
-            if (entity != null) {
-                session.getEntityCache().removeEntity(entity, false);
-            } else {
-                ItemFrameEntity.removePosition(session, position);
-            }
+            // Otherwise, let's still store our reference to the item frame, but let the new block take precedence for now
         }
 
         SkullPlayerEntity skull = session.getSkullCache().get(position);

diff --git a/connector/src/main/java/org/geysermc/connector/utils/InventoryUtils.java b/connector/src/main/java/org/geysermc/connector/utils/InventoryUtils.java
@@ -170,6 +170,60 @@ public static ItemData createUnusableSpaceBlock(String description) {
                 .tag(root.build()).build();
     }
 
+    /**
+     * See {@link #findOrCreateItem(GeyserSession, String)}. This is for finding a specified {@link ItemStack}.
+     *
+     * @param session the Bedrock client's session
+     * @param itemStack the item to try to find a match for. NBT will also be accounted for.
+     */
+    public static void findOrCreateItem(GeyserSession session, ItemStack itemStack) {
+        PlayerInventory inventory = session.getPlayerInventory();
+
+        if (itemStack == null || itemStack.getId() == 0) {
+            return;
+        }
+
+        // Check hotbar for item
+        for (int i = 36; i < 45; i++) {
+            GeyserItemStack geyserItem = inventory.getItem(i);
+            if (geyserItem.isEmpty()) {
+                continue;
+            }
+            // If this is the item we're looking for
+            if (geyserItem.getJavaId() == itemStack.getId() && Objects.equals(geyserItem.getNbt(), itemStack.getNbt())) {
+                setHotbarItem(session, i);
+                // Don't check inventory if item was in hotbar
+                return;
+            }
+        }
+
+        // Check inventory for item
+        for (int i = 9; i < 36; i++) {
+            GeyserItemStack geyserItem = inventory.getItem(i);
+            if (geyserItem.isEmpty()) {
+                continue;
+            }
+            // If this is the item we're looking for
+            if (geyserItem.getJavaId() == itemStack.getId() && Objects.equals(geyserItem.getNbt(), itemStack.getNbt())) {
+                ClientMoveItemToHotbarPacket packetToSend = new ClientMoveItemToHotbarPacket(i); // https://wiki.vg/Protocol#Pick_Item
+                session.sendDownstreamPacket(packetToSend);
+                return;
+            }
+        }
+
+        // If we still have not found the item, and we're in creative, ask for the item from the server.
+        if (session.getGameMode() == GameMode.CREATIVE) {
+            int slot = findEmptyHotbarSlot(inventory);
+
+            ClientCreativeInventoryActionPacket actionPacket = new ClientCreativeInventoryActionPacket(slot,
+                    itemStack);
+            if ((slot - 36) != inventory.getHeldItemSlot()) {
+                setHotbarItem(session, slot);
+            }
+            session.sendDownstreamPacket(actionPacket);
+        }
+    }
+
     /**
      * Attempt to find the specified item name in the session's inventory.
      * If it is found and in the hotbar, set the user's held item to that slot.
@@ -223,15 +277,7 @@ public static void findOrCreateItem(GeyserSession session, String itemName) {
 
         // If we still have not found the item, and we're in creative, ask for the item from the server.
         if (session.getGameMode() == GameMode.CREATIVE) {
-            int slot = inventory.getHeldItemSlot() + 36;
-            if (!inventory.getItemInHand().isEmpty()) { // Otherwise we should just use the current slot
-                for (int i = 36; i < 45; i++) {
-                    if (inventory.getItem(i).isEmpty()) {
-                        slot = i;
-                        break;
-                    }
-                }
-            }
+            int slot = findEmptyHotbarSlot(inventory);
 
             ItemEntry entry = ItemRegistry.getItemEntry(itemName);
             if (entry != null) {
@@ -247,6 +293,22 @@ public static void findOrCreateItem(GeyserSession session, String itemName) {
         }
     }
 
+    /**
+     * @return the first empty slot found in this inventory, or else the player's currently held slot.
+     */
+    private static int findEmptyHotbarSlot(PlayerInventory inventory) {
+        int slot = inventory.getHeldItemSlot() + 36;
+        if (!inventory.getItemInHand().isEmpty()) { // Otherwise we should just use the current slot
+            for (int i = 36; i < 45; i++) {
+                if (inventory.getItem(i).isEmpty()) {
+                    slot = i;
+                    break;
+                }
+            }
+        }
+        return slot;
+    }
+
     /**
      * Changes the held item slot to the specified slot
      * @param session GeyserSession