Attributes won't work in main hand if they were in offhand

Please do not mark unreleased versions as affected.
You don't have access to them yet.

--- I am a robot. This action was performed automatically.

While I have pinpointed where the issue occurs using MCP for 1.10, I'm not able to discern how to fix it.

Method net.minecraft.entity.LivingBase.onUpdate() contains a for loop cycling through each equipment slot, comparing the net.minecraft.entity.LivingBase.handInventory list (the old value) with the net.minecraft.entity.player.EntityPlayer.inventory list (what to replace handInventory with), which is what gets initially updated when swapping items.

If the old and new values are not equal, and if the old value did originally exist, then the attributes from that item are removed from the player. Then if the replacement item exists, the attributes from that item are added to the player.

But the cause of the bug is dependent on processing order of the equipment slots, in which the mainhand is processed first, followed by the offhand. Swapping the order simply swaps the bug around to occur when switching from mainhand to offhand, so that cannot be used to fix it.

Processing order when switching from offhand to mainhand:

1. The mainhand gets processed first; no old item in mainhand (since it was empty), new item is queued for mainhand (the item that was switched from offhand).

   1. Modifiers are not removed because the mainhand was initially empty.

   2. Since an item was queued for the mainhand, the modifiers from that item are added to the player.

2. The offhand gets processed next; there was an old item in offhand (the item that was switched to mainhand), no new item queued.

   1. Modifiers are removed because there was originally an item in the offhand, which is where the bug happens.

   2. The modifiers are not added back because there is no item in the queue for the offhand.

The result is the modifiers being removed since offhand is processed second, but not added back.

I should clarify that this occurs for all default items as well, not just custom modifiers. The only fix I can think of currently is to explicitly check if the new mainhand is equal to the old offhand to prevent removing the modifier, but that seems like a hack of a fix.

@@unknown what about having a local variable which stores the new item in the main hand. Then after the for loop finishes the attributes of this item are applied. Like you said for switching to the offhand this should not be a problem.

@@unknown Yep, that is a viable solution, though doing it just for the mainhand is what I meant by "hacky" since it would break if the processing order changes. In another report I had the following command running, which "fixed" the bug while it ran:

/testfor @a {}

Turns out that when saving the player to NBT, all modifiers from equipment are removed from the player and then re-applied after, which fixes the bug in this report since the missing modifiers were re-applied. The same operation could be done when re-evaluating modifiers, which would then be order-independent. That is, instead of removing a modifier if items are equal or not, remove it no matter what, and then re-apply using the new item in their equipment slot (even if it was the same item).

Example change, which I've confirmed does work regardless of processing order:

for (EntityEquipmentSlot entityequipmentslot : EntityEquipmentSlot.values())
{
    ItemStack itemstack;

    switch (entityequipmentslot.getSlotType())
    {
        case HAND:
            itemstack = this.handInventory[entityequipmentslot.getIndex()];
            break;

        case ARMOR:
            itemstack = this.armorArray[entityequipmentslot.getIndex()];
            break;

        default:
            continue;
    }
    
    // Added the following if statement. Remove modifiers if item exists.
    
    if (itemstack != null)
    {
        this.getAttributeMap().removeAttributeModifiers(itemstack.getAttributeModifiers(entityequipmentslot));
    }
    
    ItemStack itemstack1 = this.getItemStackFromSlot(entityequipmentslot);
    
    if (!ItemStack.areItemStacksEqual(itemstack1, itemstack))
    {
        ((WorldServer)this.worldObj).getEntityTracker().sendToAllTrackingEntity(this, new SPacketEntityEquipment(this.getEntityId(), entityequipmentslot, itemstack1));
        
        // Removed these, as they are the primary cause of the issue.
        
        //if (itemstack != null)
        //{
        //    this.getAttributeMap().removeAttributeModifiers(itemstack.getAttributeModifiers(entityequipmentslot));
        //}
        //
        //if (itemstack1 != null)
        //{
        //    this.getAttributeMap().applyAttributeModifiers(itemstack1.getAttributeModifiers(entityequipmentslot));
        //}

        switch (entityequipmentslot.getSlotType())
        {
            case HAND:
                this.handInventory[entityequipmentslot.getIndex()] = itemstack1 == null ? null : itemstack1.copy();
                break;

            case ARMOR:
                this.armorArray[entityequipmentslot.getIndex()] = itemstack1 == null ? null : itemstack1.copy();
        }
    }
    
    // Added the rest of the following. "getItemStackFromSlot()" (itemstack1) now represents the most up-to-date item.
    
    if (itemstack1 != null)
    {
        this.getAttributeMap().applyAttributeModifiers(itemstack1.getAttributeModifiers(entityequipmentslot));
    }
}

Can confirm for MC 1.12.1.

Can confirm in 1.18.2 and 22w15a.

Can confirm in 1.19.

Can confirm in 1.19.2.

Can confirm in 24w06a.

@@unknown Eight-year-old comment, but it seems to me that the "correct" solution would be to do attribute application in two passes. First, go over all the slots as you described, but only process item removals. Then go over all the slots a second time and process all the item additions.

Can confirm in 24w20a

this is especially annoying since enchantments use the attribute system now. I just realized that the bug ive found was not only related to the enchantements but to attributes themselves. (MC-272181)