Merge pull request #502 from JCThePants/skins2-2

bug fix and optimizations in NPC skins

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

@@ -34,6 +34,7 @@ import net.citizensnpcs.api.event.NPCDeathEvent;
 import net.citizensnpcs.api.event.NPCDespawnEvent;
 import net.citizensnpcs.api.event.NPCLeftClickEvent;
 import net.citizensnpcs.api.event.NPCRightClickEvent;
+import net.citizensnpcs.api.event.NPCSpawnEvent;
 import net.citizensnpcs.api.event.PlayerCreateNPCEvent;
 import net.citizensnpcs.api.npc.NPC;
 import net.citizensnpcs.api.npc.NPCRegistry;
@@ -81,6 +82,7 @@ import org.bukkit.event.world.WorldLoadEvent;
 import org.bukkit.event.world.WorldUnloadEvent;
 import org.bukkit.inventory.meta.SkullMeta;
 import org.bukkit.scheduler.BukkitRunnable;
+import org.bukkit.scheduler.BukkitTask;
 import org.bukkit.scoreboard.Team;
 
 public class EventListen implements Listener {
@@ -322,6 +324,16 @@ public class EventListen implements Listener {
         toRespawn.put(coord, event.getNPC());
     }
 
+    @EventHandler
+    public void onNPCSpawn(NPCSpawnEvent event) {
+        SkinnableEntity skinnable = NMS.getSkinnable(event.getNPC().getEntity());
+        if (skinnable == null)
+            return;
+
+        // reset nearby players in case they are not looking at the NPC when it spawns.
+        resetNearbyPlayers(skinnable);
+    }
+
     @EventHandler
     public void onNPCDespawn(NPCDespawnEvent event) {
         if (event.getReason() == DespawnReason.PLUGIN || event.getReason() == DespawnReason.REMOVAL) {
@@ -441,7 +453,6 @@ public class EventListen implements Listener {
     // a player moves a certain distance from their last position.
     @EventHandler(priority = EventPriority.MONITOR, ignoreCancelled = true)
     public void onPlayerMove(final PlayerMoveEvent event) {
-
         SkinUpdateTracker updateTracker = skinUpdateTrackers.get(event.getPlayer().getUniqueId());
         if (updateTracker == null)
             return;
@@ -460,17 +471,25 @@ public class EventListen implements Listener {
             if (player.hasMetadata("NPC"))
                 continue;
 
-            skinUpdateTrackers.put(player.getUniqueId(), new SkinUpdateTracker(player));
+            SkinUpdateTracker tracker = skinUpdateTrackers.get(player.getUniqueId());
+            if (tracker == null)
+                continue;
+
+            tracker.hardReset(player);
         }
     }
 
-    public void recalculatePlayer(final Player player, long delay, final boolean isInitial) {
+    public void recalculatePlayer(final Player player, long delay, boolean reset) {
-
         if (player.hasMetadata("NPC"))
             return;
 
-        if (isInitial) {
+        SkinUpdateTracker tracker = skinUpdateTrackers.get(player.getUniqueId());
-            skinUpdateTrackers.put(player.getUniqueId(), new SkinUpdateTracker(player));
+        if (tracker == null) {
+            tracker = new SkinUpdateTracker(player);
+            skinUpdateTrackers.put(player.getUniqueId(), tracker);
+        }
+        else if (reset) {
+            tracker.hardReset(player);
         }
 
         new BukkitRunnable() {
@@ -480,20 +499,42 @@ public class EventListen implements Listener {
 
                 List<SkinnableEntity> nearbyNPCs = getNearbySkinnableNPCs(player);
                 for (SkinnableEntity npc : nearbyNPCs) {
-                     npc.getSkinTracker().updateViewer(player);
+                    npc.getSkinTracker().updateViewer(player);
-                }
-
-                if (!nearbyNPCs.isEmpty() && isInitial) {
-                    // one more time to help when resource pack load times
-                    // prevent immediate skin loading
-                    recalculatePlayer(player, 40, false);
                 }
             }
         }.runTaskLater(CitizensAPI.getPlugin(), delay);
     }
 
-    private List<SkinnableEntity> getNearbySkinnableNPCs(Player player) {
+    // hard reset skin update trackers for players near a skinnable NPC
+    private void resetNearbyPlayers(SkinnableEntity skinnable) {
+        Entity entity = skinnable.getBukkitEntity();
+        if (entity == null || !entity.isValid())
+            return;
 
+        double viewDistance = Setting.NPC_SKIN_VIEW_DISTANCE.asDouble();
+        viewDistance *= viewDistance;
+        Location location = entity.getLocation(NPC_LOCATION);
+        List<Player> players = entity.getWorld().getPlayers();
+        for (Player player : players) {
+            if (player.hasMetadata("NPC"))
+                continue;
+
+            double distanceSquared = player.getLocation(CACHE_LOCATION).distanceSquared(location);
+            if (distanceSquared > viewDistance)
+                continue;
+
+            SkinUpdateTracker tracker = skinUpdateTrackers.get(player.getUniqueId());
+            if (tracker == null) {
+                tracker = new SkinUpdateTracker(player);
+                skinUpdateTrackers.put(player.getUniqueId(), tracker);
+            }
+            else {
+                tracker.hardReset(player);
+            }
+        }
+    }
+
+    private List<SkinnableEntity> getNearbySkinnableNPCs(Player player) {
         List<SkinnableEntity> results = new ArrayList<SkinnableEntity>();
 
         double viewDistance = Setting.NPC_SKIN_VIEW_DISTANCE.asDouble();
@@ -506,7 +547,7 @@ public class EventListen implements Listener {
                     && player.canSee((Player) npcEntity)
                     && player.getWorld().equals(npcEntity.getWorld())
                     && player.getLocation(CACHE_LOCATION)
-                        .distanceSquared(npc.getStoredLocation()) < viewDistance) {
+                    .distanceSquared(npc.getStoredLocation()) < viewDistance) {
 
                 SkinnableEntity skinnable = NMS.getSkinnable(npcEntity);
 
@@ -600,21 +641,25 @@ public class EventListen implements Listener {
     }
 
     private class SkinUpdateTracker {
-        float initialYaw;
         final Location location = new Location(null, 0, 0, 0);
         int rotationCount;
+        boolean hasMoved;
         float upperBound;
         float lowerBound;
 
         SkinUpdateTracker(Player player) {
-            reset(player);
+            hardReset(player);
         }
 
         boolean shouldUpdate(Player player) {
+            Location currentLoc = player.getLocation(CACHE_LOCATION);
 
-            Location currentLoc = player.getLocation(YAW_LOCATION);
+            if (!hasMoved) {
+                hasMoved = true;
+                return true;
+            }
 
-            if (rotationCount < 2) {
+            if (rotationCount < 3) {
                 float yaw = NMS.clampYaw(currentLoc.getYaw());
                 boolean hasRotated = upperBound < lowerBound
                         ? yaw > upperBound && yaw < lowerBound
@@ -643,15 +688,23 @@ public class EventListen implements Listener {
         // resets initial yaw and location to the players
         // current location and yaw.
         void reset(Player player) {
-            player.getLocation(location);
+            player.getLocation(this.location);
-            this.initialYaw = NMS.clampYaw(location.getYaw());
+            if (rotationCount < 3) {
-            float rotationDegrees = Setting.NPC_SKIN_ROTATION_UPDATE_DEGREES.asFloat();
+                float rotationDegrees = Setting.NPC_SKIN_ROTATION_UPDATE_DEGREES.asFloat();
-            this.upperBound = NMS.clampYaw(this.initialYaw + rotationDegrees);
+                float yaw = NMS.clampYaw(this.location.getYaw());
-            this.lowerBound = NMS.clampYaw(this.initialYaw - rotationDegrees);
+                this.upperBound = NMS.clampYaw(yaw + rotationDegrees);
+                this.lowerBound = NMS.clampYaw(yaw - rotationDegrees);
+            }
+        }
+
+        void hardReset(Player player) {
+            this.hasMoved = false;
+            this.rotationCount = 0;
+            reset(player);
         }
     }
 
-    private static final Location YAW_LOCATION = new Location(null, 0, 0, 0);
     private static final Location CACHE_LOCATION = new Location(null, 0, 0, 0);
+    private static final Location NPC_LOCATION = new Location(null, 0, 0, 0);
     private static final int MOVEMENT_SKIN_UPDATE_DISTANCE = 50 * 50;
 }

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

@@ -189,23 +189,11 @@ public class CitizensNPC extends AbstractNPC {
         boolean couldSpawn = !Util.isLoaded(at) ? false : mcEntity.world.addEntity(mcEntity, SpawnReason.CUSTOM);
 
         // send skin packets, if applicable, before other NMS packets are sent
-        SkinnableEntity skinnable = NMS.getSkinnable(getEntity());
+        if (couldSpawn) {
-        if (skinnable != null) {
+            SkinnableEntity skinnable = NMS.getSkinnable(getEntity());
-            final double viewDistance = Settings.Setting.NPC_SKIN_VIEW_DISTANCE.asDouble();
+            if (skinnable != null) {
-            //skinnable.getSkinTracker().updateNearbyViewers(viewDistance);
+                skinnable.getSkinTracker().onSpawnNPC();
-            Bukkit.getScheduler().scheduleSyncDelayedTask(CitizensAPI.getPlugin(), new Runnable() {
+            }
-                @Override
-                public void run() {
-                    if (getEntity() == null || !getEntity().isValid())
-                        return;
-
-                    SkinnableEntity npc = NMS.getSkinnable(getEntity());
-                    if (npc == null)
-                        return;
-
-                    npc.getSkinTracker().updateNearbyViewers(viewDistance);
-                }
-            }, 20);
         }
 
         mcEntity.setPositionRotation(at.getX(), at.getY(), at.getZ(), at.getYaw(), at.getPitch());

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

@@ -182,7 +182,6 @@ public class Skin {
     }
 
     private void fetch() {
-
         final int maxRetries = Setting.MAX_NPC_SKIN_RETRIES.asInt();
         if (maxRetries > -1 && fetchRetries >= maxRetries) {
             if (Messaging.isDebugging()) {
@@ -215,7 +214,7 @@ public class Skin {
                         }, delay);
 
                         if (Messaging.isDebugging()) {
-                            Messaging.debug("Retrying skin fetch for '" + skinName + "' in " + delay + "ticks.");
+                            Messaging.debug("Retrying skin fetch for '" + skinName + "' in " + delay + " ticks.");
                         }
                         break;
                     case SUCCESS:
@@ -227,6 +226,33 @@ public class Skin {
         });
     }
 
+    /**
+     * Get a player skin.
+     *
+     * <p>
+     * If a Skin instance does not exist, a new one is created and the skin data is automatically fetched.
+     * </p>
+     *
+     * @param skinName
+     *            The name of the skin.
+     */
+    public static Skin get(String skinName) {
+        Preconditions.checkNotNull(skinName);
+
+        skinName = skinName.toLowerCase();
+
+        Skin skin;
+        synchronized (CACHE) {
+            skin = CACHE.get(skinName);
+        }
+
+        if (skin == null) {
+            skin = new Skin(skinName);
+        }
+
+        return skin;
+    }
+
     /**
      * Get a skin for a skinnable entity.
      *
@@ -241,17 +267,7 @@ public class Skin {
         Preconditions.checkNotNull(entity);
 
         String skinName = entity.getSkinName().toLowerCase();
-
+        return get(skinName);
-        Skin skin;
-        synchronized (CACHE) {
-            skin = CACHE.get(skinName);
-        }
-
-        if (skin == null) {
-            skin = new Skin(skinName);
-        }
-
-        return skin;
     }
 
     /**
@@ -272,7 +288,8 @@ public class Skin {
     private static void setNPCSkinData(SkinnableEntity entity, String skinName, UUID skinId, Property skinProperty) {
         NPC npc = entity.getNPC();
 
-        // cache skins for faster initial skin availability
+        // cache skins for faster initial skin availability and
+        // for use when the latest skin is not required.
         npc.data().setPersistent(CACHED_SKIN_UUID_NAME_METADATA, skinName);
         npc.data().setPersistent(CACHED_SKIN_UUID_METADATA, skinId.toString());
         if (skinProperty.getValue() != null) {
@@ -287,6 +304,16 @@ public class Skin {
 
     private static void setNPCTexture(SkinnableEntity entity, Property skinProperty) {
         GameProfile profile = entity.getProfile();
+
+        // don't set property if already set since this sometimes causes
+        // packet errors that disconnect the client.
+        Property current = Iterables.getFirst(profile.getProperties().get("textures"), null);
+        if (current != null
+                && current.getValue().equals(skinProperty.getValue())
+                && current.getSignature().equals(skinProperty.getSignature())) {
+            return;
+        }
+
         profile.getProperties().removeAll("textures"); // ensure client does not crash due to duplicate properties.
         profile.getProperties().put("textures", skinProperty);
     }

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

@@ -5,12 +5,14 @@ import java.util.HashMap;
 import java.util.Map;
 import java.util.UUID;
 
+import net.citizensnpcs.npc.CitizensNPC;
 import org.bukkit.Bukkit;
 import org.bukkit.Location;
 import org.bukkit.entity.Player;
 import org.bukkit.event.EventHandler;
 import org.bukkit.event.Listener;
 import org.bukkit.event.player.PlayerQuitEvent;
+import org.bukkit.scheduler.BukkitRunnable;
 import org.bukkit.scheduler.BukkitTask;
 
 import com.google.common.base.Preconditions;
@@ -98,6 +100,23 @@ public class SkinPacketTracker {
         skin.applyAndRespawn(entity);
     }
 
+    /**
+     * Invoke when the NPC entity is spawned.
+     */
+    public void onSpawnNPC() {
+        isRemoved = false;
+        new BukkitRunnable() {
+            @Override
+            public void run() {
+                if (!entity.getNPC().isSpawned())
+                    return;
+
+                double viewDistance = Settings.Setting.NPC_SKIN_VIEW_DISTANCE.asDouble();
+                updateNearbyViewers(viewDistance);
+            }
+        }.runTaskLater(CitizensAPI.getPlugin(), 20);
+    }
+
     /**
      * Invoke when the NPC entity is removed.
      *
@@ -164,11 +183,11 @@ public class SkinPacketTracker {
         Player from = entity.getBukkitEntity();
         Location location = from.getLocation();
 
-        for (Player player : Bukkit.getServer().getOnlinePlayers()) {
+        for (Player player : world.getPlayers()) {
             if (player == null || player.hasMetadata("NPC"))
                 continue;
 
-            if (world != player.getWorld() || !player.canSee(from))
+            if (!player.canSee(from))
                 continue;
 
             if (location.distanceSquared(player.getLocation(CACHE_LOCATION)) > radius)