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

Feature: Allow setting a different port in the motd #4293

Merged
merged 8 commits into from
Jan 24, 2024

Conversation

onebeastchris
Copy link
Member

@onebeastchris onebeastchris commented Nov 15, 2023

This allows changing the broadcasted port using a system property or config option. This may be needed if the port Geyser runs on & the port Bedrock players connect on do not match - e.g. due to port forwarding/different routing.

To avoid misconfigurations, the config option is commented out. Geyser defaults to set the broadcast port to the port it's bound to - this should hopefully avoid issues.

The broadcast port is also exposed to api, since extensions such as MCXboxBroadcast or PickPack (anything that would like to automatically pick up the port that's used to connect on Bedrock) could then use it to fill out transfer packets.

In the future, when binding with multiple listeners may be possible, there might need to be a way to set different broadcast ports for ipv4 and ipv6.

… be needed if the port Geyser runs on & the port Bedrock players connect on do not match - e.g. due to port forwarding/different routing.
@Camotoy
Copy link
Member

Camotoy commented Nov 15, 2023

Throwing my opinion into the ring - I think this should be a config option, but I absolutely agree if it's not handled right, then it'll be confusing. Maybe we just comment it out and that will be fine?

@Konicai Konicai added the PR: Feature When a PR implements a new feature label Dec 1, 2023
@onebeastchris onebeastchris added PR: Needs Testing When a PR needs testing but is currently not under review PR: Needs review Indicates that a PR is functional and review-ready. labels Dec 3, 2023
…operty

# Conflicts:
#	core/src/main/java/org/geysermc/geyser/command/defaults/ConnectionTestCommand.java
@onebeastchris onebeastchris removed the PR: Needs Testing When a PR needs testing but is currently not under review label Dec 12, 2023
Copy link
Member

@Tim203 Tim203 left a comment

Choose a reason for hiding this comment

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

I think we can make this more useful by having some special (config) values as well:
-1 for disabling broadcasting
0 for using the same port as the Bedrock port
1-65535 for using the defined port

Also with the Floodgate merge we won't have a template config.yml anymore, so commented entries are not possible there. This is fine and doesn't require any changes now, but then the default value will become 0. Which is partially why I thought about those special config values.

@onebeastchris
Copy link
Member Author

I'm not against making 0 the default instead of -1, but disabling motd broadcasting doesn't seem useful tbh - wouldn't that prohibit connections altogether?

@onebeastchris onebeastchris requested a review from Tim203 January 2, 2024 16:18
@onebeastchris onebeastchris merged commit 61b3ffd into GeyserMC:master Jan 24, 2024
@onebeastchris onebeastchris deleted the port-broadcast-property branch January 25, 2024 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Feature When a PR implements a new feature PR: Needs review Indicates that a PR is functional and review-ready.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants