Merge pull request #514 from JCThePants/skins2-3

Fix NPE, improve navigating NPC skins, fix tablist setting

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

@@ -1,8 +1,6 @@
 package net.citizensnpcs;
 
-import java.util.ArrayList;
 import java.util.Collection;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -17,6 +15,8 @@ import com.mojang.authlib.properties.Property;
 
 import net.citizensnpcs.Settings.Setting;
 import net.citizensnpcs.api.CitizensAPI;
+import net.citizensnpcs.api.ai.event.NavigationBeginEvent;
+import net.citizensnpcs.api.ai.event.NavigationCompleteEvent;
 import net.citizensnpcs.api.event.CitizensDeserialiseMetaEvent;
 import net.citizensnpcs.api.event.CitizensReloadEvent;
 import net.citizensnpcs.api.event.CitizensSerialiseMetaEvent;
@@ -42,17 +42,17 @@ import net.citizensnpcs.api.trait.trait.Owner;
 import net.citizensnpcs.api.util.DataKey;
 import net.citizensnpcs.api.util.Messaging;
 import net.citizensnpcs.editor.Editor;
-import net.citizensnpcs.npc.skin.SkinnableEntity;
+import net.citizensnpcs.npc.skin.SkinUpdateTracker;
 import net.citizensnpcs.trait.Controllable;
 import net.citizensnpcs.trait.CurrentLocation;
 import net.citizensnpcs.util.Messages;
 import net.citizensnpcs.util.NMS;
+import net.minecraft.server.v1_8_R3.Navigation;
 
 import org.bukkit.Bukkit;
 import org.bukkit.Chunk;
 import org.bukkit.Location;
 import org.bukkit.Material;
-import org.bukkit.entity.Entity;
 import org.bukkit.entity.EntityType;
 import org.bukkit.entity.Player;
 import org.bukkit.event.EventHandler;
@@ -81,19 +81,17 @@ import org.bukkit.event.world.ChunkUnloadEvent;
 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 {
     private final NPCRegistry npcRegistry = CitizensAPI.getNPCRegistry();
     private final Map<String, NPCRegistry> registries;
     private final ListMultimap<ChunkCoord, NPC> toRespawn = ArrayListMultimap.create();
-    private final Map<UUID, SkinUpdateTracker> skinUpdateTrackers =
-            new HashMap<UUID, SkinUpdateTracker>(Bukkit.getMaxPlayers() / 2);
+    private final SkinUpdateTracker skinUpdateTracker;
 
     EventListen(Map<String, NPCRegistry> registries) {
         this.registries = registries;
+        this.skinUpdateTracker = new SkinUpdateTracker(npcRegistry, registries);
     }
 
     private void checkCreationEvent(CommandSenderCreateNPCEvent event) {
@@ -326,12 +324,7 @@ public class EventListen implements Listener {
 
     @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);
+        skinUpdateTracker.onNPCSpawn(event.getNPC());
     }
 
     @EventHandler
@@ -341,6 +334,17 @@ public class EventListen implements Listener {
                 toRespawn.remove(toCoord(event.getNPC().getStoredLocation()), event.getNPC());
             }
         }
+        skinUpdateTracker.onNPCDespawn(event.getNPC());
+    }
+
+    @EventHandler
+    public void onNavigationBegin(NavigationBeginEvent event) {
+        skinUpdateTracker.onNPCNavigationBegin(event.getNPC());
+    }
+
+    @EventHandler
+    public void onNavigationComplete(NavigationCompleteEvent event) {
+        skinUpdateTracker.onNPCNavigationComplete(event.getNPC());
     }
 
     @EventHandler(ignoreCancelled = true)
@@ -354,7 +358,7 @@ public class EventListen implements Listener {
 
     @EventHandler(priority = EventPriority.MONITOR)
     public void onPlayerChangeWorld(PlayerChangedWorldEvent event) {
-        recalculatePlayer(event.getPlayer(), 20, true);
+        skinUpdateTracker.updatePlayer(event.getPlayer(), 20, true);
     }
 
     @EventHandler(ignoreCancelled = true)
@@ -376,7 +380,7 @@ public class EventListen implements Listener {
 
     @EventHandler(priority = EventPriority.MONITOR)
     public void onPlayerJoin(PlayerJoinEvent event) {
-        recalculatePlayer(event.getPlayer(), 20, true);
+        skinUpdateTracker.updatePlayer(event.getPlayer(), 20, true);
     }
 
     @EventHandler(priority = EventPriority.MONITOR, ignoreCancelled = true)
@@ -388,17 +392,17 @@ public class EventListen implements Listener {
                 event.getPlayer().leaveVehicle();
             }
         }
-        skinUpdateTrackers.remove(event.getPlayer().getUniqueId());
+        skinUpdateTracker.removePlayer(event.getPlayer().getUniqueId());
     }
 
     @EventHandler(priority = EventPriority.MONITOR)
     public void onPlayerRespawn(PlayerRespawnEvent event) {
-        recalculatePlayer(event.getPlayer(), 15, true);
+        skinUpdateTracker.updatePlayer(event.getPlayer(), 15, true);
     }
 
     @EventHandler(priority = EventPriority.MONITOR)
     public void onPlayerTeleport(PlayerTeleportEvent event) {
-        recalculatePlayer(event.getPlayer(), 15, true);
+        skinUpdateTracker.updatePlayer(event.getPlayer(), 15, true);
     }
 
     @EventHandler
@@ -453,108 +457,12 @@ 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;
-
-        if (!updateTracker.shouldUpdate(event.getPlayer()))
-            return;
-
-        recalculatePlayer(event.getPlayer(), 10, false);
+        skinUpdateTracker.onPlayerMove(event.getPlayer());
     }
 
     @EventHandler(priority = EventPriority.MONITOR)
     public void onCitizensReload(CitizensReloadEvent event) {
-        skinUpdateTrackers.clear();
-        for (Player player : Bukkit.getOnlinePlayers()) {
-
-            if (player.hasMetadata("NPC"))
-                continue;
-
-            SkinUpdateTracker tracker = skinUpdateTrackers.get(player.getUniqueId());
-            if (tracker == null)
-                continue;
-
-            tracker.hardReset(player);
-        }
-    }
-
-    public void recalculatePlayer(final Player player, long delay, boolean reset) {
-        if (player.hasMetadata("NPC"))
-            return;
-
-        SkinUpdateTracker tracker = skinUpdateTrackers.get(player.getUniqueId());
-        if (tracker == null) {
-            tracker = new SkinUpdateTracker(player);
-            skinUpdateTrackers.put(player.getUniqueId(), tracker);
-        }
-        else if (reset) {
-            tracker.hardReset(player);
-        }
-
-        new BukkitRunnable() {
-
-            @Override
-            public void run() {
-
-                List<SkinnableEntity> nearbyNPCs = getNearbySkinnableNPCs(player);
-                for (SkinnableEntity npc : nearbyNPCs) {
-                    npc.getSkinTracker().updateViewer(player);
-                }
-            }
-        }.runTaskLater(CitizensAPI.getPlugin(), delay);
-    }
-
-    // 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();
-        viewDistance *= viewDistance;
-
-        for (NPC npc : getAllNPCs()) {
-
-            Entity npcEntity = npc.getEntity();
-            if (npcEntity instanceof Player
-                    && player.canSee((Player) npcEntity)
-                    && player.getWorld().equals(npcEntity.getWorld())
-                    && player.getLocation(CACHE_LOCATION)
-                    .distanceSquared(npc.getStoredLocation()) < viewDistance) {
-
-                SkinnableEntity skinnable = NMS.getSkinnable(npcEntity);
-
-                results.add(skinnable);
-            }
-        }
-        return results;
+        skinUpdateTracker.reset();
     }
 
     private void respawnAllFromCoord(ChunkCoord coord) {
@@ -639,72 +547,4 @@ public class EventListen implements Listener {
             return prime * (prime * (prime + ((worldName == null) ? 0 : worldName.hashCode())) + x) + z;
         }
     }
