Bad day for a bad obj

ID:2287724
 
Aug 26 2017, 8:20 pm
(See the best response by Ter13.)
Code:
//ERROR SHOWN DURING RUNTIME
runtime error: BYOND Error: bad obj
proc name: Afterimage (/proc/Afterimage)
  source file: Effects.dm,141
  usr:  (/mob/Player)
  src: null
  usr.loc: the grass (17,6,1) (/turf/grass)
  call stack:
Afterimage( (/mob/Player), null, "b-dodgingf")
AfterimageTrail( (/mob/Player), 0.933333, "b-dodgingf")
//CODE
proc
    Afterimage(mob/m,d,state)
        set waitfor=0
        if(!m)  return
        var/obj/M=Fx_Recycle(blueprint=/obj,blank=1)
        M.appearance=m.appearance           //line 141
        if(state)   M.icon_state=state
        M.setPos(m.gx,m.gy,m.z)
        if(d)
            M.dir=d
        else
            M.dir=m.dir
        animate(M,alpha=0,time=10)
        sleep(10)
        Fx_Recycle(M)


Problem description:
Pretty much every obj I create gives me a similar error, and I can't say I've seen it before. Variables referencing objects somehow become equal to null without me explicitly deleting them or setting them to such a value. Any help would be appreciated.
Aug 26 2017, 8:24 pm
There's probably something going on in the Fx_Recycle() proc causing it to return null.

That could be either an explicit deletion of an object in the recycling list, or something else with how the objects are being inserted in the first place.
Aug 26 2017, 8:37 pm
Hmm, I remember messing with it a bit earlier today, but it didnt seem to be at a game breaking level.

proc
    Fx_Recycle(a,blueprint,i,s,p_x,p_y,l,alph,blank)
        if(a)
            recycle+=a
            a:loc=locate(0,0,1)
        if(blueprint)
            var/datum/O
            for(var/m in recycle)
                if(istype(m,blueprint))
                    O=m
                    break
            if(!O)
                O=new blueprint
            if(blank)
                O:icon=null
                O:icon_state=null
                O:pixel_x=null
                O:pixel_y=null
                O:alpha=255
                O:transform=null
                O:color=null
            if(i)   O:icon=i
            if(s)   O:icon_state=s
            if(p_x)
                O:pixel_x=p_x
                O.basepx=p_x
            if(p_y)
                O:pixel_y=p_y
                O.basepy=p_x
            if(alph)    O:alpha=alph
            if(l)   O:layer=l
            if(O in recycle)    recycle-=O
            return O
Aug 26 2017, 9:32 pm (Edited on Aug 26 2017, 9:40 pm)
Best response
 
var/list/effect_pool = list()

proc
    StoreEffect(visual_effect/e) //call to store an effect obj in its pool
        var/list/l = effect_pool[e.type]
        l += e
        e.Stored()

    RecycleEffect(pool,appearance) //call to grab an effect from a pool or create a new one
        var/list/l = effect_pool[pool]
        var/visual_effect/e
        if(l&&l.len>1) //if the pool exists and has any elements in it
            l -= (e = l[2]) //remove the item from the list and set the return value
            e.Recycled()
        else
            e = new type() //no objects to recycle, create a new one and set return value
        if(appearance) e.appearance = appearance
        return e

    CleanPool(pool,maximum=0) //call to purge excess pool items
        if(maximum<0) return //just don't.
        var/list/l = effect_pool[pool]
        if(l&&l.len>maximum+1)
            l.Cut(maximum+1,0)

    PreparePool(pool,minimum=1) //call to prepare a pool by packing it with a minimum number of items
        if(minimum<0) return //also don't
        var/list/l = effect_pool[pool]
        if(!l) //if the pool doesn't exist, create at least one of the object and store it in the pool
            var/effect/e = new type()
            l = effect_pool[pool]
            if(minimum>0) //if we just want to prep the pool, itself, create a temporary object and let it be garbage collected.
                l += e
        if(l.len<minimum+1) //if the pool isn't at the desired length already, loop creating pooled objects until we reach the desired size
            var/count = minimum-l.len+1
            while(count--)
                l += new type()

//this atom allows you to define default properties for pooled effects
visual_effect
    parent_type = /obj

    New()
        ..()
        if(!effect_pool[type])
             effect_pool[type] = list(appearance)

    proc
        Recycled() //called when the object is pulled out of the effect pool. Not called when created when the pool is empty.

        Stored() //called when the object is put in an effect pool. Not called when initially stored in a list, only when stored using the StoreEffect() proc.
            loc = null
            appearance = effect_pool[type][1]


