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

Remove most-times unnecessary client reconfiguration #1477

Draft
wants to merge 3 commits into
base: dev/3.0.0
Choose a base branch
from

Conversation

valaphee
Copy link

@valaphee valaphee commented Dec 17, 2024

This removes the configuration phase, as it seems that the client sometimes doesn't transition correctly

This PR is not intended to be merged yet, and is just a quick fix for server owners, as it completely ignores the possibility of servers with different registries and data packs which will cause problems. (and is made so that the diff is as small as possible)

Because there is no config phase, https://bugs.mojang.com/browse/MC-272506 should also not occur anymore

@CullanP
Copy link

CullanP commented Dec 24, 2024

This also happens on our network so we were looking for solutions, there seems to be random protocol error kicks on 1.21+ client with viaversion and equipment packets, you also cannot seem to join 1.8 servers using this without getting kicked for protocol network errors. This could just be related to the 1.8 server issue as well, the equipment packet issue is the only one I've seen so far at least when using 1.21 server version. I can't say if this issue is specifically velocity related yet, but we're large enough to notice it with some players getting stuck on dirt screen and none of our plugins mess with config phase so it's not a plugin issue. The issue with equipment packets is odd because it sometimes happens like 5 seconds after joining, while others are instant, does not happen on our live proxy without this patch, so something must be causing kicks when using this patch. This patch working would be a huge help to us since this has been an ongoing issue for months

Update: I've found a way to reproduce a separate kick, on base 1.21 server if you're on 1.21.1 and have amethyst armor trims in a chest and then open it while on this patch it will kick you for decoding issue with container_set_content, this did not happen when i recompiled regular velocity without this patch, so it's not only joins, there's just a weird kick issue in general with this patch, i'd assume the 1.8 server issue is related to this same issue

@valaphee
Copy link
Author

valaphee commented Dec 27, 2024

The only problem I've seen with set_equipment packet was that we forgot to replace item ids in the SpawnPlayer Packet, which gets split by ViaVersion into 2 Packets SetEquipment, and SpawnPlayer.

This patch addresses issues I've had with 1.21 clients getting stuck in the "Reconfiguring..." screen while switching between servers. They are pretty rare though, like 50 kicks á probably 1000 joins.

Yep, I'll update this PR to send the correct known packs list.

@valaphee
Copy link
Author

valaphee commented Dec 30, 2024

The reported known packs should be more accurate

@CullanP
Copy link

CullanP commented Dec 30, 2024

Opening a chest with amethyst armor trims (probably every trim didn't check others) kicks you for container_set_content every time, this is the primary issue that's preventing us from using this patch. It doesn't happen when not on this patch, server version is base 1.21, client is 1.21.1 and the client error itself is: "java.lang.IllegalArgumentException: No value with id 0", I would really like to use this patch, however this kick is primarily preventing it as I'd be more concerned when this patch is on our live proxy as I can't imagine how many people would get kicked. You also cannot join 1.8 servers when using this patch, but I can work around this. I can't really guess if its a viaversion issue or not since this patch is proxy-side, but I do find it weird that this only happens with this patch, there's no errors in console to indicate kicks either, besides the client error itself. I'd assume whatever is preventing you from joining 1.8 servers is the same thing letting these packets through to the client that's causing these decoder kicks

@valaphee
Copy link
Author

valaphee commented Dec 30, 2024

Is the first server you join 1.8, or 1.21? and if its 1.21 does it have data packs or features/experiments enabled?
Because removing the config phase also prevents different feature sets or registry data https://minecraft.wiki/w/Minecraft_Wiki:Projects/wiki.vg_merge/Registry_Data (which would require reconfiguration).

@CullanP
Copy link

CullanP commented Dec 31, 2024

Is the first server you join 1.8, or 1.21? and if its 1.21 does it have data packs or features/experiments enabled? Because removing the config phase also prevents different feature sets or registry data https://minecraft.wiki/w/Minecraft_Wiki:Projects/wiki.vg_merge/Registry_Data (which would require reconfiguration).

Can't directly reproduce it myself, can join 1.8 without any effort. And for 1.21, both versions should have the same protocol version. (don't know about packs, because its poorly documented).

Using a fresh base 1.19 & 1.20 spigot being grabbed from buildtools with the latest viaversion on jenkins, I cloned the repository with the latest commit, compiled it, set the velocity config to use legacy forwarding, set the 1.19 server as the lobby and then /server switched to 1.20, proceeded to try to craft diamond amethyst armor using a smithing table and got kicked with set_container_slot on 1.21.4 client, with only the viaversion plugin on and everything else was untouched/generated from start-up (besides the EULA and setting online-mode/bungeecord in spigot.yml)

@bobhenl
Copy link

bobhenl commented Jan 2, 2025

Is somewhere .jar, please?

@PedroMPagani
Copy link

BTW. I just implemented this on donut, on a custom dev-proxy and the main prod we have, our environment is big because we have many regions so our RTT matters a lot. DUde, this should get merged at least with a config option.
check this out.

2025-01-11.10-46-23.mp4

@valaphee
Copy link
Author

valaphee commented Jan 11, 2025

Haha yea, this removes some round-trips and restores the old pre-config phase behavior.

I'll create a proper PR soon which implements this in a per-server configurable way. This way its also possible to support servers with different versions again, without future compatibility problems.

@electronicboy what do you think?

@electronicboy
Copy link
Member

This isn't really something that I want to support, I could maybe be convinced to accept it behind some system property in the interim, but, I can't commit to reliably managing anything this second as I'm having some fun sessions with my health

@valaphee
Copy link
Author

valaphee commented Jan 11, 2025

Oh all right, understandable, sorry forgot, get well soon

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.

5 participants