From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: Aikar Date: Thu, 28 May 2015 23:00:19 -0400 Subject: [PATCH] Handle Item Meta Inconsistencies First, Enchantment order would blow away seeing 2 items as the same, however the Client forces enchantment list in a certain order, as well as does the /enchant command. Anvils can insert it into forced order, causing 2 same items to be considered different. This change makes unhandled NBT Tags and Enchantments use a sorted tree map, so they will always be in a consistent order. Additionally, the old enchantment API was never updated when ItemMeta was added, resulting in 2 different ways to modify an items enchantments. For consistency, the old API methods now forward to use the ItemMeta API equivalents, and should deprecate the old API's. diff --git a/src/main/java/net/minecraft/world/item/ItemStack.java b/src/main/java/net/minecraft/world/item/ItemStack.java index 8335880dbbb31a3f5d10c259b567baa0d5f381a1..74e4990888d88f18cb3d800724d9156e9d1ae21d 100644 --- a/src/main/java/net/minecraft/world/item/ItemStack.java +++ b/src/main/java/net/minecraft/world/item/ItemStack.java @@ -12,6 +12,8 @@ import java.text.DecimalFormatSymbols; import java.util.Collection; import java.util.Iterator; import java.util.List; +import java.util.Collections; +import java.util.Comparator; import java.util.Locale; import java.util.Map.Entry; import java.util.Optional; @@ -151,6 +153,23 @@ public final class ItemStack { return this.getItem().getTooltipImage(this); } + // Paper start + private static final java.util.Comparator enchantSorter = java.util.Comparator.comparing(o -> o.getString("id")); + private void processEnchantOrder(@Nullable CompoundTag tag) { + if (tag == null || !tag.contains("Enchantments", 9)) { + return; + } + ListTag list = tag.getList("Enchantments", 10); + if (list.size() < 2) { + return; + } + try { + //noinspection unchecked + list.sort((Comparator) enchantSorter); // Paper + } catch (Exception ignored) {} + } + // Paper end + public ItemStack(ItemLike item) { this(item, 1); } @@ -194,6 +213,7 @@ public final class ItemStack { // CraftBukkit start - make defensive copy as this data may be coming from the save thread this.tag = nbttagcompound.getCompound("tag").copy(); // CraftBukkit end + this.processEnchantOrder(this.tag); // Paper this.getItem().verifyTagAfterLoad(this.tag); } @@ -748,6 +768,7 @@ public final class ItemStack { public void setTag(@Nullable CompoundTag nbt) { this.tag = nbt; + this.processEnchantOrder(this.tag); // Paper if (this.getItem().canBeDepleted()) { this.setDamageValue(this.getDamageValue()); } @@ -1055,6 +1076,7 @@ public final class ItemStack { ListTag nbttaglist = this.tag.getList("Enchantments", 10); nbttaglist.add(EnchantmentHelper.storeEnchantment(EnchantmentHelper.getEnchantmentId(enchantment), (byte) level)); + processEnchantOrder(this.tag); // Paper } public boolean isEnchanted() { diff --git a/src/main/java/org/bukkit/craftbukkit/inventory/CraftItemStack.java b/src/main/java/org/bukkit/craftbukkit/inventory/CraftItemStack.java index e88df908377450964ae5b6ff97ee97bd2f09c05f..ba70ac49222c517a38e20e86cee1fd38aecb6318 100644 --- a/src/main/java/org/bukkit/craftbukkit/inventory/CraftItemStack.java +++ b/src/main/java/org/bukkit/craftbukkit/inventory/CraftItemStack.java @@ -6,7 +6,6 @@ import java.util.Map; import net.minecraft.nbt.CompoundTag; import net.minecraft.nbt.ListTag; import net.minecraft.world.item.Item; -import net.minecraft.world.item.enchantment.EnchantmentHelper; import org.apache.commons.lang.Validate; import org.bukkit.Material; import org.bukkit.NamespacedKey; @@ -178,28 +177,11 @@ public final class CraftItemStack extends ItemStack { public void addUnsafeEnchantment(Enchantment ench, int level) { Validate.notNull(ench, "Cannot add null enchantment"); - if (!CraftItemStack.makeTag(this.handle)) { - return; - } - ListTag list = CraftItemStack.getEnchantmentList(this.handle); - if (list == null) { - list = new ListTag(); - this.handle.getTag().put(ENCHANTMENTS.NBT, list); - } - int size = list.size(); - - for (int i = 0; i < size; i++) { - CompoundTag tag = (CompoundTag) list.get(i); - String id = tag.getString(ENCHANTMENTS_ID.NBT); - if (ench.getKey().equals(NamespacedKey.fromString(id))) { - tag.putShort(ENCHANTMENTS_LVL.NBT, (short) level); - return; - } - } - CompoundTag tag = new CompoundTag(); - tag.putString(ENCHANTMENTS_ID.NBT, ench.getKey().toString()); - tag.putShort(ENCHANTMENTS_LVL.NBT, (short) level); - list.add(tag); + // Paper start - Replace whole method + final ItemMeta itemMeta = this.getItemMeta(); + itemMeta.addEnchant(ench, level, true); + this.setItemMeta(itemMeta); + // Paper end } static boolean makeTag(net.minecraft.world.item.ItemStack item) { @@ -216,66 +198,34 @@ public final class CraftItemStack extends ItemStack { @Override public boolean containsEnchantment(Enchantment ench) { - return this.getEnchantmentLevel(ench) > 0; + return this.hasItemMeta() && this.getItemMeta().hasEnchant(ench); // Paper - use meta } @Override public int getEnchantmentLevel(Enchantment ench) { - Validate.notNull(ench, "Cannot find null enchantment"); - if (this.handle == null) { - return 0; - } - return EnchantmentHelper.getItemEnchantmentLevel(CraftEnchantment.getRaw(ench), handle); + return this.hasItemMeta() ? this.getItemMeta().getEnchantLevel(ench) : 0; // Paper - replace entire method with meta } @Override public int removeEnchantment(Enchantment ench) { Validate.notNull(ench, "Cannot remove null enchantment"); - ListTag list = CraftItemStack.getEnchantmentList(this.handle), listCopy; - if (list == null) { - return 0; - } - int index = Integer.MIN_VALUE; - int level = Integer.MIN_VALUE; - int size = list.size(); - - for (int i = 0; i < size; i++) { - CompoundTag enchantment = (CompoundTag) list.get(i); - String id = enchantment.getString(ENCHANTMENTS_ID.NBT); - if (ench.getKey().equals(NamespacedKey.fromString(id))) { - index = i; - level = 0xffff & enchantment.getShort(ENCHANTMENTS_LVL.NBT); - break; - } - } - - if (index == Integer.MIN_VALUE) { - return 0; - } - if (size == 1) { - this.handle.getTag().remove(ENCHANTMENTS.NBT); - if (this.handle.getTag().isEmpty()) { - this.handle.setTag(null); - } - return level; - } - - // This is workaround for not having an index removal - listCopy = new ListTag(); - for (int i = 0; i < size; i++) { - if (i != index) { - listCopy.add(list.get(i)); - } + // Paper start - replace entire method + final ItemMeta itemMeta = this.getItemMeta(); + if (itemMeta == null) return 0; + int level = itemMeta.getEnchantLevel(ench); + if (level > 0) { + itemMeta.removeEnchant(ench); + this.setItemMeta(itemMeta); } - this.handle.getTag().put(ENCHANTMENTS.NBT, listCopy); + // Paper end return level; } @Override public Map getEnchantments() { - return CraftItemStack.getEnchantments(this.handle); + return this.hasItemMeta() ? this.getItemMeta().getEnchants() : ImmutableMap.of(); // Paper - use Item Meta } static Map getEnchantments(net.minecraft.world.item.ItemStack item) { diff --git a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaItem.java b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaItem.java index 39d2cd4acbce96cd7b9bd76bddf85b9b8729855a..51d37a8963a2f8f5381bf802e28e5dfcdcc27eaa 100644 --- a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaItem.java +++ b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaItem.java @@ -6,6 +6,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.LinkedHashMultimap; +import com.google.common.collect.ImmutableSortedMap; // Paper import com.google.common.collect.Lists; import com.google.common.collect.Multimap; import com.google.common.collect.SetMultimap; @@ -23,6 +24,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Base64; import java.util.Collection; +import java.util.Comparator; // Paper import java.util.EnumSet; import java.util.HashMap; import java.util.Iterator; @@ -33,6 +35,7 @@ import java.util.Map; import java.util.NoSuchElementException; import java.util.Objects; import java.util.Set; +import java.util.TreeMap; // Paper import java.util.logging.Level; import java.util.logging.Logger; import javax.annotation.Nonnull; @@ -272,7 +275,7 @@ class CraftMetaItem implements ItemMeta, Damageable, Repairable, BlockDataMeta { private List lore; // null and empty are two different states internally private Integer customModelData; private CompoundTag blockData; - private Map enchantments; + private EnchantmentMap enchantments; // Paper private Multimap attributeModifiers; private int repairCost; private int hideFlag; @@ -283,7 +286,7 @@ class CraftMetaItem implements ItemMeta, Damageable, Repairable, BlockDataMeta { private static final CraftPersistentDataTypeRegistry DATA_TYPE_REGISTRY = new CraftPersistentDataTypeRegistry(); private CompoundTag internalTag; - final Map unhandledTags = new HashMap(); // Visible for testing only + final Map unhandledTags = new TreeMap(); // Visible for testing only // Paper private CraftPersistentDataContainer persistentDataContainer = new CraftPersistentDataContainer(CraftMetaItem.DATA_TYPE_REGISTRY); private int version = CraftMagicNumbers.INSTANCE.getDataVersion(); // Internal use only @@ -304,7 +307,7 @@ class CraftMetaItem implements ItemMeta, Damageable, Repairable, BlockDataMeta { this.blockData = meta.blockData; if (meta.enchantments != null) { // Spigot - this.enchantments = new LinkedHashMap(meta.enchantments); + this.enchantments = new EnchantmentMap(meta.enchantments); // Paper } if (meta.hasAttributeModifiers()) { @@ -387,13 +390,13 @@ class CraftMetaItem implements ItemMeta, Damageable, Repairable, BlockDataMeta { } } - static Map buildEnchantments(CompoundTag tag, ItemMetaKey key) { + static EnchantmentMap buildEnchantments(CompoundTag tag, ItemMetaKey key) { // Paper if (!tag.contains(key.NBT)) { return null; } ListTag ench = tag.getList(key.NBT, CraftMagicNumbers.NBT.TAG_COMPOUND); - Map enchantments = new LinkedHashMap(ench.size()); + EnchantmentMap enchantments = new EnchantmentMap(); // Paper for (int i = 0; i < ench.size(); i++) { String id = ((CompoundTag) ench.get(i)).getString(ENCHANTMENTS_ID.NBT); @@ -546,13 +549,13 @@ class CraftMetaItem implements ItemMeta, Damageable, Repairable, BlockDataMeta { } } - static Map buildEnchantments(Map map, ItemMetaKey key) { + static EnchantmentMap buildEnchantments(Map map, ItemMetaKey key) { // Paper Map ench = SerializableMeta.getObject(Map.class, map, key.BUKKIT, true); if (ench == null) { return null; } - Map enchantments = new LinkedHashMap(ench.size()); + EnchantmentMap enchantments = new EnchantmentMap(); // Paper for (Map.Entry entry : ench.entrySet()) { // Doctor older enchants String enchantKey = entry.getKey().toString(); @@ -828,14 +831,14 @@ class CraftMetaItem implements ItemMeta, Damageable, Repairable, BlockDataMeta { @Override public Map getEnchants() { - return this.hasEnchants() ? ImmutableMap.copyOf(enchantments) : ImmutableMap.of(); + return this.hasEnchants() ? ImmutableSortedMap.copyOfSorted(this.enchantments) : ImmutableMap.of(); // Paper } @Override public boolean addEnchant(Enchantment ench, int level, boolean ignoreRestrictions) { Validate.notNull(ench, "Enchantment cannot be null"); if (this.enchantments == null) { - this.enchantments = new LinkedHashMap(4); + this.enchantments = new EnchantmentMap(); // Paper } if (ignoreRestrictions || level >= ench.getStartLevel() && level <= ench.getMaxLevel()) { @@ -1216,7 +1219,7 @@ class CraftMetaItem implements ItemMeta, Damageable, Repairable, BlockDataMeta { clone.customModelData = this.customModelData; clone.blockData = this.blockData; if (this.enchantments != null) { - clone.enchantments = new LinkedHashMap(this.enchantments); + clone.enchantments = new EnchantmentMap(this.enchantments); // Paper } if (this.hasAttributeModifiers()) { clone.attributeModifiers = LinkedHashMultimap.create(this.attributeModifiers); @@ -1451,4 +1454,22 @@ class CraftMetaItem implements ItemMeta, Damageable, Repairable, BlockDataMeta { return CraftMetaItem.HANDLED_TAGS; } } + + // Paper start + private static class EnchantmentMap extends TreeMap { + private EnchantmentMap(Map enchantments) { + this(); + putAll(enchantments); + } + + private EnchantmentMap() { + super(Comparator.comparing(o -> o.getKey().toString())); + } + + public EnchantmentMap clone() { + return (EnchantmentMap) super.clone(); + } + } + // Paper end + }