[SkullUtils] - Feature / Bug / Help: Apply Skins without calling the Mojang API

[timderspieler]

Description
With the current implementation of SkullUtils, I noticed that for every requested player name and or uuid, the Bukkit.getOfflinePlayer functionality gets called. With this, for every applySkinValue request the mojang api will be called to fetch a skin. The skin then won't be, for my understanding, cached. This results in one api call for literally every skin request (even though the skin might have been pulled already).

To prevent that, I tried a different approach. Normally the skin base64 string is stored in the CraftPlayer object. That means, whenever a skin gets requested of a player that is currently online, it should be possible to simply get the CraftPlayer object of that player, take the base64 string out of the object and just apply it to the skull.

This is the code I came up with. Sadly the ReflectionUtils.getHandle(Player player) function does not return an CraftPlayer object but an EntityPlayer object instead.

public static String getSkinValue(Player player) {
	Objects.requireNonNull(player, "Player cannot be null");
	try {
		Object handle = ReflectionUtils.getHandle(player);
		Object profile = handle.getClass().getMethod("getProfile").invoke(handle);

		Object propertyMap = profile.getClass().getMethod("getProperties").invoke(profile);
		Method[] methods = propertyMap.getClass().getMethods();
		for (Method method : methods) {
			if (method.getName().equals("get")) {
				Object value = method.invoke(propertyMap, "textures");
				if (value != null) {
					for (Object texture : ((Iterable) value)) {
						Method getValue = texture.getClass().getMethod("getValue");
						String skin = (String) (getValue.invoke(texture));
						return skin;
					}
				}
			}
		}
	} catch (NoSuchMethodException
			 | IllegalAccessException | InvocationTargetException e) {
		e.printStackTrace();
	}
	return null;
}

I am sadly not to familiar in working with minecraft related reflections, Therefore I would like to ask to get help creating such a method and or if it would be possible to add it to the XSeries lib.

Thanks in advance!

[CryptoMorin]

Can you point out the exact NMS/CraftBukkit class and method name in which you think this type of caching doesn't occur?
getOfflinePlayer(UUID) returns the online player object if they're, well, online.

Also there is no applySkinValue() method in SkullUtils anymore, can you point out the new method name?

[timderspieler]

"Can you point out the exact NMS/CraftBukkit class and method name in which you think this type of caching doesn't occur?
getOfflinePlayer(UUID) returns the online player object if they're, well, online."
If thats the case, my point is no longer valid and I am fine with that. The issue is, that I still have some slight performance issues using the SkullUtils. It's making HttpRequests where it (normally) shouldn't for my understanding. I thought that if a player is already online, the skin value of the player can be fetched using its GameProfile... or is that the case?

