-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Configuration API #12301
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
Configuration API #12301
Conversation
3b67abb to
dcb7016
Compare
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.
Copilot reviewed 45 out of 46 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- paper-api-generator.settings.gradle.kts: Language not supported
Comments suppressed due to low confidence (3)
paper-api/src/main/java/org/bukkit/event/player/PlayerLoginEvent.java:22
- The deprecation version 'idk' is unclear; please update it to a proper version string to improve clarity.
@Deprecated(since = "idk")
paper-api/src/main/java/io/papermc/paper/event/connection/configuration/PlayerConfigurationConnectionEvent.java:8
- [nitpick] The field 'loginConnection' should be renamed to 'configurationConnection' to better reflect its purpose in a configuration event.
private final PlayerConfigurationConnection loginConnection;
paper-api/src/main/java/io/papermc/paper/connection/PlayerCommonConnection.java:3
- [nitpick] Consider clarifying the name of this interface to more clearly describe its purpose, such as 'PlayerSharedConnection' or a similar term.
// TODO: Naming?
paper-api/src/main/java/io/papermc/paper/connection/PlayerConfigurationConnection.java
Outdated
Show resolved
Hide resolved
paper-api/src/main/java/io/papermc/paper/connection/CookieConnection.java
Outdated
Show resolved
Hide resolved
paper-api/src/main/java/io/papermc/paper/connection/PlayerConfigurationConnection.java
Outdated
Show resolved
Hide resolved
paper-api/src/main/java/io/papermc/paper/connection/PlayerConfigurationConnection.java
Outdated
Show resolved
Hide resolved
paper-api/src/main/java/io/papermc/paper/connection/CookieConnection.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * Completes the configuration for this player, which will cause this player to reenter the game. | ||
| * <p> | ||
| * Note, this should be only be called if you are reconfiguring the player. |
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.
This, similar to #enterConfiguration poses the interesting issue of "ah, you just requested what is basically a destruction of this connection state, what happens to this instance".
Calling #clearChat after this is illegal.
Getters might be fine. But we should look into a proper way to signal, "you destroyed this connection instance".
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.
...ava/io/papermc/paper/event/connection/configuration/AsyncPlayerConnectionConfigureEvent.java
Outdated
Show resolved
Hide resolved
...ava/io/papermc/paper/event/connection/configuration/AsyncPlayerConnectionConfigureEvent.java
Outdated
Show resolved
Hide resolved
...a/io/papermc/paper/event/connection/configuration/PlayerConnectionInitialConfigureEvent.java
Outdated
Show resolved
Hide resolved
paper-server/patches/sources/net/minecraft/network/DisconnectionDetails.java.patch
Outdated
Show resolved
Hide resolved
...erver/patches/sources/net/minecraft/server/network/ServerCommonPacketListenerImpl.java.patch
Outdated
Show resolved
Hide resolved
...java/io/papermc/paper/event/connection/configuration/PlayerConfigurationConnectionEvent.java
Outdated
Show resolved
Hide resolved
...atches/sources/net/minecraft/server/network/ServerConfigurationPacketListenerImpl.java.patch
Outdated
Show resolved
Hide resolved
...atches/sources/net/minecraft/server/network/ServerConfigurationPacketListenerImpl.java.patch
Outdated
Show resolved
Hide resolved
...atches/sources/net/minecraft/server/network/ServerConfigurationPacketListenerImpl.java.patch
Outdated
Show resolved
Hide resolved
...atches/sources/net/minecraft/server/network/ServerConfigurationPacketListenerImpl.java.patch
Outdated
Show resolved
Hide resolved
paper-api/src/main/java/io/papermc/paper/event/player/PlayerServerFullCheckEvent.java
Show resolved
Hide resolved
|
|
||
| import net.kyori.adventure.text.Component; | ||
|
|
||
| public interface PlayerConnection { |
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.
It would be really good if the hierarchy of types here was adjusted so this class could be sealed and permit the two API interfaces. Would make switching on it nicer, especially in events where you only have the PlayerConnection, and to know what type it is, you do a switch pattern match. It's not a super simple change, because of the impl CommonCookeConnection abstract class, but things could def be moved around.
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.
After playing around with this a bit, I am not sure about it really. This does require alot of moving around and possible duplication that I am not fully sure is worth it.
Additionally, its a bit of an odd situation, as I am not sure in what cases you'd be casting like this in the first place.
| /** | ||
| * Represents a connection that has properties shared between the GAME and CONFIG stage. | ||
| */ | ||
| public interface PlayerCommonConnection extends CookieConnection { |
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.
Same situation here regarding sealed interfaces being nicer for switch pattern matching (doesn't require default case)
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.
see above
...src/main/java/io/papermc/paper/event/connection/common/PlayerFinishConnectionPhaseEvent.java
Outdated
Show resolved
Hide resolved
| * Indicates that this player is being reconfigured, meaning that this connection will be held in the configuration | ||
| * stage unless kicked out through {@link PlayerConfigurationConnection#completeConfiguration()} | ||
| */ | ||
| public class PlayerConnectionReconfigurateEvent extends PlayerConfigurationConnectionEvent { |
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.
I feel like this event, and PlayerConnectionInitialConfigureEvent can be compiled with just an enum "cause" or something. They are just both fired right before switching to configuration phase, from either login or from game. Just a PlayerEnterConfigurationPhaseEvent or something with a cause?
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.
The reason why I made this separate is because both are treated very differently.
Basically I felt like differenciating that this player is going to be held forever in config phase vs they are going through the config phase as normal was an important enough disctinction.
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.
But in both cases, doesn't the player just automatically go through the tasks in the configuration phase?
In plain vanilla, if you call that function to kick the player back to config phase, won't they just come right back in as if they had just logged in? Or do you have to somehow trigger something to let them back in?
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.
No, in reconfiguration the tasks are not run. This causes the player to just be stuck in the stage until released.
startConfiguration is only called when login packet is received, so there are no "tasks" that are being executed.
You have to "somehow" trigger to let them back in.
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.
0ecb2ed to
07c1a59
Compare
|
Last updated for: ab69660e4e3be260ae7bf18e00ddebef4d7a090f. Download the Paperclip jar for this pull request: paper-12301.zip Maven PublicationThe artifacts published by this PR:
Repository DeclarationIn order to use the artifacts published by the PR, add the following repository to your buildscript: repositories {
maven("https://maven-prs.papermc.io/Paper/pr12301") {
name = "Maven for PR #12301" // https://github.com/PaperMC/Paper/pull/12301
mavenContent {
includeModule("io.papermc.paper", "dev-bundle")
includeModule("io.papermc.paper", "paper-api")
}
}
} |
kennytv
left a comment
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.
also ctrl+f and remove at least half of the mentions of the word "currently" (especially in event headers)💩
paper-api/src/main/java/io/papermc/paper/connection/PlayerConfigurationConnection.java
Outdated
Show resolved
Hide resolved
paper-api/src/main/java/io/papermc/paper/connection/PlayerGameConnection.java
Outdated
Show resolved
Hide resolved
paper-api/src/main/java/org/bukkit/event/player/AsyncPlayerPreLoginEvent.java
Outdated
Show resolved
Hide resolved
paper-server/src/main/java/io/papermc/paper/connection/HorriblePlayerLoginEventHack.java
Outdated
Show resolved
Hide resolved
| * | ||
| * @return the configuring player audience | ||
| */ | ||
| Audience getConfiguringPlayer(); |
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.
If its just an "Audience", how to I, in a callback where I only have the audience, get access to the methods I can use on the connection?
cffc660 to
65619e6
Compare
Fix disconnect continuing to tick ServerLoginPacketListenerImpl Fixes #8676 This implements a solution that preemptively exits the tick loop if we have already marked the connection as disconnecting. This avoids changing the result of Connection#isConnected in order to avoid other possibly unintentional changes. Fundamentally it should be investigated if closing the connection async is really still needed. This also additionally removes the login disconnecting logic for server stopping, as this was also a possible issue in the config stage but also shouldn't be an issue as connections are closed on server stop very early. Additionally, do not check for isConnecting() on VERIFYING, as that seemed to be an old diff originally trying to resolve this code, however isConnected is not updated at this point so it pretty much was useless.
This needs to be moved out as there is no serverplayer in the configuration stage. Additionally, fix logging inconsistency.
This implements a solution that preemptively exits the tick loop if we have already marked the connection as disconnecting. This avoids changing the result of Connection#isConnected in order to avoid other possibly unintentional changes. Fundamentally it should be investigated if closing the connection async is really still needed. This also additionally removes the login disconnecting logic for server stopping, as this was also a possible issue in the config stage but also shouldn't be an issue as connections are closed on server stop very early. Additionally, do not check for isConnecting() on VERIFYING, as that seemed to be an old diff originally trying to resolve this code, however isConnected is not updated at this point so it pretty much was useless.
This implements a solution that preemptively exits the tick loop if we have already marked the connection as disconnecting. This avoids changing the result of Connection#isConnected in order to avoid other possibly unintentional changes. Fundamentally it should be investigated if closing the connection async is really still needed. This also additionally removes the login disconnecting logic for server stopping, as this was also a possible issue in the config stage but also shouldn't be an issue as connections are closed on server stop very early. Additionally, do not check for isConnecting() on VERIFYING, as that seemed to be an old diff originally trying to resolve this code, however isConnected is not updated at this point so it pretty much was useless.
This implements a solution that preemptively exits the tick loop if we have already marked the connection as disconnecting. This avoids changing the result of Connection#isConnected in order to avoid other possibly unintentional changes. Fundamentally it should be investigated if closing the connection async is really still needed. This also additionally removes the login disconnecting logic for server stopping, as this was also a possible issue in the config stage but also shouldn't be an issue as connections are closed on server stop very early. Additionally, do not check for isConnecting() on VERIFYING, as that seemed to be an old diff originally trying to resolve this code, however isConnected is not updated at this point so it pretty much was useless.

Please provide feedback on naming, and general event consensis.
But my goal is to have the following:
CookieConnection)AsyncPlayerConnectionConfigurateEvent)