Fix/head guis - #193
Conversation
SirYwell
left a comment
SirYwell
left a comment
There was a problem hiding this comment.
I noted a few things. I think we should also be careful about compatibility, I can work on that later.
| } | ||
| head.setItemMeta(headMeta); | ||
| return head; | ||
| UUID uuid = plugin.getServer().getOnlinePlayers().stream().findAny().get().getUniqueId(); |
There was a problem hiding this comment.
What's the intention here? I think generating a random UUID here was fine before.
There was a problem hiding this comment.
Oh yes it actually is fine again. It wasnt in a previous iteration
| ItemStack getPlayerHead(UUID owner, String texture); | ||
| ItemStack getPlayerHead(OfflinePlayer owner, String texture); | ||
| ItemStack getPlayerHead(String texture); |
There was a problem hiding this comment.
Do we need all those methods? I'd probably just add the method that takes a UUID and a texture - but we should avoid Stringly typed arguments as they can be confusing (you are passing a URL string to it?). The existing methods shouldn't be touched imo.
There was a problem hiding this comment.
this is mainly about keeping compatibility with legacy, without reinventing the wheel. I mainly went for a "make it work approach" instead of producing fine code.
There was a problem hiding this comment.
Actually this is a huge mess right now. legacy might not even work anymore right now...
This is a big abstraction issue, since legacy requires the texture as base64 encoded whatever and latest requires the url encoded in the base46 string
There was a problem hiding this comment.
The latest way might not even work on older paper versions between 1.13 and 1.18 I think. once again I hate multiversion support xD
There was a problem hiding this comment.
Esp since the same method is used to 1. create a player head for the requested owner and 2. to create a player head with some texture. This really messed it up a lot.
Another issue is that the skulls class is part of the core and has to work with two completely different approaches of creating skulls.
There was a problem hiding this comment.
Yes, for proper multi-version support the old strategy must exist as fallback on versions that don't have the bukkit PlayerProfile API. I think it's fine to extract the url only as needed. Or alternatively pass an object that holds both the base64 string and the url instead of just a string - that would also help with proper typing.
2 participants
This PR is a mess, but it works. I didnt bother for backwards compatibility honestly. Also not about spigot compatibility. Too many people are using latest java, paper and stuff anyway.