Skip to content

Conversation

@Koncloud
Copy link

@Koncloud Koncloud commented Sep 21, 2025

Bedrock customized(persona) skins were ignored in previous version, leading to problems such as #4194, #4195, #5253, etc. When joining, Floodgate uploads and applies skin slightly after own skin data sent, so players couldn't see their own skin while others could. And, the skin captured from persona skin were incomplete. This PR completes the processing of persona skin, using client-sent skin instead of floodgate-uploaded skin for bedrock players. Now, bedrock players should see others' skin as in a native bedrock server; for Java players, customized skins still have display problems as before. It's worth mentioning that skins in Velocity still left partially broken, since it doesn't allow Floodgate to apply skin after joining game.

@Tim203
Copy link
Member

Tim203 commented Sep 21, 2025

It was always intentional to let Floodgate override the skin of the player, as that'd be the skin that every other player would see.
Maybe we should for now just focus on fixing the geometry of those returned skins?
Because I think the proper solution is to add some logic to Floodgate, as Floodgate truely knows which players (even across multiple Geyser instances) are Bedrock players. Geyser can only guess.
An idea that we had for quite some time is to add a property to the player's profile that indicates that a Bedrock skin is available, which would allow the Geyser instances to request the Bedrock skin.

Copy link
Member

@onebeastchris onebeastchris left a comment

Choose a reason for hiding this comment

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

Only a partial review, i personally think this could be interesting.
This PR, as-is, wouldn't call the SessionSkinApplyEvent for Bedrock player skins. This isn't acceptable; but to properly enable this we would need to add a way to represent persona skins in API somehow... assuming we want that.
What i would also be fine with would be to call the fetch the skin how it's currently done, and only after the event has been called, check whether the skin is unchanged (aka, matches the converted Floodgate skin) and to send the persona skin in its place.

However, it should always be possible for an API user / the backend server to send a skin that would override the player's own set skin

Comment on lines +69 to +76
/**
* Method to determine if the given online player is a linked Bedrock player.
*
* @param uuid the uuid of the online player
* @return true if the given online player is a Bedrock player
*/
boolean isLinkedPlayer(@NonNull UUID uuid);

Copy link
Member

Choose a reason for hiding this comment

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

pls remove, don't want to introduce this API at the moment until the merge

public boolean isLinked() {
return false; //todo
// Java and linked players' uuid version is 4, while bedrock is 0
return this.javaUuid().version() == 4;
Copy link
Member

Choose a reason for hiding this comment

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

With local linking this isn't necessarily true.. might be easier to check if the uuid isn't version 0?

SkinGeometry geometry = data.isAlex() ? SkinGeometry.SLIM : SkinGeometry.WIDE;
if (skin != null) {
serializedSkin = getSerializedSkin(skin, cape ,geometry);
}
Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of always assuming that the skin should always be overridden... e.g. there are plugins/mods that send a different skin for a player, that should always take priority.

Cape cape = SkinProvider.getCachedCape(data.capeUrl());
SkinGeometry geometry = data.isAlex() ? SkinGeometry.SLIM : SkinGeometry.WIDE;
if (skin != null) {
serializedSkin = getSerializedSkin(skin, cape ,geometry);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
serializedSkin = getSerializedSkin(skin, cape ,geometry);
serializedSkin = getSerializedSkin(skin, cape, geometry);

}

private static SerializedSkin getSkin(GeyserSession session, String skinId, Skin skin, Cape cape, SkinGeometry geometry) {
public static SerializedSkin getSerializedSkin(SkinData skinData) {
Copy link
Member

Choose a reason for hiding this comment

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

pls mark nullable

.fullSkinId(skinId)
.geometryDataEngineVersion(session.getClientData().getGameVersion())
.fullSkinId(skin.textureUrl())
.geometryDataEngineVersion("0.0.0")
Copy link
Member

Choose a reason for hiding this comment

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

does this actually work?

Copy link
Author

Choose a reason for hiding this comment

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

I hava tested it locally, finding that client sends "0.0.0" for classic skins. This may be insensitive.

@Koncloud
Copy link
Author

Got it. We should provide floodgate with the skins and let it determine which to apply, but we currently lack a bridge to send persona skins to floodgate, and floodgate doesn't have the ability to handle it now.

@onebeastchris
Copy link
Member

There's no need to send skins to floodgate; that's already sort of a thing with skin uploading - Geyser uploads the login data including skin to the Global API, which from there sends it to Floodgate

Point is; what you'd need is a way to tell if a skin sent from the backend server is actually a converted floodgate skin or not. If it is a converted skin, using the persona skin is okay; if it's not the server supplied skin should take prio

@Koncloud
Copy link
Author

Will it be feasible to judge by texture url? Converted skin directs to Global API. As for using property to indicate that may cause problems when texture is modified by other java plugins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants