Is this leaking?

[ZakkWylde-]

Is this leaking? if so, where?

scope Sliding initializer Init

private function SlidingCond takes nothing returns boolean
        return (GetUnitTypeId(GetFilterUnit()) == 'E000')// or GetUnitTypeId(GetFilterUnit()) == 'E001')//the unit rawcode
endfunction
    
private function Sliding takes nothing returns nothing
    local group g = CreateGroup()
    local unit u
    local real x
    local real y
    local real uX
    local real uY 
    local real speed = (5.5*correction)
    local integer i
    
    local boolean d1
    local boolean d2
    local boolean d3
    local boolean d4
    local boolean onDeath
    local real tKill
    
    call GroupEnumUnitsInRect(g, GetWorldBounds(), Condition(function SlidingCond))
    loop
        set u = FirstOfGroup(g)
        exitwhen u == null
        set i = GetUnitUserData(u)
        set tKill = tightness
        set uX = GetUnitX(u)
        set uY = GetUnitY(u)
        set d1 = (GetTerrainType(uX + tKill, uY + tKill) == udg_DeathTerSnow or GetTerrainType(uX + tKill, uY + tKill) == udg_DeathTerLava)
        set d2 = (GetTerrainType(uX + tKill, uY - tKill) == udg_DeathTerSnow or GetTerrainType(uX + tKill, uY - tKill) == udg_DeathTerLava)
        set d3 = (GetTerrainType(uX - tKill, uY + tKill) == udg_DeathTerSnow or GetTerrainType(uX - tKill, uY + tKill) == udg_DeathTerLava)
        set d4 = (GetTerrainType(uX - tKill, uY - tKill) == udg_DeathTerSnow or GetTerrainType(uX - tKill, uY - tKill) == udg_DeathTerLava)
        set onDeath = (d1 and d2 and d3 and d4)
        
        if not onDeath then
            if (GetTerrainType(uX, uY) == slide1 or GetTerrainType(uX, uY) == slide2) and GetUnitState(u, UNIT_STATE_LIFE) > 0 and IsUnitPaused(u)==false then
                set x = uX + slideSpeed<i> * Cos(GetUnitFacing(u) * bj_DEGTORAD) //speed affects sliding speed
                set y = uY + slideSpeed<i> * Sin(GetUnitFacing(u) * bj_DEGTORAD)
                call SetUnitPosition(u, x, y)
            elseif (GetTerrainType(uX, uY) == slide3 or GetTerrainType(uX, uY) == slide4) and GetUnitState(u, UNIT_STATE_LIFE) &gt; 0 and IsUnitPaused(u)==false then
                set x = uX + slideSpeed<i> * Cos(GetUnitFacing(u) * bj_DEGTORAD)
                set y = uY + slideSpeed<i> * Sin(GetUnitFacing(u) * bj_DEGTORAD)
                call SetUnitPosition(u, x, y)
            endif
        elseif onDeath then
            call IssueImmediateOrder(u, &quot;stop&quot;)
            if (TKInUse<i> == false and GetUnitState(u, UNIT_STATE_LIFE) &gt;= 1) then
                call TriggerExecute(udg_checkTerrain<i>)
            endif
        endif
        call GroupRemoveUnit(g, u)
    endloop
    call DestroyGroup(g)
    set g = null
endfunction

private function Init takes nothing returns nothing
    local timer p = CreateTimer()
    call TimerStart(p, 0.010, true, function Sliding) //frequency of trigger
endfunction
    
endscope
</i></i></i></i></i></i>

If not leaking, is there a way to optimize this (without TOTALLY reworking it)?
(and to whom it may concern, checkTerrain is a trigger array--it is what kills the demon hunter)

EDIT: not sure what i'm using the local real speed for...

 

[Dirac]

Bad news mate, you have to rewrite a lot of things, i advise you to use the TESH "replace" option
[ljass]GetUnitFacing()[/ljass] should be stored inside a var and used multiple times instead of calling it everytime, same thing goes for almost EVERY NATIVE you use

0.01 is way to low use 0.03125

 

[ZakkWylde-]

Would you be able to explain what you mean/how to store natives inside a variable? (I am rather new to optimizing code... D: )
EDIT: you mean just a set a real = GetUnitFacing(u) and use the real? (sorry had a brain fart)
EDIT2: should I also do this for GetTerrainType()? [because each time i have different parameters...seems a bit unhelpful, no?]