So... I took the liberty of rewriting the way your pool structures work to be a bit more efficient and less error prone.

Let's take a look at how they are gonna work:

proc
    Afterimage(mob/m,d,state)
        set waitfor=0
        if(!m)  return
        var/visual_effect/e = RecycleEffect(/visual_effect,m.appearance)
        if(state) e.icon_state = state
        if(!d) d = m.dir
        e.setPos(m.gx,m.gy,m.z,d) //Ter13.MoveLib atom/movable/setPos() allows you to set the dir when relocating the mob to a global coordinate.
        animate(e,alpha=0,time=10)
        sleep(10)
        StoreEffect(e)


A number of issues were present in your pool implementation. Among them was the way you were storing them itself, and how you were cleaning appearances. I sidestepped the problem by storing a reference to the appearance in the pool, as well as pooling different subtypes of /visual_effect individually.

We no longer need to search for visual_effect objects of the right type in order to grab one. They are always stored with their subtypes appropriately in their own individual lists.

We're also better able to cleanly handle per-effect type cleanup of leftover data by defining Recycled()/Stored() on the objects themselves. If you have a particular effect type that needs to clean up junk data on the instance itself pertaining to its specific use in the world, you can do that by overriding Recycled()/Stored() and tying that functionality down to specific types of pooled objects.
Aug 28 2017, 3:02 pm
Sorry for the late response, I didnt even use the pc at all yday. And thanks for the code and all the effort you're putting in.
 var/effect/e = new type()//provides an error

I assumed you meant for pool to replace type, as its an object path I believe, however, this produces a different error,
///ERROR
runtime error: Cannot create objects of type /list.
proc name: RecycleEffect (/proc/RecycleEffect)
  source file: Effects.dm,52 ///this is referring to the new pool() line
  usr:  (/mob/Player)
  src: null
  usr.loc: the grass (14,6,1) (/turf/grass)
  call stack:
RecycleEffect(/list (/list), , /visual_effect (/visual_effect))


////CODE
proc
    Hit_Fx(mob/M,stateuse)
        set waitfor=0
        var/obj/I=RecycleEffect(/visual_effect/Hit_fx)
        I.icon_state=stateuse
        var/xloc=rand(-15,15),yloc=rand(-15,15)
        I.pixel_x=xloc
        I.pixel_y=yloc
        I.setPos(M.gx,M.gy,M.z)
        sleep(3)
        StoreEffect(I)


I no longer receive the bad obj error though, so I guess thats a plus.
Aug 28 2017, 3:40 pm (Edited on Aug 29 2017, 1:03 am)
I made some small mistakes, it looks like. And really, it's no effort at all. I banged this snippet out off the top of my head in ~20-30 minutes or so. I rarely compile code I share on Dev Help though, so sometimes I miss small mistakes.

var/list/effect_pool = list()

proc
    StoreEffect(visual_effect/e) //call to store an effect obj in its pool
        var/list/l = effect_pool[e.type]
        l += e
        e.Stored()

    RecycleEffect(pool,appearance) //call to grab an effect from a pool or create a new one
        var/list/l = effect_pool[pool]
        var/visual_effect/e
        if(l&&l.len>1) //if the pool exists and has any elements in it
            l -= (e = l[2]) //remove the item from the list and set the return value
            e.Recycled()
        else
            e = new pool() //no objects to recycle, create a new one and set return value
        if(appearance) e.appearance = appearance
        return e

    CleanPool(pool,maximum=0) //call to purge excess pool items
        if(maximum<0) return //just don't.
        var/list/l = effect_pool[pool]
        if(l&&l.len>maximum+1)
            l.Cut(maximum+1,0)

    PreparePool(pool,minimum=1) //call to prepare a pool by packing it with a minimum number of items
        if(minimum<0) return //also don't
        var/list/l = effect_pool[pool]
        if(!l) //if the pool doesn't exist, create at least one of the object and store it in the pool
            var/visual_effect/e = new pool()
            l = effect_pool[pool]
            if(minimum>0) //if we just want to prep the pool, itself, create a temporary object and let it be garbage collected.
                l += e
        if(l.len<minimum+1) //if the pool isn't at the desired length already, loop creating pooled objects until we reach the desired size
            var/count = minimum-l.len+1
            while(count--)
                l += new pool()

