[JASS] What's wrong with my code?

[xploder7]

Posting this as a beginner to Jass in an attempt to understand what I am missing here:

globals
    unit caster
    location start_location
    integer count
    unit array srCaster
    location array srCastLoc
    real array srDmg
    integer srIndex
    integer array srLoop
    boolean srActiveCheck = false
endglobals
function slashcondition takes nothing returns boolean
    return GetSpellAbilityId() == 'AOws'
endfunction
function slashLoopInit takes nothing returns boolean
    return srActiveCheck == true
endfunction
function slashactions takes nothing returns nothing
    set caster = GetSpellAbilityUnit()
    set start_location = GetUnitLoc(caster)
    set count = 5
    set srActiveCheck = true
endfunction
function slashactions2 takes nothing returns nothing
    local unit temp
    local location temp_loc
    local effect e
    local group enemies = CreateGroup()
    call BJDebugMsg(I2S(count))
    call GroupEnumUnitsInRangeOfLoc(enemies, start_location, 500.0, null)
    if  count == 0 or enemies == null then
        call BJDebugMsg("no enemies")
        call SetUnitPositionLoc(caster, start_location)
        call DestroyGroup(enemies)
        call RemoveLocation(start_location)
    
        set start_location = null    
        set caster = null
        set enemies = null
        set temp = null
        set srActiveCheck = false
    
    else
        set temp = FirstOfGroup(enemies)
            if IsUnitEnemy(temp, GetOwningPlayer(caster)) then
                call BJDebugMsg("enemy found")
                set temp_loc = GetUnitLoc(temp)
                set e = AddSpecialEffectLoc("Abilities\\Spells\\Human\\Thunderclap\\ThunderClapCaster.mdl", temp_loc)
                call DestroyEffect(e)
                set e = null
                call SetUnitPositionLoc(caster, temp_loc)
                call UnitDamageTarget(caster, temp, 500.0, true, false, ATTACK_TYPE_CHAOS, DAMAGE_TYPE_NORMAL, null)
                set count = count - 1
                call RemoveLocation(temp_loc)
            endif 
            call BJDebugMsg("enemy removed")
            call GroupRemoveUnit(enemies, temp)
    endif
endfunction

function InitTrig_JASSTrigger takes nothing returns nothing
    local trigger s = CreateTrigger()
    
    call TriggerRegisterAnyUnitEventBJ(s, EVENT_PLAYER_UNIT_SPELL_EFFECT)
    call TriggerAddCondition(s, Condition(function slashcondition))
    call TriggerAddAction(s, function slashactions)
    set s = null
endfunction

function InitTrig_SetupInterval takes nothing returns nothing
    local trigger r = CreateTrigger()
    
    call TriggerRegisterTimerEventPeriodic(r, 0.20)
    call TriggerAddCondition(r, function slashLoopInit)
    call TriggerAddAction(r, function slashactions2)
    set r = null  
endfunction

The text "enemy removed" appears for 30 or so seconds before the sequence runs and my unit teleports and does damage. Only does damage to one unit though.

When I set the unit group "enemies" as a global variable it works once, then doesn't run again if I re-cast the spell in the same game.

Thanks!

 

[Elprede]

Since you using FirstOfGroup to get the unit from the group, you have to call it several times in a loop to get all units.

loop
    set temp = FirstOfGroup(enemies)
    exitwhen temp == null
    call GroupRemoveUnit(enemies, temp)
    
    // Do something to temp

endloop

 

[HerlySQR]

I'm seeing that you are treating the group variable enemies like it was a global variable because you are only destroying it in the last iteration of the timer while you are creating that group in every iteration, so you are leaking, my suggest is making enemies a global variable and create the group in the init function, and enum the units when the spell is casted, basically this:

globals
    group enemies
endglobals
function slashactions takes nothing returns nothing
    set caster = GetSpellAbilityUnit()
    set start_location = GetUnitLoc(caster)
    call GroupEnumUnitsInRangeOfLoc(enemies, start_location, 500.0, null)
    set count = 5
    set srActiveCheck = true
endfunction
function InitTrig_JASSTrigger takes nothing returns nothing
    local trigger s = CreateTrigger()
   
    call TriggerRegisterAnyUnitEventBJ(s, EVENT_PLAYER_UNIT_SPELL_EFFECT)
    call TriggerAddCondition(s, Condition(function slashcondition))
    call TriggerAddAction(s, function slashactions)
    set s = null

    set enemies = CreateGroup()
endfunction
function slashactions2 takes nothing returns nothing
    local unit temp
    local location temp_loc
    local effect e
    //local group enemies = CreateGroup()  Just erase these 2 lines here
    //call BJDebugMsg(I2S(count))
    call GroupEnumUnitsInRangeOfLoc(enemies, start_location, 500.0, null)

