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

Reduce session server lookups #509

Merged
merged 12 commits into from
May 18, 2024
Merged

Conversation

bridgelol
Copy link
Contributor

@bridgelol bridgelol commented May 10, 2024

Fixes #508, fixes #478.

@bridgelol bridgelol changed the title fix: add default skin to gameprofiles WIP: fix: add default skin to gameprofiles May 10, 2024
@bridgelol
Copy link
Contributor Author

Unsure if there's a better way than simply just sending an empty signature over.

@bridgelol bridgelol changed the title WIP: fix: add default skin to gameprofiles fix: add default skin to gameprofiles May 10, 2024
@bridgelol
Copy link
Contributor Author

This should solve this bungee & spigot-side as well (bungee doesn't need modifying as it doesn't have modern forwarding like velocity, fixing spigot also fixes bungee)

@onebeastchris
Copy link
Member

onebeastchris commented May 10, 2024

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.

One functional thing to keep in mind is that with this PR you always prevent the skin lookup, while you still want to do a skin lookup when the player is linked.
When a player is linked it acts like it's the linked Java player (including the same UUID etc.), meaning that it should also have the skin of the Java player. Which is something we don't store, and has to be looked up like normal.

@bridgelol
Copy link
Contributor Author

https://github.com/GeyserMC/Floodgate/blob/master/bungee%2Fsrc%2Fmain%2Fjava%2Forg%2Fgeysermc%2Ffloodgate%2Flistener%2FBungeeListener.java#L134

https://github.com/GeyserMC/Floodgate/blob/master/spigot%2Fsrc%2Fmain%2Fjava%2Forg%2Fgeysermc%2Ffloodgate%2Flistener%2FPaperProfileListener.java There are also these two occurrences where we're currently setting an empty skin - maybe it is worth it to change these too? Just for consistency

Yep, believe this would be ideal. 33fd6ac

@bridgelol
Copy link
Contributor Author

One functional thing to keep in mind is that with this PR you always prevent the skin lookup, while you still want to do a skin lookup when the player is linked. When a player is linked it acts like it's the linked Java player (including the same UUID etc.), meaning that it should also have the skin of the Java player. Which is something we don't store, and has to be looked up like normal.

Would you recommend not adding the properties if the player is linked?

@bridgelol
Copy link
Contributor Author

One functional thing to keep in mind is that with this PR you always prevent the skin lookup, while you still want to do a skin lookup when the player is linked. When a player is linked it acts like it's the linked Java player (including the same UUID etc.), meaning that it should also have the skin of the Java player. Which is something we don't store, and has to be looked up like normal.

Would you recommend not adding the properties if the player is linked?

Or, alternatively, look them up ourselves?

@onebeastchris
Copy link
Member

If the player is linked, the Java server will see a valid Java username/uuid, and should look up textures for that player

@bridgelol
Copy link
Contributor Author

bridgelol commented May 10, 2024

If the player is linked, the Java server will see a valid Java username/uuid, and should look up textures for that player

I believe we should be doing skin lookups on velocity ourselves then, having spigot handle it if floodgate's ran purely on spigot is fine, however you don't want spigot instances doing a new lookup every time a player switches server on velocity (modern forwarding).

@bridgelol
Copy link
Contributor Author

If the player is linked, the Java server will see a valid Java username/uuid, and should look up textures for that player

Should be good to merge now

@onebeastchris onebeastchris requested a review from Tim203 May 14, 2024 11:53
@Tim203 Tim203 changed the title fix: add default skin to gameprofiles Reduce session server lookups May 14, 2024
@Tim203 Tim203 merged commit 00b8b1b into GeyserMC:master May 18, 2024
2 checks passed
Tim203 added a commit that referenced this pull request May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants