Fix CITIZENS-21, partially fix CITIZENS-2 and CITIZENS-11

src/main/java/net/citizensnpcs/npc/ai/CitizensAI.java

@@ -10,7 +10,6 @@ import net.citizensnpcs.api.ai.Goal;
 import net.citizensnpcs.api.ai.NavigationCallback;
 import net.citizensnpcs.api.ai.NavigationCallback.CancelReason;
 import net.citizensnpcs.npc.CitizensNPC;
-import net.citizensnpcs.util.Messaging;
 
 import org.bukkit.Location;
 import org.bukkit.entity.LivingEntity;
@@ -18,7 +17,6 @@ import org.bukkit.entity.LivingEntity;
 import com.google.common.collect.Lists;
 
 public class CitizensAI implements AI {
-    private Runnable ai;
     private final List<WeakReference<NavigationCallback>> callbacks = Lists.newArrayList();
     private PathStrategy executing;
     private final List<GoalEntry> executingGoals = Lists.newArrayList();
@@ -40,16 +38,16 @@ public class CitizensAI implements AI {
 
     @Override
     public void cancelDestination() {
-        if (executing != null) {
+        if (executing == null)
-            Iterator<WeakReference<NavigationCallback>> itr = callbacks.iterator();
+            return;
-            while (itr.hasNext()) {
+        executing = null;
-                NavigationCallback next = itr.next().get();
+        Iterator<WeakReference<NavigationCallback>> itr = callbacks.iterator();
-                if (next == null || next.onCancel(this, CancelReason.CANCEL)) {
+        while (itr.hasNext()) {
-                    itr.remove();
+            NavigationCallback next = itr.next().get();
-                }
+            if (next == null || next.onCancel(this, CancelReason.CANCEL)) {
+                itr.remove();
             }
         }
-        executing = null;
     }
 
     @Override
@@ -89,18 +87,15 @@ public class CitizensAI implements AI {
         paused = false;
     }
 
-    @Override
-    public void setAI(Runnable ai) {
-        this.ai = ai;
-    }
-
     @Override
     public void setDestination(Location destination) {
         if (destination == null)
             throw new IllegalArgumentException("destination cannot be null");
         boolean replaced = executing != null;
-        executing = new NavigationStrategy(npc, destination);
+        executing = new MCNavigationStrategy(npc, destination);
 
+        if (!replaced)
+            return;
         Iterator<WeakReference<NavigationCallback>> itr = callbacks.iterator();
         while (itr.hasNext()) {
             NavigationCallback next = itr.next().get();
@@ -116,8 +111,10 @@ public class CitizensAI implements AI {
             throw new IllegalArgumentException("target cannot be null");
 
         boolean replaced = executing != null;
-        executing = new TargetStrategy(npc, target, aggressive);
+        executing = new MCTargetStrategy(npc, target, aggressive);
 
+        if (!replaced)
+            return;
         Iterator<WeakReference<NavigationCallback>> itr = callbacks.iterator();
         while (itr.hasNext()) {
             NavigationCallback next = itr.next().get();
@@ -128,9 +125,12 @@ public class CitizensAI implements AI {
     }
 
     public void update() {
-        if (paused)
+        if (paused || !npc.isSpawned()) {
             return;
+        }
+
         if (executing != null && executing.update()) {
+            executing = null;
             Iterator<WeakReference<NavigationCallback>> itr = callbacks.iterator();
             while (itr.hasNext()) {
                 NavigationCallback next = itr.next().get();
@@ -138,16 +138,6 @@ public class CitizensAI implements AI {
                     itr.remove();
                 }
             }
-            executing = null;
-        }
-
-        if (ai != null) {
-            try {
-                ai.run();
-            } catch (Throwable ex) {
-                Messaging.log("Unexpected error while running ai " + ai);
-                ex.printStackTrace();
-            }
         }
 
         for (GoalEntry entry : goals) {

src/main/java/net/citizensnpcs/npc/ai/MCNavigationStrategy.java

@@ -10,20 +10,28 @@ import net.minecraft.server.Navigation;
 import org.bukkit.Location;
 import org.bukkit.entity.Player;
 
-public class NavigationStrategy implements PathStrategy {
+public class MCNavigationStrategy implements PathStrategy {
     private final Navigation navigation;
     private EntityHumanNPC entity = null;
 
-    NavigationStrategy(CitizensNPC npc, Location dest) {
+    MCNavigationStrategy(CitizensNPC npc, Location dest) {
+        if (npc.getBukkitEntity() instanceof Player) {
+            entity = (EntityHumanNPC) npc.getHandle();
+            entity.onGround = true;
+            // not sure of a better way around this - if onGround is false, then
+            // navigation won't execute, and calling entity.move doesn't
+            // entirely fix the problem.
+        }
         navigation = npc.getHandle().ak();
         navigation.a(dest.getX(), dest.getY(), dest.getZ(), getSpeed(npc.getHandle()));
-        if (npc.getBukkitEntity() instanceof Player)
+
-            entity = (EntityHumanNPC) npc.getHandle();
     }
 
-    NavigationStrategy(EntityLiving entity, EntityLiving target) {
+    MCNavigationStrategy(EntityLiving entity, EntityLiving target) {
-        if (entity instanceof EntityHumanNPC)
+        if (entity instanceof EntityHumanNPC) {
             this.entity = (EntityHumanNPC) entity;
+            entity.onGround = true; // see above
+        }
         navigation = entity.ak();
         navigation.a(target, getSpeed(entity));
     }

src/main/java/net/citizensnpcs/npc/ai/MCTargetStrategy.java

@@ -8,12 +8,11 @@ import net.minecraft.server.EntityMonster;
 import org.bukkit.craftbukkit.entity.CraftLivingEntity;
 import org.bukkit.entity.LivingEntity;
 
-public class TargetStrategy implements PathStrategy {
+public class MCTargetStrategy implements PathStrategy {
     private final boolean aggro;
-    private PathStrategy current = null;
     private final EntityLiving handle, target;
 
-    public TargetStrategy(CitizensNPC handle, LivingEntity target, boolean aggro) {
+    public MCTargetStrategy(CitizensNPC handle, LivingEntity target, boolean aggro) {
         this.handle = handle.getHandle();
         this.target = ((CraftLivingEntity) target).getHandle();
         this.aggro = aggro;
@@ -33,7 +32,8 @@ public class TargetStrategy implements PathStrategy {
     public boolean update() {
         if (target == null || target.dead)
             return true;
-        current = new NavigationStrategy(handle, target);
+        new MCNavigationStrategy(handle, target).update();
+        handle.getControllerLook().a(target, 10.0F, handle.C());
         if (aggro && canAttack()) {
             if (handle instanceof EntityMonster) {
                 ((EntityMonster) handle).a(target);
@@ -42,7 +42,6 @@ public class TargetStrategy implements PathStrategy {
             }
         }
 
-        current.update();
         return false;
     }
 

src/main/java/net/citizensnpcs/npc/entity/CitizensHumanNPC.java

@@ -27,8 +27,8 @@ public class CitizensHumanNPC extends CitizensNPC implements Equipable {
     @Override
     protected EntityLiving createHandle(Location loc) {
         WorldServer ws = ((CraftWorld) loc.getWorld()).getHandle();
-        EntityHumanNPC handle = new EntityHumanNPC(ws.getServer().getServer(), ws, StringHelper
+        EntityHumanNPC handle = new EntityHumanNPC(ws.getServer().getServer(), ws,
-                .parseColors(getFullName()), new ItemInWorldManager(ws));
+                StringHelper.parseColors(getFullName()), new ItemInWorldManager(ws));
         handle.removeFromPlayerMap(getFullName());
         handle.setPositionRotation(loc.getX(), loc.getY(), loc.getZ(), loc.getYaw(), loc.getPitch());
         return handle;
@@ -62,6 +62,8 @@ public class CitizensHumanNPC extends CitizensNPC implements Equipable {
         super.update();
         if (mcEntity != null) {
             mcEntity.move(0, -0.1, 0);
+            // gravity! also works around an entity.onGround not updating issue
+            // (onGround is normally updated by the client)
         }
     }
 

src/main/java/net/citizensnpcs/npc/entity/EntityHumanNPC.java

@@ -71,6 +71,7 @@ public class EntityHumanNPC extends EntityPlayer {
 
         aX *= 0.98F;
         this.a(aW, aX);
+        X = yaw; // TODO: this looks jerky
     }
 
     public void removeFromPlayerMap(String name) {

src/main/java/net/citizensnpcs/trait/waypoint/Waypoints.java

@@ -19,6 +19,9 @@ public class Waypoints extends Trait {
 
     public Waypoints(NPC npc) {
         this.npc = npc;
+        npc.getAI().registerNavigationCallback(provider.getCallback());
+        // TODO: this is necessary because traits aren't loaded when added
+        // manually so we may be in an uninitialised state.
     }
 
     public Editor getEditor(Player player) {

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

@@ -14,7 +14,7 @@ import org.bukkit.entity.Player;
 import com.google.common.base.Joiner;
 
 public class Messaging {
-    private static final Joiner SPACE = Joiner.on(" ");
+    private static final Joiner SPACE = Joiner.on(" ").useForNull("null");
 
     public static void debug(Object... msg) {
         if (Setting.DEBUG_MODE.asBoolean())