Skip to content

Add ServerChangeMaxPlayerSlotsEvent#11710

Closed
ammodev wants to merge 1 commit into
PaperMC:masterfrom
ammodev:feat/add-max-player-slots-change-event
Closed

Add ServerChangeMaxPlayerSlotsEvent#11710
ammodev wants to merge 1 commit into
PaperMC:masterfrom
ammodev:feat/add-max-player-slots-change-event

Conversation

@ammodev
Copy link
Copy Markdown

@ammodev ammodev commented Dec 5, 2024

This pull request introduces a new event to handle changes in the maximum number of player slots on the server. The changes include the addition of a new event class and its integration into the server code.

Event Addition:

  • patches/api/0502-Add-ServerChangeMaxPlayerSlotsEvent.patch: Added a new event class ServerChangeMaxPlayerSlotsEvent to handle changes in the maximum number of player slots. This class includes methods to get and set the old and new maximum player slots, as well as to handle event cancellation.

Server Integration:

@ammodev ammodev requested a review from a team as a code owner December 5, 2024 18:51
@lynxplay
Copy link
Copy Markdown
Contributor

lynxplay commented Dec 5, 2024

Hi and welcome to paper 🥳

Thank you for your first PR.
I am a bit sceptical on the usecase for this.

We generally do not provide API for plugin calls as it makes interoperability between plugins a bit iffy if a plugin cannot rely on its API calls being executed.
In what sense does an event like this help a plugin?

@ammodev
Copy link
Copy Markdown
Author

ammodev commented Dec 5, 2024

Hi! What are you referring to? Are you referring the cancelability / setMaxPlayers in the event or are you concerned about the event in general?

Regarding the event in general:
The event as is, is providing an actual state change for eg. Queue-Systems, which run on external servers. Eg. velocity handles a player queue and needs to be informed about the change of the maxplayers in order to handle the queue, etc.

@lynxplay
Copy link
Copy Markdown
Contributor

lynxplay commented Dec 5, 2024

Yea I am concerned about the event in general.

Regarding a player queue, the max players are only ever updated this way by other plugins.
Given whatever other service needs to be informed here, something would need to actually send that message.
That would mean we'd have a plugin A that does change this number but is not self developed, then plugin B that listens to that event and then notifies the external service about it?

I am unsure of how many plugins I know that change the max player count like this, especially in the context of server networks, where this kind of logic is probably better of at the queue level itself, rather than a server deciding its own max player limit.

@ammodev
Copy link
Copy Markdown
Author

ammodev commented Dec 5, 2024

So you are saying that we would have to recode every plugin or ask for a changeevent on their side to provide this functionallity? I mean really the other choice is to send a request from the velocity to the server every tick in order to fetch this state change?

@lynxplay
Copy link
Copy Markdown
Contributor

lynxplay commented Dec 5, 2024

Well no, I am not saying anything, I was just asking what kind of third party plugins exist that change the max player numbers like this on their own without being prompted to do so. It sounds like a rather destructive way of managing a full server on the plugins part, so I am just curious.
Do you have a specific plugin for this that I can glance at?

I am mostly asking because, as stated prior, we generally are not a fan of events for this as plugins expect their API calls to work.
A third party plugin that for some reason manages the entire servers max player count like this, might not be to pleased when the value is doubled by another plugin instead.

@ammodev
Copy link
Copy Markdown
Author

ammodev commented Dec 5, 2024

https://www.spigotmc.org/resources/changeslots-change-max-players-slots-bukkit-bungee.49648/ - Easy Plugin - Could do this on our own with no issue

This plugin is the main issue we are currently having, resulting in us trying to have this event included. Other than that you could take any gamemode (BedWars, SkyWars, etc.). Some of them change the playercount when the plugin is loaded resulting in the velocity not knowing the max player count anymore.

@electronicboy
Copy link
Copy Markdown
Member

That plugin doesn't even modify the slots in a manner that this event would fire;

It's long been tradition that, outside of some very key explicit areas, events are not fired; if a plugin does something, that operation occurs. I can maybe understand notification-esque events for some of this stuff, but, not fond of aspects like cancellation

@ammodev
Copy link
Copy Markdown
Author

ammodev commented Dec 5, 2024

That plugin doesn't even modify the slots in a manner that this event would fire;

It's long been tradition that, outside of some very key explicit areas, events are not fired; if a plugin does something, that operation occurs. I can maybe understand notification-esque events for some of this stuff, but, not fond of aspects like cancellation

You are right. They did some kind of update where they are setting this via reflections for some reason. Nevermind on that plugin then. Doesnt change the fact that other plugins do it via the correct .setMaxPlayers method.

@lynxplay
Copy link
Copy Markdown
Contributor

lynxplay commented Dec 5, 2024

Well the reason I am asking so much for a usecase is that, for API we are generally not happy to include due to the mentioned reasons), we'd need a really good usecase to justify that.
If the plugin you were struggling with does not even use this in the first place, it just isn't a very good argument to merge something that breaks the existing API concepts.

As cat stated, a notification-like event should solve your usecase? (e.g. no setters or cancellation)

@ammodev ammodev closed this Dec 5, 2024
@ammodev ammodev deleted the feat/add-max-player-slots-change-event branch December 5, 2024 21:20
@ammodev ammodev restored the feat/add-max-player-slots-change-event branch December 5, 2024 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

3 participants