fix skin reload on citizens reload

reset skin and profile fetcher cache on citizens reload

fix -p flag in skin command not implemented

use latest skin by default

fix Property created for cached skins uses incorrect name

fix rotation based skin update in EventListen.SkinUpdateTracker

remove redundant skin update on first move in EventListen.SkinUpdateTracker

re-implement skin fetch retry; re-add settings (since reloading can cause too many requests)

src/main/java/net/citizensnpcs/Citizens.java

@@ -55,6 +55,8 @@ import net.citizensnpcs.npc.CitizensTraitFactory;
 import net.citizensnpcs.npc.NPCSelector;
 import net.citizensnpcs.npc.ai.speech.Chat;
 import net.citizensnpcs.npc.ai.speech.CitizensSpeechFactory;
+import net.citizensnpcs.npc.profile.ProfileFetcher;
+import net.citizensnpcs.npc.skin.Skin;
 import net.citizensnpcs.util.Messages;
 import net.citizensnpcs.util.NMS;
 import net.citizensnpcs.util.StringHelper;
@@ -340,6 +342,8 @@ public class Citizens extends JavaPlugin implements CitizensPlugin {
         Editor.leaveAll();
         config.reload();
         despawnNPCs();
+        ProfileFetcher.reset();
+        Skin.clearCache();
         saves.loadInto(npcRegistry);
 
         getServer().getPluginManager().callEvent(new CitizensReloadEvent());

src/main/java/net/citizensnpcs/EventListen.java

@@ -18,6 +18,7 @@ import com.mojang.authlib.properties.Property;
 import net.citizensnpcs.Settings.Setting;
 import net.citizensnpcs.api.CitizensAPI;
 import net.citizensnpcs.api.event.CitizensDeserialiseMetaEvent;
+import net.citizensnpcs.api.event.CitizensReloadEvent;
 import net.citizensnpcs.api.event.CitizensSerialiseMetaEvent;
 import net.citizensnpcs.api.event.CommandSenderCreateNPCEvent;
 import net.citizensnpcs.api.event.DespawnReason;
@@ -451,6 +452,18 @@ public class EventListen implements Listener {
         recalculatePlayer(event.getPlayer(), 10, false);
     }
 
+    @EventHandler(priority = EventPriority.MONITOR)
+    public void onCitizensReload(CitizensReloadEvent event) {
+        skinUpdateTrackers.clear();
+        for (Player player : Bukkit.getOnlinePlayers()) {
+
+            if (player.hasMetadata("NPC"))
+                continue;
+
+            skinUpdateTrackers.put(player.getUniqueId(), new SkinUpdateTracker(player));
+        }
+    }
+
     public void recalculatePlayer(final Player player, long delay, final boolean isInitial) {
 
         if (player.hasMetadata("NPC"))
@@ -589,8 +602,9 @@ public class EventListen implements Listener {
     private class SkinUpdateTracker {
         float initialYaw;
         final Location location = new Location(null, 0, 0, 0);
-        boolean hasMoved;
         int rotationCount;
+        float upperBound;
+        float lowerBound;
 
         SkinUpdateTracker(Player player) {
             reset(player);
@@ -598,22 +612,13 @@ public class EventListen implements Listener {
 
         boolean shouldUpdate(Player player) {
 
-            // check if this is the first time the player has moved
-            if (!hasMoved) {
-                hasMoved = true;
-                reset(player);
-                return true;
-            }
-
             Location currentLoc = player.getLocation(YAW_LOCATION);
-            float currentYaw = currentLoc.getYaw();
 
             if (rotationCount < 2) {
-
+                float yaw = NMS.clampYaw(currentLoc.getYaw());
-                float rotationDegrees = Setting.NPC_SKIN_ROTATION_UPDATE_DEGREES.asFloat();
+                boolean hasRotated = upperBound < lowerBound
-
+                        ? yaw > upperBound && yaw < lowerBound
-                boolean hasRotated =
+                        : yaw > upperBound || yaw < lowerBound;
-                        Math.abs(NMS.clampYaw(currentYaw - this.initialYaw)) < rotationDegrees;
 
                 // update the first 2 times the player rotates. helps load skins around player
                 // after the player logs/teleports.
@@ -639,7 +644,10 @@ public class EventListen implements Listener {
         // current location and yaw.
         void reset(Player player) {
             player.getLocation(location);
-            this.initialYaw = location.getYaw();
+            this.initialYaw = NMS.clampYaw(location.getYaw());
+            float rotationDegrees = Setting.NPC_SKIN_ROTATION_UPDATE_DEGREES.asFloat();
+            this.upperBound = NMS.clampYaw(this.initialYaw + rotationDegrees);
+            this.lowerBound = NMS.clampYaw(this.initialYaw - rotationDegrees);
         }
     }
 

src/main/java/net/citizensnpcs/Settings.java

@@ -88,6 +88,7 @@ public class Settings {
         KEEP_CHUNKS_LOADED("npc.chunks.always-keep-loaded", false),
         LOCALE("general.translation.locale", ""),
         MAX_NPC_LIMIT_CHECKS("npc.limits.max-permission-checks", 100),
+        MAX_NPC_SKIN_RETRIES("npc.skins.max-retries", -1),
         MAX_PACKET_ENTRIES("npc.limits.max-packet-entries", 15),
         MAX_SPEED("npc.limits.max-speed", 100),
         MAX_TEXT_RANGE("npc.chat.options.max-text-range", 500),
@@ -95,8 +96,9 @@ public class Settings {
         NEW_PATHFINDER_OPENS_DOORS("npc.pathfinding.new-finder-open-doors", false),
         NPC_ATTACK_DISTANCE("npc.pathfinding.attack-range", 1.75 * 1.75),
         NPC_COST("economy.npc.cost", 100D),
-        NPC_SKIN_UPDATE("npc.skins.update", false),
+        NPC_SKIN_USE_LATEST("npc.skins.use-latest", true),
         NPC_SKIN_VIEW_DISTANCE("npc.skins.view-distance", 100D),
+        NPC_SKIN_RETRY_DELAY("npc.skins.retry-delay", 120),
         NPC_SKIN_ROTATION_UPDATE_DEGREES("npc.skins.rotation-update-degrees", 90f),
         PACKET_UPDATE_DELAY("npc.packets.update-delay", 30),
         QUICK_SELECT("npc.selection.quick-select", false),

src/main/java/net/citizensnpcs/commands/NPCCommands.java

@@ -1307,7 +1307,7 @@ public class NPCCommands {
                 throw new CommandException();
             npc.data().setPersistent(NPC.PLAYER_SKIN_UUID_METADATA, args.getString(1));
             if (args.hasFlag('p')) {
-                npc.data().setPersistent(NPC.PLAYER_SKIN_TEXTURE_PROPERTIES_METADATA, "cache");
+                npc.data().setPersistent(NPC.PLAYER_SKIN_USE_LATEST, false);
             }
             skinName = args.getString(1);
         }

src/main/java/net/citizensnpcs/npc/CitizensNPC.java

@@ -192,7 +192,7 @@ public class CitizensNPC extends AbstractNPC {
         SkinnableEntity skinnable = NMS.getSkinnable(getEntity());
         if (skinnable != null) {
             final double viewDistance = Settings.Setting.NPC_SKIN_VIEW_DISTANCE.asDouble();
-            skinnable.getSkinTracker().updateNearbyViewers(viewDistance);
+            //skinnable.getSkinTracker().updateNearbyViewers(viewDistance);
             Bukkit.getScheduler().scheduleSyncDelayedTask(CitizensAPI.getPlugin(), new Runnable() {
                 @Override
                 public void run() {

src/main/java/net/citizensnpcs/npc/profile/ProfileFetchThread.java

@@ -57,11 +57,15 @@ class ProfileFetchThread implements Runnable {
                 requested.put(name, request);
                 return;
             }
+            else if (request.getResult() == ProfileFetchResult.TOO_MANY_REQUESTS) {
+                queue.add(request);
+            }
         }
 
         if (handler != null) {
 
-            if (request.getResult() == ProfileFetchResult.PENDING) {
+            if (request.getResult() == ProfileFetchResult.PENDING ||
+                    request.getResult() == ProfileFetchResult.TOO_MANY_REQUESTS) {
                 addHandler(request, handler);
             } else {
                 sendResult(handler, request);

src/main/java/net/citizensnpcs/npc/profile/ProfileFetcher.java

@@ -1,10 +1,10 @@
 package net.citizensnpcs.npc.profile;
 
 import java.util.Collection;
-
 import javax.annotation.Nullable;
 
 import org.bukkit.Bukkit;
+import org.bukkit.scheduler.BukkitTask;
 
 import com.google.common.base.Preconditions;
 import com.mojang.authlib.Agent;
@@ -109,12 +109,18 @@ public class ProfileFetcher {
         Preconditions.checkNotNull(name);
 
         if (PROFILE_THREAD == null) {
-            PROFILE_THREAD = new ProfileFetchThread();
+            initThread();
-            Bukkit.getScheduler().runTaskTimerAsynchronously(CitizensAPI.getPlugin(), PROFILE_THREAD, 11, 20);
         }
         PROFILE_THREAD.fetch(name, handler);
     }
 
+    /**
+     * Clear all queued and cached requests.
+     */
+    public static void reset() {
+        initThread();
+    }
+
     @Nullable
     private static ProfileRequest findRequest(String name, Collection<ProfileRequest> requests) {
         name = name.toLowerCase();
@@ -149,5 +155,15 @@ public class ProfileFetcher {
                 || (cause != null && cause.contains("too many requests"));
     }
 
+    private static void initThread() {
+        if (THREAD_TASK != null)
+            THREAD_TASK.cancel();
+
+        PROFILE_THREAD = new ProfileFetchThread();
+        THREAD_TASK = Bukkit.getScheduler().runTaskTimerAsynchronously(
+                CitizensAPI.getPlugin(), PROFILE_THREAD, 21, 20);
+    }
+
     private static ProfileFetchThread PROFILE_THREAD;
+    private static BukkitTask THREAD_TASK;
 }

src/main/java/net/citizensnpcs/npc/skin/Skin.java

@@ -1,6 +1,8 @@
 package net.citizensnpcs.npc.skin;
 
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.UUID;
 import java.util.WeakHashMap;
@@ -12,11 +14,15 @@ import com.google.common.collect.Iterables;
 import com.mojang.authlib.GameProfile;
 import com.mojang.authlib.properties.Property;
 
+import org.bukkit.Bukkit;
+import org.bukkit.scheduler.BukkitTask;
+
 import net.citizensnpcs.Settings.Setting;
+import net.citizensnpcs.api.CitizensAPI;
 import net.citizensnpcs.api.event.DespawnReason;
 import net.citizensnpcs.api.npc.NPC;
+import net.citizensnpcs.api.util.Messaging;
 import net.citizensnpcs.npc.profile.ProfileFetchHandler;
-import net.citizensnpcs.npc.profile.ProfileFetchResult;
 import net.citizensnpcs.npc.profile.ProfileFetcher;
 import net.citizensnpcs.npc.profile.ProfileRequest;
 
@@ -24,11 +30,14 @@ import net.citizensnpcs.npc.profile.ProfileRequest;
  * Stores data for a single skin.
  */
 public class Skin {
+    private boolean hasFetched;
     private volatile boolean isValid = true;
     private final Map<SkinnableEntity, Void> pending = new WeakHashMap<SkinnableEntity, Void>(15);
+    private BukkitTask retryTask;
     private volatile Property skinData;
     private volatile UUID skinId;
     private final String skinName;
+    private int fetchRetries = -1;
 
     /**
      * Constructor.
@@ -46,21 +55,7 @@ public class Skin {
             CACHE.put(this.skinName, this);
         }
 
-        ProfileFetcher.fetch(this.skinName, new ProfileFetchHandler() {
+        fetch();
-            @Override
-            public void onResult(ProfileRequest request) {
-
-                if (request.getResult() == ProfileFetchResult.NOT_FOUND) {
-                    isValid = false;
-                    return;
-                }
-
-                if (request.getResult() == ProfileFetchResult.SUCCESS) {
-                    GameProfile profile = request.getProfile();
-                    setData(profile);
-                }
-            }
-        });
     }
 
     /**
@@ -74,41 +69,40 @@ public class Skin {
      * @param entity
      *            The skinnable entity.
      *
-     * @return True if the skin data was available and applied, false if the data is being retrieved.
+     * @return True if skin was applied, false if the data is being retrieved.
      */
     public boolean apply(SkinnableEntity entity) {
         Preconditions.checkNotNull(entity);
 
         NPC npc = entity.getNPC();
 
-        if (!hasSkinData()) {
         // Use npc cached skin if available.
         // If npc requires latest skin, cache is used for faster
         // availability until the latest skin can be loaded.
         String cachedName = npc.data().get(CACHED_SKIN_UUID_NAME_METADATA);
-            if (this.skinName.equals(cachedName)) {
+        String texture = npc.data().get(NPC.PLAYER_SKIN_TEXTURE_PROPERTIES_METADATA, "cache");
-                skinData = new Property(this.skinName,
+        if (this.skinName.equals(cachedName) && !texture.equals("cache")) {
-                        npc.data().<String> get(NPC.PLAYER_SKIN_TEXTURE_PROPERTIES_METADATA),
+            Property localData = new Property("textures", texture,
                     npc.data().<String> get(NPC.PLAYER_SKIN_TEXTURE_PROPERTIES_SIGN_METADATA));
-
+            setNPCTexture(entity, localData);
-                skinId = UUID.fromString(npc.data().<String> get(CACHED_SKIN_UUID_METADATA));
-                setNPCSkinData(entity, skinName, skinId, skinData);
 
             // check if NPC prefers to use cached skin over the latest skin.
-                if (!entity.getNPC().data().get("update-skin", Setting.NPC_SKIN_UPDATE.asBoolean())) {
+            if (!entity.getNPC().data().get(NPC.PLAYER_SKIN_USE_LATEST,
-                    // cache preferred
+                    Setting.NPC_SKIN_USE_LATEST.asBoolean())) {
-                    return true;
-                }
-
-                if (!Setting.NPC_SKIN_UPDATE.asBoolean()) {
                 // cache preferred
                 return true;
             }
         }
 
+        if (!hasSkinData()) {
+            if (hasFetched) {
+                return true;
+            }
+            else {
                 pending.put(entity, null);
                 return false;
             }
+        }
 
         setNPCSkinData(entity, skinName, skinId, skinData);
 
@@ -180,9 +174,57 @@ public class Skin {
         skinId = profile.getId();
         skinData = Iterables.getFirst(profile.getProperties().get("textures"), null);
 
-        for (SkinnableEntity entity : pending.keySet()) {
+        List<SkinnableEntity> entities = new ArrayList<SkinnableEntity>(pending.keySet());
+        for (SkinnableEntity entity : entities) {
             applyAndRespawn(entity);
         }
+        pending.clear();
+    }
+
+    private void fetch() {
+
+        final int maxRetries = Setting.MAX_NPC_SKIN_RETRIES.asInt();
+        if (maxRetries > -1 && fetchRetries >= maxRetries) {
+            if (Messaging.isDebugging()) {
+                Messaging.debug("Reached max skin fetch retries for '" + skinName + "'");
+            }
+            return;
+        }
+
+        ProfileFetcher.fetch(this.skinName, new ProfileFetchHandler() {
+            @Override
+            public void onResult(ProfileRequest request) {
+
+                hasFetched = true;
+
+                switch (request.getResult()) {
+                    case NOT_FOUND:
+                        isValid = false;
+                        break;
+                    case TOO_MANY_REQUESTS:
+                        if (maxRetries == 0) {
+                            break;
+                        }
+                        fetchRetries++;
+                        long delay = Setting.NPC_SKIN_RETRY_DELAY.asLong();
+                        retryTask = Bukkit.getScheduler().runTaskLater(CitizensAPI.getPlugin(), new Runnable() {
+                            @Override
+                            public void run() {
+                                fetch();
+                            }
+                        }, delay);
+
+                        if (Messaging.isDebugging()) {
+                            Messaging.debug("Retrying skin fetch for '" + skinName + "' in " + delay + "ticks.");
+                        }
+                        break;
+                    case SUCCESS:
+                        GameProfile profile = request.getProfile();
+                        setData(profile);
+                        break;
+                }
+            }
+        });
     }
 
     /**
@@ -212,6 +254,21 @@ public class Skin {
         return skin;
     }
 
+    /**
+     * Clear all cached skins.
+     */
+    public static void clearCache() {
+        synchronized (CACHE) {
+            for (Skin skin : CACHE.values()) {
+                skin.pending.clear();
+                if (skin.retryTask != null) {
+                    skin.retryTask.cancel();
+                }
+            }
+            CACHE.clear();
+        }
+    }
+
     private static void setNPCSkinData(SkinnableEntity entity, String skinName, UUID skinId, Property skinProperty) {
         NPC npc = entity.getNPC();
 
@@ -221,16 +278,19 @@ public class Skin {
         if (skinProperty.getValue() != null) {
             npc.data().setPersistent(NPC.PLAYER_SKIN_TEXTURE_PROPERTIES_METADATA, skinProperty.getValue());
             npc.data().setPersistent(NPC.PLAYER_SKIN_TEXTURE_PROPERTIES_SIGN_METADATA, skinProperty.getSignature());
-
+            setNPCTexture(entity, skinProperty);
-            GameProfile profile = entity.getProfile();
-            profile.getProperties().removeAll("textures"); // ensure client does not crash due to duplicate properties.
-            profile.getProperties().put("textures", skinProperty);
         } else {
             npc.data().remove(NPC.PLAYER_SKIN_TEXTURE_PROPERTIES_METADATA);
             npc.data().remove(NPC.PLAYER_SKIN_TEXTURE_PROPERTIES_SIGN_METADATA);
         }
     }
 
+    private static void setNPCTexture(SkinnableEntity entity, Property skinProperty) {
+        GameProfile profile = entity.getProfile();
+        profile.getProperties().removeAll("textures"); // ensure client does not crash due to duplicate properties.
+        profile.getProperties().put("textures", skinProperty);
+    }
+
     private static final Map<String, Skin> CACHE = new HashMap<String, Skin>(20);
     public static final String CACHED_SKIN_UUID_METADATA = "cached-skin-uuid";
     public static final String CACHED_SKIN_UUID_NAME_METADATA = "cached-skin-uuid-name";