Fix NPE if NPC removed without spawning

Update players based on navigating NPC's. Update player when NPC navigates into players field of view.

Moved skin update tracker code to own class (SkinUpdateTracker)

fix use incorrect setting for tab list

rename PlayerListRemover to TabListRemover

rename recently added NMS#sendPlayerListRemove, #sendPlayerListAdd methods to #sendTabList*

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);