Death proc not killing the right person.

ID:139102
 
Jun 27 2011, 3:45 am
I made these fighting procs and death proc, and found that when someone clicks on another person, it makes them fight if they aren't fighting something else, and when their health goes down to 0, they die. Everything works up until the "die" part, because when it executes the death proc, it kills the person who didn't die, basically meaning, the person who died isn't effected by the Death proc, however the person who killed them is the killer and gets killed! Heres the code:
mob
    Click()
        for(var/mob/M in oview(1))
            if(M==usr)
                return
            if(M.fighting==1)
                usr << "Someone else is fighting them!"
                return
            if(usr.fighting==1)
                usr << "You are fighting someone else!"
                return
            if(M.fighting==0)
                M.fighting = 1
                if(usr.weapon == null)
                    usr.punchfight(M)
                if(usr.weapon == "sword")
                    usr.swordfight(M)
mob
    proc
        punchfight(mob/M)
            fightstart
                flick("punch",usr)
                sleep(1)
                var/damage = usr.attack-M.defense+usr.accuracy/2
                if(damage <= 0)
                    damage = 0
                usr << "You hit [M] for [damage] damage!"
                M << "[usr] hit you for [damage] damage!"
                M.health -= damage
                if(M.health <= 0)
                    M.death(usr)
                    return
                sleep(5)
                if(M.weapon == null)
                    flick("punch",M)
                    sleep(1)
                    var/damage2 = M.attack-usr.defense+M.accuracy/2
                    if(damage2 <= 0)
                        damage2 = 0
                    M << "You hit [M] for [damage2] damage!"
                    usr << "[M] hit you for [damage2] damage!"
                    usr.health -= damage2
                    if(usr.health <= 0)
                        usr.death(M)
                        return
                    sleep(5)
                    goto fightstart
                if(M.weapon == "sword")
                    flick("punch",M)
                    sleep(1)
                    var/damage3 = M.attack+1-usr.defense+M.accuracy
                    if(damage3 <= 0)
                        damage3 = 0
                    M << "You hit [M] for [damage3] damage!"
                    usr << "[M] hit you for [damage3] damage!"
                    usr.health -= damage3
                    if(usr.health <= 0)
                        usr.death(M)
                        return
                    sleep(5)
                    goto fightstart
        swordfight(mob/M)
            fightstart
                flick("slash",usr)
                sleep(1)
                var/damage = usr.attack+1-M.defense+usr.accuracy
                if(damage <= 0)
                    damage = 0
                usr << "You hit [M] for [damage] damage!"
                M << "[usr] hit you for [damage] damage!"
                M.health -= damage
                if(M.health <= 0)
                    M.death(usr)
                    return
                sleep(5)
                if(M.weapon == null)
                    flick("punch",M)
                    sleep(1)
                    var/damage2 = M.attack-usr.defense+M.accuracy/2
                    if(damage2 <= 0)
                        damage2 = 0
                    M << "You hit [M] for [damage2] damage!"
                    usr << "[usr] hit you for [damage2] damage!"
                    usr.health -= damage2
                    if(usr.health <= 0)
                        usr.death(M)
                        return
                    sleep(5)
                    goto fightstart
                if(M.weapon == "sword")
                    flick("punch",M)
                    sleep(1)
                    var/damage3 = M.attack+1-usr.defense+M.accuracy
                    if(damage3 <= 0)
                        damage3 = 0
                    M << "You hit [M] for [damage3] damage!"
                    usr << "[usr] hit you for [damage3] damage!"
                    usr.health -= damage3
                    if(usr.health <= 0)
                        usr.death(M)
                        return
                    sleep(5)
                    goto fightstart
        death(mob/M)
            world << "[usr] was killed by [M]!"
            usr << "You died"
            M << "You killed [usr]"
            usr.health = usr.maxhealth
            M.exp += usr.expgain
            if(M.exp >= M.maxexp)
                M << "You gained a combat level!"
                M.level += 1
                M.maxexp += 10
                M.exp = 0
            M.magicexp += usr.maggain
            if(M.magicexp >= M.magicmaxexp)
                M << "You gained a magic level!"
                M.maxmagic += 10
                M.magiclvl += 1
                M.magicmaxexp += 10
                M.magicexp = 0
            M.healthexp += usr.hpgain
            if(M.healthexp >= M.healthmaxexp)
                M << "You gained a health level!"
                M.maxhealth += 10
                M.healthlvl += 10
                M.healthmaxexp += 10
                M.healthexp = 0
            M.accuracyexp+=usr.accgain
            if(M.accuracyexp >= M.accuracymaxexp)
                M << "You gained an accuracy level!"
                M.accuracy += 1
                M.accuracymaxexp += 10
                M.accuracyexp = 0
            M.defenseexp+=usr.defgain
            if(M.defenseexp >= M.defensemaxexp)
                M << "You gained a defense level!"
                M.defense += 1
                M.defensemaxexp += 10
                M.defenseexp = 0
            M.attackexp+=usr.attgain
            if(M.attackexp >= M.attackmaxexp)
                M << "You gained an attack level!"
                M.attack += 1
                M.attackmaxexp += 10
                M.attackexp = 0
            usr.fighting = 0
            M.fighting = 0
            return


If anyone could please tell me where I'm wrong on this string of code, I would really appreciate it!
Jun 27 2011, 4:11 am (Edited on Jun 27 2011, 6:11 am)
You can only use usr in verbs and click/dblclick, not in procs. Replace usr with src in everything you posted EXCEPT Click() and it should work for you.
Jun 27 2011, 5:08 am
In response to Jotdaniel
Heh, berbs.

I'm trying to unwind that goto logic. If I'm correct, the fight continues as long as you have a weapon?
Jun 27 2011, 5:19 am
There are a few problems in this code.

One of the things I recommend changing is that your death proc should really be a death check. That is, the HP check should happen inside that proc instead of before you call it.

Because punchfight(), swordfight(), and death() are procs, you should not be using usr in them at all. Think of usr as only being available in verbs, and you'll save yourself a lot of headaches. I believe in each of those cases src is actually what you want.

Calling the death routine as M.death(src) is correct, since you want the killer to be the argument and the owner of the proc should be the mob that's potentially dying.

Leveling up the killer should not be done in death(). You should have a separate proc for that, so inside of death() you can call M.levelup(). To explain why, notice how all of your code regarding the leveling up has M.something in practically every line. That's all stuff that has to do with the killer, and it's complex enough that it really should be handled in its own proc.

Finally, you should get rid of that goto logic. What you should use instead is a while() loop. In each of the places where you're currently using goto, you only need a continue statement instead. For instance:

while(fighting && M && M.fighting)
 if(...)
   ...
   continue
 if(...)
   ...
   continue
Jun 27 2011, 6:10 am
In response to Stephen001
Nah, he returns the proc after the death(). Not an error as such, but not really good.
Jun 27 2011, 10:04 am
In response to Lummox JR
Lummox JR wrote:
There are a few problems in this code.

One of the things I recommend changing is that your death proc should really be a death check. That is, the HP check should happen inside that proc instead of before you call it.

Because punchfight(), swordfight(), and death() are procs, you should not be using usr in them at all. Think of usr as only being available in verbs, and you'll save yourself a lot of headaches. I believe in each of those cases src is actually what you want.

Calling the death routine as M.death(src) is correct, since you want the killer to be the argument and the owner of the proc should be the mob that's potentially dying.

Leveling up the killer should not be done in death(). You should have a separate proc for that, so inside of death() you can call M.levelup(). To explain why, notice how all of your code regarding the leveling up has M.something in practically every line. That's all stuff that has to do with the killer, and it's complex enough that it really should be handled in its own proc.

Finally, you should get rid of that goto logic. What you should use instead is a while() loop. In each of the places where you're currently using goto, you only need a continue statement instead. For instance:

while(fighting && M && M.fighting)
>  if(...)
>    ...
>    continue
>  if(...)
>    ...
>    continue


So, my code would be this?
mob
    var
        fighting = 0
        weapon
    Click()
        for(var/mob/M in oview(1))
            if(M==usr)
                return
            if(M.fighting==1)
                usr << "Someone else is fighting them!"
                return
            if(usr.fighting==1)
                usr << "You are fighting someone else!"
                return
            if(M.fighting==0)
                M.fighting = 1
                if(usr.weapon == null)
                    usr.punchfight(M)
                if(usr.weapon == "sword")
                    usr.swordfight(M)
mob
    proc
        punchfight(mob/M)
            while(fighting && M && M.fighting)
                flick("punch",src)
                sleep(1)
                var/damage = src.attack-M.defense+src.accuracy/2
                if(damage <= 0)
                    damage = 0
                src << "You hit [M] for [damage] damage!"
                M << "[src] hit you for [damage] damage!"
                M.health -= damage
                M.death(src)
                sleep(5)
                if(M.weapon == null)
                    flick("punch",M)
                    sleep(1)
                    var/damage2 = M.attack-src.defense+M.accuracy/2
                    if(damage2 <= 0)
                        damage2 = 0
                    M << "You hit [M] for [damage2] damage!"
                    src << "[M] hit you for [damage2] damage!"
                    src.health -= damage2
                    src.death(M)
                    sleep(5)
                    continue
                if(M.weapon == "sword")
                    flick("punch",M)
                    sleep(1)
                    var/damage3 = M.attack+1-src.defense+M.accuracy
                    if(damage3 <= 0)
                        damage3 = 0
                    M << "You hit [M] for [damage3] damage!"
                    src << "[M] hit you for [damage3] damage!"
                    src.health -= damage3
                    src.death(M)
                    sleep(5)
                    continue
        swordfight(mob/M)
            while(fighting && M && M.fighting)
                flick("slash",src)
                sleep(1)
                var/damage = src.attack+1-M.defense+src.accuracy
                if(damage <= 0)
                    damage = 0
                src << "You hit [M] for [damage] damage!"
                M << "[src] hit you for [damage] damage!"
                M.health -= damage
                M.death(src)
                sleep(5)
                if(M.weapon == null)
                    flick("punch",M)
                    sleep(1)
                    var/damage2 = M.attack-src.defense+M.accuracy/2
                    if(damage2 <= 0)
                        damage2 = 0
                    M << "You hit [M] for [damage2] damage!"
                    src << "[src] hit you for [damage2] damage!"
                    src.health -= damage2
                    src.death(M)
                    sleep(5)
                    continue
                if(M.weapon == "sword")
                    flick("punch",M)
                    sleep(1)
                    var/damage3 = M.attack+1-src.defense+M.accuracy
                    if(damage3 <= 0)
                        damage3 = 0
                    M << "You hit [M] for [damage3] damage!"
                    src << "[src] hit you for [damage3] damage!"
                    src.health -= damage3
                    src.death(M)
                    sleep(5)
                    continue
        death(mob/M)
            if(M.health != 0)
                return
            if(M.health <= 0)
                world << "[src] was killed by [M]!"
                src << "You died"
                M << "You killed [src]"
                src.health = src.maxhealth
                M.Levelup(src)
mob
    proc
        Levelup(mob/M)
            src.exp += M.expgain
            if(src.exp >= src.maxexp)
                src << "You gained a combat level!"
                src.level += 1
                src.maxexp += 10
                src.exp = 0
            src.magicexp += M.maggain
            if(src.magicexp >= src.magicmaxexp)
                src << "You gained a magic level!"
                src.maxmagic += 10
                src.magiclvl += 1
                src.magicmaxexp += 10
                src.magicexp = 0
            src.healthexp += M.hpgain
            if(src.healthexp >= src.healthmaxexp)
                src << "You gained a health level!"
                src.maxhealth += 10
                src.healthlvl += 10
                src.healthmaxexp += 10
                src.healthexp = 0
            src.accuracyexp+=M.accgain
            if(src.accuracyexp >= src.accuracymaxexp)
                src << "You gained an accuracy level!"
                src.accuracy += 1
                src.accuracymaxexp += 10
                src.accuracyexp = 0
            src.defenseexp+=M.defgain
            if(src.defenseexp >= src.defensemaxexp)
                src << "You gained a defense level!"
                src.defense += 1
                src.defensemaxexp += 10
                src.defenseexp = 0
            src.attackexp+=M.attgain
            if(src.attackexp >= src.attackmaxexp)
                src << "You gained an attack level!"
                src.attack += 1
                src.attackmaxexp += 10
                src.attackexp = 0
            src.fighting = 0
            M.fighting = 0
            return

