Reducing damage of an EntityDamageEvent allows future damage to bypass NoDamageTicks partially

[D1SR3G4RD]

Expected behavior

This issue applies to all damage types, but is specifically more noticeable in damage types that attempt to deal damage each tick (lava, cactus), so i will use lava as an example.

When standing in lava, I expect armor to take damage every 10 ticks as it does in vanilla, regardless of the amount of damage the lava deals.

Observed/Actual behavior

Armor takes damage and lava burn sounds are played every tick.

Steps/models to reproduce

1. Create a plugin with the following handler in a Listener: @EventHandler public void onLava(EntityDamageEvent event) { if (event.getCause() == EntityDamageEvent.DamageCause.LAVA) { event.setDamage(event.getDamage() * .99); } }
2. Run the server and equip armor.
3. Stand in lava.
4. Observe your armor taking damage each tick, instead of every 10th tick.

Plugin and Datapack List

Nothing except this listener.

Paper version

This server is running Silvera version 1.21.1-DEV-main@9d0a210 (2024-09-13T02:50:33Z) (Implementing API version 1.21.1-R0.1-SNAPSHOT)

This is a private fork, but is based off of latest and has no related changes, several other servers have run into this issue, presumably on other forks or standard Paper. I can replicate it on latest if necessary.

Other

This appears to be the source of the issue. https://github.com/PaperMC/Paper/pull/11045/commits/d98db3945a76eb6203062c7ccfa2a71bce22f4e4

This is caused by amount being set to a lower value before lastHurt is assigned to this value.
For example, lava deals 4 damage, but if this is reduced to 3 damage, lastHurt will be 3 on the next tick, but amount will be 4, resulting in the if case failing and damage attempting to be dealt.

[D1SR3G4RD]

Presumably something like this should work, but unsure what other issues this could create if the damage actually dealt and CraftLivingEntity.getLastDamage are not the same. this.lastHurt = Math.max(amount, originalAmount);

[strainxx]

Can reproduce

[Doc94]

Confirmed and in teory the issue is https://github.com/PaperMC/Paper/blob/master/patches/server/1028-Only-call-EntityDamageEvents-before-actuallyHurt.patch because in upstream this not happen. but really unsure what is a proper fix

[lynxplay]

Well this is a bit rough. The behaviour is what you'd expect from vanilla when the damage event is interpreted as "This entity is about to take n points of damage".

Idk if there is a point in potentially exposing a "pls re-check if the damage should still be dealt after damage modification" option to the entity damage event, but the alternative would be to either call entity damage event before that check, which obviously leads to completely wrong calls for the event, or to set the lastDamageAmount to the wrong value, which is also incorrect.

Such a "pls recheck" option cannot be true by default either, because it would break a plugin just setting the value to something less but still expecting the damage to be dealt.

[okx-code]

@lynxplay Just run into this issue myself, perhaps a PreEntityDamageEvent could be added to mean an event where damage can be modified, but it's not guaranteed to be actually applied? It would require some work from the plugins to fix this, but it also wouldn't break any compatibility.

[Leymooo]

We also run into this issue (1.21.4).

Our case is reducing any incoming damage by some %, and this issue makes armor to break very very fast, and incoming damage is also very high

  @EventHandler
  public void onEntityDamage(EntityDamageEvent event) {
    if (event.getEntity() instanceof Player player) {
      event.setDamage(event.getDamage() * 0.85);
    }
  }

Result is: Paper: https://github.com/user-attachments/assets/fec98e08-d0c1-4a2d-b75d-6ddaa7c69b57

Expected (on vanilla): https://github.com/user-attachments/assets/d4e1b53d-85e5-415e-95d2-ac06bebb5fa2

[lynxplay]

Yea, I really have no idea how we'd properly solve this. The temporary hack we could look for is setting the lastHurt value to the Math.max(originalDamage, postEventDamage) and call it a day. That would lead us with technically incorrect state, but better than everyones plugins breaking.

[okx-code]

Can we close this now?

[lynxplay]

No, this is still an issue.

[okx-code]

The armour damage issue was fixed here, https://github.com/PaperMC/Paper/pull/12190/files#diff-3991edc9191addaab9f9701589f8fb4a05001c6f3c9ce1f51d22001f8de805caR1126

I tried reproducing the issue with the code provided and I couldn't reproduce on build #185 which was this PR, while I could reproduce on build #184.