Two skulls with the same UUID / name can have different skins, but 3D Skin Layers does not account for this.
coderbot16 opened this issue ยท 13 comments
Steps to reproduce:
/give @p player_head{SkullOwner:"(your name)"}
- Place that skull in the world.
- Exit Minecraft.
- Change your skin.
- Enter Minecraft.
/give @p player_head{SkullOwner:"(your name)"}
- Place that skull in the world.
- Note that the 3D layers are incorrect on one of the skulls.
Diagnosis:
GameProfile#equals considers two profiles to be equal if the name and UUID match, however skulls store the full GameProfile in their NBT, which includes all of the cached texture links. This means that when the user changes their skin, the skull maintains its appearance from when it was created.
This means that for skulls, it is not possible to correctly associate 3D head layer data to a GameProfile / UUID - rather, it must be associated to a texture location, whether that is the texture location stored in the NBT, or the ResourceLocation of the texture in TextureManager.
Actually, it appears that this is handled in some way:
Sorry for the confusion.
Yea the way it's cached using the WeakHashMap with the ItemStack as a key is a biiit hacky, since there is nothing persistent to really hook into (like for Players, and I don't want to mixin into EVERY Itemstack, just for heads). If you have a better/cleaner solution, be my guest :).
One idea I had was to cache it based on the ResourceLocation used for the texture in rendering, or the skin link in GameProfile - this would mean that having a bunch of the same skull in the world or inventory would not result in a bunch of duplicate skin layer data being created. I suppose that would mostly be relevant on something like Hypixel or other custom servers that really like to spam skulls everywhere.
I guess the GameProfile could work to better share cached built meshes between ItemStacks/placed heads. Will have to look into it.
Yeah, GameProfile could definitely help for better sharing. Just need to make sure to be careful to remember that GameProfile#equals lies to you in this case!
I mean technically, could also throw out the game profile and just use the encoded skin URL. Same URL = same skin. Then the UUID won't matter at all.
Right, that would probably be the best approach. Or the loaded ResourceLocation, depending on what is easiest to get access to in the rendering injections.
But I'll release the changes I made yesterday first, containing the memory leak fix, optimizing how the outer shell gets rendered(basically rendering it like vanilla), and combining the entire mesh data into one float[] array. In my quick tests before and after it sped up the rendering of just the 3d layers by about 2 times, cut the vertex amount by 350 on the test skin and it allows it to GC all the un-needed cube classes. To be fair the speed-up numbers were measured quick and dirty, just to get a reference on how the changes impact performance (positive or negative).
That's impressive and a pretty huge improvement!
One thing to watch out for though is to just make sure that you aren't affected by similar stitching issues to vanilla item models, which can be avoided by shifting quads "inwards" a little bit.
I wasn't able to see any issue so far, I'll do some more checking, but it seems to be fine. The only issue I ran across is transparency on the outside then causing issues with some inwards faces. I've added a setting to disable the outer shell rendering, which does increase the required performance by a bit again(still less than the current release), but does seem to work around the issue(even if it's just by chance).
Short video of that issue.
Sounds good, seems like it might be confusing the vanilla quad sorter but there might not be a lot of ways around that besides disabling the optimization on translucent models.
In any case, this issue came across from me testing a locally modified version of the mod to add the skull layer shared caching optimization, but I forgot to check whether the issue actually applied to the GitHub version. In any case, glad I was able to help a bit by letting you know about the idea!
It's interesting when ppl have private forks, because just recently Gaider10 crashed in with a massive pr of a private fork that he had for a while that fixed the messy math stuff from hell, and also introduced the really neat blending around corners 1942927