-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
Implement max player limit #194
Conversation
Also removed unnecessary debug log.
pumpkin/src/server/mod.rs
Outdated
pub async fn get_player_count(&self) -> u32 { | ||
let mut count = 0; | ||
for world in &self.worlds { | ||
count += world | ||
.current_players | ||
.lock() | ||
.await | ||
.len() | ||
.to_u32() | ||
.expect("Unable to convert to u32"); | ||
} | ||
count | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need to convert it to u32, just use usize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BASIC_CONFIG.max_players
is u32
which is why I used the conversion, also usize
is platform dependent , please let me know what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So generally we want to use usize for such things like a player count. I also want to give the option to disable a player limit, maybe by setting it to -1 ?. the config value can stay u32 because this will be more than enough if someone wants to set a limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR return usize. If you compare it with the config value, cast it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers - should be resolved in 8aba1ff. I've also added logic that setting the max players to zero will disable the limit. (since it's unsigned, we can't / shouldn't use -1
)
Looks good now. |
Resolves #189
Ready for review
TBD in future: allow operators to bypass full server. need to wait for operator/permission stuff to be implemented first.