Making NPC's walk on a path on their own account

ID:161095
 
Jun 19 2008, 4:10 am
Basically the code snippet i am using that might be causing the problem is
    NPCAII() //name of proc
            var/mob/player/M 
            while(src)
                var/obj/path/P
                if(P in oview(rand(10,15)))
                    sleep(rand(10,15))
                    step_towards(src,P)
                    sleep(rand(10,15))
                    step_towards(src,P)
                    sleep(rand(10,15))
                    step_towards(src,P)
                    sleep(rand(10,15))
                    step_towards(src,P)
                    sleep(rand(10,15))
                    step_towards(src,P)
                    sleep(rand(10,15))
                    step_towards(src,P)
                    sleep(rand(10,15))
                    step_towards(src,P)
                if(M in oview(5)
                    if(M.name in src.killlist) 
                        walk_to(src,M,1,4) 
                        if(M in oview(1)) 
                            step_towards(src,M)
                
                    else 
                        sleep(rand(10,15))//
                        step_rand(src)
                        break
                        
                else 
                    for(M in view(src)) 
                        break 
                sleep(5)
            spawn(2) 
                NPCAI()
mob
            //...so on so forth so they walk to the path and if no path is in oview later in the code it makes them walk randomly the problem is their more driven to walk randomly then to walk path driven
Jun 19 2008, 4:10 am
wait wrong forum -embarrassed-
Jun 19 2008, 8:16 am
Problems:

--Your checks: 
var/obj/path/P
if(P in oview(rand(10,15)))

var/mob/player/M
if(M in oview(5)


Both do nothing, because the variables P and M both have no value (set to null). You're just looking for null in a specific range, and other silly things in the rest of the code that involves those vars... Looks like what you're trying to use could be the for()-in-list statement - this usage is proper only there. However, you should probably do better with simply using locate() to find an instance of the wanted objects.

Also, you have usr abuse in your oview() call. You specify no Center/Ref argument, only the range, and so it defaults to usr. This certainly causes problems.
Lastly, you need 2 closing parentheses to finish this line. One for the if(), and one for oview(). It won't even compile otherwise.

--Badly designed repeating code: 
>                   sleep(rand(10,15))
>                   step_towards(src,P)
>                   sleep(rand(10,15))
>                   step_towards(src,P)
>                   sleep(rand(10,15))
>                   step_towards(src,P)
>                   // [...]
>


When you see repeating or otherwise code that is copied again over and over, it is surefire badly made code. This should be done by simply looping how many times you want to do the action, using a 'regular' for() loop (or a while()): 
for(var/num=5,num,num--)
   sleep(rand(10,15))
   step_towards(src,P)

The for() line could also naturally be replaced with: 
for(var/num=5,num--)

Due to how the operator works (look up the -- operator for more information). Lastly, there is another way to do this, using a built-in DM shorthand: 
for(var/num = 1 to 5) //loop 5 times


--Using names to identify objects: 
   if(M.name in src.killlist)


I don't know how your killlist works, but regardless it bad to keep tracks of objects by their name; it's not robust and not foolproof at all. Better simply use the objects 'themselves' (references to them), ie: 
//bad:
killlist += O.name
if(X.name in killist)

//good:
killlist += O
if(X in killist)
//O and X being variables that reference objects


--Recursively infinitely calling the same procedure 
>           spawn(2) 
>               NPCAI()

While this works, it is unneeded extra overhead and brings unnecessary side effects. Also, from a design perspective, you should keep all of the "proc" in the "proc itself", so to speak. What I mean is that you shouldn't have your code span many new procs and proc calls (you're calling nwe NPCAI()s all the time with your current code) when you can simply confine it to 1 single proc.

What you should do instead of this is simple, and you are probably already familiar with it at least somewhat. If you want to make your whole proc loop, simply use a looping construct such as while() or for() on the whole proc (if you want an infinite loop, use no condition, such as an empty for() - there is no need to check if src still exists at all, because if src is deleted, the proc would be stopped and not reach your check anyway). If you want to put a delay between loop iterations, simply use sleep() again. 
proc/myloopingproc()
   for() //loop forever
      world << "hello"
      sleep(10) //wait a little before moving on to the next iteration(s)



As you can probably deduce from all the above, it would be an easier and better job to just scrap that whole block of code (possibly other related code you have to this as well) and rewrite it rather than trying to fix everything, bearing in mind your previous mistakes. It would also be a very good idea to review the DM Guide and some tutorials before proceeding, to eliminate some future errors and improve your grasp on the language.
Also, paths shouldn't really be implemented by /objs anyway, or anything on the map itself for that matter. You should use a datum object instead, or you can even possibly use only a list. Basically you just need a method of keeping track of the turfs in the path, and walking through it. Just a list (and a proc or two, of course) will suffice, but you may need to use a datum for more complex things.

Choka wrote:
wait wrong forum -embarrassed-

Don't worry about that, a moderator will take care of it and move this topic. Just don't make another topic for it - multiple topics for the same thing is bad.
EDIT: Uhm, you already made another topic for it in the right forum... my bad, but, yours more. >_>