EntityAttributeMixin and LivingEntityMixin will double-modify attributes

[nightpool]

Hi, I have the following power defined:

{
  "name": "Swiftly",
  "type": "origins:modify_attribute",
  "attribute": "minecraft:generic.movement_speed",
  "modifier": {
    "operation": "add_base_early",
    "value": 1
  }
}

(simplified for ease of reproduction)

I would expect the resulting movement speed for the player to be 1.1 (.1 base, 1 additional)

However, calling /attribute @p minecraft:generic.movement_speed get in the console, I can see that my resulting speed is actually 2.1

This only happens for attributes that have a EntityAttributeInstance value. This is because LivingEntity#getAttributeValue will call AttributeMap#getValue, which will return the instance modified value if it exists, or the base value if the instance does not yet exist. This means that PowerHolderComponent,modify gets called twice, once by EntityAttributeInstanceMixin and once by LivingEntityMixin (taking the already-modified double as the base value).

However, to make this more confusing, this only happens when code has explicitly called getAttributeInstance (which setSprinting does), it doesn't happen when AttributeInstances are generated through other means (such as method_30129 which calls addTemporaryModifiers to create EntityAttributeInstances for attackSpeed and damage), because in that case the entity field never gets set on EntityAttributeInstanceMixin (since it's created directly by the AttributeContainer instead of by the LivingEntity).

The right solution here seems to be to set entity on AttributeContainer, so that it can correctly hook in to all of the right places, and do all of the calculations in a single place rather then doing them both in getAttributeValue and in Entity#getValue.

Additionally it would probably be helpful to mix the AttributeModifications from Origins in with the standard minecraft ones, so that they all take place on the same level rather then being their own separate system, which is kinda confusing. Right now, if a user has a speed pot (20% MULTIPLY_TOTAL) and an add_base_early increase of, say, .2 units, the order of operations would be the opposite of what the user expects (the 20% increase would apply to only the vanilla base value, not the add_base_early "base" value)

[eggohito]

Thanks for the detailed report! I somehow missed that those two would overlap. I think I have an idea on how to merge the vanilla and custom modifiers to preserve the order, but it would have to override how the value is computed in EntityAttributeInstance#computeValue (compatibility shouldn't be a problem with MixinExtras though!)