(Spark Report a user gave me: https://spark.lucko.me/7iMSIN2ZrX )

[CryptoMorin]

This is certainly a problem when the string name is used, but it's cached, so I'm not sure why this is happening. The only thing I can think of is the explanation I marked with "@@@" in the second method.

If that is the case, I guess I can detect it by comparing the UUID to the one I marked with "***" in the first method in my own custom cache?

public OfflinePlayer getOfflinePlayer(String name) {
  Preconditions.checkArgument(name != null, "name cannot be null");
  Preconditions.checkArgument(!name.isBlank(), "name cannot be empty");

  // Checks if the player is online to get the online instance
  OfflinePlayer result = this.getPlayerExact(name);
  if (result == null) {
      GameProfile profile = null;
      if (this.getOnlineMode() || SpigotConfig.bungee) {
          // Profile lookup is performed here, obfuscated version of get() method below
          profile = (GameProfile)this.console.ap().a(name).orElse((Object)null);
      }

      if (profile == null) {
          result = this.getOfflinePlayer(new GameProfile(UUID.nameUUIDFromBytes(("OfflinePlayer:" + name).getBytes(Charsets.UTF_8)), name));
          // *** Profiles that are not found are returned with a special UUID, I can check the returned offlineplayer's UUID and if its the same as the one encoded here, i can blacklist it.
         // For reference the this.getOfflinePlayer above makes a new offlineplayer with the same UUID as its profile, but it never actually uses it back later for some reasons...
      } else {
          result = this.getOfflinePlayer(profile);
      }
  } else {
      this.offlinePlayers.remove(((OfflinePlayer)result).getUniqueId());
  }

  return (OfflinePlayer)result;
}

public Optional<GameProfile> get(String s) {
    String s1 = s.toLowerCase(Locale.ROOT);
    UserCache.UserCacheEntry usercache_usercacheentry = (UserCache.UserCacheEntry) this.profilesByName.get(s1);
    boolean flag = false;

    if (usercache_usercacheentry != null && (new Date()).getTime() >= usercache_usercacheentry.expirationDate.getTime()) {
        this.profilesByUUID.remove(usercache_usercacheentry.getProfile().getId());
        this.profilesByName.remove(usercache_usercacheentry.getProfile().getName().toLowerCase(Locale.ROOT));
        flag = true;
        usercache_usercacheentry = null;
    }

    Optional optional;

    if (usercache_usercacheentry != null) {
        usercache_usercacheentry.setLastAccess(this.getNextOperation());
        optional = Optional.of(usercache_usercacheentry.getProfile());
    } else {
        optional = lookupGameProfile(this.profileRepository, s); // Spigot - use correct case for offline players
        // Lookup ^^^^^^^^^^^^^^^^^^^^^^^^^
        if (optional.isPresent()) {
            // @@@ So if for some reasons this player's profile doesn't exist on the server,
            // there is no cache to prevent it from looking it up again?
            this.add((GameProfile) optional.get());
            // Cache ^^^^^^^^^^^^^^^
            flag = false;
        }
    }

    if (flag && !org.spigotmc.SpigotConfig.saveUserCacheOnStopOnly) { // Spigot - skip saving if disabled
        this.save();
    }

    return optional;
}

public void add(GameProfile gameprofile) {
    Calendar calendar = Calendar.getInstance();

    calendar.setTime(new Date());
    calendar.add(2, 1);
    // Cached for a month ^
    Date date = calendar.getTime();
    UserCache.UserCacheEntry usercache_usercacheentry = new UserCache.UserCacheEntry(gameprofile, date);

    this.safeAdd(usercache_usercacheentry);
    if( !org.spigotmc.SpigotConfig.saveUserCacheOnStopOnly ) this.save(); // Spigot - skip saving if disabled
}

After looking a bit more it appears that what I described can only happen to online mode servers (probably):

private static Optional<GameProfile> lookupGameProfile(GameProfileRepository gameprofilerepository, String s) {
    final AtomicReference<GameProfile> atomicreference = new AtomicReference();
    ProfileLookupCallback profilelookupcallback = new ProfileLookupCallback() {
        public void onProfileLookupSucceeded(GameProfile gameprofile) {
            atomicreference.set(gameprofile);
        }

        public void onProfileLookupFailed(String s1, Exception exception) {
            atomicreference.set(null); // CraftBukkit - decompile error
        }
    };

    gameprofilerepository.findProfilesByNames(new String[]{s}, profilelookupcallback);
    GameProfile gameprofile = (GameProfile) atomicreference.get();

    if (!usesAuthentication() && gameprofile == null) {
        // I'm not sure what usesAuthentication() exactly means but
        // I guess it has something to do with online mode?
        // If that's the case, the report you sent shows it's in online mode 
        // so this block is never getting called anyways.
        UUID uuid = UUIDUtil.createOfflinePlayerUUID(s);

        return Optional.of(new GameProfile(uuid, s));
    } else {
        return Optional.ofNullable(gameprofile);
    }
}

[timderspieler]

This is actually interesting. So I should check if it makes a difference using the UUID of a player instead of the playername?

[CryptoMorin]

If you can use the player's UUID and you have access to that, that's definitely better, you should use the UUID whenever you can instead of the name.

But if your only option to get the skull that you want is the player name (or if it's a configuration option which allows both), I can push a commit to test if my solution would work but I noticed something else. What does DeathLogic.requestHeadDrop() actually do? Is it possible to see the line that uses SkullUtils? What type of argument is passed (I'm assuming it's the player name) and where does it come from?
Because from the looks of it, whatever skull it's requesting should exist and be cached normally.

[timderspieler]

Of course!

requestHeadDrop is a simple method that calculates the chance of a player head being dropped after the player has been killed.
I won't go into too much detail here because despite generating the player head using the SkullUtils, it does some economy related stuff that has nothing to do with XSeries.

But this is the way the SkullUtils are called:

drop_head = getPlugin().getItemUtils().buildSkull(displayname, pp.getName());

and this is the buildSkull method inside the ItemUtils class:

public ItemStack buildSkull(String name, String owner) {

	// Replace special characters in
	owner = owner.replaceAll("[^a-zA-Z0-9]", "");

	ItemStack result = XMaterial.PLAYER_HEAD.parseItem();
	applyShort(result, (short) 3);
	SkullMeta result_meta = SkullUtils.applySkin(result.getItemMeta(), owner);
	result_meta.setDisplayName(MessageManager.colorize(name));
	result.setItemMeta(result_meta);
	
	return result;
	
}

[CryptoMorin]

So it gets the skull of a player after they died. That means the player is online and the server should cache it properly. Can't you just directly use their UUID instead?
This question is kind of unrelated to the main issue, but I'm not sure what's the difference between name and owner don't they both originate from Player#getName()? Why is the replaceAll() line necessary?

Anyways, I pushed some changes: d98b12c so for now copy paste this as a standalone class in your plugin until we can decide if this is a viable fix.

[timderspieler]

@CryptoMorin sorry for the late reply but I really appreciate your help. Yes I completely changed the library calls to primarly use UUIDs wherever I can.

Thank you very much. I will close this issue for now.

Have a great weekend!