On another note (sorry for jamming this in with the the above...), how could I optimize/leak-remove this?

function rPatrol takes rect r1, rect r2 returns nothing
    local integer xi = GetRandomInt(0,1)
    local integer yi = GetRandomInt(0,4)
    local real x = GetRandomReal(-8, 0)
    local real y = GetRandomReal(0, 8)
    
    local real x1
    local real y1
    local real x2
    local real y2
    
    local unit u
    
    local integer v
    
    if xi == 0 then
        set x1 = GetRectCenterX(r1) + GetRandomReal(x, y)
        set y1 = GetRectCenterY(r1) + GetRandomReal(x, y)
        set x2 = GetRectCenterX(r2) + GetRandomReal(x, y)
        set y2 = GetRectCenterY(r2) + GetRandomReal(x, y)
    elseif xi == 1 then
        set x2 = GetRectCenterX(r1) + GetRandomReal(x, y)
        set y2 = GetRectCenterY(r1) + GetRandomReal(x, y)
        set x1 = GetRectCenterX(r2) + GetRandomReal(x, y)
        set y1 = GetRectCenterY(r2) + GetRandomReal(x, y)
    endif
    
    if yi == 0 then
        set v = 'n000'
    elseif yi == 1 then
        set v = 'n001'
    elseif yi == 2 then
        set v = 'n002'
    elseif yi == 3 then
        set v = 'n003'
    elseif yi == 4 then
        set v = 'n004'
    endif
        
    set u = CreateUnit(Player(PLAYER_NEUTRAL_AGGRESSIVE), v, x1, y1, GetRandomReal(0, 360))
    call SetUnitVertexColorBJ(u, mRed, mGreen, mBlue, 0)
    call TriggerRegisterUnitInRange(gg_trg_Kill, u, uC, null)
    call IssuePointOrderById(u,851990,x2,y2)
    call SetUnitFlyHeight(u, 0, 99999)
 
    call GroupAddUnit(udg_EnemyUnits, u)
    if RectContainsCoords(gg_rct_Level2Kill1, GetUnitX(u), GetUnitY(u)) or RectContainsCoords(gg_rct_Level2Kill2, GetUnitX(u), GetUnitY(u)) then
        call GroupAddUnit(level2Murlocs, u)
    endif
    
    set u = null
    
endfunction

EDIT3: I see I could set variables to the GetRectCenterX/Y and GetRandomReal natives I have there...anything else?

 

[Dirac]

local real facing=GetUnitFacing(u)

insdead of using GetUnitFacing everywhere now you use "facing"

goes the same for all natives you use more than once

 

[ZakkWylde-]

EDIT: COMPLETELY removed old post--didn't want to double...
Are there any improvements to be made here? (personally I think it looks sexy after the reworking )

scope Sliding initializer Init

private function SlidingCond takes nothing returns boolean
        return (GetUnitTypeId(GetFilterUnit()) == 'E000')
endfunction
    
