[JASS] Leakless? (my first VJass spell)

[Vulcano]

I made a simple spell for my tower defense. But i want to know that my trigger is ok, so i can avoid things i did wrong in this spell next time i make spell.

This is code(added some documentation for easier reading):

//****************************************************************************
//*
//* Spell made by DarkWatcher
//*
//* Requires TimerUtils made by Vexorian ([url]www.wc3c.net[/url])
//*
//* Bombard Tower ability (passive)
//* Spell description:
//*
//* Excitement
//*
//* When the dwarfs inside the bombard tower kill a target, they get very
//* excited. Making them imprudent, pressing the "fire AA rocket" button.
//* This shoots a rocket in a occasional group of passing flying machines.
//* Each crashing flying machine deals 300 damage. The dwarfs don't get
//* excited when a crashing flying machine kills a unit.
//*
//****************************************************************************

scope Excitement initializer Init
    globals
        //Number of flying machines crashing
        private constant integer MACHINES = 3
        //The radius of the area where flying machines can crash(rectangular area)
        private constant real AOE = 300.00
        //time between start flying machine crash and crashing damage
        private constant real timerTime = 1.30
        //Special effect when flying machine crashes
        private constant string crashEffect = "Abilities\\Spells\\Orc\\WarStomp\\WarStompCaster.mdl"
    endglobals

    //Killing player, position of crashing flying machine
    private struct Data
        real cX
        real cY
        player id

        static method create takes player id, real x, real y returns Data
            local Data data = Data.allocate()
            set data.id = id
            set data.cX = x
            set data.cY = y
            return data
        endmethod
    endstruct

    //Damage area where flying machine crashes
    private function damageArea takes nothing returns nothing
        local Data place = GetTimerData(GetExpiredTimer())
        local unit dummy = CreateUnit( place.id, 'h009', place.cX, place.cY, bj_UNIT_FACING)

        call UnitAddAbility(dummy, 'A01E')
        call IssueImmediateOrder(dummy, "thunderclap")
        call DestroyEffect(AddSpecialEffect(crashEffect, place.cX, place.cY))

        call place.destroy()
        call ReleaseTimer(GetExpiredTimer())

        set dummy = null
    endfunction

    //Create special effect on killer, create crashing flying machine and launch timer
    private function Actions takes nothing returns nothing
        local unit dummy
        local real deadX = GetUnitX(GetTriggerUnit())
        local real deadY = GetUnitY(GetTriggerUnit())
        local real angle
        local integer index = 1
        local timer time
        local player id = GetOwningPlayer(GetKillingUnit())
        local Data place

        call DestroyEffect(AddSpecialEffectTarget("Abilities\\Spells\\Human\\Flare\\FlareCaster.mdl", GetKillingUnit(), "head"))

        loop
            exitwhen index > MACHINES
            set angle = GetRandomReal(0, bj_DEGTORAD*360)
            set place = Data.create( GetOwningPlayer(GetKillingUnit()), (deadX + GetRandomReal(0, AOE)*Cos(angle)), (deadY + GetRandomReal(0, AOE)*Sin(angle)))

            set dummy = CreateUnit( place.id, 'u004', place.cX, place.cY, bj_UNIT_FACING)
            call SetUnitFlyHeight( dummy, 100.00, 0.00 ) //lower flying height so flying machine hits ground with dead animation
            call KillUnit( dummy )

            set time = NewTimer()
            call TimerStart(time, timerTime, false, function damageArea)
            call SetTimerData(time, place)

            set index = index + 1
        endloop

        set dummy = null
        set time = null
    endfunction

    //Check killing unit to have Excitement ability
    private function Conditions takes nothing returns boolean
        return GetUnitAbilityLevel(GetKillingUnit(), 'A01A') == 1
    endfunction

    //Initializer
    private function Init takes nothing returns nothing
        local trigger gg_trg_Excitement = CreateTrigger( )
        call TriggerRegisterAnyUnitEventBJ( gg_trg_Excitement, EVENT_PLAYER_UNIT_DEATH )
        call TriggerAddCondition( gg_trg_Excitement, Condition( function Conditions ) )
        call TriggerAddAction( gg_trg_Excitement, function Actions )
    endfunction
endscope

(it compiles and works perfectly the way i want it when i test my map)

My main question is what locals should be nulled to prevent leaking. I'm also not completely sure i properly destroyed timer and struct.

 

[Cokemonkey11]

You don't appear to be having any leaks, and you're destroying the struct just fine.

 

[Vulcano]

great

 

[Saia_Djinn]

scope Excitement initializer Init
    globals

        private constant integer MACHINES = 3        

        private constant real AOE = 300.00        

        private constant real timerTime = 1.30        

        private constant string crashEffect = "Abilities\\Spells\\Orc\\WarStomp\\WarStompCaster.mdl"        
    endglobals    
    
   private struct Data
        real cX
        real cY
        player id
    endstruct     

    private function Conditions takes nothing returns boolean
        local unit killer
        local unit dummy
        local real deadX
        local real deadY  
        local real angle
        local integer index = 0
        local timer time
        local Data place

   if GetUnitAbilityLevel(GetKillingUnit(), 'A01A') > 0 then

        set killer =  GetKillingUnit()
        set deadX = GetUnitX(GetTriggerUnit())
        set deadY =  = GetUnitY(GetTriggerUnit())
        set place = Data.create()    
        set place.id = GetOwningPlayer(killer )    
       
        call DestroyEffect(AddSpecialEffectTarget("Abilities\\Spells\\Human\\Flare\\FlareCaster.mdl", killer, "head"))
        
        loop 
            set index = index + 1   
            exitwhen index > MACHINES
            set place = Data.create()
            
            set angle = GetRandomReal(0, bj_DEGTORAD*360)
            set place.cX = deadX + GetRandomReal(0, AOE)*Cos(angle) 
//If my math is correct than bj_DEGTORAD*360 = Pi 
// NO ITS NOT A CIRCLE CONTAINS 2PI SO U NEED *180 FOR 1PI
            set place.cY = deadY + GetRandomReal(0, AOE)*Sin(angle)
            set dummy = CreateUnit( place.id, 'u004', place.cX, place.cY, bj_UNIT_FACING)
            call SetUnitFlyHeight( dummy, 100.00, 0.00 ) 
            call RemoveUnit( dummy )
            set dummy = null    
            set time = NewTimer()
            call TimerStart(time, timerTime, false, function damageArea)
            call SetTimerData(time, place)                      
        endloop    
        endif
       return false
    endfunction
    
   private function damageArea takes nothing returns nothing
        local Data place = GetTimerData(GetExpiredTimer())
        local unit dummy = CreateUnit( place.id, 'h009', place.cX, place.cY, 0.)
                
        call UnitAddAbility(dummy, 'A01E')
        call IssueImmediateOrder(dummy, "thunderclap")
        call DestroyEffect(AddSpecialEffect(crashEffect, place.cX, place.cY))
        
        call place.destroy()
        call ReleaseTimer(GetExpiredTimer())
    endfunction
 
   private function Init takes nothing returns nothing
        local trigger t = CreateTrigger(  )
        call TriggerRegisterAnyUnitEventBJ( t, EVENT_PLAYER_UNIT_DEATH )
        call TriggerAddCondition( t, Condition( function Conditions ) )
    endfunction
endscope

i just went for some code improvements more can be done but my editor is not here so i had to do it freehand
tip: u sometimes used variables not needed like the player id(not the one in the struct)
i skipped the action to use lesser lines and getting a faster trigger
also use private functions in future like i did there or u will get errors with your next spell

 

[Vulcano]

oh the bj_DEGTORAD*360 = Pi was a typo , of course i want a full circle area.
i already changed functions into private ones (got error when making other spell )

ty for help

EDIT: Why do you declare locals before the If? Doesn't it use more memory when other spells are cast?

EDIT2: I think i found a bug: you leak the object in which you set the id: you create, set id and then you make a new object in loop

 

[Saia_Djinn]

i just pasted the trigger so its the same u must have done but yeah now i've seen it just fix that

to edit one:
local need to stand at the beginning of a trigger and i declare them behind the if to prevent what u thougt doing it like i did will not make them leak only thing is u got slightly longer code but skip a action coz of it(and conditions is always called before actions so its faster =D)

 

[SanKakU]

first of all, all local unit variables need to be nulled. so both killer and dummy will need to be nulled. also, when you declare them, and you have ifs that set them, you need to declare them like local unit killer = null then you'll set them as normal under the if, and make sure not to forget to null it after you're done with everything else. reals and integers i know for you you won't need to null, but not so sure about players and timers. i don't usually use those kinds of local variables i guess. if i did some research i'd figure it out but i don't got time now.

 

[Cokemonkey11]

forgetting to null units does not leak, it just makes the game's recycle system take a little longer. Kind of like a minileak..

 

[Element of Water]

The game has no recycle system. That's the reason why handles have to be nulled. What it actually does, is it lets the handle id allocator use lower values, which makes systems like Blue Timer Utils and others work. This is the only benefit of it. It doesn't actually speed up anything.

 

[Saia_Djinn]

if you null killer its just useless and gives your script an additional line(which we dont want it to have, the shorter the code the faster the code so using system like xe will also be never bad)

 

[Element of Water]

You don't even need the killer variable. Just use GetKillingUnit(). It's constant anyway, which means it's actually faster to use it as a function than to assign it to a variable.

 

[Saia_Djinn]

i dont think so coz GetKillingUnit() always does a call while the variable just holds a value so the variabe will surely be faster then GetKillingUnit()

 

[Element of Water]

constant native GetKillingUnit takes nothing returns unit

Totice the keyword constant. This means it will be faster. A constant function is just as fast as using the raw value. Say you had a function like this:

constant function DAMAGE takes nothing returns real
    return 100.00
endfunction

Now take this code:

//...
call UnitDamageTarget(u, target, DAMAGE(), ...)
//...

This will be just as fast as:

//...
call UnitDamageTarget(u, target, 100.00, ...)
//...

 

[Saia_Djinn]

constant native GetKillingUnit takes nothing returns unit

Totice the keyword constant. This means it will be faster. A constant function is just as fast as using the raw value. Say you had a function like this:

constant function DAMAGE takes nothing returns real
    return 100.00
endfunction

Now take this code:

//...
call UnitDamageTarget(u, target, DAMAGE(), ...)
//...

This will be just as fast as:

//...
call UnitDamageTarget(u, target, 100.00, ...)
//...

but not as fast as

//...
globals
    private constant real damage = 100.
endglobals
call UnitDamageTarget(u, target, damage, ...)
//...

 

[Element of Water]

Yes it is. It's all the same speed. Just in different formats.

 

[Cokemonkey11]

The game has no recycle system. That's the reason why handles have to be nulled. What it actually does, is it lets the handle id allocator use lower values, which makes systems like Blue Timer Utils and others work. This is the only benefit of it. It doesn't actually speed up anything.

It was my understanding that the game nulled it, but it waited until the unit was done decaying and removed.

 

[Element of Water]

No, local variables are never automatically nulled. You have to null them manually.

 

[Cheezeman]

I've added some comments on which handles you've never nulled.
Oh and why not make the functions private?
You have a scope and all but you're not really utilizing it.v

//****************************************************************************
//*
//* Spell made by DarkWatcher
//*
//* Requires TimerUtils made by Vexorian ([url]www.wc3c.net[/url])
//*
//*Bombard Tower ability (passive)
//* Spell description:
//* 
//* Excitement
//*
//* When the dwarfs inside the bombard tower kill a target, they get very 
//* excited. Making them imprudent, pressing the "fire AA rocket" button. 
//* This shoots a rocket in a occasional group of passing flying machines.
//* Each crashing flying machine deals 300 damage. The dwarfs don't get 
//* excited when a crashing flying machine kills a unit.
//*
//****************************************************************************

scope Excitement initializer Init
    globals
        //Number of flying machines crashing
        private constant integer MACHINES = 3        
        //The radius of the area where flying machines can crash(rectangular area)
        private constant real AOE = 300.00        
        //time between start flying machine crash and crashing damage
        private constant real timerTime = 1.30        
        //Special effect when flying machine crashes
        private constant string crashEffect = "Abilities\\Spells\\Orc\\WarStomp\\WarStompCaster.mdl"        
    endglobals    
    
    //Check killing unit to have Excitement ability
    function Conditions takes nothing returns boolean
        return GetUnitAbilityLevel(GetKillingUnit(), 'A01A') == 1
    endfunction
    
    //Killing player, position of crashing flying machine
    struct Data
        real cX
        real cY
        player id
    endstruct     
    
    //Damage area where flying machine crashes
    function damageArea takes nothing returns nothing
        local Data place = GetTimerData(GetExpiredTimer())
        local unit dummy = CreateUnit( place.id, 'h009', place.cX, place.cY, bj_UNIT_FACING)
//dummy is never nulled
                
        call UnitAddAbility(dummy, 'A01E')
        call IssueImmediateOrder(dummy, "thunderclap")
        call DestroyEffect(AddSpecialEffect(crashEffect, place.cX, place.cY))
        
        call place.destroy()
        call ReleaseTimer(GetExpiredTimer())
    endfunction
    
    //Create special effect on killer, create crashing flying machine and launch timer
    function Actions takes nothing returns nothing
        local unit killer = GetKillingUnit()
        local unit dummy
//dummy is once again never nulled
        local real deadX = GetUnitX(GetTriggerUnit())
        local real deadY = GetUnitY(GetTriggerUnit())      
        local real angle
        local integer index = 1
        local timer time
//time is never nulled
        local player id = GetOwningPlayer(killer)
        local Data place = Data.create()     
       
        call DestroyEffect(AddSpecialEffectTarget("Abilities\\Spells\\Human\\Flare\\FlareCaster.mdl", killer, "head"))
        
        loop
            exitwhen index > MACHINES
            set place = Data.create()
            set place.id = id
            set angle = GetRandomReal(0, bj_DEGTORAD*360)
            set place.cX = deadX + GetRandomReal(0, AOE)*Cos(angle) //If my math is correct than bj_DEGTORAD*360 = Pi
            set place.cY = deadY + GetRandomReal(0, AOE)*Sin(angle)
            //set place.cX = deadX + GetRandomReal(-AOE, AOE)
            //set place.cY = deadY + GetRandomReal(-AOE, AOE)
            
            set dummy = CreateUnit( place.id, 'u004', place.cX, place.cY, bj_UNIT_FACING)
            call SetUnitFlyHeight( dummy, 100.00, 0.00 ) //lower flying height so flying machine hits ground with dead animation
            call KillUnit( dummy )
            
            set time = NewTimer()
            call TimerStart(time, timerTime, false, function damageArea)
            call SetTimerData(time, place)                      
            
            set index = index + 1     
        endloop         
    endfunction

    //Initializer
    function Init takes nothing returns nothing
        local trigger gg_trg_Excitement = CreateTrigger(  )
        call TriggerRegisterAnyUnitEventBJ( gg_trg_Excitement, EVENT_PLAYER_UNIT_DEATH )
        call TriggerAddCondition( gg_trg_Excitement, Condition( function Conditions ) )
        call TriggerAddAction( gg_trg_Excitement, function Actions )
        set gg_trg_Excitement = null
//nulling a trigger isn't neccessary if they are never destroyed. Same goes for players.
    endfunction
endscope

 

[Vulcano]

I found a tutorial about variables. All variable types that extend handle must be nulled.
So only real and integer locals don't need to be nulled.

@Cheezeman: I still don't completely understand how the Init function works. I don't understand when the Init function is called. I saw someone else nulling trigger and thats why I nulled it.

 

[Viikuna]

That nulling thing is some bug, the game should take care of it, as it does with those argument locals. For some stupid reason it doesnt work same way with those locals declared at the top of some function.

You only have to null local agent type variables that point to some agent that is going to get removed in some point of the game. ( So, no need to null players, non-dynamic triggers, units that are heroes, recycled dummies, recycled timers, recycled groups etc.. )

 

[Cheezeman]

@Cheezeman: I still don't completely understand how the Init function works. I don't understand when the Init function is called. I saw someone else nulling trigger and thats why I nulled it.

Well, only when a handle is destroyed do we need to null any local pointing to it. Since I persume you never destroy your trigger (thus disabling it forever), you don't have to null it.
Unless, of course, you really want to save that 4 byte of memmory for the RAM instead of decreasing loading time.

Yes, this means that locals pointing to players doesn't have to be nulled either. But locals pointing to units which dies, locations which are removed and effects which are destroyed needs to be nullified.

scope Excitement initializer Init

This is the line which calls Init, from "initializer Init". It's basically like InitTrig_[Window_Name] but you can name it whatever you like.

 

[Vulcano]

Another question : does structs need to be nulled after you destroyed them?
And (if it's possible) would it be better to change damageArea into a method in Data or can't timers call methods?

This is my code now:

//****************************************************************************
//*
//* Spell made by DarkWatcher
//*
//* Requires TimerUtils made by Vexorian ([url]www.wc3c.net[/url])
//*
//* Bombard Tower ability (passive)
//* Spell description:
//* 
//* Excitement
//*
//* When the dwarfs inside the bombard tower kill a target, they get very 
//* excited. Making them imprudent, pressing the "fire AA rocket" button. 
//* This shoots a rocket in a occasional group of passing flying machines.
//* Each crashing flying machine deals 300 damage. The dwarfs don't get 
//* excited when a crashing flying machine kills a unit.
//*
//****************************************************************************

scope Excitement initializer Init
    globals
        //Number of flying machines crashing
        private constant integer MACHINES = 3        
        //The radius of the area where flying machines can crash(rectangular area)
        private constant real AOE = 300.00        
        //time between start flying machine crash and crashing damage
        private constant real timerTime = 1.30        
        //Special effect when flying machine crashes
        private constant string crashEffect = "Abilities\\Spells\\Orc\\WarStomp\\WarStompCaster.mdl"        
    endglobals        
   
    //Killing player, position of crashing flying machine
    private struct Data
        real cX
        real cY
        player id
        
        static method create takes player id, real x, real y returns Data
            local Data data = Data.allocate()
            set data.id = id
            set data.cX = x
            set data.cY = y
            return data
        endmethod       
    endstruct     
    
    //Damage area where flying machine crashes
    private function damageArea takes nothing returns nothing
        local Data place = GetTimerData(GetExpiredTimer())
        local unit dummy = CreateUnit( place.id, 'h009', place.cX, place.cY, bj_UNIT_FACING)
                
        call UnitAddAbility(dummy, 'A01E')
        call IssueImmediateOrder(dummy, "thunderclap")
        call DestroyEffect(AddSpecialEffect(crashEffect, place.cX, place.cY))
        
        call place.destroy()
        call ReleaseTimer(GetExpiredTimer())
        
        set dummy = null
    endfunction
    
    //Create special effect on killer, create crashing flying machine and launch timer
    private function Actions takes nothing returns nothing
        local unit dummy
        local real deadX = GetUnitX(GetTriggerUnit())
        local real deadY = GetUnitY(GetTriggerUnit())      
        local real angle
        local integer index = 1
        local timer time
        local player id = GetOwningPlayer(GetKillingUnit())
        local Data place   
       
        call DestroyEffect(AddSpecialEffectTarget("Abilities\\Spells\\Human\\Flare\\FlareCaster.mdl", GetKillingUnit(), "head"))
        
        loop
            exitwhen index > MACHINES
            set angle = GetRandomReal(0, bj_DEGTORAD*360)
            set place = Data.create( GetOwningPlayer(GetKillingUnit()), (deadX + GetRandomReal(0, AOE)*Cos(angle)), (deadY + GetRandomReal(0, AOE)*Sin(angle)))
            
            set dummy = CreateUnit( place.id, 'u004', place.cX, place.cY, bj_UNIT_FACING)
            call SetUnitFlyHeight( dummy, 100.00, 0.00 ) //lower flying height so flying machine hits ground with dead animation
            call KillUnit( dummy )
            
            set time = NewTimer()
            call TimerStart(time, timerTime, false, function damageArea)
            call SetTimerData(time, place)                      
            
            set index = index + 1     
        endloop
        
        set dummy = null
        set time = null       
    endfunction
    
    //Check killing unit to have Excitement ability
    private function Conditions takes nothing returns boolean
        return GetUnitAbilityLevel(GetKillingUnit(), 'A01A') == 1
    endfunction

    //Initializer
    private function Init takes nothing returns nothing
        local trigger gg_trg_Excitement = CreateTrigger(  )
        call TriggerRegisterAnyUnitEventBJ( gg_trg_Excitement, EVENT_PLAYER_UNIT_DEATH )
        call TriggerAddCondition( gg_trg_Excitement, Condition( function Conditions ) )
        call TriggerAddAction( gg_trg_Excitement, function Actions )
    endfunction
endscope

 

[Element of Water]

Structs do not need to be nulled. Timers may call static members, but not non-static ones.

 

[Sophismata]

A timer may be used to call a function that calls a non-static member, however.

 

[Element of Water]

Yeah, but you need some kind of timer attachment system in order to get the struct in the timer callback, so it defeats the object.