And, what is that condition of enemies == null? because enemies is never null because is a created group, if you wanna check if is empty, do:

if IsUnitGroupEmptyBJ(enemies) then

 

[Tristronic]

As for your question about "what you are missing", here are some things that stand out to me:

Spoiler

I'll try to keep these in some order of importance.

1. It looks like you have this system made with two triggers and then some script in the map header; why not just put it all neatly in one place? I went ahead and made this system into a library with your trigger initializers combined into one system initializer called "srSlashInit". You could now copy and paste this into any map and as long as there's a unit with the Tauren Chieftain's Warstomp ability, they can use this ability.
2. The periodic trigger as you've made it has some significant cons, even if it was totally debugged - it is very rigged, every multiple of 0.20 seconds game-time a slash can be done, no if I slash at 300.01 seconds, and you slash at 300.19 seconds, (and if this was MUI so we could) we would both slash at 300.20 seconds, and the repeated slashes would be in an unforeseeable order.
3. I'm guessing a lot of things fell apart from the testing and re-testing, if you're anything like me, it gets ugly then gets cleaned up when troubleshooting (probably not a great habit) so I don't want to be too critical of things like it being only possible for one unit at a time, or issues with targeting and grouping. (I think I see what you were going for with the FirstOfGroup and you were right not to loop since you wanted the periodic trigger to grab one each time, not loop the full group each time, nice moves)
4. Speaking of my collision system, I borrowed the 'micro-indexer' concept I used in it.. I like this concept of a 'micro-indexer' because it keeps the (temporarily) indexed units to lower indexes which makes them more practical to loop through. Hopefully you can tell what the 'micro-loop' is by looking at the system, it's the unit-tracing system made by the way I use the global integers 'srIndex' and 'srIntegerA' and the local 'subIndex' in the function 'SlashLoopActions'. I'll describe it better if someone asks about it.
5. For the trigger which is given the action slashaction (better named SlashAction or SlashActions) and the condition slashcondition (likewise on the name)
6. Notice the habits Blizzard uses when naming things: functions use "PascalCasing", variables use "camelCasing", this is a good habit and makes it really easy to know what you are reading. Otherwise, t's like when my boy Blue says, "that letter is to red" and then I spend far too long wondering if the letter is a letter for a person named Red, or if the letter is more red than Blue should like. You also save space by avoiding underscores this way thereIsSoMuchMoreRoomForMoreLettersNow. There are some good tips on how to do this online and here on the hive.
7. Speaking of habits, one bad habit you didn't do is over-nesting. I'm only bringing it up because I'm guessing since you're new to JASS you might not be familiar with the idea.. to put it briefly, there are those who think that if your nested loops (IF/THENs, Loops) go deeper than three tiers, you had better make some part of that nest into a function and call it, otherwise things can get messy. There's always a balance and you didn't come close to over-doing it, but it's something that made a big difference to me when I started keeping it in mind.
8. There are several ways you could have sliced it, so many of the choices that you made that I didn't choose to repeat, aren't necessarily because they weren't a good idea, it's just that since I was re-designing, it made more sense to go with what made more sense to me.
9. I'm hoping my version of this serves as a better commentary than my actual commentary - best ways to get good:
   1. 'Watch' posts when people are asking for help and the system as at or around your skill level.
   2. Pay attention to some of these more experienced people here and their tips.
   3. Troubleshoot with debug messages (like you did, good work!)
   4. Ask for help when you don't understand why your troubleshooting/ efforts fail
   5. Don't just hand your idea off and expect others to want to help if you aren't willing to put similar/ capital effort in.
      • Seems like you did your part on the effort here, I made a few assumptions, but it was clear to me what you wanted, for the most part, so I was able to make something and didn't feel like I was dragging a horse to water.

Nice Ability idea, maybe adding some animation to the caster on each strike would be a good next step for a polishing step. I tried to polish it a bit by having the caster at least face the right way on each strike, but I didn't want to get carried away. Maybe have them use the spell animation each strike.

---

Here's how I would do it:

Spoiler

library srSlash initializer srSlashInit

    globals
        private group array srEnemies
        private integer srIndex = 0
        private integer srIntegerA = 0
        private integer array srInterval
        private integer array srSlashCount
        private integer array srStepCount
        private location array srStartLocation
        private unit array srCaster
        private real array srAngle
        private real array srDamage
        private real array srRange
        private trigger srSlashLoop = CreateTrigger()

        constant real SR_DAMAGE_FACTOR = 1.00
        constant real SR_RANGE_FACTOR = 1.00
        constant real SR_LOOP_PERIOD = 0.02
    endglobals

    private function AreUnitsEnemies takes unit thisUnit, unit thatUnit returns boolean
        local player thatPlayer = GetOwningPlayer( thatUnit )
        return IsUnitEnemy( thisUnit, thatPlayer )
    endfunction

    // You might want to expand on this filter.  Can this ability target structures, invulnerable units, flying, et c?
    // I tried to structure this in such a way that you could see a pattern.  If it passes all the checks, it can be slashed.
    // Try to consider two things when you choose the order of the conditions:
    //  1. Conditions that are likely to return false should go first to end the check early when possible.
    //  2. Performance-dragging conditions (conditions containing larger functions) should go later
    //     to avoid having to run then if the more likely conditions return false.
    private function SlashEnemyFilter takes nothing returns boolean
        local unit u = GetFilterUnit()
        if GetUnitState( u, UNIT_STATE_LIFE ) <= 0 then
            return false
        endif
        if IsUnitInGroup( u, srEnemies[srIntegerA] ) then
            return false
        endif
        if not AreUnitsEnemies( u, srCaster[srIntegerA] ) then
            return false
        endif
        return true
    endfunction

    private function SlashLoopActions takes nothing returns nothing
        local effect e
        local group g = CreateGroup()
        local integer subIndex = 0
        local location targetLocation
        local location casterLocation
        local unit target
        set srIntegerA = 0

        loop
            exitwhen srIntegerA == srIndex
            if srStepCount[srIntegerA] > srInterval[srIntegerA] then
                call GroupEnumUnitsInRangeOfLoc( g, srStartLocation[srIntegerA], srRange[srIntegerA], function SlashEnemyFilter )
                set target = FirstOfGroup( g )
                call DestroyGroup( g )
                if GroupAddUnit( srEnemies[srIntegerA], target ) then
                    set targetLocation = GetUnitLoc( target )
                    set e = AddSpecialEffectLoc( "Abilities\\Spells\\Human\\Thunderclap\\ThunderClapCaster.mdl", targetLocation )
                    call DestroyEffect( e )
                    set e = null
                    call SetUnitPositionLoc( srCaster[srIntegerA], targetLocation )
                    set casterLocation = GetUnitLoc( srCaster[srIntegerA] )
                    call UnitDamageTarget( srCaster[srIntegerA], target, srDamage[srIntegerA], true, false, ATTACK_TYPE_CHAOS, DAMAGE_TYPE_NORMAL, null )
                    call BlzSetUnitFacingEx( srCaster[srIntegerA], AngleBetweenPoints( casterLocation, targetLocation ) )
                    call RemoveLocation( targetLocation )
                    call RemoveLocation( casterLocation )
                else
                    call SetUnitPositionLoc( srCaster[srIntegerA], srStartLocation[srIntegerA] )
                    call BlzSetUnitFacingEx( srCaster[srIntegerA], srAngle[srIntegerA] )
                    set srSlashCount[srIntegerA] = 0
                endif
                set srSlashCount[srIntegerA] = srSlashCount[srIntegerA] - 1
                set srDamage[srIntegerA] = srDamage[srIntegerA] * SR_DAMAGE_FACTOR
                set srRange[srIntegerA] = srRange[srIntegerA] * SR_RANGE_FACTOR
                set srStepCount[srIntegerA] = 0
            endif
            if srSlashCount[srIntegerA] > 0 then
                set srStartLocation[srIntegerA - subIndex] = srStartLocation[srIntegerA]
                set srSlashCount[srIntegerA - subIndex] = srSlashCount[srIntegerA]
                set srStepCount[srIntegerA - subIndex] = srStepCount[srIntegerA] + 1
                set srInterval[srIntegerA - subIndex] = srInterval[srIntegerA]
                set srEnemies[srIntegerA - subIndex] = srEnemies[srIntegerA]
                set srCaster[srIntegerA - subIndex] = srCaster[srIntegerA]
                set srAngle[srIntegerA - subIndex] = srAngle[srIntegerA]
                set srRange[srIntegerA - subIndex] = srRange[srIntegerA]
                set srDamage[srIntegerA - subIndex] = srDamage[srIntegerA]
            else
                call DestroyGroup( srEnemies[srIntegerA] )
                call SetUnitPositionLoc( srCaster[srIntegerA], srStartLocation[srIntegerA] )
                call RemoveLocation( srStartLocation[srIntegerA] )
                set srSlashCount[srIntegerA] = 0
                set srStepCount[srIntegerA] = 0
                set srInterval[srIntegerA] = 0
                set srCaster[srIntegerA] = null
                set srAngle[srIntegerA] = 0.00
                set srRange[srIntegerA] = 0.00
                set srDamage[srIntegerA] = 0.00
                set subIndex = subIndex + 1
            endif
            set srIntegerA = srIntegerA + 1
        endloop
        set srIndex = srIndex - subIndex
        if srIndex < 1 then
            call DisableTrigger( srSlashLoop )
        endif
    endfunction

    function BeginSlash takes unit caster, integer slashCount, real interval, real range, real damage returns nothing
        if interval < SR_LOOP_PERIOD then
            set interval = SR_LOOP_PERIOD
        endif
        set srCaster[srIndex] = caster
        set srStartLocation[srIndex] = GetUnitLoc( caster )
        set srSlashCount[srIndex] = slashCount
        set srStepCount[srIndex] = R2I( interval / SR_LOOP_PERIOD ) // Make this the same as srInterval[srIndex] for an immediate attack, lower is slower.
        set srInterval[srIndex] = R2I( interval / SR_LOOP_PERIOD ) // A value of 1 is 0.02 seconds, 10 is 0.20, 100 is 2.00, et c. Must be greater than 0!
        set srEnemies[srIndex] = CreateGroup()
        set srAngle[srIndex] = GetUnitFacing( caster )
        set srRange[srIndex] = range
        set srDamage[srIndex] = damage
        set srIndex = srIndex + 1
        call EnableTrigger( srSlashLoop )
    endfunction

    private function SlashConditions takes nothing returns boolean
        return GetSpellAbilityId() == 'AOws'
    endfunction

    private function SlashActions takes nothing returns nothing
        call BeginSlash( GetSpellAbilityUnit(), 5, 0.20, 500.00, 500.00 )
    endfunction

    private function srSlashInit takes nothing returns nothing
        local trigger s = CreateTrigger()

        call TriggerRegisterAnyUnitEventBJ(s, EVENT_PLAYER_UNIT_SPELL_EFFECT)
        call TriggerAddCondition(s, Condition(function SlashConditions))
        call TriggerAddAction(s, function SlashActions)

        call DisableTrigger( srSlashLoop )
        call TriggerRegisterTimerEventPeriodic( srSlashLoop, SR_LOOP_PERIOD )
        call TriggerAddAction( srSlashLoop, function SlashLoopActions )
    endfunction

endlibrary

If you have any questions, I'd rather let you ask than have me explain unnecessarily. I meant to mark it up with some tips on how to name your variables and functions (basically it's "camelCasing" for variables and "PascalCasing" for functions).
I would also recommend learning more on the subject because it really can make a huge difference for yourself and for working with others.
I wanted to make more changes, but I didn't want to confuse the system by changing the naming structure altogether.

I didn't test it with multiple units using the ability, but it should work for up to 8192 units at the same time lol.
Not sure how well that would perform, but I think technically it could be possible if it happened somehow.
Anyway, I did test it with an empowered Tauren Chieftain against many murloc foes.

The idea as far as I could tell was to have the unit strike 5 units 0.20 seconds apart in a 500.00 unit radius of the casting location, and then to return to that location.
I hope I understood what you wanted well, and I added a few things I thought you might like:

1. It will not allow you to restrike the same target in a single chain.
2. Every time the caster strikes, the radius is checked again based on the current units in the area.
3. The caster will face the target of each strike to show who the target is a little bit better,
4. If there is no target, the ability will end as if the strikes had been completed (returning to the start location).
5. When the ability is finished, the target will face the original direction.
6. Added support to make it easier to set unique ranges and damage amounts per individual slasher.
   • call BeginSlash( caster, slashCount, interval, range, damage ) <-{ unit, integer, real, real, real
7. Added support to make more freedom in your strikes: There is not a damage factor constant, a range factor constant, and a slash-loop interval constant; I'll explain the intention of these below.

One shortfall that I will address soon is that the caster shouldn't be able to attack during this ability IMO and as it is, the caster can attack and can be attacked. I haven't decided how I want to handle that yet, I honestly didn't think of it until just now as I'm editing this post for like the third time.
Should be a quick fix anyway so I'll deal with it tomorrow.

You should find it easier if you want to tweak something like the interval, range, or the number of strikes.
I think you'll find it easier to make this things more dynamic now so that, for example, as an ability level increases the number of strikes or the range could increase or the interval could decrease.

Also, if I use this I'll give ideas credit, but I'm claiming the right to use it if I want to now lol.

Edit: I added a GUI trigger to show how you could use this with other abilities. I didn't do anything fancy though...

• TriggerSlash
  • Events
    • Unit - A unit Starts the effect of an ability
  • Conditions
    • (Ability being cast) Equal to Thunder Clap
  • Actions
    • Custom script: call BeginSlash( GetTriggerUnit(), 10, 0.10, 250.00, 250.00 )

 

[xploder7]

As for your question about "what you are missing", here are some things that stand out to me:

Spoiler

I'll try to keep these in some order of importance.

1. It looks like you have this system made with two triggers and then some script in the map header; why not just put it all neatly in one place? I went ahead and made this system into a library with your trigger initializers combined into one system initializer called "srSlashInit". You could now copy and paste this into any map and as long as there's a unit with the Tauren Chieftain's Warstomp ability, they can use this ability.
2. The periodic trigger as you've made it has some significant cons, even if it was totally debugged - it is very rigged, every multiple of 0.20 seconds game-time a slash can be done, no if I slash at 300.01 seconds, and you slash at 300.19 seconds, (and if this was MUI so we could) we would both slash at 300.20 seconds, and the repeated slashes would be in an unforeseeable order.
3. I'm guessing a lot of things fell apart from the testing and re-testing, if you're anything like me, it gets ugly then gets cleaned up when troubleshooting (probably not a great habit) so I don't want to be too critical of things like it being only possible for one unit at a time, or issues with targeting and grouping. (I think I see what you were going for with the FirstOfGroup and you were right not to loop since you wanted the periodic trigger to grab one each time, not loop the full group each time, nice moves)
4. Speaking of my collision system, I borrowed the 'micro-indexer' concept I used in it.. I like this concept of a 'micro-indexer' because it keeps the (temporarily) indexed units to lower indexes which makes them more practical to loop through. Hopefully you can tell what the 'micro-loop' is by looking at the system, it's the unit-tracing system made by the way I use the global integers 'srIndex' and 'srIntegerA' and the local 'subIndex' in the function 'SlashLoopActions'. I'll describe it better if someone asks about it.
5. For the trigger which is given the action slashaction (better named SlashAction or SlashActions) and the condition slashcondition (likewise on the name)
6. Notice the habits Blizzard uses when naming things: functions use "PascalCasing", variables use "camelCasing", this is a good habit and makes it really easy to know what you are reading. Otherwise, t's like when my boy Blue says, "that letter is to red" and then I spend far too long wondering if the letter is a letter for a person named Red, or if the letter is more red than Blue should like. You also save space by avoiding underscores this way thereIsSoMuchMoreRoomForMoreLettersNow. There are some good tips on how to do this online and here on the hive.
7. Speaking of habits, one bad habit you didn't do is over-nesting. I'm only bringing it up because I'm guessing since you're new to JASS you might not be familiar with the idea.. to put it briefly, there are those who think that if your nested loops (IF/THENs, Loops) go deeper than three tiers, you had better make some part of that nest into a function and call it, otherwise things can get messy. There's always a balance and you didn't come close to over-doing it, but it's something that made a big difference to me when I started keeping it in mind.
8. There are several ways you could have sliced it, so many of the choices that you made that I didn't choose to repeat, aren't necessarily because they weren't a good idea, it's just that since I was re-designing, it made more sense to go with what made more sense to me.
9. I'm hoping my version of this serves as a better commentary than my actual commentary - best ways to get good:
   1. 'Watch' posts when people are asking for help and the system as at or around your skill level.
   2. Pay attention to some of these more experienced people here and their tips.
   3. Troubleshoot with debug messages (like you did, good work!)
   4. Ask for help when you don't understand why your troubleshooting/ efforts fail
   5. Don't just hand your idea off and expect others to want to help if you aren't willing to put similar/ capital effort in.
      • Seems like you did your part on the effort here, I made a few assumptions, but it was clear to me what you wanted, for the most part, so I was able to make something and didn't feel like I was dragging a horse to water.

Nice Ability idea, maybe adding some animation to the caster on each strike would be a good next step for a polishing step. I tried to polish it a bit by having the caster at least face the right way on each strike, but I didn't want to get carried away. Maybe have them use the spell animation each strike.

---

Here's how I would do it:

Spoiler

library srSlash initializer srSlashInit

    globals
        private group array srEnemies
        private integer srIndex = 0
        private integer srIntegerA = 0
        private integer array srInterval
        private integer array srSlashCount
        private integer array srStepCount
        private location array srStartLocation
        private unit array srCaster
        private real array srAngle
        private real array srDamage
        private real array srRange
        private trigger srSlashLoop = CreateTrigger()

        constant real SR_DAMAGE_FACTOR = 1.00
        constant real SR_RANGE_FACTOR = 1.00
        constant real SR_LOOP_PERIOD = 0.02
    endglobals

    private function AreUnitsEnemies takes unit thisUnit, unit thatUnit returns boolean
        local player thatPlayer = GetOwningPlayer( thatUnit )
        return IsUnitEnemy( thisUnit, thatPlayer )
    endfunction

    // You might want to expand on this filter.  Can this ability target structures, invulnerable units, flying, et c?
    // I tried to structure this in such a way that you could see a pattern.  If it passes all the checks, it can be slashed.
    // Try to consider two things when you choose the order of the conditions:
    //  1. Conditions that are likely to return false should go first to end the check early when possible.
    //  2. Performance-dragging conditions (conditions containing larger functions) should go later
    //     to avoid having to run then if the more likely conditions return false.
    private function SlashEnemyFilter takes nothing returns boolean
        local unit u = GetFilterUnit()
        if GetUnitState( u, UNIT_STATE_LIFE ) <= 0 then
            return false
        endif
        if IsUnitInGroup( u, srEnemies[srIntegerA] ) then
            return false
        endif
        if not AreUnitsEnemies( u, srCaster[srIntegerA] ) then
            return false
        endif
        return true
    endfunction

    private function SlashLoopActions takes nothing returns nothing
        local effect e
        local group g = CreateGroup()
        local integer subIndex = 0
        local location targetLocation
        local location casterLocation
        local unit target
        set srIntegerA = 0

        loop
            exitwhen srIntegerA == srIndex
            if srStepCount[srIntegerA] > srInterval[srIntegerA] then
                call GroupEnumUnitsInRangeOfLoc( g, srStartLocation[srIntegerA], srRange[srIntegerA], function SlashEnemyFilter )
                set target = FirstOfGroup( g )
                call DestroyGroup( g )
                if GroupAddUnit( srEnemies[srIntegerA], target ) then
                    set targetLocation = GetUnitLoc( target )
                    set e = AddSpecialEffectLoc( "Abilities\\Spells\\Human\\Thunderclap\\ThunderClapCaster.mdl", targetLocation )
                    call DestroyEffect( e )
                    set e = null
                    call SetUnitPositionLoc( srCaster[srIntegerA], targetLocation )
                    set casterLocation = GetUnitLoc( srCaster[srIntegerA] )
                    call UnitDamageTarget( srCaster[srIntegerA], target, srDamage[srIntegerA], true, false, ATTACK_TYPE_CHAOS, DAMAGE_TYPE_NORMAL, null )
                    call BlzSetUnitFacingEx( srCaster[srIntegerA], AngleBetweenPoints( casterLocation, targetLocation ) )
                    call RemoveLocation( targetLocation )
                    call RemoveLocation( casterLocation )
                else
                    call SetUnitPositionLoc( srCaster[srIntegerA], srStartLocation[srIntegerA] )
                    call BlzSetUnitFacingEx( srCaster[srIntegerA], srAngle[srIntegerA] )
                    set srSlashCount[srIntegerA] = 0
                endif
                set srSlashCount[srIntegerA] = srSlashCount[srIntegerA] - 1
                set srDamage[srIntegerA] = srDamage[srIntegerA] * SR_DAMAGE_FACTOR
                set srRange[srIntegerA] = srRange[srIntegerA] * SR_RANGE_FACTOR
                set srStepCount[srIntegerA] = 0
            endif
            if srSlashCount[srIntegerA] > 0 then
                set srStartLocation[srIntegerA - subIndex] = srStartLocation[srIntegerA]
                set srSlashCount[srIntegerA - subIndex] = srSlashCount[srIntegerA]
                set srStepCount[srIntegerA - subIndex] = srStepCount[srIntegerA] + 1
                set srInterval[srIntegerA - subIndex] = srInterval[srIntegerA]
                set srEnemies[srIntegerA - subIndex] = srEnemies[srIntegerA]
                set srCaster[srIntegerA - subIndex] = srCaster[srIntegerA]
                set srAngle[srIntegerA - subIndex] = srAngle[srIntegerA]
                set srRange[srIntegerA - subIndex] = srRange[srIntegerA]
                set srDamage[srIntegerA - subIndex] = srDamage[srIntegerA]
            else
                call DestroyGroup( srEnemies[srIntegerA] )
                call SetUnitPositionLoc( srCaster[srIntegerA], srStartLocation[srIntegerA] )
                call RemoveLocation( srStartLocation[srIntegerA] )
                set srSlashCount[srIntegerA] = 0
                set srStepCount[srIntegerA] = 0
                set srInterval[srIntegerA] = 0
                set srCaster[srIntegerA] = null
                set srAngle[srIntegerA] = 0.00
                set srRange[srIntegerA] = 0.00
                set srDamage[srIntegerA] = 0.00
                set subIndex = subIndex + 1
            endif
            set srIntegerA = srIntegerA + 1
        endloop
        set srIndex = srIndex - subIndex
        if srIndex < 1 then
            call DisableTrigger( srSlashLoop )
        endif
    endfunction

    function BeginSlash takes unit caster, integer slashCount, real interval, real range, real damage returns nothing
        if interval < SR_LOOP_PERIOD then
            set interval = SR_LOOP_PERIOD
        endif
        set srCaster[srIndex] = caster
        set srStartLocation[srIndex] = GetUnitLoc( caster )
        set srSlashCount[srIndex] = slashCount
        set srStepCount[srIndex] = R2I( interval / SR_LOOP_PERIOD ) // Make this the same as srInterval[srIndex] for an immediate attack, lower is slower.
        set srInterval[srIndex] = R2I( interval / SR_LOOP_PERIOD ) // A value of 1 is 0.02 seconds, 10 is 0.20, 100 is 2.00, et c. Must be greater than 0!
        set srEnemies[srIndex] = CreateGroup()
        set srAngle[srIndex] = GetUnitFacing( caster )
        set srRange[srIndex] = range
        set srDamage[srIndex] = damage
        set srIndex = srIndex + 1
        call EnableTrigger( srSlashLoop )
    endfunction

    private function SlashConditions takes nothing returns boolean
        return GetSpellAbilityId() == 'AOws'
    endfunction

    private function SlashActions takes nothing returns nothing
        call BeginSlash( GetSpellAbilityUnit(), 5, 0.20, 500.00, 500.00 )
    endfunction

    private function srSlashInit takes nothing returns nothing
        local trigger s = CreateTrigger()

        call TriggerRegisterAnyUnitEventBJ(s, EVENT_PLAYER_UNIT_SPELL_EFFECT)
        call TriggerAddCondition(s, Condition(function SlashConditions))
        call TriggerAddAction(s, function SlashActions)

        call DisableTrigger( srSlashLoop )
        call TriggerRegisterTimerEventPeriodic( srSlashLoop, SR_LOOP_PERIOD )
        call TriggerAddAction( srSlashLoop, function SlashLoopActions )
    endfunction

endlibrary

If you have any questions, I'd rather let you ask than have me explain unnecessarily. I meant to mark it up with some tips on how to name your variables and functions (basically it's "camelCasing" for variables and "PascalCasing" for functions).
I would also recommend learning more on the subject because it really can make a huge difference for yourself and for working with others.
I wanted to make more changes, but I didn't want to confuse the system by changing the naming structure altogether.