However when I click the mob, it changes the fighting var for both mobs to 1, but the fighting doesn't occur... Is there anything wrong with this new string of code?
Jun 27 2011, 10:19 am
In response to Narutorox123456
I think you're on the right track but I still see some issues. One critical problem is in the beginning of death() where you're bailing out of hp isn't exactly 0--but you still need to account for the <0 case.

There's also a design issue to be looked at here, where you're using different fighting procs and a complex set of ifs and damage calculations. It seems to me you should have a single attack proc, and just apply the correct flick() for the weapon involved and calculate damage based on procs belonging to both mobs. What I'm seeing in particular is that you're implementing the fight-back portion as part of the main fighting proc, when really you should simply have a single attack proc and have your battle manager call that whenever it's appropriate.

Another problem I notice here is the weapon=="sword" part. This suggests the equipment system isn't quite up to snuff. If you have a weapon var, it should be set to the weapon that you have equipped--not a string.

Here's a small piece of how a modified battle system would look:

mob/proc/Attack(mob/target)
  var/damage = max(0, AttackPoints() - target.DefensePoints())
  src << "You hit [target] for [damage] hp."
  target << "[src] hits you for [damage] hp."
  target.hp -= damage
  target.DeathCheck(src)

mob/AttackPoints()
  if(!weapon)
    return attack + round(accuracy/2)
  return attack * weapon.attack_modifier + accuracy * weapon.accuracy_modifier

mob/DefensePoints()
  return defense


Then the battle could be initiated like so:

mob/proc/Battle(mob/target)
  fighting = 1
  target.fighting = 1
  while(fighting && target && target.fighting)
    Attack(target)
    // is the battle over?
    if(!target || !target.fighting) break
    sleep(5)
    // hit back
    target.Attack(src)
    // still not over?
    if(!fighting) break
    sleep(5)
  fighting = 0
  if(target) target.fighting = 0
Jun 27 2011, 10:29 am
In response to Lummox JR
Lummox JR wrote:
I think you're on the right track but I still see some issues. One critical problem is in the beginning of death() where you're bailing out of hp isn't exactly 0--but you still need to account for the <0 case.

There's also a design issue to be looked at here, where you're using different fighting procs and a complex set of ifs and damage calculations. It seems to me you should have a single attack proc, and just apply the correct flick() for the weapon involved and calculate damage based on procs belonging to both mobs. What I'm seeing in particular is that you're implementing the fight-back portion as part of the main fighting proc, when really you should simply have a single attack proc and have your battle manager call that whenever it's appropriate.

Actually, it's also a damage difference too
Another problem I notice here is the weapon=="sword" part. This suggests the equipment system isn't quite up to snuff. If you have a weapon var, it should be set to the weapon that you have equipped--not a string.
Well, currently I have one equippable weapon which is a mithirl sword, which when equipped, will set the var to "sword" ATM
Here's a small piece of how a modified battle system would look:

mob/proc/Attack(mob/target)
>   var/damage = max(0, AttackPoints() - target.DefensePoints())
>   src << "You hit [target] for [damage] hp."
>   target << "[src] hits you for [damage] hp."
>   target.hp -= damage
>   target.DeathCheck(src)
>
> mob/AttackPoints()
>   if(!weapon)
>     return attack + round(accuracy/2)
>   return attack * weapon.attack_modifier + accuracy * weapon.accuracy_modifier
>
> mob/DefensePoints()
>   return defense

Then the battle could be initiated like so:

mob/proc/Battle(mob/target)
>   fighting = 1
>   target.fighting = 1
>   while(fighting && target && target.fighting)
>     Attack(target)
>     // is the battle over?
>     if(!target || !target.fighting) break
>     sleep(5)
>     // hit back
>     target.Attack(src)
>     // still not over?
>     if(!fighting) break
>     sleep(5)
>   fighting = 0
>   if(target) target.fighting = 0

Everything in bold above is my response.
I'll try that and tell you my results.
Jun 27 2011, 10:58 am
In response to Narutorox123456
Ok, so now I reduced to one Attack var, however, since yours gave errors, I tried to get to merge both swordfight() and punchfight() so I can get fight().
mob
    var
        fighting = 0
        weapon
    Click()
        for(var/mob/M in oview(1))
            if(M==usr)
                return
            if(M.fighting==1)
                usr << "Someone else is fighting them!"
                return
            if(usr.fighting==1)
                usr << "You are fighting someone else!"
                return
            if(M.fighting==0)
                M.fighting = 1
                usr.fighting = 1
                usr.fight(M)
mob
    proc
        attack(mob/M)
            if(src.weapon=="sword")
                flick("slash",src)
                sleep(1)
                var/damage = src.attack+1-M.defense+src.accuracy
                if(damage <= 0)
                    damage = 0
                src << "You hit [M] for [damage] damage!"
                M << "[src] hit you for [damage] damage!"
                M.health -= damage
                M.death(src)
            return
            if(src.weapon==null)
                flick("punch",src)
                sleep(1)
                var/damage = src.attack-M.defense+src.accuracy/2
                if(damage <= 0)
                    damage = 0
                src << "You hit [M] for [damage] damage!"
                M << "[src] hit you for [damage] damage!"
                M.health -= damage
                M.death(src)
            return
        fight(mob/M)
            while(fighting && M && M.fighting)
                src.attack(M)
                sleep(5)
                M.attack(src)
        death(mob/M)
            if(M.health <= 0)
                world << "[src] was killed by [M]!"
                src << "You died"
                M << "You killed [src]"
                src.health = src.maxhealth
                M.Levelup(src)
        Levelup(mob/M)
            src.exp += M.expgain
            if(src.exp >= src.maxexp)
                src << "You gained a combat level!"
                src.level += 1
                src.maxexp += 10
                src.exp = 0
            src.magicexp += M.maggain
            if(src.magicexp >= src.magicmaxexp)
                src << "You gained a magic level!"
                src.maxmagic += 10
                src.magiclvl += 1
                src.magicmaxexp += 10
                src.magicexp = 0
            src.healthexp += M.hpgain
            if(src.healthexp >= src.healthmaxexp)
                src << "You gained a health level!"
                src.maxhealth += 10
                src.healthlvl += 10
                src.healthmaxexp += 10
                src.healthexp = 0
            src.accuracyexp+=M.accgain
            if(src.accuracyexp >= src.accuracymaxexp)
                src << "You gained an accuracy level!"
                src.accuracy += 1
                src.accuracymaxexp += 10
                src.accuracyexp = 0
            src.defenseexp+=M.defgain
            if(src.defenseexp >= src.defensemaxexp)
                src << "You gained a defense level!"
                src.defense += 1
                src.defensemaxexp += 10
                src.defenseexp = 0
            src.attackexp+=M.attgain
            if(src.attackexp >= src.attackmaxexp)
                src << "You gained an attack level!"
                src.attack += 1
                src.attackmaxexp += 10
                src.attackexp = 0
            src.fighting = 0
            M.fighting = 0
            return

So now it gives no errors when I compile. I couldn't avoid using usr for the click() because I noticed src would be the person clicked and usr would be the person clicking, and just made src become M to avoid confusion for me.
Also for some reason when I click on them, it doesn't execute fight(), but, for some reason, the mob I click gets the var "fighting" set to one and I get my fighting var set to one. I fixed it like you said, but still no dice.
Jun 27 2011, 11:25 am
In response to Narutorox123456
Narutorox123456 wrote:
...I couldn't avoid using usr for the click() because I noticed src would be the person clicked and usr would be the person clicking, and just made src become M to avoid confusion for me.

