Hmm Smoke Code Gone Wrong

ID:140596
 
Nov 11 2009, 7:42 am
Code:
mob
    verb
        test_smoke()
            src<<"Creating smoke."
            src.SmokeSpawn(5,2)
            
            
    proc
        SmokeSpawn(time,delay)
            while(time>0)
                var/obj/smoke/C=new(src)
                C.owner=src
                time--  
                sleep(delay)


obj
    var/owner=""
    smoke
        icon='smoke.dmi'
        opacity=1
        layer=MOB_LAYER+1
    
        
        New()
            spawn(80)
                del(src)
            src.Moverand()
            
            ..()

                


    proc
        Moverand()
            if(src)
                step_to(src,src.owner,3)
                sleep(4)
                src.Moverand()


Problem description:

When i test the code the smoke doesnt get created. I'm probably doing this the wrong way either way so i thought id ask for the best way to do this with minimal lag.

Nov 11 2009, 8:11 am
You are creating the smoke item within the src's contents, not its location.
Nov 11 2009, 9:38 am
Rapmaster wrote:
When i test the code the smoke doesnt get created

It gets created, however, as GhostAnime said, it is placed at the wrong location.


>               var/obj/smoke/C=new(src)
>               C.owner=src


You should just do such initializations within the object's New() proc, which is more manageable, cleaner, and more appropriate. Just pass to New() (via new() - read the Reference for more info) the arguments it needs (owner) and handle them as needed in it (by setting the var). Just remember that when dealing with atoms, new()'s first argument is hardcoded to be used as the atom's initial location, so if you're going to supply atom values (i.e. valid locations), don't give them as the first argument unless you want them to be the location.
Also, don't overuse text/quotes. A lot of newbies do things such as this: 
    var/owner=""

And this: 
var/pickstate = input("choose a state")
icon_state = "[pickstate]"

However, this abuse of empty strings ("") and embedding values in text when that's not needed is useless at best, and harmful at worse.
Mind that setting a var's initial value to "" is actually mostly useless even if that var is supposed to hold a text string - the default value of null is just as suitable. And in this case, owner is supposed to hold an atom (or null if there is none) - don't give it inappropriate (other) value types, such as text strings.

obj/smoke
>       New()
>           spawn(80)
>               del(src)
>           src.Moverand()
>           
>           ..()
>   proc
>       Moverand()
>           if(src)
>               step_to(src,src.owner,3)
>               sleep(4)
>               src.Moverand()

A number of issues there.
First, note that all the time <small>(except in the rare case when you're modifying the value of src to null or a potentially inappropriate value)</small>, checking if the source object (src) is still valid is entirely useless. This is because that if the source object of a proc is deleted, that proc is automatically killed (look up del for more info), meaning in that case, those checks won't even be processed. This applies to other forms of checks as well, such as while(src).

Second, in general when overriding procs (especially built-in procs, and especially New()), you should refrain from preventing them from reaching a timely return. This is especially important here because doing this with New() is actually known to cause Bad Stuff - what will appear as seemingly random issues.
In this case, you're preventing it from ever returning, because you're calling a proc that loops infinitely from it (meaning it has to wait until that proc ends before it can continue (and finish), but that proc never ends). In these cases, we use spawn() to isolate the infinite loop into a separate thread, so it doesn't bog down its caller (New() in this case).
You can either spawn() off the call to the procedure, or the body of the procedure itself. Which method to use is pretty negligible, but in cases where the procedure does a dedicated continuing action which you know that other procs will never want to wait until it ends, the better solution is to spawn-off the body of the proc itself. This is more robust and convenient, because this way is global, i.e.: you only need to spawn it off once, instead of in every call to it.

Lastly, your method of looping the Moverand() proc is bad (as well as improper) and will eventually cause a runtime error. Garthor explains why throughout this thread: [link], so read it.
You should use a proper looping construct to loop instead of restarting the current proc over and over again with no changes, which is less efficient and less proper. You have while() and for(). In this case, you want to use a conditionless (infinite) loop, which for() is suitable for (you can just not include the condition part in it), however to use while(), you'd have to work-around it requiring a condition by giving it one which is always true.
Nov 11 2009, 9:50 am
In response to Kaioken
ah i never knew about the New Proc being stalled by a never ending proc. thanks for the info, anyway i redid the whole thing and stil working on it to provide the best effects.

obj
    effect
        smoke
            icon='smoke.dmi'
            layer=MOB_LAYER+1
            opacity=1
    
            
            New()
                spawn(80)
                    del(src)
                spawn()
                    src.Moverand()
                    
                ..()
            
            proc
                Moverand()
                    if(src)
                        walk_rand(src,3)
                        spawn(4)
                            src.Moverand()

mob
    verb
        test_smoke()
            src<<"Creating smoke."
            var/obj/F= new/obj/effect/smoke(locate(src.x,src.y,src.z))
            SmokeSpawn(3,2,F)
            
  
           
proc
    SmokeSpawn(time,delay,atom/movable/target)
        if(time>0)
            var/rand1=target.x+pick(1,2,3,4,5)//random dirs, improve.
            var/rand2=target.y+pick(1,2,3,4,5)
            var/obj/D= new/obj/effect/smoke(locate(rand1,rand2,target.z))
            if(D)
                D.dir=target.dir
            time--//time limit  
            spawn(delay) 
                SmokeSpawn(time,delay,D)//starts smoke on new obj created
        else
            return


EDIT: Im not a total noob :( just had a bad day sigh
Nov 11 2009, 9:55 am
In response to Jezblud
ah crap i was logged in as my brother when i posted that :( bad day :)
Nov 11 2009, 11:49 am
In response to Jezblud
You still happened not to cover most of what I've said.
Also, locate(src.x,src.y,src.z) is vastly silly. You don't need to find the mob's turf because it's available directly, in its location variable.