[JASS] Life or Death !! Help fast rep+ !!! Chain spell

GUYS this is a spell for a competition. I know the is not greatly optimized, but according to the time, was the best I could do.
Anyway, if you post it in JassCraft, in line 69, (the "call ForGroup") has an error.
Please tell me why it is wrong !!! I don't understand why !
Also, if you see any instruction with "//" uts because I am not sure about that same instruction. It means that I don't know if it will compromise the spell, so comments on that would also be appreciated.

Guys, I don't want any optimizations, please the spell is already difficult enough for me to understand (keep in mind that I am its creator). Please just tell what is wrong and What can I do to fix it !
Fast plz !

Chain:

function Trig_Chain_Conditions takes nothing returns boolean
    return GetSpellAbilityId() == 'A000'
endfunction
//=========================================================
//ver local vars MB_targ, MB_SMax, MB_alltargs
function forUnit takes unit targ, real max, group g returns nothing
    local unit u = GetEnumUnit()
    local player p = GetOwningPlayer(GetTriggerUnit())
    local unit MB_targ = targ
    local real MB_SMax = max
local group MB_alltarg = g
    if IsUnitType(u, UNIT_TYPE_STRUCTURE) == false and IsUnitType(u, UNIT_TYPE_MECHANICAL) and IsUnitType(u, UNIT_TYPE_MAGIC_IMMUNE) and IsUnitEnemy(u, p) and (GetUnitState(u, UNIT_STATE_LIFE) > 0) and IsUnitInGroup(u, MB_alltarg) == false and (GetUnitState(u, UNIT_STATE_MAX_MANA) > 0) and (GetUnitState(u, UNIT_STATE_MANA) >= MB_SMax) then
        set MB_targ = u
        set MB_SMax = GetUnitState(u, UNIT_STATE_MANA)
    endif
    //call DestroyGroup(MB_alltarg)
    //set MB_alltarg = null
    //set u = null
    //set p = null
    //set MB_targ = null
endfunction
//=========================================================
function Trig_Chain_Actions takes nothing returns nothing
    //variables part
    local unit MB_targ = GetSpellTargetUnit()//current target
    local unit MB_prev = GetTriggerUnit() //previous target, the source where it all came
    local integer MB_level = GetUnitAbilityLevel(MB_prev, 'A000')
    local integer MB_targetsnum = (4 + (2 * (MB_level - 1))) //maximum number of affected unit
    local integer MB_cur //current number of targets
    local group MB_alltarg //Group that will save hit units in the past
    local unit dummy //casts the spells
    local group MB_TempGroup //will pick a random unit, that will be the next target
    local real MB_SMax//chooses the unit with more mana
    local location l
    //action part
    loop
        exitwhen (MB_cur > MB_targetsnum)
        //start
        call GroupAddUnit(MB_alltarg, MB_targ)
        set dummy = CreateUnit(GetOwningPlayer(GetTriggerUnit()), 'h000', GetUnitX(MB_prev), GetUnitY(MB_prev), 0.0)
        call UnitAddAbility(dummy, 'A002')
        call IssueTargetOrder(dummy, "shadowstrike", MB_targ)
        call UnitApplyTimedLife(dummy, 'BTLF', 1.0)
        //set dummy = null
        //Wait Unit Unit was reached by effect
        loop
            exitwhen ((GetUnitAbilityLevel(MB_targ, 'BEsh') > 0) or (GetUnitState(MB_targ, UNIT_STATE_LIFE) <= 0))
            call TriggerSleepAction(0.30)
        endloop
        //If unit is alive, take mana
        if (GetUnitState(MB_targ, UNIT_STATE_LIFE) <= 0) == false then
            call CreateUnit(GetOwningPlayer(GetTriggerUnit()), 'h000', GetUnitX(MB_targ), GetUnitY(MB_targ), 0.0)
            call UnitApplyTimedLife(dummy, 'BTLF', 1.0)
            call UnitAddAbility(dummy, 'A001')
            call SetUnitAbilityLevel(dummy, 'A001', 8 * (MB_level - 1) + MB_cur)
            call IssueTargetOrder(dummy, "manaburn", MB_targ)
        else
            set MB_cur = MB_targetsnum + 1
        endif
        //Choose New target
        if (MB_cur < MB_targetsnum) then
            set MB_prev = MB_targ
            set MB_SMax = -1
            set l = GetUnitLoc(MB_prev)
            call ForGroup(GetUnitsInRangeOfLocAll(500, l), function forUnit(MB_targ, MB_SMax, MB_alltarg))//error . WHYY!!!!!
            call RemoveLocation(l)
            set l = null
            call GroupClear(MB_alltarg)
endif
        set MB_cur = MB_cur + 1
    endloop
    call DestroyGroup(MB_alltarg)
    call DestroyGroup(MB_TempGroup)
    set MB_alltarg = null
    set MB_TempGroup = null
    set MB_targ = null
    set MB_prev = null
    set dummy = null