Click()/DblClick() is one of the very few procedures where usr is okay to be used in as it is typically defined (unsafe usage would be where procedures are indirectly called).

Lets break this down:
mob
    var
        fighting = 0
        weapon
    Click()
        for(var/mob/M in oview(1))
            if(M==usr)
                return
            if(M.fighting==1)
                usr << "Someone else is fighting them!"
                return
            if(usr.fighting==1)
                usr << "You are fighting someone else!"
                return
            if(M.fighting==0)
                M.fighting = 1
                usr.fighting = 1
                usr.fight(M)

  • M==usr check is useless here due to the oview(), which excludes the usr (which is the center of oview(1, usr <- by default))
  • X.fighting == 1/0: checking if M.fighting == 0 is useless here as you already checked if it was == 1 earlier.
    To be more robust, omit the == 1 if you are checking for a TRUE variable (which is anything except a false variable) and use the ! operator to check if the value is FALSE (= 0, null, ""). Ex: 
    if(!X.var) --> FALSE = 0, "", null
    if(X.var) --> TRUE = Anything but a FALSE value
  • This would be much better if you made a procedure to check if these situations: 
    mob/proc/CheckFight(mob/M)
                if(M == src) return 0
                if(M.fighting)
                    src << "Someone else is fighting them!"
                    return 0
                if(src.fighting)
                    src << "You are fighting someone else!"
                    return 0
                return 1

    mob/Click()
       if(usr.CheckFight(src))  //  src = M, this checks if it is okay to attack
             Attack procedure here

       else  //  Do not need to have this unless you want an exception...
            Not okay to attack

Also for some reason when I click on them, it doesn't execute fight(), but, for some reason, the mob I click gets the var "fighting" set to one and I get my fighting var set to one. I fixed it like you said, but still no dice.

If you're not indenting with tabs, you should. Look where you have the return in the attack(), they're OUTSIDE the if(). It's stopping the procedure before the weapon==null check.

Also, maybe you should make "if(src.weapon==null)" become an "else"? Or if you are planning to have more weapons, maybe a switch() statement.

In programming, most people try to make things less redundant. This includes making modules, procedures for repeating events/checks (such as the CheckFight() procedure above) and the easiest way to tell if something can potentially be cut down is if you see a duplicate, such as the damage/attack message in your attack() ... and this is how we'll remove it: 
        attack(mob/M)
         var/damage = 0
            if(src.weapon=="sword")
                flick("slash",src)
                damage = src.attack+1-M.defense+src.accuracy
            else    //  old statement was if(src.weapon==null).
                flick("punch",src)
                damage = src.attack-M.defense+src.accuracy/2

           damage = min(max(0, damage), M.health)  //  This limits the damage value to be between 0 - M's current HP (so it doesn't go in to a negative value). If you want 0 - X damage, remove the min() portion, leaving max(0, damage)
           src << "You hit [M.name] for [damage] damage!"
           M << "[src.name] hit you for [damage] damage!"
           M.health = max(0, M.health - damage)  // Making the health as low as 0 and nothing less
           M.death(src)


If you will have other damaging events included that does not call attack(), I suggest replacing the damage = ... and settings M's health in to another procedure so you do not have to make a redundant check: 
mob/proc/TakeHP(damage = 0, mob/killer)
   damage = min(max(0, damage), M.health)

       M.health = max(0, M.health - damage)  // Making the health as low as 0 and nothing less
       M.death(src)  //  This procedure will be halted until death() is done. If you do not want that, add spawn() prior to this line
       return damage  //  returns how much damage was dealt


   attack()
       ...
       damage = M.TakeHP(damage, src)   //  Damage set to the amount of damage dealt
       X << damage message

Jun 27 2011, 12:42 pm
In response to GhostAnime
Thanks a lot! It works perfectly! The only thing is, the Death verb seems reversed with M and src, but I can fix it easily without help! Again, thanks!