Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix /kill behaving differently to /minecraft:kill #5850

Open
wants to merge 5 commits into
base: 2.x
Choose a base branch
from

Conversation

zp4rker
Copy link

@zp4rker zp4rker commented Jun 30, 2024

Information

This PR fixes #5842.

Details

Proposed fix:

  • Use Damageable#damage(double) instead of Damageable#setHealth(double)
  • Use Float.MAX_VALUE as that is what /minecraft:kill uses

Environments tested:

OS: Windows 11

Java version: Corretto-21.0.3.9.1

  • Most recent Paper version (1.20.6-147-ver/1.20.6@e41d44f)
  • CraftBukkit/Spigot/Paper 1.12.2 (Just tested successful killing, not trigger of advancement)
  • CraftBukkit 1.8.8 (Just tested successful killing, not trigger of advancement)

Demonstration:
Triggers minecraft:entity_hurt_player which was previously not being triggered as mentioned in #5842
image

@CyberFlameGO
Copy link

CyberFlameGO commented Jul 1, 2024

I thought it intentionally was different to vanilla behavior - this change would also break /kill bypassing god mode iirc

@zp4rker
Copy link
Author

zp4rker commented Jul 1, 2024

Thanks for pointing that out, it does indeed break when in god mode. If older versions had a damage cause, could just use that but doesn't seem they do. Other solution I can think of is to check for Float.MAX_VALUE since I can't think of any example of something other than /kill using that value.

- Remove phantom EntityDamageEvent
- Check if `/kill` command used by checking if `getDamage() == Float.MAX_VALUE`
@GrantGryczan
Copy link

GrantGryczan commented Jul 11, 2024

Unlike /kill in vanilla, I just found that this makes /kill no longer work on creative players. The source damage type should be minecraft:generic_kill (not minecraft:generic, Spigot's default) in order to affect creative players. Unfortunately Spigot's API started letting you set the damage source only recently (I believe as of a Spigot version for MC 1.20.4, but not the first so this fix might have to only apply to 1.20.5+ to be completely safe), but I think it's fine if #5842 is only fixed for newer Minecraft versions (1.20.5+).

Here's how another plugin implemented this fix: ChanceSD/PvPManager@aa358e9

In general, the condition vanilla Minecraft uses to check if an invulnerable player should be damaged is whether the source damage type is in the minecraft:bypasses_invulnerability damage type tag (which includes minecraft:generic_kill by default). EssentialsX god mode should check that same condition--it's another data-pack-breaking bug that it doesn't already, since data packs can create their own damage types and add them to the minecraft:bypasses_invulnerability tag.

Might want to apply this to /suicide too. minecraft:out_of_world is also a damage type that bypasses invulnerability if you want to use that instead for the sake of its death message. And if you don't want to use a vanilla damage type, then any custom instant-kill damage types should be added to the appropriate damage type tags such as minecraft:bypasses_invulnerability (look at all of the damage type tags that minecraft:generic_kill is a part of).

@@ -29,12 +28,7 @@ protected void updatePlayer(final Server server, final CommandSource sender, fin
if (sender.isPlayer() && user.isAuthorized("essentials.kill.exempt") && !ess.getUser(sender.getPlayer()).isAuthorized("essentials.kill.force")) {
throw new PlayerExemptException("killExempt", matchPlayer.getDisplayName());
}
final EntityDamageEvent ede = ess.getDamageEventProvider().callDamageEvent(matchPlayer, sender.isPlayer() && sender.getPlayer().getName().equals(matchPlayer.getName()) ? EntityDamageEvent.DamageCause.SUICIDE : EntityDamageEvent.DamageCause.CUSTOM, Float.MAX_VALUE);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the damage sources here and all the cancellation and permission checks below definitely isn't the way to go. Please see my previous comment on the PR.

@zp4rker
Copy link
Author

zp4rker commented Jul 17, 2024

Thanks for sharing! I did miss those things. I wonder how legacy versions of MC implemented the vanilla kill command to account for invulnerability 🤔. Might take a look at that. I'm sorry I haven't addressed this earlier, have been absolutely battered at work recently.

@zp4rker
Copy link
Author

zp4rker commented Jul 29, 2024

It seems that all Minecraft versions use a damage source from 1.8 onwards to account for invulnerability. Just to satisfy my curiosity and also to get back into touch with plugin dev and nms work I decided to make a version agnostic simple kill command (basically just suicide) @ https://github.com/zp4rker/bukkitplayground/blob/kill_command/src/main/java/com/zp4rker/bukkitplayground/commands/CmdKill.java (warning: it's a little messy. Tested on Paper 1.8-1.21, haven't tested on Spigot yet but in theory should work)

I could probably implement this into a reflection provider but I don't know if I'm missing some better way to do this.

@mdcfe mdcfe self-requested a review July 29, 2024 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

/kill doesn't call minecraft:entity_hurt_player advancement trigger (breaking data packs)
3 participants