Fixed NoSuchElementException if threshold is negative

[FlorianMichael]

This PullRequest fixes a NoSuchElementException if the server sends a negative threshold in the compression, you can test it yourself:

1. start Krypton
2. join "raphimc.net".

Without my fix you will be kicked for a NoSuchElementException.

[Kichura]

This seems to fail upon compile attempt as both PacketInflater and PacketDeflater are marked as symbols that cannot be found.

[FlorianMichael]

These are Minecraft classes, how can you not have them?

[astei]

That's presumably because he just pulled the logic from the decompiled code. Even if the correct imports were added, this would still be broken because it doesn't account for Krypton outright replacing the handlers.

[FlorianMichael]

Fixed @Kichura

[FlorianMichael]

@astei You could also check if the handlers are null

[FlorianMichael]

@astei Why didn't you merged my PR, it's simpler than the current code.

[astei]

Fixed in 80720b286b66b3b0506b8454d2c3264f7bdbea09, the null check isn't even necessary as it's implied in the instanceof checks.

what exactly is wrong about it? it simulates the vanilla-like behaviour and when you don't follow or at least try simulating the vanilla behaviour everything breaks down and everything gets detectable server-side

You don't seem to understand that Krypton is already detectable on the server side, and that this change does nothing but fix a variance from vanilla, and a painfully minor one too. The goal of this mod is wire compatibility, not to be 100% undetectable.

[astei]

@FlorianMichael I am the author of this mod and I decide what code goes in.

Your fix was only partially correct, since it did not account for the fact Krypton uses entirely different compression handlers.

[FlorianMichael]

Can you add compactibility for ViaFabricPlus for compression?

[astei]

I'll entertain adding a pipeline event for reordering that you can listen to, but not calling out to any mods. The approach I took for ViaFabric was a mistake I'm not going to repeat

[FlorianMichael]

I won't do it, but okay, thank you for your time

[astei]

5896b6064b8e5ed24da583a823c884f11d8f4b79 has the events you need

[FlorianMichael]

I don't see why I should implement this, Krypton overwrites the whole compression and not me, you could just add a second event handler or modify the Minecraft elements and not replace them.

[astei]

I don't think you understand that I did.

I added a generic event handler that any mod can listen to know when Krypton has rearranged the compression handlers. At some point I'm going to get rid of the ViaFabric-specific one as well.

[FlorianMichael]

Can't you just drag the ViaFabric stuff into its own class then I can inject in there, I don't want to add Krypton as a dependency because that's just not my job as a mod to fix that, Krypton ends up overwriting everything and not me.

[astei]

Why would you prefer that? Mind you, "moving it into its own class" still introduces a dependency on Krypton, and it's arguably a worse design choice than just emitting an event through the pipeline, since you have to introduce a mixin that only applies when Krypton is in use, which necessarily implies Krypton as an optional dependency (so that Krypton can be loaded and present before your mod loads, too). You already inject into ClientConnection, after all, you can go the extra step and inject into userEventTriggered().

Here, you can use some reflection to see if the Krypton event class exists and extract the compression enabled/disabled events.

I'm really not sure what the problem with this approach is. I've provided a solution that you should be able to work with. If I'm missing something, please go into detail.

[FlorianMichael]

I find a pseudo mixin like I did in Sodium just nicer than having random reflection code that has to check for something, especially because my mod is not the problem. But okay

[FlorianMichael]

When will the new changes be uploaded to modrinth?

[astei]

At some point in the future. I'm not in a rush to cut a new release given that 0.2.3 currently has very little of note.

[FlorianMichael]

Looks like the API will work without reflection, thanks, but I'm pretty sure that with isKryptonOrVanillaDecompressor and isKryptonOrVanillaCompressor the vanilla check is unnecessary.

[astei]

It can't hurt, though, in case there's a mod that tries to undo what Krypton does...