Projetile Bump() only works for enemies but not players

ID:1948334
 
Sep 24 2015, 6:55 am
(See the best response by Ter13.)
Code:
Bump(mob/M)
    call(/mob/proc/dmg)(M,src.dice,src.wtype)
    del src


dmg(mob/M,dice,wtype)
    var/dmg = 0
    if(istype(usr,/mob/player))
        if(wtype == "melee")
            dmg = roll(dice,8) + usr.str
        else if(wtype == "spell")
            dmg = roll(dice,8) + usr.int
    else
        dmg = roll(dice,8)
    M.hp = M.hp - dmg
    usr << "You did \blue[dmg] dmg!"
    M << "You've been hit for \red[dmg] dmg!"
    if (M.hp <= 0)
        del M


So I have a function which shoots out a projectile based on the weapon you are using. Each weapon has a dice variable which the dmg() proc uses.
For some reason when an enemy uses this projectile function Bump() and dmg() works fine, yet when a player uses it the damage isn't calculated. I've done some tests and it seems that all the variables that are needed are being passed through Bump() yet I never get any results.

Please ask if you need to see more code. I tried to post as little as possible for simplicity.

Sep 24 2015, 7:07 am
usr never refers to NPCs, so it's usually bad to use usr in procs. It's also improper to use call()() in a situation like this.

Usually, projectile interaction goes like this:
mob
    proc
        // example proc for a player or NPC to shoot a projectile
        FireProjectile(ProjectileType)
            var obj/projectile/new_projectile = new ProjectileType (src)
            walk(new_projectile, dir)
            return new_projectile

obj/projectile
    var
        mob/owner

    New(Owner)
        owner = Owner
        loc = owner.loc

    Bump(atom/movable/Obstacle)
        if(owner && ismob(Obstacle))
            var mob/hit = Obstacle
            //  do damage calculation: 
            // [hit] was hit by [owner] with the projectile [src]

        // let garbage collection delete this projectile
        loc = null
Sep 24 2015, 7:42 am
Best response
You've got a few things going on here that are gonna cause you major headaches.

A few pointers:

1) You probably shouldn't be using call() here at all. Just make sure the object bumped is a mob before calling the function normally.
call(/mob/proc/dmg)(M,src.dice,src.wtype)


2) Manual deletion is slow. You should avoid it. It's better to just keep track of spare references to objects, set their loc to null, and manually clean up the references.
del src


3) You are misuing usr. If an NPC causes a projectile to exist, usr will never be that NPC. Usr should never be used in anything except a verb, or a proc that acts like a verb (click procs), or anything that's meant to ONLY be called from a verb. It shouldn't even be used in all verbs either. It should only be used in verbs where the src setting is not usr. Usr should be avoided because it's typecast to /mob. Src is more specific, and thus will be more reliable when it's the object you intend. Otherwise, you need to pass a reference to the object you intend instead.
if(istype(usr,/mob/player))
dmg = roll(dice,8) + usr.str
dmg = roll(dice,8) + usr.int
usr << "You did \blue[dmg] dmg!"


4) You are treating /mob/proc/dmg as a static global. You should not do this. There is never a reason to embed a static global into a class in DM unless you intend to use that class as a namespace. There is questionable reason for this at the best of times. This is not one of those times.
call(/mob/proc/dmg)(M,src.dice,src.wtype)
dmg(mob/M,dice,wtype)


What I'm going to do is go ahead and clean this up a bit to give you a demonstration of a clean, extendable, simple call structure. A few things that I noticed could be improved after fixing the bug:

* The projectile's damage should be calculated when it's created, not during the hit. This will simplify quite a lot of the call structure.

* The TakeDamage function should be a proper member of the mob class. Not a global static. This will allow you to take advantage of polymorphic variation later on.

* You probably want to create some hooks for dealing with deaths and kills. This will simplify your code much later.

