Broken Aoe

ID:2042706
 
Feb 23 2016, 1:11 pm
Code:
mob/verb/WM()
    set
        name = "Windmill"
        category = "skills"

    for(var/mob/M in oview(1,usr))
        if(usr.SkillUse == 0 && M.Attackable == 1 && usr.Saber == 1)
            usr.SkillUse = 1
            flick('SkillWM.dmi',usr)
            var/Dmg = src.Off - M.Def
            Dmg *=5
            Critcheck()
            if(src.crit == 1)
                Dmg *= src.CritRate


            M.life -= Dmg
            view(M)<<"[src] hit [M] for [Dmg] Damage!"
            src.crit = 0
            M.Damagecheck()
            M.Deathcheck()

            sleep(usr.CoolDown * 5)
            usr.SkillUse = 0
        else return


Problem description:
My only problem is the code does not loop through all mobs in a one tile space from my character. I have tried several changes to the code, but here was my original piece that works on one mob, but not all.

What I want the code to do is hit all mobs in a one tile space from my character. An Area of Attack skill. But the darn thing just hits the one I am facing.

Feb 23 2016, 1:36 pm
I see two things:

sleep(usr.CoolDown * 5)


Not really the best place for that to be. You want the attack to affect all within range, so you can't afford to be delaying the loop like that.

if(usr.SkillUse == 0 && M.Attackable == 1 && usr.Saber == 1)
// ... code in between ...
else return


With this, if it encounters a mob that cannot be hit or the user cannot attack, it will end the skill entirely. What you want to do is skip over these mobs, which you can do with continue(). There may be certain instances where you do want the skill to stop altogether though, and for that you have break().

But as for the way you are using the SkillUse variable, I'd say you should be doing something different. Something like:

if(!SkillUse) return
SkillUse = 1

// skill is activated,
// AOE attack ensues.

// the skill is no longer active, so...
SkillUse = 0
Feb 23 2016, 1:48 pm
Thanks. The cool down was causing the issue. Im not sure how to implement it back in without breaking the code again however. Any suggestions? Perhaps spawn the skill so that sleep doesn't effect the loop. That probably sounds stupid...now that I typed it XD
Feb 23 2016, 2:18 pm
In response to Salaina555
Salaina555 wrote:
Thanks. The cool down was causing the issue. Im not sure how to implement it back in without breaking the code again however. Any suggestions? Perhaps spawn the skill so that sleep doesn't effect the loop. That probably sounds stupid...now that I typed it XD

You could use spawn(). Something as simple as:

spawn(usr.CoolDown * 5)
    SkillUse = 0


That would get the job done. Is it the best way to handle skill cooldowns though? Definitely not, but I suppose that's a topic for another day.

Another thing: I noticed you switch between usr and src, too. I don't think you should continue that practice. It's safe to say that the user of the verb will always be the mob it belongs to, so go ahead and use src.

usr, on the other hand, can be ambiguous and cause problems down the line if you get in the habit of using it the way you are.

Relevant resources on usr:
Feb 24 2016, 12:09 am
The cooldown wasn't the problem--the problem was that you put your loop in the wrong place, so that the cooldown was done inside the loop instead of after it.

Here's a quick rundown of the problems:

- You're checking if the player can even use this verb inside the loop, rather than before it. This doesn't break anything necessarily, but it causes the same condition to be checked over and over.

- You're checking if M is attackable in the loop, which is good, but if it's not attackable you're using return instead of continue. The return statement is bailing out of the loop completely.

- Setting SkillUse and calling flick() are done inside the loop, not before where they should be.

- The cooldown is being done inside the loop, not afterwards where it belongs.

- You're using ==1 and ==0 to check on true/false values. This is a bad, bad thing, because it makes your code brittle. You need to go through your code and get rid of everything like that. If you want to check if a value is true, you use if(value), and for false you check if(!false). The only time you need ==1 and ==0 is if you're literally looking for a 1 or a 0 specifically.

mob/verb/WM()
    set
        name = "Windmill"
        category = "skills"
    // Bail out if the player is using a skill or... whatever Saber is for
    if(SkillUse || !Saber) return
    SkillUse = 1
    flick('SkillWM.dmi',src)
    for(var/mob/M in oview(1,src))
        if(!M.Attackable) continue
        var/Dmg = max(0, Off - M.Def)
        Dmg *=5
        Critcheck()
        if(src.crit) Dmg *= src.CritRate
        M.life -= Dmg
        view(M) << "[src] hit [M] for [Dmg] Damage!"
        src.crit = 0
        M.Damagecheck()
        M.Deathcheck()   // I'm guessing you probably want to pass src as an argument here
    sleep(CoolDown * 5)
    SkillUse = 0

There's still a lot of question marks in that code. For instance, what is the Saber var for? Is it to check if the player is using a saber as a weapon? That's a bad use for a true/false value. Seeing that var makes me think maybe you have a hundred others just like it, one for each weapon type, and if so you need to change that to something a lot more robust.

In the call to Deathcheck(), I noticed you're not passing any arguments. If the killer matters at all to Deathcheck(), you need to pass it along as an argument. Otherwise, the only way you can get at that is by using usr, which is unreliable and has no business in a proc. Normally in any kind of combat-oriented game, the killer matters in Deathcheck() so that XP can be awarded.
Feb 24 2016, 4:31 pm
In response to Lummox JR
Lummox JR wrote:
If you want to check if a value is true, you use if(value), and for false you check if(!false).

I'm assuming you'd figure this out, but just in case, that should be 'if(!value)' instead of 'if(!false)'.