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

Add PlayerLoadedWorldEvent #11940

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

FlorianMichael
Copy link

Starting with Minecraft 1.21.4, the client sends a newly player_loaded packet as soon as the client closes the downloading terrain screen / starts rendering the world. This PR adds a new event so plugins can execute/delay code until a player is considered as loaded, which happens if they send the packet or if they haven't sent the packet for 60 ticks.

Same as PaperMC/Velocity#1466

I would appreciate any feedback on the API.

@FlorianMichael FlorianMichael requested a review from a team as a code owner January 9, 2025 22:13
@emilyy-dev emilyy-dev added the type: feature Request for a new Feature. label Jan 9, 2025
Copy link
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

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

Generally a bit unsure on both the naming of the event and the exposed caused.
PlayerLoadedEvent can mean a lot, the packet name is fine-ish given it is a packet.
But in the context of the API, it could easily be confused for player data being loaded. As for alternative suggestions, uh nothing immediately comes to mind?
PlayerClientLoadedEvent?

In regards to the cause, I'd rather not introduce the concept of "packet" triggered.
Tho I am having a bit of a hard time to think of a usecase for the cause, so if you can elaborate on that, maybe that'll help my understanding 👍

@FlorianMichael
Copy link
Author

Hm yeah I can try to find a better name for the class (my personal favourite would be PlayerLoadedPacketEvent or the name you provided).

I added the Cause so that people can make a difference between the client actually setting it's local state to loaded or the server doing it, I personally don't need it.

Copy link
Contributor

@masmc05 masmc05 left a comment

Choose a reason for hiding this comment

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

Since lynxplay mentioned bad naming, I have some suggestions

@FlorianMichael FlorianMichael changed the title Add PlayerLoadedEvent Add PlayerWorldLoadEvent Jan 10, 2025
Copy link
Contributor

@masmc05 masmc05 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 you accidentally removed unused imports in the newest commits

@molor
Copy link

molor commented Jan 10, 2025

Why not PlayerReadyEvent? Because World already loaded at the time of event call, so current event name looks a little unclear IMO.

@FlorianMichael FlorianMichael force-pushed the api/player-loaded-event branch from 83ef0b8 to 7193f14 Compare January 10, 2025 07:38
@FlorianMichael FlorianMichael changed the title Add PlayerWorldLoadEvent Add PlayerLoadedWorldEvent Jan 10, 2025
@FlorianMichael FlorianMichael force-pushed the api/player-loaded-event branch from 7193f14 to edba3c0 Compare January 10, 2025 07:40
@FlorianMichael FlorianMichael force-pushed the api/player-loaded-event branch from edba3c0 to f60ad31 Compare January 10, 2025 07:41
@FlorianMichael
Copy link
Author

I don't like the term Ready honestly, renamed it to PlayerLoadedWorldEvent which should be clear enough now.

@Cryptite
Copy link
Contributor

Cryptite commented Jan 10, 2025

PlayerLoadedWorldEvent to me sounds too much still like WorldLoadEvent and doesn't strongly enough indicate this is a client-state/behavior event. Might I propose PlayerFinishJoinEvent or PlayerPostJoinEvent (since we have some PostXXXEvents, or something to that effect?

I just feel it's important not to name it something that sounds like Bukkit-level actions, but rather is a later phase of joining a server.

Don't mind PlayerReadyEvent as well, personally.

Use-case wise I can see a lot of visual-state things I would move into something like this, setting team packets, checking for player notifications, or anything that a player wouldn't need to "see" while still loading the game. Many clients do crash/timeout/etc during the join process as well, so not having to do extra work in those cases is attractive.

@nostalfinals
Copy link
Contributor

Maybe PlayerLoadedInWorldEvent?

@lynxplay
Copy link
Contributor

PlayerReadyEvent - player ready for what tho xD
`PlayerFinishJoinEvent' and the other one don't work because this is also called when the player respawns.

Realistically, it is an event that is called when the client finished loading into the world.
But yea, I'll throw some names around the PR parties if we can't find a good one xD

@Malfrador
Copy link
Member

PlayerClientLoadedWorldEvent? That would make it explicitly clear that it has nothing to do with Bukkit world loading. And the naming would align with another client-triggered event, the PlayerClientOptionsChangeEvent

@lynxplay
Copy link
Contributor

I am also good with PlayerClientLoadedWorldEvent but will discuss that with machine tomorrow prior to merge so that rename can be done by us if we do feel like it. Beyond that, needs testing but LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature Request for a new Feature.
Projects
Status: PR Party candidate
Development

Successfully merging this pull request may close these issues.

10 participants