* You don't want your damage function generating outputs. You want to leave that to the specific attack. Instead, make the TakeDamage function return how much damage it dealt so the attack itself can print its own messages. That way you can have variation of messages, or even silent damage if you want.

obj/projectile
    var/tmp
        mob/owner
        damage
        damage_type

    Bump(mob/M)
        if(istype(M))
            var/dealt = M.TakeDamge(owner,src,damage,damage_type)
            if(owner)
                if(M.client) M << "You were hit by [owner]'s [src] for [dealt] damage!"
                if(owner.client) M << "Your [src] hit [M] for [dealt] damage!"
            else if(M.client) M << "[src] hit you for [dealt] damage!"
        loc = null //don't call manual deletion if it can be avoided.

    New(loc,mob/owner,dice,sides,damage_type)
        damage = roll(dice,sides)
        if(owner&&istype(owner))
            src.owner = owner
            switch(damage_type)
                if("melee")
                    damage += owner.str
                if("magic","fire","ice")
                    damage += owner.int
        src.damage_type = damage_type

mob
    proc
        //TakeDamage is called when this mob is damaged by something.
        //attacker is the mob that attacked this one, or null.
        //source is the object that actually did the damage.
        //damage is the amount of damage done
        //damage type is the kind of damage done
        //attacker can be null if you want.
        TakeDamage(mob/attacker,atom/source,damage,damage_type)
            . = min(hp,damage)
            hp -= .

            if(!hp)
                Die(attacker,source,damage,damage_type)

        //this is called when a mob dies.
        //killer is the mob that caused the death
        //source is the weapon/spell that caused the death
        //damage/damage_type are included for special deaths. For instance, if you want a mob to turn to ash when they are killed by >60 fire damage, etc.
        Die(mob/killer,atom/source,damage,damage_type)
            if(killer&&istype(killer))
                killer.Kill(src,source,damage,damage_type)
            loc = null

        //this is called when a mob dies by this one's hand.
        //victim is the mob that this mob killed.
        //source is the object that this mob used to kill that one.
        //damage/damage_type are included for special kills, say if you want this mob to gain bonus experience for killing someone with >50 physical damage.
        Kill(mob/victim,source,damage,damage_type)
            //do something when you kill someone
Sep 24 2015, 10:47 am
Ho wow. I expected that I had maybe one or two lines wrong, this kind of response is surprising.

I'm still a novice at programming in general so all this information is very useful for me going forward and not just for this issue.

Thanks a lot guys!
Sep 24 2015, 11:14 am
Actually, I have one more question about how usr works.

My projectile proc is set in /obj/weapon so that I can use src to refer to the weapon being used while usr refers to whoever activated the weapon. This works fine even when usr is an NPC.
Should I be changing the placement of the projectile proc to /mob so I can change out usr for src?
Sep 24 2015, 11:27 am
No. You'll want to pass a reference to the weapon to the projectile if you want to reference it.
Sep 24 2015, 1:37 pm
Kaiochao's projectile constructor is a great example of how to do projectiles right, with one exception: the walk() proc should be called by the projectile's New(), not by the caller. So to clean up that part of his example:

mob/proc
    // example proc for a player or NPC to shoot a projectile
    FireProjectile(ProjectileType)
        return new ProjectileType(src)

obj/projectile
    var/tmp/mob/owner

    New(mob/_owner)
        owner = _owner
        loc = owner.loc
        walk(src, owner.dir)

If you want a reference to the weapon that fired it, pass that as another argument.
mob/proc
    // example proc for a player or NPC to shoot a projectile
    FireProjectile(obj/weapon)
        var/tp = weapon.projectile   // each weapon has its own projectile type
        return new tp(src, weapon)

obj/projectile
    var/tmp/mob/owner
    var/tmp/obj/weapon/weapon

    New(mob/_owner, obj/weapon/_weapon)
        owner = _owner
        weapon = _weapon
        loc = owner.loc
        walk(src, owner.dir)

If you're worried about the weapon being deleted before the projectile, you might want to keep track of the weapon type rather than the weapon itself.