Incorrect damager returned for firework damage

[TonytheMacaroni]

trafficstars

Expected behavior

The EntityDamageByEntityEvent called for when a firework explodes and deals damage should return the firework for #getDamager.

Observed/Actual behavior

The owner of the exploding firework is used for #getDamager if it exists, and an EntityDamageEvent is fired instead if there is no owner.

Steps/models to reproduce

Fire a firework rocket, record the damager. Tested using this listener:

import org.bukkit.Bukkit;
import org.bukkit.event.Listener;
import org.bukkit.event.EventHandler;
import org.bukkit.event.entity.EntityDamageEvent;
import org.bukkit.event.entity.EntityDamageByEntityEvent;

public class TestListener implements Listener {

    @EventHandler
    public void onDamage(EntityDamageEvent event) {
        if (event instanceof EntityDamageByEntityEvent) return;
        Bukkit.broadcastMessage("Damage event:\n    %s".formatted(event.getEntity()));
    }

    @EventHandler
    public void onDamage(EntityDamageByEntityEvent event) {
        Bukkit.broadcastMessage("Damage by entity event:\n    %s\n    %s".formatted(event.getEntity(),
            event.getDamager()));
    }

}

Plugin and Datapack List

Just the test plugin.

Paper version

This server is running Paper version git-Paper-430 (MC: 1.20.4) (Implementing API version 1.20.4-R0.1-SNAPSHOT) (Git: 4939f87) You are running the latest version Previous version: git-Paper-424 (MC: 1.20.4)

Other

Caused by the upstream damage changes introduced in build 423 (#10242). The firework is properly returned as the damager in build 422. The issue is caused by a missing call to DamageSource#customCausingEntity in the following two lines of FireworkRocketEntity#dealExplosionDamage:

Line 242:

this.attachedToEntity.hurt(this.damageSources().fireworks(this, this.getOwner()), 5.0F + (float) (nbttaglist.size() * 2));

Line 269:

entityliving.hurt(this.damageSources().fireworks(this, this.getOwner()), f1);

[Machine-Maker]

This is probably gonna be caused by upstream's DamageSource API and the logic changes surrounding that.

[TonytheMacaroni]

This is probably gonna be caused by upstream's DamageSource API and the logic changes surrounding that.

Yes, the exact cause and fix is mentioned in the "Other" section. This issue isn't present in similar cases, such as TNT, because DamageSource#customCausingEntity is called to set a custom causing entity.

For example, line 106 of Explosion, inside its constructor:

this.damageSource = damageSource == null ? world.damageSources().explosion(this).customCausingEntity(entity) : damageSource.customCausingEntity(entity); // CraftBukkit - handle source entity

Although on second thought, their approach just ends up basically overriding the causing entity in the damage source? Not sure why they didn't do something similar but with just setting/using the direct entity. For the other usages of DamageSource#customCausingEntity the causing/direct entity aren't set anyway, but they are for explosions.

[Doc94]

The customCausingEntity is used in a priority check for try to get damager (if not use the vanilla damager) this was for avoid unexpected behaviours (like mention MM in the Upstream PR)

i still dont check the code but maybe the issue is more the CraftEventFactory where is is set the damager in the event... if you check the DamageSource in the event in teory the entities are correct.

[TonytheMacaroni]

Damage types with the is_explosion tag are handled differently in CraftEventFactory, where the code only uses the causing entity. The rest of the code uses the direct entity if one exists. So, I guess a proper fix would actually be to use the direct entity for explosions as well. That still leaves explosions overriding an already existing causing entity, though. You can see that if you ignite a piece of TNT and check DamageSource#getCausingEntity, which should return the igniter, but returns the TNT instead because of the usage of customCausingEntity.

[Doc94]

the direct entity is used in projectile and that was the behaviour previous the DamageSource PR

[TonytheMacaroni]

the direct entity is used in projectile and that was the behaviour previous the DamageSource PR

Not exactly sure what you mean by that? Damage from firework rockets trigger this part of CraftEventFactory: https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/browse/src/main/java/org/bukkit/craftbukkit/event/CraftEventFactory.java#941-948 Which doesn't check the direct entity. It reaches here and not the later projectile stuff because the firework damage type has the is_explosion tag.

[Doc94]

okay i think now understand what happen... For my side i need test another danages for make sure a few behaviours but looks need replicate the same logic than projectile for check if is indirect for use the direct entity and not the causing in that case... Buy wanna test another damage for remenber why in first place i apply the is indirect logic only in projectile and not in all cases.

[quiquelhappy]

was this fixed by https://github.com/PaperMC/Paper/pull/10307?

[lynxplay]

Indeed.