endfunction
//=========================================================
function InitTrig_Chain takes nothing returns nothing
    set gg_trg_Chain = CreateTrigger()
    call TriggerRegisterAnyUnitEventBJ( gg_trg_Chain, EVENT_PLAYER_UNIT_SPELL_EFFECT )
    call TriggerAddCondition(gg_trg_Chain, Condition(function Trig_Chain_Conditions))
    call TriggerAddAction(gg_trg_Chain, function Trig_Chain_Actions)
endfunction

If you don't understand something, tell me so I can help you help me ! (lol)
Hurry ! The clock is tickling !

Btw, Credits will be given as well !

If im correct your not alowed to pass arguments to a codefunc.

Correct Usage:

call ForGroup(GetUnitsInRangeOfLocAll(500, l), function forUnit)

Crap ...you are rit ... and because i can't use any timer, using katana's var system is out of option ... crap ... does any1 know of a good solution for this problem plz ?

The correct way of doing this is to use an other group loop. Like:

local group g = CreateGroup()
local unit tmpunit
local location l = *some random location*
call GroupEnumUnitsInRangeOfLoc(g, l, 500, null)
call DestroyLocation(l)
set l = null
loop
      set tmpunit = FirstOfGroup(g)
      call GroupRemoveUnit(g, tmpunit)
      exitwhen tmpunit == null
      *Do your functions here*
endloop
call DestroyGroup(g)
set g = null

I dont know if its agains the contests rules if someone edits your code to make it work. If so then dont open this.

Fix

function Trig_Chain_Conditions takes nothing returns boolean
    return GetSpellAbilityId() == 'A000'
endfunction
//=========================================================
//ver local vars MB_targ, MB_SMax, MB_alltargs
function forUnit takes unit whichUnit, unit targ, real max, group g returns nothing
    local player p = GetOwningPlayer(GetTriggerUnit())
    local unit MB_targ = targ
    local real MB_SMax = max
local group MB_alltarg = g
    if IsUnitType(whichUnit, UNIT_TYPE_STRUCTURE) == false and IsUnitType(whichUnit, UNIT_TYPE_MECHANICAL) and IsUnitType(whichUnit, UNIT_TYPE_MAGIC_IMMUNE) and IsUnitEnemy(whichUnit, p) and (GetUnitState(whichUnit, UNIT_STATE_LIFE) > 0) and IsUnitInGroup(whichUnit, MB_alltarg) == false and (GetUnitState(whichUnit, UNIT_STATE_MAX_MANA) > 0) and (GetUnitState(whichUnit, UNIT_STATE_MANA) >= MB_SMax) then
        set MB_targ = whichUnit
        set MB_SMax = GetUnitState(u, UNIT_STATE_MANA)
    endif
    //call DestroyGroup(MB_alltarg)
    //set MB_alltarg = null
    //set u = null
    //set p = null
    //set MB_targ = null
endfunction
//=========================================================
function Trig_Chain_Actions takes nothing returns nothing
    //variables part
    local unit MB_targ = GetSpellTargetUnit()//current target
    local unit MB_prev = GetTriggerUnit() //previous target, the source where it all came
    local integer MB_level = GetUnitAbilityLevel(MB_prev, 'A000')
    local integer MB_targetsnum = (4 + (2 * (MB_level - 1))) //maximum number of affected unit
    local integer MB_cur //current number of targets
    local group MB_alltarg //Group that will save hit units in the past
    local unit dummy //casts the spells
    local group MB_TempGroup //will pick a random unit, that will be the next target
    local real MB_SMax//chooses the unit with more mana
    local location l
    local group tmpgroup = CreateGroup()
    local unit tmpunit
    //action part
    loop
        exitwhen (MB_cur > MB_targetsnum)
        //start
        call GroupAddUnit(MB_alltarg, MB_targ)
        set dummy = CreateUnit(GetOwningPlayer(GetTriggerUnit()), 'h000', GetUnitX(MB_prev), GetUnitY(MB_prev), 0.0)
        call UnitAddAbility(dummy, 'A002')
        call IssueTargetOrder(dummy, "shadowstrike", MB_targ)
        call UnitApplyTimedLife(dummy, 'BTLF', 1.0)
        //set dummy = null
        //Wait Unit Unit was reached by effect
        loop
            exitwhen ((GetUnitAbilityLevel(MB_targ, 'BEsh') > 0) or (GetUnitState(MB_targ, UNIT_STATE_LIFE) <= 0))
            call TriggerSleepAction(0.30)
        endloop
        //If unit is alive, take mana
        if (GetUnitState(MB_targ, UNIT_STATE_LIFE) <= 0) == false then
            call CreateUnit(GetOwningPlayer(GetTriggerUnit()), 'h000', GetUnitX(MB_targ), GetUnitY(MB_targ), 0.0)
            call UnitApplyTimedLife(dummy, 'BTLF', 1.0)
            call UnitAddAbility(dummy, 'A001')
            call SetUnitAbilityLevel(dummy, 'A001', 8 * (MB_level - 1) + MB_cur)
            call IssueTargetOrder(dummy, "manaburn", MB_targ)
        else
            set MB_cur = MB_targetsnum + 1
        endif
        //Choose New target
        if (MB_cur < MB_targetsnum) then
            set MB_prev = MB_targ
            set MB_SMax = -1
            set l = GetUnitLoc(MB_prev)
            call GroupEnumUnitsInRangeOfLoc(tmpgroup, l, 500, null)
            loop
                set tmpunit = FirstOfGroup(tmpgroup)
                call GroupRemoveUnit(tmpgroup, tmpunit)
                exitwhen tmpunit == null
                call forUnit(tmpunit, MB_targ, MB_SMax, MB_alltarg)
            endloop
            call DestroyGroup(tmpgroup)
            set tmpgroup = null
            call RemoveLocation(l)
            set l = null
            call GroupClear(MB_alltarg)
