Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[XSkull] - Inconsistent behavior in different server versions & online/offline servers based on usernames and UUIDs #254

Closed
Condordito opened this issue May 15, 2024 · 2 comments
Assignees
Labels
bug Something isn't working enhancement New feature or request opinions Extra attention is needed performance Performance improvements special case Rare cases that need to be handled in the code manually by hardcoding a part of the system

Comments

@Condordito
Copy link
Contributor

Condordito commented May 15, 2024

Description

At the moment, applying textures does not work correctly in multiple versions. Each one has slightly different behaviors. This issue was previously discussed in another post, but no further details were given on the topic (#235)

1.12 (Online Server)

When applying a meta to an item stack, it generates the textures only if its GameProfile has a defined name or its UUID is stored in the server cache. Compared to the rest, the textures are applied synchronously, before being placed in an inventory

Online 1 12

Screenshot 2024-05-15 at 1 48 03 AM Screenshot 2024-05-15 at 1 47 42 AM

1.12.2 (Offline Mode)

Only textures applied via a cached UUID will be visible and stacked. None applied with a name are visible, but this is because SkullUtils.applySkinFromName skips these cases. They apply correctly otherwise

Offline 1 12

1.19.4 (Online Mode)

Textures from offline users will be applied on the next tick, causing them not to stack when placed in an inventory. As in 1.12, users with a non-cached UUID are not visible

Online 1 19

Screenshot 2024-05-15 at 2 22 42 AM Screenshot 2024-05-15 at 2 22 33 AM

1.19.4 (Offline Mode)

Textures applied with usernames are visible if the SkullUtils.applySkinFromName's UUID check is ignored, except when the user is logged in. If applied via UUID, the texture will be visible, but only if the server caches it and the user is offline

Offline 1 19

1.20.4 (Online Mode)

It works the same as in 1.19. The difference is that this throws an error if the UUID is not cached, since here it returns null for the offline player instance

Online 1 20

1.20.4 (Offline Mode)

No textures are applied when using usernames, regardless of disabling SkullUtils.applySkinFromName checking for offline uuids

Offline 1 20

At first, I had in mind grabbing the server cache, as mentioned in #235, and then requesting an external API. However, this still maintains inconsistencies between versions and online/offline modes

Draft for how it could be handled (b69f315)

It caches all textures but doesn't make those first requests asynchronous, blocking the main thread

1.12.2 (Offline Mode)

Mod Offline 1 12

1.20.4 (Online Mode)

Mod Online 1 20

@Condordito Condordito added the bug Something isn't working label May 15, 2024
@CryptoMorin
Copy link
Owner

CryptoMorin commented May 17, 2024

Thank you for the detailed report. I briefly reviewed this. I'm just going to sum up the issues with the proposed solution:

  • The code will block the main thread for uncached values. This seems to be the case with the server's cache too in older versions. But it's kind of async in newer ones. I'm not really sure about that.
  • The current code will not properly cache values in case of a temporary network issue (like timeouts) which can be fixed by either requesting again or not caching it and waiting for the next request.
  • Even if all the above were fixed, this will double the server's memory, CPU and network usage when the same textures are requested from another plugin since we're keeping our own separate cache. I guess this can probably be fixed with some reflection hackery, but I don't think it's worth it.

Anyways, I've pushed some changes in 421a61a based on your proposed solution. I also made UUID/OfflinePlayer requests compatible with offline mode servers.

@CryptoMorin CryptoMorin added enhancement New feature or request opinions Extra attention is needed special case Rare cases that need to be handled in the code manually by hardcoding a part of the system performance Performance improvements labels May 17, 2024
@CryptoMorin CryptoMorin changed the title [SkullUtils] - Applying textures by UUIDs and names [XSkull] - Inconsistent behavior in different server versions & online/offline servers May 17, 2024
@CryptoMorin CryptoMorin changed the title [XSkull] - Inconsistent behavior in different server versions & online/offline servers [XSkull] - Inconsistent behavior in different server versions & online/offline servers based on usernames and UUIDs May 17, 2024
Condordito added a commit to Condordito/XSeries that referenced this issue May 22, 2024
- Add support for offline-mode servers
- Implement asynchronous texture fetching option
- Remove MOJANG_SHA_FAKE_ID_ENUMERATOR to prevent item stacking issues after restarts (UUIDs are now generated using texture hashes)
- Store UUIDs in usercache.json and cache textures on first request
- Ensure cached textures remain consistent across plugin interactions
@Condordito
Copy link
Contributor Author

I made a PR to address those issues, but I'm unsure if XSeries should cache profiles when using base64 textures

  • Other plugins may apply additional logic before caching, making XSeries caching redundant
  • Recreating those profiles will not affect network usage or significantly consume CPU resources

Although less likely, I think it's not worth doubling memory usage when multiple plugins request the same texture

Condordito added a commit to Condordito/XSeries that referenced this issue May 26, 2024
- Fix `XSkull.TEXTURES` constant (old clients require the HTTP protocol to render properly).
- Add 1.8 remap for MinecraftServer getter.
- Replace `XSkull.applySkin` with `XSkull#of` to handle both async and sync tasks.
- Remove cache for profiles generated from base64 (CryptoMorin#254).
- Log exceptions using debug level.
- Rename `XSkull#getSkinValue` to `XSkull#getTextureValue`.
Condordito added a commit to Condordito/XSeries that referenced this issue May 26, 2024
- Fix `XSkull.TEXTURES` constant (old clients require the HTTP protocol to render properly).
- Add 1.8 remap for MinecraftServer getter.
- Deprecate `XSkull.applySkin` in favor of `XSkull#of`
- Remove cache for profiles generated from base64 (CryptoMorin#254).
- Log exceptions using debug level.
CryptoMorin added a commit that referenced this issue May 30, 2024
Fix XSkull#applySkin with name or UUID inputs (#254)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request opinions Extra attention is needed performance Performance improvements special case Rare cases that need to be handled in the code manually by hardcoding a part of the system
Projects
None yet
Development

No branches or pull requests

2 participants