//this atom allows you to define default properties for pooled effects
visual_effect
    parent_type = /obj

    New()
        ..()
        if(!effect_pool[type])
             effect_pool[type] = list(appearance)

    proc
        Recycled() //called when the object is pulled out of the effect pool. Not called when created when the pool is empty.

        Stored() //called when the object is put in an effect pool. Not called when initially stored in a list, only when stored using the StoreEffect() proc.
            loc = null
            appearance = effect_pool[type][1]


The above is the corrected code.

The runtime error you were getting though, is actually telling me that you aren't showing me the code that is actually the problem:

runtime error: Cannot create objects of type /list.
proc name: RecycleEffect (/proc/RecycleEffect)
  source file: Effects.dm,52 ///this is referring to the new pool() line
  usr:  (/mob/Player)
  src: null
  usr.loc: the grass (14,6,1) (/turf/grass)
  call stack:
RecycleEffect(/list (/list), , /visual_effect (/visual_effect)) //look at the args you are passing to RecycleEffect().


You are passing the wrong arguments to RecycleEffect() somewhere else in your code.

It looks like by switching to my code, we actually found your original error. You were passing list references to your old effect recycling code. Your old recycling code was brittle enough that it didn't actually catch the mistake, and was causing you all kinds of data misalignment errors. The error was a byproduct of junk data in your old recycle list. This means (totally jerking myself off here) that what I said here was spot on the nose:
http://www.byond.com/forum/?post=2287724#comment23216586

But even if you went back to your old code, the new code will perform way better for you. The old search is o(n) complexity where n is the number of recycled nodes in the global list, while the new list is o(log(n)) complexity where n is the number of unique recycled node types.

Imagine a situation where you have 100 different types of recycled objects with 1000 nodes each in the recycled list.

That's 100K objects. Your old search would be worst case 100K node traversals, while mine would only need 7 traversals worst case with the same constraints. In almost every case, the O(log(n)) is gonna be faster.

while the new search would be:

(Alright, I'm done jerking my code off.)

Figure out where that list is being passed into RecycleEffect and you'll be solid.
Aug 29 2017, 9:19 pm
Ah yep, I found the problem. Thanks for your help man, and I think I'll keep using your code (not because I'm a cheapo who feels no qualms or guilt from benefiting from someone else's work though....probably)
Ah, and while we're here I've got another consultation, if you don't mind.

obj
    Hitbox
        density=0
        icon='Clear.dmi'
        layer=EFFECT_LAYER
        alpha=150

        New(mob/o,d,bw,bh)
            if(!o)
                return
            Owner=o
            dir=Owner.dir
            if(d)   dir=d
            if(!bh && !bw)  CRASH("No hitbox size specified")
            if(!bw) bw=bh   ; if(!bh)   bh=bw
            var/icon/I=new('Clear.dmi')
            I.DrawBox(RED,1,1,bw,bh)
            icon=I
            bound_width=bw
            bound_height=bh


    proc
        HitboxCheck()
            var/bw,bh
            xloc=Owner.gx
            yloc=Owner.gy
            var/list/hit=list()
            switch(dir)
                if(NORTH)
                    yloc+=bound_height
                    if(bound_width>Owner.bound_width)
                        bw=bound_width-Owner.bound_width
                        step_x=-(bw/2)
                if(SOUTH)
                    yloc-=bound_height
                    if(bound_width>Owner.bound_width)
                        bw=bound_width-Owner.bound_width
                        step_x=-(bw/2)
                if(EAST)
                    xloc+=bound_width
                    if(bound_height>Owner.bound_height)
                        bh=bound_height-Owner.bound_height
                        step_y=-(bh/2)
                if(WEST)
                    xloc-=bound_width
                    if(bound_height>Owner.bound_height)
                        bh=bound_height-Owner.bound_height
                        step_y=-(bh/2)
            setPos(xloc,yloc,Owner.z)
            for(var/mob/M in bounds(src))
                if(M.alive)
                    if(M!=Owner)    hit+=M
            return hit

It does what its supposed to do, except for one miniscule part, the icon isn't displayed on the map. Its able to catch mobs within its bounding box which i think should mean its being placed on the map without problems, however regardless of whether I use DrawBox() or set an icon to it exclusively, I don't see them on the map.