I didn't test it with multiple units using the ability, but it should work for up to 8192 units at the same time lol.
Not sure how well that would perform, but I think technically it could be possible if it happened somehow.
Anyway, I did test it with an empowered Tauren Chieftain against many murloc foes.

The idea as far as I could tell was to have the unit strike 5 units 0.20 seconds apart in a 500.00 unit radius of the casting location, and then to return to that location.
I hope I understood what you wanted well, and I added a few things I thought you might like:

1. It will not allow you to restrike the same target in a single chain.
2. Every time the caster strikes, the radius is checked again based on the current units in the area.
3. The caster will face the target of each strike to show who the target is a little bit better,
4. If there is no target, the ability will end as if the strikes had been completed (returning to the start location).
5. When the ability is finished, the target will face the original direction.
6. Added support to make it easier to set unique ranges and damage amounts per individual slasher.
   • call BeginSlash( caster, slashCount, interval, range, damage ) <-{ unit, integer, real, real, real
7. Added support to make more freedom in your strikes: There is not a damage factor constant, a range factor constant, and a slash-loop interval constant; I'll explain the intention of these below.

One shortfall that I will address soon is that the caster shouldn't be able to attack during this ability IMO and as it is, the caster can attack and can be attacked. I haven't decided how I want to handle that yet, I honestly didn't think of it until just now as I'm editing this post for like the third time.
Should be a quick fix anyway so I'll deal with it tomorrow.

You should find it easier if you want to tweak something like the interval, range, or the number of strikes.
I think you'll find it easier to make this things more dynamic now so that, for example, as an ability level increases the number of strikes or the range could increase or the interval could decrease.

Also, if I use this I'll give ideas credit, but I'm claiming the right to use it if I want to now lol.

Edit: I added a GUI trigger to show how you could use this with other abilities. I didn't do anything fancy though...

• TriggerSlash
  • Events
    • Unit - A unit Starts the effect of an ability
  • Conditions
    • (Ability being cast) Equal to Thunder Clap
  • Actions
    • Custom script: call BeginSlash( GetTriggerUnit(), 10, 0.10, 250.00, 250.00 )

Okay, this is great. Thank you for expanding on this so much. FYI I got this spell from the Beginner JASS Tutorial Series. My questions on this are below:

1. Is there a reason your "BeginSlash" is below your "SlashActions" in the syntax?
2. What is the purpose of comparing the interval (0.02) to the stepcount (10) in the loop function? You lost me there - i know it's something to do with the interval speed of execution. The rest of this makes sense to me for the most part.

 

[Uncle]

@xploder7
It helps to explain what you're trying to do instead of just saying it doesn't work. Anyway, It looks like you want Omnislash from DotA 1 (it's a bit different in DotA 2) and Tristronic provided what I believe is almost exactly that.

If you want it to be more like Omnislash then you may want to do the following:
1) Make the caster Invulnerable during the effect of the ability.
2) Disable the caster during the effect, I recommend using BlzPauseUnitEx().
3) Play the caster's attack animation each time it strikes.
4) I think the Unit Group for valid targets is supposed to search around the position of the caster instead of only checking within the initial area of effect.

