Fix #1165 crash from item tooltip with duplicate enchantments

[BlueAgent]

Fixes #1165.

Removing Integer from the map and assigning to an int was causing a null pointer exception because of the implicit unboxing to a value type.

Feel free to close this PR if you already have a WIP fix or a preferred way to implement the fix. You also have push access to this PR's branch if that is preferred.

[Shadows-of-Fire]

The proper fix should really just be to strip duplicates, either here or earlier - duplicate enchantments are generally invalid as they have to resolve to a Map<Enchantment, Integer> which forces use of the last copy of an enchantment found in the list.

[BlueAgent]

That is some interesting behaviour. Made it only show the last duplicate enchantment. Since, as you mentioned, that is the one that ends up actually being used.

Tested that the enchantment order is as expected using the following items:

Mending, Sharpness III:
/give @s diamond_sword{Enchantments:[{lvl:1s,id:"minecraft:mending"},{lvl:1s,id:"minecraft:sharpness"},{lvl:3s,id:"minecraft:sharpness"}]}
/give @s diamond_sword{Enchantments:[{lvl:1s,id:"minecraft:sharpness"},{lvl:1s,id:"minecraft:mending"},{lvl:3s,id:"minecraft:sharpness"}]}

Sharpness III, Mending:
/give @s diamond_sword{Enchantments:[{lvl:1s,id:"minecraft:sharpness"},{lvl:3s,id:"minecraft:sharpness"},{lvl:1s,id:"minecraft:mending"}]}

[Shadows-of-Fire]

Actually (and this is even more annoying) the behavior is inconsistent: The "get the level of a single enchantment" method EnchantmentHelper.getTagEnchantmentLevel returns the level of the first duplicate enchantment, while the "get the levels of all enchantments" method EnchantmentHelper.deserializeEnchantments returns the level of the last duplicate.

[BlueAgent]

Oh geez that's not a very good inconsistency. Both those methods seem to be used for various gameplay affecting features too. What should we do about it?

[Shadows-of-Fire]

Honestly given that the performance of getTagEnchantmentLevel is already O(n), it may be better to just mixin that method to call deserializeEnchantments and then grab the relevant one so it always uses the last one. Then we can just always use the last one in the tooltip.

[BlueAgent]

Seems like getTagEnchantmentLevel is a method forge added so required remap = false.

Went with an overwrite since I couldn't think of any alternative mixin that isn't effectively an overwrite anyway.

Tested with this pickaxe on 64 diamond ore to make sure it dropped around 2.2 (actual dropped (64+64+9)/64 ~= 2.14 in dev and (64+64+14)/64 ~= 2.22 live):

/give @s diamond_pickaxe{Enchantments:[{lvl:1s,id:"minecraft:fortune"},{lvl:1s,id:"minecraft:mending"},{lvl:3s,id:"minecraft:fortune"}]}

[SiverDX]

can't you just reverse the loop Avoids unneeded iterations and is probably more compatible with other mixins, since it only changes the loop condition (no other things like variable names, returns etc.)

   public static int getTagEnchantmentLevel(Enchantment pEnchantment, ItemStack pStack) {
      if (pStack.isEmpty()) {
         return 0;
      } else {
         ResourceLocation resourcelocation = getEnchantmentId(pEnchantment);
         ListTag listtag = pStack.getEnchantmentTags();

         for (/* Return last relevant enchantment */ int i = listtag.size() - 1; i >= 0; i--) {
            CompoundTag compoundtag = listtag.getCompound(i);
            ResourceLocation resourcelocation1 = getEnchantmentId(compoundtag);
            if (resourcelocation1 != null && resourcelocation1.equals(resourcelocation)) {
               return getEnchantmentLevel(compoundtag);
            }
         }

         return 0;
      }
   }

[BlueAgent]

I'm not sure how to target the loop condition with a mixin but I'll investigate doing that later today. I like the idea of reversing the loop condition though, that's way better!

[Shadows-of-Fire]

Modifying control flow (i.e. modifying the loop) is not a task that mixin is good at, so it would still need an overwrite that way.

I would probably prefer just making the individual level method call the map method, and then query the map. It's a bit more wasteful memory wise, though I could probably put together a CachedObject for it to reduce that overhead.

[SiverDX]

    @ModifyArg(method = "getTagEnchantmentLevel", at = @At(value = "INVOKE", target = "Lnet/minecraft/nbt/ListTag;getCompound(I)Lnet/minecraft/nbt/CompoundTag;"), remap = false)
    private static int apotheosis$reverseLoop(int index, @Local final ListTag tags) {
        return tags.size() - 1 - index;
    }

since mixinextras is present in anymodpack from 1.19.2 and 1.20 anyway could just add it

[BlueAgent]

I really like that solution using Mixin Extras that @SiverDX suggested but I really wanted to try modifying the loop order somehow as an exercise. Will switch to using the Mixin Extras solution.

[BlueAgent]

Okay so I've implemented the suggested mixin by @SiverDX.

I needed to add remap = true to the @At like:

@At(value = "INVOKE", target = "Lnet/minecraft/nbt/ListTag;getCompound(I)Lnet/minecraft/nbt/CompoundTag;")
@At(value = "INVOKE", target = "Lnet/minecraft/nbt/ListTag;getCompound(I)Lnet/minecraft/nbt/CompoundTag;", remap = true)

Otherwise the mixin would not create a refmap for it (and was not working in a live environment, but worked fine in-dev). Which is weird because the default value for @At.remap is true... But yes I tested it without setting it to true and my guess is mixin does some kinda inheritence for the remap value.

Also had to do some weird shenanigans in the build script to get it to use the normal jar location for the output of the jar-in-jar system and also make gradle happy. Gradle was complaining about dependencies not being explicit.

Lastly, I updated the imports in files to match the existing style.

Hopefully this all looks good to you two.

[Shadows-of-Fire]

Opted for a vanilla mixin redirect using the same approach