Add player kill credit to beds/respawn anchors#10200
Conversation
|
Maybe a suggestion for improvement: how about adding the "igniter" into BlockExplodeEvent as well? |
|
This pr currently breaks BlockExplodeEvent since it only gets called when the entity in the explosion is null, I'll fix this soon. |
Are you sure? I did not look too much into the code, but aren't you mixing up the entity that exploded with the entity that caused the entity/block to explode? I think the one you have added is in the DamageSource... |
No, prior to the commit I just made, to differentiate whether to call an EntityExplodeEvent or BlockExplodeEvent, Explosion.java would just check if the source field is null, which doesn't work because it's getting set to the source of the explosion now (for beds and anchors). The source is the entity that caused the block to explode, which could probably be implemented differently but I think this is the cleanest. |
Ah you are right... The entity exploding (tnt/creeper) in that case is the entity that caused the explosion... Thank you! |
| private final List<Block> blocks; | ||
| private float yield; | ||
| private final org.bukkit.block.BlockState explodedBlockState; // Paper | ||
| + private final org.bukkit.entity.LivingEntity igniter; |
There was a problem hiding this comment.
// Paper - add player kill credit
| this.yield = yield; | ||
| this.cancel = false; | ||
| this.explodedBlockState = explodedBlockState; // Paper | ||
| + this.igniter = igniter; |
There was a problem hiding this comment.
// Paper - add player kill credit
|
Currently testing everything to make sure I can't find anything broken |
Beds now work properly Fixed the wrong type of LivingEntity in a check
lynxplay
left a comment
lynxplay
left a comment
There was a problem hiding this comment.
🎉 Welcome to paper, thank you for your first PR!
I have yet had time to test this or go through the rest of the affected code, but I added some general questions that popped into my mind on the first read-through.
The diff you need in Explosion is kind of ugly, depending on your answers to the questions we might have to look for a better way to implement this.
Manually checking for the damage source being bad_respawn_point seems not too future proof.
| Vec3 vec3d = explodedPos.getCenter(); | ||
|
|
||
| - world.explode((Entity) null, world.damageSources().badRespawnPointExplosion(vec3d, explodedBlockState), explosiondamagecalculator, vec3d, 5.0F, true, Level.ExplosionInteraction.BLOCK); // Paper - add exploded state | ||
| + world.explode(player, world.damageSources().badRespawnPointExplosion(vec3d, player, explodedBlockState), explosiondamagecalculator, vec3d, 5.0F, true, Level.ExplosionInteraction.BLOCK); // Paper - add exploded state, add player kill credit |
There was a problem hiding this comment.
What is the benefit of passing the player as the source here ?
You are already forwarding the "causing" player via the damage source.
Setting the source for the explosion to not null is what is responsible for the pretty ugly checks you have to do in Explosion now.
There was a problem hiding this comment.
You're right, I totally glossed over this when I was writing it, I'll fix it in my next commit and just pass the entity through the DamageSource.
| + // Paper start - add player kill credit | ||
| + public DamageSource(Holder<DamageType> type, Vec3 position, @Nullable Entity attacker) { | ||
| + this.type = type; | ||
| + this.causingEntity = attacker; |
There was a problem hiding this comment.
I am unsure how happy I am with hijacking this field.
Given neither paper nor spigot have a damage source API yet it might be fine, but this is changing vanilla behaviour and spigot behaviour.
Does anything stop us from going down the same path as the critical damage API and add a field + getter/setter for the "igniter" entity ref ?
There was a problem hiding this comment.
I think it makes more sense to just add a setter for the causingEntity field, since this is already used in LivingEntity's hurt method to determine kill credit, I can verify that this works later.
There was a problem hiding this comment.
I was more referring to the fact that other things might use the causingEntity field.
Changing that might break things we are not aware of and testing that everything still works fine is kind of hard.
There was a problem hiding this comment.
it's more the fact that "causingEntity" generally has a very explict meaning within the servers code and reusing this field is likely to cause some issues, if not now, maybe in the future; would much rather just add a field this rather than trying to hijack th emeaning of an exisitng one
There was a problem hiding this comment.
Wouldn't this just require more changes to hurt() in LivingEntity? It seems like all the existing logic in there would just have to be implemented twice, since the entity "causing" the event is still the person who ignited the block. I know that this avoids having unintended side effects with other parts of the code but I don't see a way it could be implemented cleanly without just mostly duplicating the logic. I'll do it if everyone thinks it's the best way, but I just want to put that out there.
There was a problem hiding this comment.
Well that is exactly the point tho.
Vanilla does not consider the "igniting entity" the causing entity of the damage.
This PR should not change this. The logic in LivingEntity#hurt acts on the causingEntity of the damage source, which should remain null for explosions from e.g. respawn anchors.
You are merely supposed to hijack the DamageSource to pass through the entity that ignited the block so that the API event can expose it.
Changing what vanilla considers the actual causingEntity is not a good idea, which is why cat and I suggested this to be moved to its own field so existing logic does not change.
There was a problem hiding this comment.
I see how I was misunderstanding, it's definitely better for plugin developers to implement this themselves, sorry for the confusion. I'm starting to think this change would be better to make upstream, so I might create a similar pr to spigot that only changes the API. I'll close this pr and will come back to it if I run into problems with spigot.
4 participants
Tracks the player who blew up a bed/respawn anchor so PlayerDeathEvent can be used to get the player who caused the death from the explosion.