Fix /kill behaving differently to /minecraft:kill

[zp4rker]

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

[CyberFlameGO]

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

[zp4rker]

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.

[GrantGryczan]

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).

[GrantGryczan]

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]

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]

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.