-
-    private class SkinUpdateTracker {
-        final Location location = new Location(null, 0, 0, 0);
-        int rotationCount;
-        boolean hasMoved;
-        float upperBound;
-        float lowerBound;
-
-        SkinUpdateTracker(Player player) {
-            hardReset(player);
-        }
-
-        boolean shouldUpdate(Player player) {
-            Location currentLoc = player.getLocation(CACHE_LOCATION);
-
-            if (!hasMoved) {
-                hasMoved = true;
-                return true;
-            }
-
-            if (rotationCount < 3) {
-                float yaw = NMS.clampYaw(currentLoc.getYaw());
-                boolean hasRotated = upperBound < lowerBound
-                        ? yaw > upperBound && yaw < lowerBound
-                        : yaw > upperBound || yaw < lowerBound;
-
-                // update the first 2 times the player rotates. helps load skins around player
-                // after the player logs/teleports.
-                if (hasRotated) {
-                    rotationCount++;
-                    reset(player);
-                    return true;
-                }
-            }
-
-            // update every time a player moves a certain distance
-            double distance = currentLoc.distanceSquared(this.location);
-            if (distance > MOVEMENT_SKIN_UPDATE_DISTANCE) {
-                reset(player);
-                return true;
-            }
-            else {
-                return false;
-            }
-        }
-
-        // resets initial yaw and location to the players
-        // current location and yaw.
-        void reset(Player player) {
-            player.getLocation(this.location);
-            if (rotationCount < 3) {
-                float rotationDegrees = Setting.NPC_SKIN_ROTATION_UPDATE_DEGREES.asFloat();
-                float yaw = NMS.clampYaw(this.location.getYaw());
-                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 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/entity/HumanController.java

@@ -129,12 +129,12 @@ public class HumanController extends AbstractEntityController {
 
     @Override
     public void remove() {
-
-        NMS.removeFromWorld(getBukkitEntity());
-
-        SkinnableEntity npc = NMS.getSkinnable(getBukkitEntity());
-        npc.getSkinTracker().onRemoveNPC();
-
+        Player entity = getBukkitEntity();
+        if (entity != null) {
+            NMS.removeFromWorld(entity);
+            SkinnableEntity npc = NMS.getSkinnable(entity);
+            npc.getSkinTracker().onRemoveNPC();
+        }
         super.remove();
     }
 

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

@@ -5,7 +5,6 @@ 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;
@@ -135,8 +134,8 @@ public class SkinPacketTracker {
                 continue;
 
             // send packet now and later to ensure removal from player list
-            NMS.sendPlayerListRemove(player, entity.getBukkitEntity());
-            PLAYER_LIST_REMOVER.sendPacket(player, entity);
+            NMS.sendTabListRemove(player, entity.getBukkitEntity());
+            TAB_LIST_REMOVER.sendPacket(player, entity);
         }
     }
 
@@ -147,27 +146,24 @@ public class SkinPacketTracker {
         entry.removeTask = Bukkit.getScheduler().runTaskLater(CitizensAPI.getPlugin(), new Runnable() {
             @Override
             public void run() {
-                if (shouldRemoveFromPlayerList()) {
-                    PLAYER_LIST_REMOVER.sendPacket(entry.player, entity);
+                if (shouldRemoveFromTabList()) {
+                    TAB_LIST_REMOVER.sendPacket(entry.player, entity);
                 }
             }
         }, PACKET_DELAY_REMOVE);
     }
 
     private void scheduleRemovePacket(PlayerEntry entry, int count) {
-        if (!shouldRemoveFromPlayerList())
+        if (!shouldRemoveFromTabList())
             return;
 
         entry.removeCount = count;
         scheduleRemovePacket(entry);
     }
 
-    private boolean shouldRemoveFromPlayerList() {
-        boolean isTablistDisabled = Settings.Setting.DISABLE_TABLIST.asBoolean();
-        boolean isNpcRemoved = entity.getNPC().data().get("removefromplayerlist",
-                Settings.Setting.REMOVE_PLAYERS_FROM_PLAYER_LIST.asBoolean());
-
-        return isNpcRemoved && isTablistDisabled;
+    private boolean shouldRemoveFromTabList() {
+        return entity.getNPC().data().get("removefromtablist",
+                Settings.Setting.DISABLE_TABLIST.asBoolean());
     }
 
     /**
@@ -216,11 +212,11 @@ public class SkinPacketTracker {
             entry = new PlayerEntry(player);
         }
 
-        PLAYER_LIST_REMOVER.cancelPackets(player, entity);
+        TAB_LIST_REMOVER.cancelPackets(player, entity);
 
         inProgress.put(player.getUniqueId(), entry);
         skin.apply(entity);
-        NMS.sendPlayerListAdd(player, entity.getBukkitEntity());
+        NMS.sendTabListAdd(player, entity.getBukkitEntity());
 
         scheduleRemovePacket(entry, 2);
     }
@@ -248,12 +244,12 @@ public class SkinPacketTracker {
         private void onPlayerQuit(PlayerQuitEvent event) {
             // this also causes any entries in the "inProgress" field to
             // be removed.
-            PLAYER_LIST_REMOVER.cancelPackets(event.getPlayer());
+            TAB_LIST_REMOVER.cancelPackets(event.getPlayer());
         }
     }
 
     private static final Location CACHE_LOCATION = new Location(null, 0, 0, 0);
     private static PlayerListener LISTENER;
     private static final int PACKET_DELAY_REMOVE = 1;
-    private static final PlayerListRemover PLAYER_LIST_REMOVER = new PlayerListRemover();
+    private static final TabListRemover TAB_LIST_REMOVER = new TabListRemover();
 }

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

@@ -0,0 +1,460 @@
+package net.citizensnpcs.npc.skin;
+
+import java.util.ArrayDeque;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Queue;
+import java.util.Set;
+import java.util.UUID;
+import java.util.WeakHashMap;
+
+import javax.annotation.Nullable;
+
+import com.google.common.base.Preconditions;
+import com.google.common.base.Predicates;
+import com.google.common.collect.Iterables;
+
+import net.citizensnpcs.Settings;
+import net.citizensnpcs.api.CitizensAPI;
+import net.citizensnpcs.api.npc.NPC;
+import net.citizensnpcs.api.npc.NPCRegistry;
+import net.citizensnpcs.util.NMS;
+
+import org.bukkit.Bukkit;
+import org.bukkit.Location;
+import org.bukkit.entity.Entity;
+import org.bukkit.entity.Player;
+import org.bukkit.scheduler.BukkitRunnable;
+
+/**
+ * Tracks skin updates for players.
+ *
+ * @see net.citizensnpcs.EventListen
+ */
+public class SkinUpdateTracker {
+
+    private final Map<SkinnableEntity, Void> navigating = new WeakHashMap<SkinnableEntity, Void>(25);
+    private final NPCRegistry npcRegistry;
+    private final Map<String, NPCRegistry> registries;
+    private final Map<UUID, PlayerTracker> playerTrackers =
+            new HashMap<UUID, PlayerTracker>(Bukkit.getMaxPlayers() / 2);
+    private final NPCNavigationUpdater updater = new NPCNavigationUpdater();
+
+    /**
+     * Constructor.
+     *
+     * @param npcRegistry
+     *            The primary citizens registry.
+     * @param registries
+     *            Map of other registries.
+     */
+    public SkinUpdateTracker(NPCRegistry npcRegistry, Map<String, NPCRegistry> registries) {
+        Preconditions.checkNotNull(npcRegistry);
+        Preconditions.checkNotNull(registries);
+
+        this.npcRegistry = npcRegistry;
+        this.registries = registries;
+
+        updater.runTaskTimer(CitizensAPI.getPlugin(), 1, 1);
+        new NPCNavigationTracker().runTaskTimer(CitizensAPI.getPlugin(), 3, 7);
+    }
+
+    /**
+     * Update a player with skin related packets from nearby skinnable NPC's.
+     *
+     * @param player
+     *            The player to update.
+     * @param delay
+     *            The delay before sending the packets.
+     * @param reset
+     *            True to hard reset the players tracking info, otherwise false.
+     */
+    public void updatePlayer(final Player player, long delay, final boolean reset) {
+        if (player.hasMetadata("NPC"))
+            return;
+
+        new BukkitRunnable() {
+            @Override
+            public void run() {
+                List<SkinnableEntity> visible = getNearbyNPCs(player, reset, false);
+                for (SkinnableEntity skinnable : visible) {
+                    skinnable.getSkinTracker().updateViewer(player);
+                }
+            }
+        }.runTaskLater(CitizensAPI.getPlugin(), delay);
+    }
+
+    /**
+     * Remove a player from the tracker.
+     *
+     * <p>
+     * Used when the player logs out.
+     * </p>
+     *
+     * @param playerId
+     *            The ID of the player.
+     */
+    public void removePlayer(UUID playerId) {
+        Preconditions.checkNotNull(playerId);
+        playerTrackers.remove(playerId);
+    }
+
+    /**
+     * Reset all players currently being tracked.
+     *
+     * <p>
+     * Used when Citizens is reloaded.
+     * </p>
+     */
+    public void reset() {
+        for (Player player : Bukkit.getOnlinePlayers()) {
+            if (player.hasMetadata("NPC"))
+                continue;
+
+            PlayerTracker tracker = playerTrackers.get(player.getUniqueId());
+            if (tracker == null)
+                continue;
+
+            tracker.hardReset(player);
+        }
+    }
+
+    /**
+     * Invoke when an NPC is spawned.
+     *
+     * @param npc
+     *            The spawned NPC.
+     */
+    public void onNPCSpawn(NPC npc) {
+        Preconditions.checkNotNull(npc);
+        SkinnableEntity skinnable = getSkinnable(npc);
+        if (skinnable == null)
+            return;
+
+        // reset nearby players in case they are not looking at the NPC when it spawns.
+        resetNearbyPlayers(skinnable);
+    }
+
+    /**
+     * Invoke when an NPC is despawned.
+     *
+     * @param npc
+     *            The despawned NPC.
+     */
+    public void onNPCDespawn(NPC npc) {
+        Preconditions.checkNotNull(npc);
+        SkinnableEntity skinnable = getSkinnable(npc);
+        if (skinnable == null)
+            return;
+
+        navigating.remove(skinnable);
+
+        for (PlayerTracker tracker : playerTrackers.values()) {
+            tracker.fovVisibleSkins.remove(skinnable);
+        }
+    }
+
+    /**
+     * Invoke when an NPC begins navigating.
+     *
+     * @param npc
+     *            The navigating NPC.
+     */
+    public void onNPCNavigationBegin(NPC npc) {
+        Preconditions.checkNotNull(npc);
+        SkinnableEntity skinnable = getSkinnable(npc);
+        if (skinnable == null)
+            return;
+
+        navigating.put(skinnable, null);
+    }
+
+    /**
+     * Invoke when an NPC finishes navigating.
+     *
+     * @param npc
+     *            The finished NPC.
+     */
+    public void onNPCNavigationComplete(NPC npc) {
+        Preconditions.checkNotNull(npc);
+        SkinnableEntity skinnable = getSkinnable(npc);
+        if (skinnable == null)
+            return;
+
+        navigating.remove(skinnable);
+    }
+
+    /**
+     * Invoke when a player moves.
+     *
+     * @param player
+     *            The player that moved.
+     */
+    public void onPlayerMove(Player player) {
+        Preconditions.checkNotNull(player);
+        PlayerTracker updateTracker = playerTrackers.get(player.getUniqueId());
+        if (updateTracker == null)
+            return;
+
+        if (!updateTracker.shouldUpdate(player))
+            return;
+
+        updatePlayer(player, 10, false);
+    }
+
+    // hard reset players near a skinnable NPC
+    private void resetNearbyPlayers(SkinnableEntity skinnable) {
+        Entity entity = skinnable.getBukkitEntity();
+        if (entity == null || !entity.isValid())
+            return;
+
+        double viewDistance = Settings.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;
+
+            PlayerTracker tracker = playerTrackers.get(player.getUniqueId());
+            if (tracker != null) {
+                tracker.hardReset(player);
+            }
+        }
+    }
+
+    private List<SkinnableEntity> getNearbyNPCs(Player player, boolean reset, boolean checkFov) {
+        List<SkinnableEntity> results = new ArrayList<SkinnableEntity>();
+        PlayerTracker tracker = getTracker(player, reset);
+        for (NPC npc : getAllNPCs()) {
+
+            SkinnableEntity skinnable = getSkinnable(npc);
+            if (skinnable == null)
+                continue;
+
+            // if checking field of view, don't add skins that have already been updated for FOV
+            if (checkFov && tracker.fovVisibleSkins.contains(skinnable))
+                continue;
+
+            if (canSee(player, skinnable, checkFov)) {
+                results.add(skinnable);
+            }
+        }
+        return results;
+    }
+
+    private Iterable<NPC> getAllNPCs() {
+        return Iterables.filter(Iterables.concat(npcRegistry, Iterables.concat(registries.values())),
+                Predicates.notNull());
+    }
+
+    // get all navigating skinnable NPC's within the players FOV that have not been "seen" yet
+    private void getNewVisibleNavigating(Player player, Collection<SkinnableEntity> output) {
+        PlayerTracker tracker = getTracker(player, false);
+
+        for (SkinnableEntity skinnable : navigating.keySet()) {
+
+            // make sure player hasn't already been updated to prevent excessive tab list flashing
+            // while NPC's are navigating and to reduce the number of times #canSee is invoked.
+            if (tracker.fovVisibleSkins.contains(skinnable))
+                continue;
+
+            if (canSee(player, skinnable, true))
+                output.add(skinnable);
+        }
+    }
+
+    @Nullable
+    private SkinnableEntity getSkinnable(NPC npc) {
+        Entity entity = npc.getEntity();
+        if (entity == null)
+            return null;
+
+        return NMS.getSkinnable(entity);
+    }
+
+    // get a players tracker, create new one if not exists.
+    private PlayerTracker getTracker(Player player, boolean reset) {
+        PlayerTracker tracker = playerTrackers.get(player.getUniqueId());
+        if (tracker == null) {
+            tracker = new PlayerTracker(player);
+            playerTrackers.put(player.getUniqueId(), tracker);
+        }
+        else if (reset) {
+            tracker.hardReset(player);
+        }
+        return tracker;
+    }
+
+    // determines if a player is near a skinnable entity and, if checkFov set, if the
+    // skinnable entity is within the players field of view.
+    private boolean canSee(Player player, SkinnableEntity skinnable, boolean checkFov) {
+        Player entity = skinnable.getBukkitEntity();
+        if (entity == null)
+            return false;
+
+        if (!player.canSee(entity))
+            return false;
+
+        if (!player.getWorld().equals(entity.getWorld()))
+            return false;
+
+        Location playerLoc = player.getLocation(CACHE_LOCATION);
+        Location skinLoc = entity.getLocation(NPC_LOCATION);
+
+        double viewDistance = Settings.Setting.NPC_SKIN_VIEW_DISTANCE.asDouble();
+        viewDistance *= viewDistance;
+
+        if (playerLoc.distanceSquared(skinLoc) > viewDistance)
+            return false;
+
+        // see if the NPC is within the players field of view
+        if (checkFov) {
+            double deltaX = skinLoc.getX() - playerLoc.getX();
+            double deltaZ = skinLoc.getZ() - playerLoc.getZ();
+            double angle = Math.atan2(deltaX, deltaZ);
+            float skinYaw = NMS.clampYaw(-(float) Math.toDegrees(angle));
+            float playerYaw = NMS.clampYaw(playerLoc.getYaw());
+            float upperBound = playerYaw + FIELD_OF_VIEW;
+            float lowerBound = playerYaw - FIELD_OF_VIEW;
+
+            return skinYaw >= lowerBound && skinYaw <= upperBound;
+        }
+
+        return true;
+    }
+
+    // Tracks player location and yaw to determine when the player should be updated
+    // with nearby skins.
+    private class PlayerTracker {
+        final Location location = new Location(null, 0, 0, 0);
+        final Set<SkinnableEntity> fovVisibleSkins = new HashSet<SkinnableEntity>(20);
+        int rotationCount;
+        boolean hasMoved;
+        float upperBound;
+        float lowerBound;
+
+        PlayerTracker(Player player) {
+            hardReset(player);
+        }
+
+        boolean shouldUpdate(Player player) {
+            Location currentLoc = player.getLocation(CACHE_LOCATION);
+
+            if (!hasMoved) {
+                hasMoved = true;
+                return true;
+            }
+
+            if (rotationCount < 3) {
+                float yaw = NMS.clampYaw(currentLoc.getYaw());
+                boolean hasRotated = yaw < lowerBound || yaw > upperBound;
+
+                // update the first 3 times the player rotates. helps load skins around player
+                // after the player logs/teleports.
+                if (hasRotated) {
+                    rotationCount++;
+                    reset(player);
+                    return true;
+                }
+            }
+
+            // make sure player is in same world
+            if (!currentLoc.getWorld().equals(this.location.getWorld())) {
+                reset(player);
+                return true;
+            }
+
+            // update every time a player moves a certain distance
+            double distance = currentLoc.distanceSquared(this.location);
+            if (distance > MOVEMENT_SKIN_UPDATE_DISTANCE) {
+                reset(player);
+                return true;
+            }
+            else {
+                return false;
+            }
+        }
+
+        // resets initial yaw and location to the players current location and yaw.
+        void reset(Player player) {
+            player.getLocation(this.location);
+            if (rotationCount < 3) {
+                float rotationDegrees = Settings.Setting.NPC_SKIN_ROTATION_UPDATE_DEGREES.asFloat();
+                float yaw = NMS.clampYaw(this.location.getYaw());
+                this.upperBound = yaw + rotationDegrees;
+                this.lowerBound = yaw - rotationDegrees;
+            }
+        }
+
+        // reset all
+        void hardReset(Player player) {
+            this.hasMoved = false;
+            this.rotationCount = 0;
+            this.fovVisibleSkins.clear();
+            reset(player);
+        }
+    }
+
+    // update players when the NPC navigates into their field of view
+    private class NPCNavigationTracker extends BukkitRunnable {
+        @Override
+        public void run() {
+            if (navigating.isEmpty() || playerTrackers.isEmpty())
+                return;
+
+            List<SkinnableEntity> nearby = new ArrayList<SkinnableEntity>(10);
+            Collection<? extends Player> players = Bukkit.getOnlinePlayers();
+
+            for (Player player : players) {
+                if (player.hasMetadata("NPC"))
+                    continue;
+
+                getNewVisibleNavigating(player, nearby);
+
+                for (SkinnableEntity skinnable : nearby) {
+                    PlayerTracker tracker = getTracker(player, false);
+                    tracker.fovVisibleSkins.add(skinnable);
+                    updater.queue.offer(new UpdateInfo(player, skinnable));
+                }
+
+                nearby.clear();
+            }
+        }
+    }
+
+    // Updates players. Repeating task used to schedule updates without
+    // causing excessive scheduling.
+    private class NPCNavigationUpdater extends BukkitRunnable {
+        Queue<UpdateInfo> queue = new ArrayDeque<UpdateInfo>(20);
+        @Override
+        public void run() {
+            while (!queue.isEmpty()) {
+                UpdateInfo info = queue.remove();
+                info.entity.getSkinTracker().updateViewer(info.player);
+            }
+        }
+    }
+
+    private static class UpdateInfo {
+        Player player;
+        SkinnableEntity entity;
+        UpdateInfo(Player player, SkinnableEntity entity) {
+            this.player = player;
+            this.entity = entity;
+        }
+    }
+
+    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;
+    private static final float FIELD_OF_VIEW = 70f;
+}

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

@@ -25,10 +25,10 @@ import net.citizensnpcs.util.NMS;
  * Collects entities to remove and sends them all to the player in a single packet.
  * </p>
  */
-public class PlayerListRemover {
+public class TabListRemover {
     private final Map<UUID, PlayerEntry> pending = new HashMap<UUID, PlayerEntry>(Bukkit.getMaxPlayers() / 2);
 
-    PlayerListRemover() {
+    TabListRemover() {
         Bukkit.getScheduler().runTaskTimer(CitizensAPI.getPlugin(), new Sender(), 2, 2);
     }
 
@@ -143,7 +143,7 @@ public class PlayerListRemover {
                 }
 
                 if (entry.player.isOnline())
-                    NMS.sendPlayerListRemove(entry.player, skinnableList);
+                    NMS.sendTabListRemove(entry.player, skinnableList);
 
                 // notify skin trackers that a remove packet has been sent to a player
                 for (SkinnableEntity entity : skinnableList) {

src/main/java/net/citizensnpcs/util/NMS.java

@@ -117,7 +117,7 @@ public class NMS {
         return null;
     }
 
-    public static void sendPlayerListAdd(Player recipient, Player listPlayer) {
+    public static void sendTabListAdd(Player recipient, Player listPlayer) {
         Preconditions.checkNotNull(recipient);
         Preconditions.checkNotNull(listPlayer);
 
@@ -127,7 +127,7 @@ public class NMS {
                 PacketPlayOutPlayerInfo.EnumPlayerInfoAction.ADD_PLAYER, entity));
     }
 
-    public static void sendPlayerListRemove(Player recipient, Player listPlayer) {
+    public static void sendTabListRemove(Player recipient, Player listPlayer) {
         Preconditions.checkNotNull(recipient);
         Preconditions.checkNotNull(listPlayer);
 
@@ -137,8 +137,8 @@ public class NMS {
                 PacketPlayOutPlayerInfo.EnumPlayerInfoAction.REMOVE_PLAYER, entity));
     }
 
-    public static void sendPlayerListRemove(Player recipient,
-                                            Collection<? extends SkinnableEntity> skinnableNPCs) {
+    public static void sendTabListRemove(Player recipient,
+                                         Collection<? extends SkinnableEntity> skinnableNPCs) {
         Preconditions.checkNotNull(recipient);
         Preconditions.checkNotNull(skinnableNPCs);