endif
        set MB_cur = MB_cur + 1
    endloop
    call DestroyGroup(MB_alltarg)
    call DestroyGroup(MB_TempGroup)
    set MB_alltarg = null
    set MB_TempGroup = null
    set MB_targ = null
    set MB_prev = null
    set dummy = null
endfunction
//=========================================================
function InitTrig_Chain takes nothing returns nothing
    set gg_trg_Chain = CreateTrigger()
    call TriggerRegisterAnyUnitEventBJ( gg_trg_Chain, EVENT_PLAYER_UNIT_SPELL_EFFECT )
    call TriggerAddCondition(gg_trg_Chain, Condition(function Trig_Chain_Conditions))
    call TriggerAddAction(gg_trg_Chain, function Trig_Chain_Actions)
endfunction

I am asking for help, and you are helping me. According to the contest rules, I am allowed to get helped, as long as the final work is mine, which is the case.
You solved a bug, but the final work is mine (although if your solution works, I will surely credit you for your help)

I will your solution soon and see what happens.

EDIT

It is of no use ... it doesn't work, there is some bug around there and I just can't find it ....
The map I am using as a base has a bug, so my spell has that bug as well. It never jumps to more than 2 units ...
This way I know I won't be joining the contest (although I would like to).

I guess this is a dead situation a dead end.

I was trying to use this map as source, so then I could change it. But it simply doesn't matter any more.
Anyway, I'll post it here.

If you guys can find the bug on the original spell, then plz comment it.

This map came included with a tutorial (also bugged probably), but if you wanna know, I can give you guys the link.

Flame, ForGroups are ugly functions. Never use them. GroupEnum > all.

Meh, it is a little to late now but still thx for the comment.
Also, ForGroup is the fastest way to work with groups, Vexorian proved it ...

Really super easy way to fix your problem. As long as you don't have a wait inside the function your ForGroup is calling, use global variables. It'll still be MUI because there's no waits.

I would definitely prefer to avoid globals. They make spell not MUI and more complicated. Besides globals in this spell would prevent it from being MUI. That's why I use jass =)

the only way of using globals, and still allow the spell to become MUI, would be to use arrays, which is far more complicated than using JAss

Globals would not affect your MUI at all, unless you used a wait.

Threads run linearly (except for interrupts, but thats a whole nother story). Meaning that they don't randomly start running your code in one section then jump to another. A TriggerSleepAction allows it to work on a different trigger. But besides that (and timers) a trigger will run from start to stop without interruption.

So if you stored all your variables in globals then used a ForGroup (without waits) it would have no effect [whatsoever] on the MUI of your ability.

Waits remove the MUI of global variables because they put a pause on the thread. This means another instance of that same trigger could modify your variables in that wait. This would heavily screw up the MUI.

OMG, this is not a joke. Globals will prevent my spell from being MUI because of the target global. This global will be replaced. Don't force me to prove it.

I only use a global if I have no other choice, and yet, when I do so, I also create another local with the same name, to take advantage of the bug created (thus making it MUI again).

But it is a tested fact here, if I replace all vars by globals, this spell won't be MUI.
The only way of making this MUI with globals is using arrays.

You can't replace all variables with globals. Only the ones that don't have to retain their value past the TriggerSleepAction.

Or you use temporary globals just for the passing of the variable.

I'm a big GUI'er and hence use globals + arrays quite often

Believe it or not, arrays are quite simple, I don't see why people think they are so complicated

I suppose if you mix them with locals, it could get rather confusing



Also, where could I find a page on the "bug" created when you name a local variable the same name as a global???

It's not a bug. All you need to do is add a custom script:

Custom script : local integer udg_Myglobalintegervariable
Myglobalintegervariable is the name of the variable you made in the trigger editor. Everytime you use myglobalintegervariable in your trigger, it'll be referencing to the local integer you created instead of the global integer you made.

Lol, it is a bug. Give me 5 secs, I will give you the link to the tutorial.
And using arrays with GUI makes things work, doesn't make them efficient, which is also a goal I intent to achieve.

Here is the link to the tutorial, I hope it helps:
Multi-Instancible GUI Spell Making