[JASS] optimize my trigger

[SanKakU]

edit: ok so far it has been determined that a global point is not necessary...
so, all believe it's better to either use local location/point and then null or else use real y and x to get the point eh?

what would you recommend for optimizing this trigger?

i'm such a newb at jass and i forget a lot of the stuff i learn.

so, i had to convert a gui trigger into custom text because my map making partner can't use new map editor for whatever reason.

anyway, i just want to know like i already asked above.

function Trig_itemspawns_Func005C takes nothing returns boolean
    if ( not ( IsUnitGroupDeadBJ(udg_isg_SanVar) == true ) ) then
        return false
    endif
    return true
endfunction

function Trig_itemspawns_Actions takes nothing returns nothing
    // change the events to what you need for random spawns in one of the 8 locations
    // change the other trigger to edit what items are selected for item spawn.
    set udg_itemnmbr_SanVar = GetRandomInt(1, 2)
    set udg_isg_SanVar = GetUnitsOfTypeIdAll('n00L')
    if ( Trig_itemspawns_Func005C() ) then
        set udg_p_SanVar = GetRectCenter(udg_isr_SanVar[udg_itemnmbr_SanVar])
        call CreateNUnitsAtLoc( 1, 'n00L', Player(bj_PLAYER_NEUTRAL_EXTRA), udg_p_SanVar, bj_UNIT_FACING )
        call RemoveLocation( udg_p_SanVar )
    else
    endif
    call GroupClear( udg_isg_SanVar )
endfunction

//===========================================================================
function InitTrig_itemspawns takes nothing returns nothing
    set gg_trg_itemspawns = CreateTrigger(  )
    call TriggerRegisterTimerEventSingle( gg_trg_itemspawns, 5.00 ) //first item spawns
    call TriggerRegisterTimerEventPeriodic( gg_trg_itemspawns, 15.00 ) //item spawning again
    call TriggerAddAction( gg_trg_itemspawns, function Trig_itemspawns_Actions )
endfunction

i figured i could go ahead and start optimizing it myself, but i rather wanted to make sure i don't make any mistakes...plus you could give me some tips on whether it matters if i use globals and locals and stuff like that. for example that variable i'm using in the condition i think i want to keep global right? and the other stuff could probably be local right? and if i make them local i'll have to null them right? and all that good stuff.

So yeah, experts, please take care of me in this little trigger. i should learn a lot hopefully.

edit: well i just realized that the variable names alone don't say what the variables are maybe i should clarify that?

isg is a unit group (item spawn group)
isr is a region/rect (item spawn rect) actually there are two regions and the item spawn is random...which brings us to itemnmbr and if you need me to tell you that it's an integer then i probably don't need your advice lol but whatever.
sanvar just is a suffix don't pay attention to that sort of thing.
p is point

and in case you're wondering what the heck i'm up to it's a treasure chest that when you attack it then it will drop an item. that part is taken care of no problem in another trigger and some editing in objects as well.

 

[Day-Elven]

function Trig_itemspawns_Actions takes nothing returns nothing
    local location loc=GetRectCenter(udg_isr_SanVar[GetRandomInt(1, 2)])
    set udg_isg_SanVar = GetUnitsOfTypeIdAll('n00L')
    if IsUnitGroupDeadBJ(udg_isg_SanVar) == true then
        call CreateNUnitsAtLoc( 1, 'n00L', Player(bj_PLAYER_NEUTRAL_EXTRA), loc, bj_UNIT_FACING )
        call RemoveLocation( loc )
    endif
    call GroupClear( udg_isg_SanVar )
    set loc=null
endfunction

//===========================================================================
function InitTrig_itemspawns takes nothing returns nothing
    set gg_trg_itemspawns = CreateTrigger( )
    call TriggerRegisterTimerEventSingle( gg_trg_itemspawns, 5.00 ) //first item spawns
    call TriggerRegisterTimerEventPeriodic( gg_trg_itemspawns, 15.00 ) //item spawning again
    call TriggerAddAction( gg_trg_itemspawns, function Trig_itemspawns_Actions )
endfunction

 

[Child_0f_Bodom]

This may not be completely optimized but is surely better than previous code. I suggest you downloading JASSCraft from this site (Tools section) because there you have complete list of JASS function.

function Trig_itemspawns_Actions takes nothing returns nothing

    local real X
    local real Y

    set udg_itemnmbr_SanVar = GetRandomInt(1, 2)
    set udg_isg_SanVar = GetUnitsOfTypeIdAll('n00L')

    if (IsUnitGroupDeadBJ(udg_isg_SanVar)==true) then
        set X= GetRectCenterX(udg_isr_SanVar[udg_itemnmbr_SanVar]))
        set Y= GetRectCenterY(udg_isr_SanVar[udg_itemnmbr_SanVar]))
        call CreateUnit( Player(14), 'n00L', X, Y, 0.00 )
        
    endif

    call GroupClear( udg_isg_SanVar )
    call DestroyGroup( udg_isg_SanVar )

endfunction

//===========================================================================
function InitTrig_itemspawns takes nothing returns nothing
    set gg_trg_itemspawns = CreateTrigger( )
    call TriggerRegisterTimerEventSingle( gg_trg_itemspawns, 5.00 ) 
    call TriggerRegisterTimerEventPeriodic( gg_trg_itemspawns, 15.00 ) 
    call TriggerAddAction( gg_trg_itemspawns, function Trig_itemspawns_Actions )
endfunction

It is better when using coordinates X and Y, if conditions in same function, native functions (can be found in JASSCraft), etc...

 

[SanKakU]

lol i love how at least one of you read the part about the unit group being dead wrong...

at least that's how it looks unless i'm reading it wrong lol.

 

[Child_0f_Bodom]

IsUnitGroupDeadBJ(udg_isg_SanVar)==true

If group is dead, then execute functions.

 

[SanKakU]

yeah the real x and y is what was used in the item drop system that is what i use now for lots of stuff it's so good. so i might use that instead of the point if that's ok.

also what's the significance of clearing and destroying the group?

 

[Child_0f_Bodom]

You must destroy group, or it creates leaks and I though clearing was not a bad idea xD

 

[Day-Elven]

He used global variable for group.
He can just clear it and use again.

 

[ZlatniLav]

COB, This helped me too!!!

+rep!

 

[SanKakU]

He used global variable for group.
He can just clear it and use again.

that's true...but since the entire trigger is in one function maybe i should make the group be local, right?

with my trigger i had two functions.

that's kindof some of the more advanced feedback i'd like to get...but your ideas are great guys, thanks a lot!

anyway i wasn't sure if he was attempting to destroy local or else remove a group or units from the game. see if he did remove units from the game then the units that are checked for dead won't exist and they can't be dead and the trigger won't fire.

 

[Child_0f_Bodom]

Yea, you could make it local.

 

[SanKakU]

another quick question...does anyone know how to disable the jass tags in some text?
of course i mean in someone else's post. it's annoying to have to scroll to the enter spot and hit enter so many times when trying to paste a trigger.

edit: oh you have to hit quote...silly me.

edit2: ok well Child 0f Bodom i noticed that you have too many parentheses in one area.
also the unit that spawns is at 0 degrees but 270 is much better. this part... call CreateUnit( Player(14), 'n00L', X, Y, 0.00 ) can i change that 0 to 270.00 to change that?

oh yea, that's right.

so here is the newly optimized trigger...what do you guys think?

function Trig_itemspawns_Actions takes nothing returns nothing

    local real X
    local real Y
    local integer i = GetRandomInt(1, 2)
    local group g = GetUnitsOfTypeIdAll('n00L')

    if (IsUnitGroupDeadBJ(g)==true) then
        set X= GetRectCenterX(udg_isr_SanVar[i])
        set Y= GetRectCenterY(udg_isr_SanVar[i])
        call CreateUnit( Player(14), 'n00L', X, Y, 270.00 )
        
    endif

    call GroupClear( g )
    call DestroyGroup( g )

endfunction

//===========================================================================
function InitTrig_itemspawns takes nothing returns nothing
    set gg_trg_itemspawns = CreateTrigger( )
    call TriggerRegisterTimerEventSingle( gg_trg_itemspawns, 5.00 ) 
    call TriggerRegisterTimerEventPeriodic( gg_trg_itemspawns, 15.00 ) 
    call TriggerAddAction( gg_trg_itemspawns, function Trig_itemspawns_Actions )
endfunction

is that as good as it gets? plus i gotta know if it's necessary to clear group...maybe it will leak if i do that or something because it's not a global anymore so destroying it is enough right?

 

[Child_0f_Bodom]

You must nullfy group like this.

set g = null

Still, I don't know how much is IsUnitGroupDeadBJ function efficient, but I don't think it will cause troubles.