ID:139703
 
Code:
mob
General
verb/Punch()
set category="Unarmed"
for(var/mob/X in range(1))
if(get_dir(src,X))
if(src.punchcooldown==0)
if(src.canattack==1)
if(X.attackable==1)
src.damage=src.unarmed
src.attacking=pick(90;1,10;2)
if(src.attacking==1)
src<<"You punch [X.name] for [src.damage]."
X.hp -= src.unarmed
X.npcdeathcheck()
src.punchcooldown=1
spawn(20)src.punchcooldown=0
if(src.attacking==2)
src<<"You punch [X.name] for [src.unarmed*2]!"
X.hp -= src.unarmed*2
X.npcdeathcheck()
src.punchcooldown=1
spawn(20)src.punchcooldown=0
else
src<<"You can't attack that!"
else
src<<"Your attacks have been disabled!"
else
src<<"That power is on cooldown."


Problem description: It works just fine at first, but then, long after the player has walked away, it repeats the messages 'You punch so and so for something!'. I don't know how to fix this, so I would appreciate help.

You're using a loop for all mob types in one space of you. And, you're calling proc()'s inside the loops so, the loop is stalled until those proc()'s finish, and by that time you may have moved or other player's moved...so, to remedy this, it looks as if you're trying to just attack something directly infront of you.


mob
General
verb/Punch()
set category="Unarmed"
for(var/mob/X in (get_step(usr,usr.dir)))
spawn()
if(!src.punchcooldown)
if(src.canattack)
if(X.attackable)
src.damage=src.unarmed
src.attacking=pick(90;1,10;2)
switch(src.attacking)
if(1)
src<<"You punch [X.name] for [src.damage]."
X.hp -= src.unarmed
X.npcdeathcheck()
src.punchcooldown=1
spawn(20) src.punchcooldown=0
if(2)
src<<"You punch [X.name] for [src.unarmed*2]!"
X.hp -= src.unarmed*2
X.npcdeathcheck()
src.punchcooldown=1
spawn(20) src.punchcooldown=0
else
src<<"You can't attack that!"
return 0
else
src<<"Your attacks have been disabled!"
return 0
else
src<<"That power is on cooldown."
return 0
In response to Teh Governator
Teh Governator wrote:
> 
> mob
> General
> verb/Punch()
> set category="Unarmed"
> for(var/mob/X in (get_step(usr,usr.dir)))
> spawn()
> if(!src.punchcooldown)
> if(src.canattack)
> if(X.attackable)
> src.damage=src.unarmed
> src.attacking=pick(90;1,10;2)
> switch(src.attacking)
> if(1)
> src<<"You punch [X.name] for [src.damage]."
> X.hp -= src.unarmed
> X.npcdeathcheck()
> src.punchcooldown=1
> spawn(20) src.punchcooldown=0
> if(2)
> src<<"You punch [X.name] for [src.unarmed*2]!"
> X.hp -= src.unarmed*2
> X.npcdeathcheck()
> src.punchcooldown=1
> spawn(20) src.punchcooldown=0
> else
> src<<"You can't attack that!"
> return 0
> else
> src<<"Your attacks have been disabled!"
> return 0
> else
> src<<"That power is on cooldown."
> return 0
>


Or to make it even more compact and efficient...

mob
General
verb/Punch()
set category="Unarmed"
if(src.punchcooldown) src<<"That power is on cooldown."
if(!src.canattack) src<<"Your attacks have been disabled!"
if(src.punchcooldown||!src.canattack) return
for(var/mob/X in (get_step(usr,usr.dir)))
if(X.attackable)
src.damage=src.unarmed
if(prob(10)) src.damage=src.damage*2
src<<"You punch [X.name] for [src.damage]."
X.hp -= src.damage
X.npcdeathcheck()
break
src.punchcooldown=1
spawn(20) if(src) src.punchcooldown=0

What did I do? Well first off, some of those checks are for if you can punch period, which means they can stop the proc long before you're targeting someone. Also, the way you had it before would've spammed messages if there were multiple targets on 1 location in front of the attacker. If you really want those messages, feel free to put them back after break. In fact, I would advise against taking the messages out period. People understand skills have delays now, if you leave those messages in they will most likely be spammed (as a player cannot specifically count down the exact delay, so they will press the button early and probably a few times depending how into it they are).

Anyway, next. Your attacking=1;2 thing, that was not necessary as it was a 10% chance of doing something else, so by doing the prob() I was able to take out the switch and the attacking=pick() part. Also, the only difference between if(1) and if(2) was the damage, which by using the prob, I just changed the damage before the proc said how much you hit for, thus works the same, but less coding. I removed the delay part to after the break, just because I can. It has nothing to do with the person your attacking so for safety precautions, has no reason to be in that part anyway. And just in case you don't know, what is break? Well, break is like return() for loops, but slightly different. Take it's name into a literal account, it breaks the loop. This means, that once you attack someone (in this proc) the loop breaks, it won't attack multiple people even if there are multiple people on the same tile, while at the same time continuing the verb as you can see by the variable settings I put after.

Lastly, after the spawn() I put an if(src), why? Well over any given period of time, the user may not exist anymore.