Of course, it's up to you to decide how you want it to work.

Also, the Array size limit is 32,768 on patch 1.31+.

 

[Tristronic]

1. Is there a reason your "BeginSlash" is below your "SlashActions" in the syntax?
2. What is the purpose of comparing the interval (0.02) to the stepcount (10) in the loop function? You lost me there - i know it's something to do with the interval speed of execution. The rest of this makes sense to me for the most part.

I have a version that is much further along, honestly I liked this idea so I started fine tuning it. If I can get back to you later today, I had a work deadline so I dropped it for a few days and still need a few hours to polish it.

To answer your questions:

1. Technically there is no reason. Basically, I just wrote it that way because I like to (when it makes sense to do so) put my functions immediately above the functions that will call them based on the priority the function calls them in, so I considered it like this:
   • Init Function (mother of all) -> Bottom (Imagine this list upside down)
     • Slash trigger (called by Init first)
       • Slash Event Function -> native (so N/A in this case) I would place these immediately above the highest function that uses them, or in a library that is required by this library or the very top of this library if they were seemingly going to be used many times.
       • Slash Condition Function -> Immediately above Event function (unless the events are elsewhere located, then it's immediately above calling function)
       • Slash Action Function -> I actually broke my consistency here, according to me I should have put them above the conditions, but only because that's my preference
         • Begin Slash function is called by actions so I put it above actions
     • Slash Loop trigger (called by Init second)
       • Event Functions (again, N/A sam reason)
       • Condition Functions (none in the case of this trigger
       • Action Functions (gets placed right above Basic Slash by default)
     • If there were a third trigger made after Slash Loop trigger
       • Event function
       • Condition function
       • Action function
2. I'm just going to over-explain to avoid a lot of back and forth:
   • Step Count is what 'step' a unit is at
   • Interval is the 'critical step' where we do the action and then reset the Step Count to zero
   • This is done so that we can change the interval (third argument, a real, of BeginSlash) to an amount of time (in my example I used 0.20)
   • The real argument of BeginSlash becomes an integer srInterval determined by dividing the Real 'interval' by the constant SR_LOOP_PERIOD
   • SR_LOOP_PERIOD is the frequency that SlashLoop fires at, I defaulted it at 0.02.
   • So doing the math, 0.20 / 0.02 our integer srInterval is set to 10
   • Therefore Step Count starts at 10, doing an immediate slash (if I was smart enough to make that '>' into a '>=' it would have fired immediately, I accidentally made it fire at T-0.02 and each interval is effectively 0.02 seconds too long.. oops) then it gets set to zero and gets +1 every 0.02 seconds until we are back at the interval value

Essentially, the comparison is to determine something that I think is obvious, I'm guessing you only didn't realize that this allows you to conveniently change the interval of the slashes.

Here's a list of some features I've added:

Slash Interval factor (can get faster or slower after consecutive hits)
Slash types, listed below in my replies to Uncle
Some fixes
Honestly a lot more but my grandmother just flew into town and I have to go say hi.... I typed this part last, just saying as an excuse of why there is so much text after saying I have to go talk to my grandma...

@xploder7
It helps to explain what you're trying to do instead of just saying it doesn't work. Anyway, It looks like you want Omnislash from DotA 1 (it's a bit different in DotA 2) and Tristronic provided what I believe is almost exactly that.

If you want it to be more like Omnislash then you may want to do the following:
1) Make the caster Invulnerable during the effect of the ability.
2) Disable the caster during the effect, I recommend using BlzPauseUnitEx().
3) Play the caster's attack animation each time it strikes.
4) I think the Unit Group for valid targets is supposed to search around the position of the caster instead of only checking within the initial area of effect.

As for number 4, this was intended as I believe the posted example did this and I attempted to emulate it, I was thinking this was a fail-safe against escaping playable bounds, coupled with the return to the starting point that was in the original system.
In the version I mentioned above in my response to xploder7, I have added 2 more 'types' of slashes so now there are three types:

1. The current slash, bound to an AoE around caster, could become a good slash the mob ability if set to a fast interval and high number of attacks
2. Chain slash (I actually started calling slashes strikes in my remake) - picks a new range after each slash
3. Multi-slash - hits the same target a number of times or until it is dead

Also, the Array size limit is 32,768 on patch 1.31+.

I think I even knew that at one point. Now I have to wonder what a reasonable limit might be for the loop... I'm just gonna max it out I think, but that sounds way more breakable as far as my 'mini-index' concept goes. I doubt you would be able to get enough units (thousands of units) slashing at the same time though, so I think I'll just change the cap to 32,768. Thanks for pointing that out for me!