DeathLog fails to detect player deaths on certain types of servers

[akemin-dayo]

I've been meaning to report this issue for… almost a year now, but things kept coming up and I kept forgetting l o l

※ Disclaimer: I haven't actually looked too closely at DeathLog's code to see how it's detecting player deaths, and also most of my Minecraft dev experience lies with server-side Bukkit plugins instead of client-side Fabric/Forge mods.

---

Description

The current mechanisms of detecting deaths that DeathLog is using (ServerPlayerEntityMixin and ClientPlayNetworkHandlerMixin) do not work with certain types of servers, notably old legacy servers put through some kind of protocol translator, like ViaVersion(ViaFabric)/VIAaaS/ViaProxy.

You can test this by connecting to something like the Glowstone Test Server (mc.glowstone.net, native protocol version 1.12.2), killing yourself, and seeing how DeathLog fails to detect the death event.

This is reproducible across all DeathLog versions from 0.2.10 (1.19.2〜1.19) all the way down to 0.1.2 (1.16.5〜1.16.2).

---

Proposed solution

I think it would be a good idea to consider adding an additional player death detection mechanism, and OR'ing it (not replacing it, see below as to why) with the current mechanism in order to get around this.

Specifically, since the death screen usually always appears upon death (the only time when it doesn't is if the doImmediateRespawn gamerule is set to true), hooking net.minecraft.client.gui.screens.DeathScreen would probably be a solution for detecting deaths that ServerPlayerEntityMixin fails to detect.

It would also probably be good to add a check for a HealthUpdateS2CPacket sent from the server with a value of 0, since this is another method of triggering deaths used by legacy servers.

It may also be worth taking a look at whatever DeathFinder (MPL2.0-licensed) is doing for its player death detection mechanism, since I know that definitely works for the servers above as I sometimes use it in combination with DeathLog as a workaround to at least get coordinates on the death screen when playing on these kind of servers. ;P (I haven't really looked at their code yet, though.)

[gliscowo]

While I can't investigate this closely right now, it does seem weird to me that protocol translators would break death detection. When on the client, DeathLog detects your death by virtue of the client receiving the DeathMessageS2CPacket. This is also the precise packet that displays the death message in chat and opens the death screen. To my knowledge, this is the only way the death screen is opened by the game - is there some other way I don't remember? In any case, detecting the death screen being opened might already be too late as the player's inventory is possibly already cleared. I'll look more into this later, but thanks for your detailed report Cheers

[akemin-dayo]

it does seem weird to me that protocol translators would break death detection

I'm fairly sure it's actually not the protocol translators themselves that are breaking them, but rather that the behaviour of older servers (or alternative server implementations) don't quite match the behaviour of a modern server.

For instance, I'm pretty sure that it's still possible to trigger player death even on modern clients with simply just a HealthUpdateS2CPacket that has a value of 0 — which is how legacy servers handled killing players. (※ While I am fairly confident about this, I am also going by memory here and haven't tested this or looked at a Wireshark packet dump recently ;P)

In such a case, if you're listening specifically only for a DeathMessageS2CPacket, you'll obviously never find one because the server never sent one to begin with (and the protocol translators wouldn't bother doing so either if it isn't necessary for vanilla clients).

[gliscowo]

I've now implemented an attempt at mitigating this. This build gives includes a new "Legacy Detection" button in the Death Log screen, which makes it also listen for a HealthUpdateS2CPacket with value 0. I've specifically opted to not detect the death screen itself because that would most likely cause false positives, especially when joining a server or world while already dead. Can you give this a try and report whether it fixes the problem?

libs.zip

[akemin-dayo]

I think you may have uploaded the incorrect build there (and there's no branch pertaining to it either so I can't just compile my own copy ;P)

The jar you attached contains only one change in DeathLogScreen (compared to upstream 0.2.10), which is the addition of this method:

    public boolean method_25403(double mouseX, double mouseY, int button, double deltaX, double deltaY) {
        return (super.method_25403(mouseX, mouseY, button, deltaX, deltaY) || this.deathList.method_25403(mouseX, mouseY, button, deltaX, deltaY));
    }

There is no such "Legacy Detection" button in the DeathLog UI or in the config file, nor can I find any code pertaining to HealthUpdateS2CPacket.

[gliscowo]

I did in fact troll myself here, you may use this build instead. And yeah I don't currently have the changes comitted as they'll probably just go straight to master once we have confirmed that they work libs-but-this-time-with-death-detection-i-promise.zip

[akemin-dayo]

Don't worry, it happens ;P

Anyway, I can confirm that the HealthUpdateS2CPacket detection method works as expected!

※ Tested with VIAaaS, ViaFabric, ViaProxy, and DirtMultiVersion across… a bunch of servers and protocol versions.

(Also, curious — will this change be backported to older Minecraft versions? Or will it just be for 1.19.x?)