meiskam / PlayerHeads

Bukkit Plugin - Drops a player's head when s/he dies, also mob heads
https://dev.bukkit.org/projects/player-heads
Other
17 stars 39 forks source link

Keep player head skin #18

Open ehetteNandaYo opened 4 years ago

ehetteNandaYo commented 4 years ago

Describe the solution you'd like When players change their skin, the head also changes the skin, It would be better if it saved the head skin forever, maybe add a setting for those who dont want this behavior?

crashdemons commented 4 years ago

unfortunately this behavior is controlled by your server - PlayerHeads does not update the head texture for players, your server does - and there isn't usually much configuration for that. Somewhat ironically, a lot of people have the reverse problem also - the server failing to update head skins.

This will happen as long as the head's name or UUID match the user - and if you change it, it would no longer be their head...

ehetteNandaYo commented 4 years ago

huh, so there is no way to make it work like the plugin head database, which has tons of heads which I think they never change?

crashdemons commented 4 years ago

If the head is linked to a name/uuid of a player they will change (PH player drops) - it shouldn't matter the plugin.

but PH mob heads / custom heads (vanilla or custom UUID/texture) won't change since there is no mojang profile to download.

It's important to distinguish between a player's head and a custom head since they have different behaviors.

I imagine the head database plugin handles both player-linked (updating) and custom heads.

It's possible to make a 'custom' head with the old texture for a player, but with different UUID. However the plugin would need to generate and store a log of UUIDs for each old head/texture to be recognized the same and prevent updating - and they wouldn't stack or work in villager trades with more updated heads.

rayzr522 commented 4 years ago

hey, author of DecoHeads here -- i see you modify the output of BlockBreakEvents so that the dropped item has the correct name, lore, etc. when you break a placed PlayerHead. i also see you have an option convertvanillaheads in the config which appears to default to false, yet looking at your BlockBreakEvent listener, I don't see this option being used anywhere.

would it be possible to make this option or some other option that simply prevents custom handling of BlockBreakEvents? or, perhaps even better, fetches the GameProfile from the block's state (I think you can do this) and then uses this information directly to set the GameProfile on the item? custom GameProfile data is how plugins like DecoHeads use custom textures without associating usernames

would be nice to have a way to make your plugin play nice with other plugins which deal in custom heads :)

EDIT: I'm not sure if it's as easily accessible in all versions, but I just checked a 1.8 JAR i had lying around, and it looks like this is trivial to do:

public class CraftSkull extends CraftBlockState implements Skull {
  private static final int MAX_OWNER_LENGTH = 16;

  private final TileEntitySkull skull;

  private GameProfile profile;

and then there are pretty easy version-independent ways to set GameProfiles on skull items with reflection so you could preserve the GameProfile itself when creating the custom item to drop

crashdemons commented 4 years ago

hey, author of DecoHeads here -- i see you modify the output of BlockBreakEvents so that the dropped item has the correct name, lore, etc. when you break a placed PlayerHead. i also see you have an option convertvanillaheads in the config which appears to default to false, yet looking at your BlockBreakEvent listener, I don't see this option being used anywhere.

'convertvanillaheads' is used by admins that want PH to use all-custom or mixed-vanilla-custom heads (depending on dropvanillaheads) - (the default behavior is to only use custom heads to extend the existing head list)

If you look in the method you linked, you will see it calls blockDrop which can modify the dropped item. blockDrop itself calls createConvertedMobhead and by extension canConversionHappen which does check the "convertvanillaheads" as a condition for using nonvanilla head

createConvertedMobhead is the same method used in the ItemSpawn listener so that item conversion either form block-breaking or normal item drops is the same (players dropping - or even pistons/water popping head blocks, which historically was not capturable despite being an event now).

would it be possible to make this option or some other option that simply prevents custom handling of BlockBreakEvents? or, perhaps even better, fetches the GameProfile from the block's state (I think you can do this) and then uses this information directly to set the GameProfile on the item? custom GameProfile data is how plugins like DecoHeads use custom textures without associating usernames

For nonvanilla mobheads, PH pretty much relies on a reserved-UUID (unused UUID) system for heads and no username. getOwner() generally fails for these types of heads, and this seems to be the same way things like the MCHeads db do custom heads. Beginning with version 4.x, username-based mobheads were phased out for texture/uuid pairs and in 5.x support for username-mobheads was removed entirely - so PH doesn't associate usernames... except when the head is legitimately owned by a user, which was the intended purpose I think. If we want to convert a user's head into a custom one with no name it should be possible with some work, but it wouldn't be recognizable in any other plugin if you change both name/uuid. If you're having a conflict with your/other plugins, I'd be interested in specific examples.

For re-setting the profile should be possible, I think. I believe both methods were already tested and coded into the ProfileUtils class. (PlayerHeads-craftbukkit-support) However a couple concerns first:

that said, resetting the game profiile like you mention should be possible to prevent re-retrieving the skin for players (like in OPs concern - detecting custom heads is a different issue), I didn't think of it saving the block profile for later, but that's totally doable.

would be nice to have a way to make your plugin play nice with other plugins which deal in custom heads :)

EDIT: I'm not sure if it's as easily accessible in all versions, but I just checked a 1.8 JAR i had lying around, and it looks like this is trivial to do:

public class CraftSkull extends CraftBlockState implements Skull {
  private static final int MAX_OWNER_LENGTH = 16;

  private final TileEntitySkull skull;

  private GameProfile profile;

and then there are pretty easy version-independent ways to set GameProfiles on skull items with reflection so you could preserve the GameProfile itself when creating the custom item to drop

indeed and I use an implementation already because of textures support and issues with bukkiti api owner resolution. https://github.com/meiskam/PlayerHeads/blob/master/PlayerHeads-craftbukkit-support/src/main/java/com/github/crashdemons/playerheads/compatibility/craftbukkit/ProfileUtils.java This has been tested on craftbukkit builds from 1.8 to 1.16.3 without issue. Apparently authlib and profile fields haven't changed very much as long as you don't use the rare first-day server build with broken mapping.

I should be able to modify it to do what you suggest, I think.

So again, two takeaways I see:

EDIT: Although I will comment though that even when heads are not from the plugin, and their profile is maintained, their texture can still expire in line with the timestamp field. Although it may(?) be possible to update the timestamp to delay skin updates. EDIT 2: Paper-API in GlowstoneMC does not support a method for getPlayerProfile for Skull (blockstate), so it is not possible to get that. (Paper-api 1.12.2-R0.1-SNAPSHOT / Glowkit 1.12.2-R0.1-SNAPSHOT / Glowstone 2018.10.0-SNAPSHOT) - the profile field I believe may not be present since this is a novel implementation of paper-api. EDIT 3: Correction for convertvanillaheads behavior - dependent on another setting.

crashdemons commented 4 years ago

Here is a development build that adds two configuration options:

I don't notice any immediate big change (or benefit) and this has not been tested heavily, but I can tell you that if "fixdroppedheads" and "fixbrokenheads" are both disabled, head-blocks that are broken will just become undecorated "Player Head" (but retain the texture/uuid).

Dev build/CI: https://ci.meme.tips/job/PlayerHeads-5.x/1088/ Code snapshot: https://github.com/crashdemons/PlayerHeads/tree/43d89a04326710e5bcd1ce1ce17db5beda19e496

The easiest way to implement the second feature was to set the profile on the meta right before setting the meta on the item [1][2] - hopefully wiping out the previous owner information with the restored version, but I still need the verify the behavior.

crashdemons commented 4 years ago

Regarding issues with PH and other plugins, I don't know if you were referring to your own plugin or not, but when I test DecoHeads, you appear to set the SkullOwner.Name tag with your item-name. See below:

image This is not the displayname/customname, but the owner username. This was generated with only DecoHeads on the server.

SkullOwner.Name is typically blank for custom heads, and reserved for putting usernames into. This is what confuses PH because it thinks (because a username exists on the head), it thinks it belongs to a user - and PH is a plugin which handles Player drops also, not just mob drops. I don't use the username to determine the texture of mobs/custom-heads, but when a player dies and drops their own head, their own head should have the correct username in it...

I don't see a way to avoid the issue outside of:

Aside: In this line https://github.com/Rayzr522/DecoHeads/blob/master/src/main/java/me/rayzr522/decoheads/util/CustomHead.java#L57 you construct the GameProfile for a head using the UUID and Username, but the username you pass is your item name https://github.com/Rayzr522/DecoHeads/blob/2b50ac3be36f04bfd12257db0562b316c3e58bde/src/main/java/me/rayzr522/decoheads/data/Head.java#L144 Setting a name is not required at all - refer to this thread: https://www.spigotmc.org/threads/using-custom-head-textures.311562/#post-2947987 which uses a null name.

rayzr522 commented 4 years ago

I see, I originally copied the custom head code from mrCookieSlime, and was not aware that you could simply pass null for the name. that actually makes things a good bit easier :) I've opened a corresponding issue to address this: https://github.com/Rayzr522/DecoHeads/issues/30

so assuming I make that change, along with the new options present here, this should eliminate any compatibility issues? at least with decoheads, I'm unsure of how heads database handles custom heads or whether or not this will work for them directly.

crashdemons commented 4 years ago

I won't guarantee it fixes all issues, but it should fix the issue where PH changes DecoHeads into "Microwave's Head" etc. I didn't notice any other immediate issues though.

So I think that's a good start.

Regarding HeadDB, I'll admit I haven't looked at their code yet (it's a paid plugin), but their website (minecraft-heads.com, similar to freshcoal) presents the generated commands without a name field. (and the resulting head does not have one set)

crashdemons commented 1 year ago

@rayzr522

You may have already found out, but I wanted to mention that, much to my displeasure, the newest Authlib versions now disallow passing null as the username when creating a GameProfile (the data structure that carries the head UUID/name/texture) - so I'm probably going to go the way of DropHeads and HeadDB plugins where the username will be "PlayerHeads:uuid".

As the attached commit shows, ":" is what I'm filtering-out at the moment when detecting an actual player-named head.

I'm not sure why they decided on this change since nameless heads seem like they would make perfect sense for a head that is not attached to an account. I'm hoping the invalid head name + uuid will reduce the amount of Mojang API calls servers make to resolve heads, but I worry about that.