private function Sliding takes nothing returns nothing
    local group g = CreateGroup()
    
    local unit u
    
    local real x
    local real y
    local real uX
    local real uY 
    local real facing
    local real xPosNew
    local real yPosNew
    local real tKill
    
    local integer terType
    local integer t1
    local integer t2
    local integer t3
    local integer t4
    
    local integer i
    
    local boolean d1
    local boolean d2
    local boolean d3
    local boolean d4
    local boolean onDeath
    local boolean shouldSlide
    local boolean properTerrain
    local boolean alive
    
    call GroupEnumUnitsInRect(g, GetWorldBounds(), Condition(function SlidingCond))
    set tKill = tightness
    
    loop
        set u = FirstOfGroup(g)
        exitwhen u == null
        set i = GetUnitUserData(u)
        set uX = GetUnitX(u)
        set uY = GetUnitY(u)
        
        set t1 = GetTerrainType(uX + tKill, uY + tKill)
        set t2 = GetTerrainType(uX + tKill, uY - tKill)
        set t3 = GetTerrainType(uX - tKill, uY + tKill)
        set t4 = GetTerrainType(uX - tKill, uY - tKill)
        
        set d1 = (t1 == udg_DeathTerSnow or t1 == udg_DeathTerLava)
        set d2 = (t2 == udg_DeathTerSnow or t2 == udg_DeathTerLava)
        set d3 = (t3 == udg_DeathTerSnow or t3 == udg_DeathTerLava)
        set d4 = (t4 == udg_DeathTerSnow or t4 == udg_DeathTerLava)
        set onDeath = (d1 and d2 and d3 and d4)
        
        set alive = (GetWidgetLife(u) >= .405)
        if not onDeath then
        
            set terType = GetTerrainType(uX, uY)
            set properTerrain = (terType == slide1 or terType == slide2 or terType == slide3 or terType == slide4)
            set shouldSlide = (alive and IsUnitPaused(u) == false and properTerrain)
            
            if shouldSlide then
                set facing = (GetUnitFacing(u) * bj_DEGTORAD)
                set xPosNew = uX + ((slideSpeed<i> * Cos(facing))* 3.125)
                set yPosNew = uY + ((slideSpeed<i> * Sin(facing))* 3.125)
                call SetUnitPosition(u, xPosNew, yPosNew)
            endif
        elseif onDeath then
            call IssueImmediateOrder(u, &quot;stop&quot;)
            if (TKInUse<i> == false and alive) then
                call TriggerExecute(udg_checkTerrain<i>)
            endif
        endif
        
        
        call GroupRemoveUnit(g, u)
    endloop
    
    call DestroyGroup(g)
    set g = null
endfunction

private function Init takes nothing returns nothing
    local timer p = CreateTimer()
    call TimerStart(p, 0.03125, true, function Sliding) //frequency of trigger
endfunction
    
endscope
</i></i></i></i>

 

[BlackRose]

1.)

private function Init takes nothing returns nothing
    local timer p = CreateTimer()
    call TimerStart(p, 0.03125, true, function Sliding) //frequency of trigger
endfunction

In this function, there is no need for the timer variable "p", simply replace "p" with [LJASS]CreateTimer()[/LJASS].

2.) In the line [LJASS]call GroupEnumUnitsInRect(g, GetWorldBounds(), Condition(function SlidingCond))[/LJASS], you should set [LJASS]GetWorldBounds()[/LJASS] with a constant global to avoid calling it every 0.03125 seconds. Minimize unecessary function calls.

3.) You shouldn't be creating a group and then destroying it so frequently. I recommend you use one global group as you do everything instantly (you don't need to save the units in a group for later reference). That way you cut down on one local variable and two function calls.

4.) You don't really need the "onDeath" variable. Your code is currently structured:
'

//
        if not onDeath then
        elseif onDeath then
        endif

If "onDeath" is false, then the only other option is true, so basically it can be:

//        
		if not (d1 and d2 and d3 and d4) then
        elseif onDeath then
        endif

That should work I hope.

5.) You can definitely inline variables: "xPosNew" and "yPosNew". You only use them once.

6.) Can't you just use "tightness" instead of "tKill"?

7.) You can also inline "d1", "d2", "d3", and "d4". If you want to keep the code readable, use [LJASS]/* put line here*/[/LJASS].

I'd say work on the way you structure your code first, and then work on fixing the nitpicky things such as leaks, etc.

 

[ZakkWylde-]

Very thorough, thank you.
Regarding number 7, I'm not sure exactly what you mean. (does the compiler not look at line-returns to distinguish separate the lines? I thought it used that instead of, say, the ; in C# to end lines)

Also, am I missing something?--because much to my dismay, I have never been able to comment out segments of code simply with /* <code> */ in JASS.
EDIT: Oh wow--it does actually comment it out, just didn't turn it to the green italicized text I was used to...

EDIT2: Regarding using a global group, will [ljass]GroupEnumUnitsInRect(g, GetWorldBounds(), Condition(function SlidingCond))[/ljass] add those units to an existing group? (as I'm thinking about it, there is no difference between a freshly created group, and an empty group, so in my case, it seems it will work...but if there were already units in global group, would using this ADD the units to the group or REPLACE those units in the group?)

Also, regarding #6, I will probably replace it once I decide that I wont need to do any math with tightness (I wasn't sure if I needed to modify the value to...say, tightness+5 or tightness/1.2)