Open blablubbabc opened 3 years ago
Thanks for the thorough report and analysis!
Paper, at least as of right now, does also attempt to sort the NMS enchantment order when loading an item. This however
StoredEnchantments
. While the API representation of the enchantment order (at least for normal enchantments) is ordered due to the EnchantmentMap
, should the NMS sorting logic be extended to StoredEnchantments
?
A not perfectly consistent enchantment order of the NMS stack should be fine tho, as no plugin should run equality checks on it anyway.
Isn't the API representation of the enchantment order through EnchantmentMap used to re-create the nms itemstacks NBT? so the ordering there would affect the nms order.
This has already been fixed upstream by using LinkedHashMaps instead of HashMaps in a bunch of places.
To be clear, its fixed for them, that fix won't apply to paper I don't think because we do different sorting.
As far as I can tell, the NMS enchantment order is purely based on insertion order. Paper actively sorts the enchantment list of the itemstack on load (in this patch).
The linked hashmap added by upstream will preserve the order existing in the NMS but that is about it. Paper on the other hand uses a TreeMap and (similarly to the NMS sorting) enforces alphabetical order in API representation.
I think a problem with auto sorting the stored enchantments, is the order has some influence over combining books, or putting enchants on items. It tries to go from top to bottom, and we are changing that order. What is the problem with just not using EnchantmentMap for stored enchants, and just go back to LinkedHashMaps?
EDIT: this also is a smaller diff, and won't change any items that already were sorted, they'll just preserve their existing order, and no more will be re-ordered (talking about stored enchants here)
I think a problem with auto sorting the stored enchantments, is the order has some influence over combining books, or putting enchants on items. It tries to go from top to bottom, and we are changing that order. What is the problem with just not using EnchantmentMap for stored enchants, and just go back to LinkedHashMaps?
I see no problem with that.
However, enchanted book items with differently ordered enchantments would then not be considered equal, similar to how normal items with differently ordered enchantments were not considered equal before Paper added its automatic reordering. But if the order of stored enchantments has an effect on game mechanics, I don't see a way for Paper to achieve both: The order of stored enchantments to not matter in some cases (item stack comparisons), but for it to matter in others (Minecraft applying the stored enchantments in an anvil).
I have briefly checked the anvil code to see how the order of the stored enchantments might affect the outcome when they are applied to some item in an anvil: As far as I can tell, the order is only relevant for checking if an enchantment to add is compatible with the already present enchantments on the item. The only case in which the order might affect the outcome is if the enchanted book item already contains enchantments that are incompatible with each other.
Especially since the current reordering also works completely randomly. Only when you move items in inventories, it sometimes happens that they also get them.
Expected behavior
The order of stored enchantments of enchanted books is either preserved, or consistently/alphabetically sorted similar to normal item enchantments (https://github.com/PaperMC/Paper/blob/master/patches/server/0062-Handle-Item-Meta-Inconsistencies.patch).
Observed/Actual behavior
In some cases, the stored enchantments are getting alphabetically sorted due to the above mentioned patch (the patch changed the 'buildEnchantments' methods to return a sorted Map, and this method is in some cases also used by CraftMetaEnchantedBook for its stored enchantments).
But there are also cases in which the stored enchantments of enchantment books are stored inside a non-ordered HashMap, which breaks this ordering again. One effect of this enchantment reordering is that comparisons between (Craft) items stacks with and without reordered stored enchantments may fail.
This is also an issue on Spigot: https://hub.spigotmc.org/jira/browse/SPIGOT-6716 However, compared to Spigot, since Paper changed the 'buildEnchantments' method to reorder the enchantments of items, this issue behaves slightly differently on Paper and affects enchanted book items with slightly differently ordered stored enchantments.
Steps/models to reproduce
1.) Create an enchanted book ItemStack with a particular order of its stored enchantments.
One option to create this item stack is to use the vanilla give command:
give @s minecraft:enchanted_book{StoredEnchantments:[{lvl:4s,id:"minecraft:bane_of_arthropods"},{lvl:3s,id:"minecraft:power"},{lvl:3s,id:"minecraft:piercing"}]} 1
Another option is to deserialize an enchanted book item stack with these stored enchantments:
During deserialization, the inner deserialized CraftMetaEnchantedBook will initially store the enchantments alphabetically in a sorted Map (see CraftMetaEnchantedBook(Map) constructor, which calls 'buildEnchantments', which Paper changed to return a sorted TreeMap). But during the deserialization of the outer ItemStack itself, this deserialized ItemMeta is copied (ItemStack#deserialize calls ItemStack#setItemMeta, which calls ItemStack#setItemMeta0, which ends up cloning the ItemMeta). CraftMetaEnchantedBook#clone transfers the enchantments from the sorted Map to a non-sorted HashMap. The enchantment order of this deserialized ItemStack will end up like the item given by the above give command (depends on the HashMap implementation though).
Another option is to create a Bukkit ItemStack via the Bukkit API and manually add the stored enchantments (see the below reproduction plugin): CraftMetaEnchantedBook#addStoredEnchantment will create a non-sorted HashMap to store the enchantments in.
2.) In some situations, the stored enchantments of this item are sorted.
One exemplary situation is when a plugin gets the ItemMeta of a CraftItemStack with the above unsorted enchantments order and applies it to another CraftItemStack (see the attached example plugin below): The CraftMetaEnchantedBook(NBTTagCompound) constructor invokes 'buildEnchantments', which ends up sorting the enchantments. Applying this ItemMeta to another CraftItemStack preserves this sorted enchantments order. The two items end up with differently sorted stored enchantments.
Another case is when a plugin deserializes only the ItemMeta of an enchanted book (not the whole ItemStack) and then applies it to a CraftItemStack: The deserialized ItemMeta ends up with alphabetically sorted stored enchantments, which is preserved when it is applied to a CraftItemStack.
The reverse situation is also possible: When a plugin applies the ItemMeta of a CraftItemStack that has a alphabetically sorted stored enchantments list to a Bukkit ItemStack, the Bukkit ItemStack will clone the passed ItemMeta, which results in a non-sorted enchantments order. Transfering this Bukkit ItemStack into the inventory of a player will preserve the unsorted enchantments order.
There are probably more situations one can come up with in which this back and forth conversion between CraftItemStacks and Bukkit ItemStacks / Bukkit ItemMeta can end up either sorting the enchantments, or mixing up the previously sorted enchantments.
3.) When comparing (via isSimilar or equals) two CraftItemStack (eg. two item stack retrieved from a player's inventory) with differently ordered stored enchantments, the item stacks are considered non-equal (see the example plugin below).
Plugin list
Only my testing plugin.
Paper version
This server is running Paper version git-Paper-196 (MC: 1.17.1) (Implementing API version 1.17.1-R0.1-SNAPSHOT) (Git: 59d449d)
Agreements
Other
Example reproduction plugin:
Right-clicking some block with an empty hand triggers the listener. Output:
The suggested fix is to replace all use of HashMap inside
CraftMetaEnchantedBook
with the sortedEnchantmentMap
implementation that is also used for normal item enchantments: https://github.com/PaperMC/Paper/blob/master/patches/server/0062-Handle-Item-Meta-Inconsistencies.patchEdit: As pointed out in the comments, to ensure a consistent enchantment order among all enchanted book items on the server and prevent the enchantment order to dynamically change when converting from/to Bukkit representations, one may also have to reorder the stored enchantments when NMS ItemStacks are loaded, similar to how how it is done for normal item enchantments via
processEnchantOrder
.