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

Refactor GameProtocol to remove version slash-combinations #4979

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Konicai
Copy link
Member

@Konicai Konicai commented Aug 21, 2024

changes

  • bedrock versions listed in disconnect messages now lists each version individually, rather than combining versions of the same protocol with a slash
  • bedrockVersions field in Geyser dumps has the same effective change ^
  • BedrockCodecs no longer have their minecraftVersion changed
  • Keep bedrock MinecraftVersions statically in GameProtocol rather than always creating new in GeyserImpl
  • Keep version strings statically in VersionCommand

api changes

  • GeyserApI#supportedBedrockVersions() now returns a list whose MinecraftVersions are all a single minecraft version; no slash combinations. This means there are multiple MinecraftVersions in the list of that same protocol version, but the minecraft versions are actually parseable.

other notes

Unrelated to this PR, disconnect messages for outdated clients for versions >=622 and <712 are currently broken. CloudburstMC/Protocol#255 will allow us to fix that in the future.

@Konicai Konicai added the PR: Cosmetic When a PR adds something that is purely cosmetic, like changing the README label Aug 21, 2024
@@ -84,7 +84,7 @@ public final class GeyserServer {
/*
The following constants are all used to ensure the ping does not reach a length where it is unparsable by the Bedrock client
*/
private static final String PING_VERSION = pingVersion();
private static final String PING_VERSION = GameProtocol.DEFAULT_BEDROCK_CODEC.getMinecraftVersion();
Copy link
Member

@Tim203 Tim203 Aug 24, 2024

Choose a reason for hiding this comment

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

I'm not against this, but one thing that should be noted here is that because we no longer override minecraftVersion the ping version will now become 1.21.20 instead of 1.21.21 (which was the version that'd show in the ping before this PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch

@onebeastchris
Copy link
Member

image
please also adjust the javadocs of the minecraftversion api class to show that / will not be included

@Konicai
Copy link
Member Author

Konicai commented Aug 27, 2024

thanks

@Konicai Konicai requested a review from Tim203 August 28, 2024 18:58
@Konicai
Copy link
Member Author

Konicai commented Aug 28, 2024

These changes need to be re-tested

…ctor-gameprotocol

# Conflicts:
#	core/src/main/java/org/geysermc/geyser/network/UpstreamPacketHandler.java
@Konicai Konicai mentioned this pull request Sep 17, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Cosmetic When a PR adds something that is purely cosmetic, like changing the README
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants