[JASS] [Solved] Help on detecting leaks

[Astronoot]

Hi, I'm new and it's my first time posting a thread here :] (Not sure if this is right forum to ask.)

So, um I started migrating from GUI to JASS recently. I tried browsing tutorials here and on Helper.net on making this spell. It's an MUI ability that uses hashtables as data bridge and timers for intervals. It is working as I planned but I'm not quite confident if I did it nicely or leakless as possible.

Idk if these vague details are needed but the spell basically does these every seconds:

• Pick all enemy units in an area nearby the caster as targetGroup
• Every 0.2 seconds, randomly choose 1 unit from the targetGroup and damage it

So every second, the spell damages 5 different enemy units. The spell duration is 12 seconds.

So my question is: Can I ask if there's a (handle or reference) leak that I failed to null or destroy here? Especially the group that I stored on the hashtable.

I also gladly take suggestions on making the spell better.

Here's my trigger:

function GetTheReckoning_SpellId takes nothing returns integer
    return 'A000'
endfunction

function Trig_The_Reckoning_Conditions takes nothing returns boolean
    return ( GetSpellAbilityId() == GetTheReckoning_SpellId() )
endfunction

function GetTheReckoning_SpellAoE takes nothing returns real
    return 600.0
endfunction

function GetTheReckoning_Damage takes integer level returns real
    return 60.0 + ( 40.0 * I2R( level ) )
endfunction

function TheReckoning_CreateExplosion takes location where returns nothing
    call AddSpecialEffectLocBJ( where, "Abilities\\Weapons\\SteamTank\\SteamTankImpact.mdl" )
    call DestroyEffectBJ( GetLastCreatedEffectBJ() )
endfunction

function TheReckoning_PrePostCastEffect takes location where returns nothing
    call AddSpecialEffectLocBJ( where, "Abilities\\Spells\\NightElf\\BattleRoar\\RoarCaster.mdl" )
    call DestroyEffectBJ( GetLastCreatedEffectBJ() )
endfunction

// This function runs every 0.2 seconds.
function TheReckoning_HandlerFunc takes nothing returns nothing

    local timer t = GetExpiredTimer()
    local integer id = GetHandleId(t)
 
    local unit u
    local unit caster = LoadUnitHandle( udg_TR_Hashtable, id, StringHash("caster") )
    local integer counter = LoadInteger( udg_TR_Hashtable, id, StringHash("counter") )
    local group targetGroup = LoadGroupHandle( udg_TR_Hashtable, id, StringHash("targetGroup") )
    local group targetGroupRaw
    local group tempGroup
 
    local location casterLoc = GetUnitLoc( caster )
    local location uLoc
    local boolean isUnitValid
    local real damage
 
    // Makes this block only run every second. This block sets up the `targetGroup` units to be damaged.
    if ( ModuloInteger( counter, 5 ) == 0 ) then
        call DestroyGroup( targetGroup )
        set targetGroup = CreateGroup()
        set targetGroupRaw = GetUnitsInRangeOfLocAll( GetTheReckoning_SpellAoE( ), casterLoc )
    
        // Filters `targetGroupRaw` and puts filtered units on `targetGroup`
        loop
            set u = FirstOfGroup( targetGroupRaw )
            exitwhen u == null
            if ( IsUnitEnemy( u, GetOwningPlayer( caster ) ) and IsUnitAliveBJ( u ) ) then
                call GroupAddUnit( targetGroup, u )
            endif
            call GroupRemoveUnit( targetGroupRaw, u )
        endloop
        call DestroyGroup( targetGroupRaw )
    endif
 
    //  If no target, randomly create VFX at random point nearby. Else, damage the target and create VFX on target.
    if ( FirstOfGroup( targetGroup ) == null ) then
        set uLoc = PolarProjectionBJ(casterLoc, GetRandomReal(80.00, GetTheReckoning_SpellAoE() - 80.00), GetRandomReal(0, 360.00))
        call TheReckoning_CreateExplosion( uLoc )
    else
        // Selects one random unit on `targetGroup`
        set tempGroup = GetRandomSubGroup(1, targetGroup)
        set u = FirstOfGroup( tempGroup )
    
        set damage = GetTheReckoning_Damage( GetUnitAbilityLevel( caster, GetTheReckoning_SpellId( ) ) )
    
        // Makes damage only half on `structure` and `mechanical` units.
        if ( IsUnitType( u, UNIT_TYPE_STRUCTURE ) or IsUnitType( u, UNIT_TYPE_MECHANICAL ) ) then
            call UnitDamageTargetBJ( caster, u, 0.5 * damage, ATTACK_TYPE_MAGIC, DAMAGE_TYPE_NORMAL )
        else
            call UnitDamageTargetBJ( caster, u, damage, ATTACK_TYPE_MAGIC, DAMAGE_TYPE_NORMAL )
        endif
    
        set uLoc = GetUnitLoc( u )
        call TheReckoning_CreateExplosion( uLoc )

        // Removes the unit from the `targetGroup` so it won't be called again for 1 second.
        call GroupRemoveUnit( targetGroup, u )
        call DestroyGroup( tempGroup )
    endif

    // After 12 seconds.
    if ( counter >= 59 ) then
        // Makes the caster now able to attack.
        call UnitRemoveAbilityBJ( 'S000', caster )
        call UnitAddAbilityBJ( 'S001', caster )

        call TheReckoning_PrePostCastEffect( casterLoc )
        call PauseTimer( t )
        call DestroyTimer( t )
        call DestroyGroup( targetGroup )
        call FlushChildHashtable( udg_TR_Hashtable, id )
    endif
 
    call SaveInteger( udg_TR_Hashtable, id, StringHash("counter"), counter + 1 )
    call SaveGroupHandle( udg_TR_Hashtable, id, StringHash("targetGroup"), targetGroup )
 
    call RemoveLocation( uLoc )
    call RemoveLocation( casterLoc )
    set t = null
    set u = null
    set caster = null
    set targetGroup = null
    set targetGroupRaw = null
    set tempGroup = null
 
endfunction

function Trig_The_Reckoning_New_Actions takes nothing returns nothing
 
    local timer t = CreateTimer( )
    local unit caster = GetTriggerUnit( )
    local location casterLoc = GetUnitLoc( caster )

    call TheReckoning_PrePostCastEffect( casterLoc )

    call SaveUnitHandle( udg_TR_Hashtable, GetHandleId(t), StringHash("caster"), caster )
    call SaveInteger( udg_TR_Hashtable, GetHandleId(t), StringHash("counter"), 0 )
    call SaveGroupHandle( udg_TR_Hashtable, GetHandleId(t), StringHash("targetGroup"), null )
 
    // This makes the caster unable to attack for the duration.
    call UnitAddAbilityBJ( 'S000', caster )
    call UnitRemoveAbilityBJ( 'S001', caster )
 
    call TimerStart( t, 0.2, true, function TheReckoning_HandlerFunc )
 
    call RemoveLocation( casterLoc )
    set t = null
    set caster = null
    set casterLoc = null
 
endfunction

Uh sorry for my bad English btw. English is not my native language. I also attached the map if it is needed. Thanks!

 

[MyPad]

There are a few things I've spotted right off the bat:

• The targetGroup field does not actually hold a group handle, since it is initially assigned to null. I suppose this might be intentional?
• You can actually work with coordinates directly in JASS/vJASS, only needing location handles when dealing with the z-axis. Usually, the syntax for coordinate getters is Get<Type>X and Get<Type>Y, with the first letter in <Type> being capitalized.
• There are a lot of unnecessary location handles being created and destroyed.

So far, there isn't a prominent chunk of code where I could find a memory leak, but this script can be further optimized to take full advantage of JASS/vJASS.

 

[Astronoot]

Hello, thanks for responding!

The targetGroup field does not actually hold a group handle, since it is initially assigned to null. I suppose this might be intentional?

If you are referring to call SaveGroupHandle( udg_TR_Hashtable, GetHandleId(t), StringHash("targetGroup"), null ), I meant it the to be initially null (I could replace it with CreateGroup() instead, though) since it will be destroyed and initialized on the first loop. Is there a problem on the way I did it or a way to simplify it?

You can actually work with coordinates directly in JASS/vJASS, only needing location handles when dealing with the z-axis. Usually, the syntax for coordinate getters is Get<Type>X and Get<Type>Y, with the first letter in <Type> being capitalized.

Oho, I never thought it was actually thing. (I was so used to the GUI tradition my bad). I just found one post about it and I'll try practicing functions that take x- and y-coordinates instead of locations.

Anyways, thanks. Looks like my 6 hour sloppy-coding-that-spell-session paid off.

 

[MyPad]

If you are referring to call SaveGroupHandle( udg_TR_Hashtable, GetHandleId(t), StringHash("targetGroup"), null ), I meant it the to be initially null (I could replace it with CreateGroup() instead, though) since it will be destroyed and initialized on the first loop. Is there a problem on the way I did it or a way to simplify it?

Now that you've put it like that, you can remove that line entirely and it will have no effect on the resulting game.

 

[Astronoot]

Oh wow I tried removing it along with call SaveInteger( udg_TR_Hashtable, GetHandleId(t), StringHash("counter"), 0 ) and it indeed did not affect anything. Btw, here's the final code. I tried my best to avoid using locations but I still have to use one since I need it for the polar projection.

function TheReckoning_CreateExplosion takes real x, real y returns nothing
    call DestroyEffect( AddSpecialEffect( "Abilities\\Weapons\\SteamTank\\SteamTankImpact.mdl", x, y ) )
endfunction

function TheReckoning_PrePostCastEffect takes real x, real y returns nothing
    call DestroyEffect( AddSpecialEffect( "Abilities\\Spells\\NightElf\\BattleRoar\\RoarCaster.mdl", x, y ) )
endfunction

// This function runs every 0.2 seconds.
function TheReckoning_HandlerFunc takes nothing returns nothing

    local timer t = GetExpiredTimer()
    local integer id = GetHandleId(t)
    
    local unit u
    local unit caster = LoadUnitHandle( udg_TR_Hashtable, id, StringHash("caster") )
    local integer counter = LoadInteger( udg_TR_Hashtable, id, StringHash("counter") )
    local group targetGroup = LoadGroupHandle( udg_TR_Hashtable, id, StringHash("targetGroup") )
    local group tempGroup = CreateGroup()
    
    local real damage
    local real casterX = GetUnitX(caster)
    local real casterY = GetUnitY(caster)
    
    local location randomLoc
    
    // This block is only called once every second.
    if ( ModuloInteger( counter, 5 ) == 0 ) then
    
        // Destoys the previous `targetGroup` before re-setting it.
        call DestroyGroup(targetGroup)
        set targetGroup = CreateGroup()
        
        // Creates a group `tempGroup` and puts filtered units on `targetGroup`
        call GroupEnumUnitsInRange( tempGroup, casterX, casterY, GetTheReckoning_SpellAoE(), null )
        loop
            set u = FirstOfGroup(tempGroup)
            exitwhen u == null
            if ( IsUnitEnemy( u, GetOwningPlayer(caster) ) and IsUnitAliveBJ(u) ) then
                call GroupAddUnit(targetGroup, u)
            endif
            call GroupRemoveUnit(tempGroup, u)
        endloop

        call DestroyGroup(tempGroup)
        
    endif
    
    //  If no target, randomly create VFX at random point nearby. Else, damage the target and create VFX on target.
    if ( FirstOfGroup(targetGroup) == null ) then
    
        set randomLoc = PolarProjection_Alt( casterX, casterY, GetRandomReal(80.00, GetTheReckoning_SpellAoE() - 80.00), GetRandomReal(0, 360.00) )
        call TheReckoning_CreateExplosion( GetLocationX(randomLoc), GetLocationY(randomLoc) )
        call RemoveLocation(randomLoc)
        
    else
    
        // Selects one random unit on `targetGroup`
        set tempGroup = GetRandomSubGroup(1, targetGroup)
        set u = FirstOfGroup(tempGroup)
        ...
        call UnitDamageTargetBJ(caster, u, damage, ATTACK_TYPE_MAGIC, DAMAGE_TYPE_NORMAL)
        call TheReckoning_CreateExplosion( GetUnitX(u), GetUnitY(u) )

        // Removes the unit from the `targetGroup` so the unit won't be damaged again for the second.
        call GroupRemoveUnit(targetGroup, u)
        call DestroyGroup(tempGroup)
        
    endif

    // This block runs after 12 seconds.
    if ( counter >= 59 ) then
    
        ...
        call TheReckoning_PrePostCastEffect( casterX, casterY )
        call PauseTimer(t)
        call DestroyTimer(t)
        call DestroyGroup(targetGroup)
        call FlushChildHashtable(udg_TR_Hashtable, id)
        
    endif
    
    call SaveInteger( udg_TR_Hashtable, id, StringHash("counter"), counter + 1 )
    call SaveGroupHandle( udg_TR_Hashtable, id, StringHash("targetGroup"), targetGroup )
    
    set t = null
    set u = null
    set randomLoc = null
    set caster = null
    set targetGroup = null
    set tempGroup = null
    
endfunction

function Trig_The_Reckoning_New_Actions takes nothing returns nothing
    
    local timer t = CreateTimer()
    local integer id = GetHandleId(t)
    local unit caster = GetTriggerUnit()

    call TheReckoning_PrePostCastEffect( GetUnitX(caster), GetUnitY(caster) )
    call SaveUnitHandle( udg_TR_Hashtable, id, StringHash("caster"), caster )
    ...
    call TimerStart( t, 0.2, true, function TheReckoning_HandlerFunc )

    set t = null
    set caster = null
    
endfunction

And oh thanks @MyPad for helping me out! :]

 

[Uncle]

If you wanted to get rid of the Location entirely you could do something like this:

public static (float x, float y) PositionWithPolarOffset(float x, float y, float offset, float degrees) {
    return (
        x + (offset * Cos(DEG2RAD * degrees)),
        y + (offset * Sin(DEG2RAD * degrees))
    );
}

I realize this isn't Jass but what's important is this chunk of code right here which does basically the same thing as Polar projection:

x + (offset * Cos(DEG2RAD * degrees))
y + (offset * Sin(DEG2RAD * degrees))

Which looks like this when converted over to Jass and applied to your code:

set offset = GetRandomReal(80.00, GetTheReckoning_SpellAoE() - 80.00)
set degrees = GetRandomReal(0.00, 360.00) * bj_DEG2RAD
call TheReckoning_CreateExplosion( casterX + (offset * Cos(degrees)), casterY + (offset * Sin(degrees)) )

Obviously you'll need two Real variables, offset and degrees, to make it work.

 

[Gnuoy]

i'm impressed you found a use for trig functions in warcraft 3

 

[Uncle]

i'm impressed you found a use for trig functions in warcraft 3

I wouldn't be too impressed, this is taken straight from Blizzard's code

function PolarProjectionBJ takes location source, real dist, real angle returns location
    local real x = GetLocationX(source) + dist * Cos(angle * bj_DEGTORAD)
    local real y = GetLocationY(source) + dist * Sin(angle * bj_DEGTORAD)
    return Location(x, y)
endfunction

When it comes to math I can count up to about 20 if my shoes and socks are off.

But that's the beauty of programming, smarter people have done the work for you