Trinkets (Fabric)

Trinkets (Fabric)

22M Downloads

Modifiers only applied once

CammiePone opened this issue ยท 2 comments

commented

There appears to be a slight problem with the getModifiers() method with Trinkets: for the tooltip, it's calculated multiple times a second, but for the purposes of actually applying damage, it only runs once: whenever you take the trinket off or put it on. This means hotswapping code in this method also doesn't work without also removing and reequipping the trinket, because it just rolls with whatever it stored before.

This also means any kind of variably set modifiers (like for instance, the one I'm applying, where there's a damage increase if the player doesn't have a weapon in their hand) aren't possible while still retaining the tooltip.

	@Override
	public Multimap<EntityAttribute, EntityAttributeModifier> getModifiers(ItemStack stack, SlotReference slot, LivingEntity entity, UUID uuid) {
		boolean hasWeapon = entity.getMainHandStack().getAttributeModifiers(EquipmentSlot.MAINHAND).containsKey(EntityAttributes.GENERIC_ATTACK_DAMAGE);
		ImmutableMultimap.Builder<EntityAttribute, EntityAttributeModifier> builder = ImmutableMultimap.builder();

		if(!hasWeapon)
			builder.put(EntityAttributes.GENERIC_ATTACK_DAMAGE, new EntityAttributeModifier(DAMAGE_UUID, "Damage modifier", 6, EntityAttributeModifier.Operation.ADDITION));

		return builder.build();
	}```
commented

Just to clarify, I have tried everything short of not using the getModifiers() method. I have tried doing it via the constructor like most vanilla items do, I have tried it the way you see above, I have tried removing the attribute from the entity directly in the tick method, I have even tried removing it when damage is applied.

None of it worked. The only thing I can see making it work is setting and removing it in the tick method directly on the entity, and just creating my own tooltip.

commented

This is by design. The documentation of the method states that calls to this method should remain pure, though I realize now that this was not necessarily restrictive enough. There are a lot of issues with the approach of having extremely dynamic attributes, as comparing attributes is a pretty exhaustive operation, so the only time attribute mutation is allowed to happen is when an itemstack changes. This is a cheap comparison and ensures that data presented to the client is accurate per slot and isn't constantly changing. In your use case, you should be applying these EAMs manually, as you are not applying trinket attributes (you are using a hardcoded UUID instead of the one trinkets provides for you) and you have a simple comparison heuristic.

For now I will be closing this issue, but it will be put on my list of things to revise in the next pass over the codebase. I will need to profile the performance implications of full deep multimap comparison per living entity every tick, and if it's cheap enough, I will likely toss the concept of getModifiers remaining pure in respect to item stack and slot.