Require skull texture lookup on paper servers

[sovdeeth]

Description

Paper does not seem to fetch textures synchronously for skull metas like spigot does, meaning playerprofiles without texture data will resolve to untextured skulls. This forces a synchronous lookup as a non-breaking change when applying skull owners.

It may be better to move this lookup to the original offlineplayer acquisition, opinions welcome.

---

Target Minecraft Versions: any
Requirements: none
Related Issues: #7068

[Fusezion]

Is it possible to add a regression test using the skull owner expression?

[sovdeeth]

Is it possible to add a regression test using the skull owner expression?

No, the owner works whether textures are there or not.

[Pikachu920]

i'm a little concerned about adding this without a way to opt out or complete the texture grabbing in a non-blocking way. presumably the paper devs changed this for good reason.

[sovdeeth]

i'm a little concerned about adding this without a way to opt out or complete the texture grabbing in a non-blocking way. presumably the paper devs changed this for good reason.

I think that should come in a feature, since this is just maintaining the behavior from before.

[Pikachu920]

i'm a little concerned about adding this without a way to opt out or complete the texture grabbing in a non-blocking way. presumably the paper devs changed this for good reason.

I think that should come in a feature, since this is just maintaining the behavior from before.

I get that it maintains the behavior from before, but I don't think that the old behavior was a guarantee Skript made. I think we should trust the server implementation to do what makes the most sense as it has the most context. I would personally prefer to see a feature added to grab the textures on paper in a non-blocking manner. I worry that this is going to reinstate the lag on users servers that Paper is most likely trying to avoid. I'd rather have my GUIs look weird and investigate a fix rather than have my server lag/get killed by watchdog in a much harder to debug way.

[sovdeeth]

i'm a little concerned about adding this without a way to opt out or complete the texture grabbing in a non-blocking way. presumably the paper devs changed this for good reason.

I think that should come in a feature, since this is just maintaining the behavior from before.

I get that it maintains the behavior from before, but I don't think that the old behavior was a guarantee Skript made. I think we should trust the server implementation to do what makes the most sense as it has the most context. I would personally prefer to see a feature added to grab the textures on paper in a non-blocking manner. I worry that this is going to reinstate the lag on users servers that Paper is most likely trying to avoid. I'd rather have my GUIs look weird and investigate a fix rather than have my server lag/get killed by watchdog in a much harder to debug way.

The only fix i can think of requires a completely new effect to load textures of an offline player, which I just don't think is appropriate for patch. Otherwise we're either blocking during offlineplayer parsing or during skull of x.

[Pikachu920]

i'm a little concerned about adding this without a way to opt out or complete the texture grabbing in a non-blocking way. presumably the paper devs changed this for good reason.

I think that should come in a feature, since this is just maintaining the behavior from before.

I get that it maintains the behavior from before, but I don't think that the old behavior was a guarantee Skript made. I think we should trust the server implementation to do what makes the most sense as it has the most context. I would personally prefer to see a feature added to grab the textures on paper in a non-blocking manner. I worry that this is going to reinstate the lag on users servers that Paper is most likely trying to avoid. I'd rather have my GUIs look weird and investigate a fix rather than have my server lag/get killed by watchdog in a much harder to debug way.

The only fix i can think of requires a completely new effect to load textures of an offline player, which I just don't think is appropriate for patch. Otherwise we're either blocking during offlineplayer parsing or during skull of x.

Yeah, it would need a new effect but I think that its fine for this to wait until the next feature release. The breaking change was on Paper's end, so if users want to avoid it they should not upgrade Paper for now. Soon enough, we can get an effect out there and they can update it.

[sovdeeth]

Yeah, it would need a new effect but I think that its fine for this to wait until the next feature release. The breaking change was on Paper's end, so if users want to avoid it they should not upgrade Paper for now. Soon enough, we can get an effect out there and they can update it.

This is where I disagree. I think it's better for the skull effect to work in Paper, despite any lag (that's no worse than before) rather than not work at all for offline players. I'd rather allow players to update paper and just not receive the benefits of this particular change, rather than be forced to not update/use paper at all if they need the skull feature.

[Pikachu920]

Yeah, it would need a new effect but I think that its fine for this to wait until the next feature release. The breaking change was on Paper's end, so if users want to avoid it they should not upgrade Paper for now. Soon enough, we can get an effect out there and they can update it.

This is where I disagree. I think it's better for the skull effect to work in Paper, despite any lag (that's no worse than before) rather than not work at all for offline players. I'd rather allow players to update paper and just not receive the benefits of this particular change, rather than be forced to not update/use paper at all if they need the skull feature.

I get where you are coming from for sure. I'm looking at this from a security perspective where this is a potential DoS vector. A GUI full of heads can easily cause a lot of lag. From that perspective, it seems a little off to me to reinstate the issue even though it is no worse than before. I think if anyone should provide an option for that it should be Paper. I think I'd even say that #7068 should really be a Paper issue if anything.

Another perspective I am looking is a user who might not be too familiar with Skript and debugging for perfomance. It's a lot easier for me to notice my heads aren't loading and go get help for that than it is to debug or get help for a vague performance issue.

I'm happy to contribute the effect to grab skulls, and in the meantime we could provide a skript-reflect snippet to resolve the issue.

[sovdeeth]

Yeah, it would need a new effect but I think that its fine for this to wait until the next feature release. The breaking change was on Paper's end, so if users want to avoid it they should not upgrade Paper for now. Soon enough, we can get an effect out there and they can update it.

This is where I disagree. I think it's better for the skull effect to work in Paper, despite any lag (that's no worse than before) rather than not work at all for offline players. I'd rather allow players to update paper and just not receive the benefits of this particular change, rather than be forced to not update/use paper at all if they need the skull feature.

I get where you are coming from for sure. I'm looking at this from a security perspective where this is a potential DoS vector. A GUI full of heads can easily cause a lot of lag. From that perspective, it seems a little off to me to reinstate the issue even though it is no worse than before. I think if anyone should provide an option for that it should be Paper. I think I'd even say that #7068 should really be a Paper issue if anything.

Another perspective I am looking is a user who might not be too familiar with Skript and debugging for perfomance. It's a lot easier for me to notice my heads aren't loading and go get help for that than it is to debug or get help for a vague performance issue.

I'm happy to contribute the effect to grab skulls, and in the meantime we could provide a skript-reflect snippet to resolve the issue.

The security thing does make sense, but I would like to avoid breaking changes where possible for 2.9, at least.
Perhaps we can do a config option to enable lazy-loading of offline players? That way we maintain default behavior but give a way to avoid any security risks if users wish.

[sovdeeth]

I also thought of another possible solution, where when a offlineplayer is parsed, we start loading textures in the background. When they're needed by skulls, we either have the textures, or we block and wait for the textures. Still not great, but a slight improvement for some situations. Downside is now we're loading textures for players that may not be used for skulls. Let me know what you think.

[APickledWalrus]

Seems okay for a patch. I definitely agree that a more async-friendly approach would be good for the future.