[JASS] Help With Optimisation

[DairyQueen]

Hey everyone, I'm just starting to use JASS instead of GUI and I would ask a favour. I figure the best way to learn is from the experts, and so I was wondering if you guys could tell me how I could make this code more efficient:
(Basically it's for a spell that is like Force of Nature, but for corpses instead of trees)

function RaiseMinionsYesNo takes nothing returns boolean
    return GetSpellAbilityId() == 'A00P'
endfunction

function RaiseMinionsDo takes nothing returns nothing
    local unit caster = GetSpellAbilityUnit()
    local location target_pos = GetSpellTargetLoc()
    local group corpses = CreateGroup()
    local unit temp
    local integer count = GetUnitAbilityLevel(caster, 'A00P')
    local location temp_loc
    local effect raiseEffect
    local unit minion
    
        call GroupEnumUnitsInRangeOfLoc(corpses, target_pos, 100+50*count, null)
        
        loop
            set temp = FirstOfGroup(corpses)
            exitwhen temp == null  or count == -1
            
            if GetUnitState(temp, UNIT_STATE_LIFE) <= 0 then
                set temp_loc = GetUnitLoc(temp)
                call RemoveUnit(temp)
                set raiseEffect = AddSpecialEffectLoc("Objects\\Spawnmodels\\Human\\HumanLargeDeathExplode\\HumanLargeDeathExplode.mdl", temp_loc)
                set minion = CreateUnitAtLoc(GetOwningPlayer(caster), 'n001', temp_loc, GetRandomInt(0, 360))
                call SetUnitAnimation(minion, "birth")
                call UnitApplyTimedLife(minion, 'B002', 60.)
                set count = count - 1
            endif
            call  GroupRemoveUnit(corpses, temp)
            call DestroyEffect(raiseEffect)
            set raiseEffect = null
        endloop
        
    call DestroyGroup(corpses)
    call RemoveLocation(target_pos)
    call RemoveLocation(temp_loc)
    set corpses = null
    set target_pos = null
    set temp_loc = null
    set caster = null
    set temp = null
    set minion = null
    
endfunction

function RaiseMinionsCant takes nothing returns nothing
    local unit caster = GetSpellAbilityUnit()
    local location target_pos = GetSpellTargetLoc()
    local group corpses = CreateGroup()
    local group allUnits = CreateGroup()
    local integer count = GetUnitAbilityLevel(caster, 'A00P')
    local unit temp
    
    call GroupEnumUnitsInRangeOfLoc(allUnits, target_pos, 100+50*count, null)
    
    loop
        set temp = FirstOfGroup(allUnits)
        exitwhen temp == null
        if GetUnitState(temp, UNIT_STATE_LIFE) <= 0 then
            call GroupAddUnit(corpses, temp)
        endif
            call GroupRemoveUnit(allUnits, temp)    
    endloop
    if IsUnitGroupEmptyBJ(corpses) then
         call IssueImmediateOrder(caster, "stop")
         call DisplayTextToPlayer(GetOwningPlayer(caster), 0., 0., "Must target at least one corpse")
    endif
    
    call DestroyGroup(corpses)
    call DestroyGroup(allUnits)
    call RemoveLocation(target_pos)
    set corpses = null
    set allUnits = null
    set target_pos = null
    set caster = null
    set temp = null
    
endfunction

One thing I can think of but don't know how to do is when I set the unit group 'corpses' to all units in an area, then check if they are dead, I figure that last field 'boolexpr' or something in GroupEnumUnitsInRangeOfLoc() could be used to only add dead units to the group. Like I said though, I don't know how to do that.

 

[Ciebron]

here are some minior changes i made removed, use x/y instead of loc's and some other minor shit

Code

function RaiseMinionsYesNo takes nothing returns boolean
    return GetSpellAbilityId() == 'A00P'
endfunction

function filter takes nothing returns boolean //Boolexp is faster then 'if else then'
    return GetWidgetLife(GetFilterUnit()) <= .305
endfunction

function RaiseMinionsDo takes nothing returns nothing
    local unit caster = GetTriggerUnit() // GetTriggerUnit is fastest use it :)
    local location target_pos = GetSpellTargetLoc()
    local real x = GetLocationX(target_pos)
    local real y = GetLocationY(target_pos)
    local group corpses = CreateGroup()
    local unit temp
    local integer count = GetUnitAbilityLevel(caster, 'A00P')
    local unit minion
    local player P = GetOwningPlayer(caster)
    
        call GroupEnumUnitsInRange(corpses,x,y, 100+50*count,Filter(function filter))

        loop
            set temp = FirstOfGroup(corpses)
            exitwhen temp == null or count == -1
            //reuse x/y gives u less locals
            set x = GetUnitX(temp)
            set y = GetUnitY(temp)
            call RemoveUnit(temp)
            //create and destroy, save it if u need to destroy if later.
            call DestroyEffect(AddSpecialEffect("Objects\\Spawnmodels\\Human\\HumanLargeDeathExplode\\HumanLargeDeathExplode.mdl",x,y))
            set minion = CreateUnit(P, 'n001',x,y,GetRandomInt(0, 360))
            call SetUnitAnimation(minion, "birth")
            call UnitApplyTimedLife(minion, 'B002', 60.)
            set count = count - 1
            call GroupRemoveUnit(corpses, temp)
        endloop

    call DestroyGroup(corpses)
    call RemoveLocation(target_pos)
    
    set P = null
    set corpses = null
    set target_pos = null
    set caster = null
    set temp = null
    set minion = null
endfunction

function RaiseMinionsCant takes nothing returns nothing
    local unit caster = GetTriggerUnit() // GetTriggerUnit is fastest use it :P
    local location target_pos = GetSpellTargetLoc()
    local real x = GetLocationX(target_pos)
    local real y = GetLocationY(target_pos)
    local group corpses = CreateGroup()
    local integer count = GetUnitAbilityLevel(caster, 'A00P')
    
    call GroupEnumUnitsInRange(corpses,x,y,100+50*count,Filter(function filter)) //filter instead
    
    if IsUnitGroupEmptyBJ(corpses) then //didnt figure out a work around for this bj and im not going to try to day
         //also do this work? dont u need to pause the unit, order it too stop and then unpause it
         call IssueImmediateOrder(caster, "stop")
         call DisplayTextToPlayer(GetOwningPlayer(caster), 0., 0., "Must target at least one corpse")
    endif

    call DestroyGroup(corpses)
    call RemoveLocation(target_pos)
    set corpses = null
    set target_pos = null
    set caster = null
endfunction

 

[DairyQueen]

Thanks for showing me how to do the boolexpr thing. I will take your advice and use Triggering unit, and the x and y values, as well as the Special Effect thing.
I suppose instead of checking if the group is empty near the end, could I just use

if FirstOfGroup(corpses) == null then

And yes, just ordering the unit to stop does work, I have tested it several times. However, I have noticed in other maps of mine it doesn't.

 

[Ciebron]

ah yes that will work but think it will leak, save it to a local unit

Edit: also i would recommend you to use a scope with a initializer instead of InitTrig

eg.

scope Spell initializer init

private function init takes nothing returns nothing
    local trigger Trig = CreateTrigger()
    //other stuff to make the trigger fire on cast
endfunction

endscope