unit groups...efficiency and leak fixing help?

[SanKakU]

so i've been working on a couple of heroes, day and night.

well...i've got a day trigger a lot like this one...anyway i was looking to turn the trigger into jass and while doing so i'm wondering how i'm doing at getting rid of the leaks and at efficiency...

night jass trigger

scope Night initializer I

private function agilnfilter takes nothing returns boolean
    return ( GetUnitAbilityLevel( GetFilterUnit(), 'A00E') > 0 )
endfunction

private function agilnact takes nothing returns nothing
    call UnitRemoveAbility(GetEnumUnit(), 'A005')
    call UnitAddAbility(GetEnumUnit(), 'A002')
    call SetUnitAbilityLevel(GetEnumUnit(), 'A002', GetUnitAbilityLevel(GetEnumUnit(),'A00E') )
    call UnitAddAbility(GetEnumUnit(), 'A00I')
    call SetUnitAbilityLevel(GetEnumUnit(), 'A00I', GetUnitAbilityLevel(GetEnumUnit(),'A00E') )
endfunction

private function nsfilter takes nothing returns boolean
    return ( GetUnitAbilityLevel(GetFilterUnit(),'A00B') > 0 )
endfunction

private function nsact takes nothing returns nothing
    call UnitRemoveAbility(GetEnumUnit(), 'A007' )
    call UnitAddAbility(GetEnumUnit(), 'A008' )
    call SetUnitAbilityLevel(GetEnumUnit(), 'A008', GetUnitAbilityLevel(GetEnumUnit(),'A00B') )
endfunction

private function agildfilter takes nothing returns boolean
    return ( GetUnitAbilityLevel(GetFilterUnit(),'A00D') > 0 )
endfunction

private function agildact takes nothing returns nothing
    call UnitRemoveAbility(GetEnumUnit(), 'A004' )
    call UnitAddAbility(GetEnumUnit(), 'A006' )
    call SetUnitAbilityLevel(GetEnumUnit(), 'A006', GetUnitAbilityLevel(GetEnumUnit(),'A00D') )
    call UnitRemoveAbility(GetEnumUnit(), 'A00I' )
endfunction

private function dsfilter takes nothing returns boolean
    return ( GetUnitAbilityLevel(GetFilterUnit(),'A00F') > 0 )
endfunction

private function dsact takes nothing returns nothing
    call UnitRemoveAbility(GetEnumUnit(), 'A00A' )
    call UnitAddAbility(GetEnumUnit(), 'A009' )
    call SetUnitAbilityLevel(GetEnumUnit(), 'A009', GetUnitAbilityLevel(GetEnumUnit(),'A00F') )
endfunction

private function dmfilter takes nothing returns boolean
    return ( GetUnitAbilityLevel(GetFilterUnit(),'A00P') > 0 )
endfunction

private function dmact takes nothing returns nothing
    call UnitRemoveAbility(GetEnumUnit(), 'A00N' )
    call UnitAddAbility(GetEnumUnit(), 'A00O' )
    call SetUnitAbilityLevel(GetEnumUnit(), 'A00O', GetUnitAbilityLevel(GetEnumUnit(),'A00P') )
endfunction

private function defilter takes nothing returns boolean
    return ( GetUnitAbilityLevel(GetFilterUnit(),'A00Q') > 0 )
endfunction

private function deact takes nothing returns nothing
    call UnitRemoveAbility(GetEnumUnit(), 'A00O' )
    call UnitAddAbility(GetEnumUnit(), 'A00S' )
    call SetUnitAbilityLevel(GetEnumUnit(), 'A00S', GetUnitAbilityLevel(GetEnumUnit(),'A00Q') )
endfunction

private function A takes nothing returns nothing
    local group g
    set g = GetUnitsInRectMatching(bj_mapInitialPlayableArea,Condition(function agilnfilter))
    call ForGroup(g, function agilnact )
    set g = GetUnitsInRectMatching(bj_mapInitialPlayableArea,Condition(function agildfilter))
    call ForGroup(g, function agildact )
    set g = GetUnitsInRectMatching(bj_mapInitialPlayableArea,Condition(function nsfilter))
    call ForGroup(g, function nsact )
    set g = GetUnitsInRectMatching(bj_mapInitialPlayableArea,Condition(function dsfilter))
    call ForGroup(g, function dsact )
    set g = GetUnitsInRectMatching(bj_mapInitialPlayableArea,Condition(function dmfilter))
    call ForGroup(g, function dmact )
    set g = GetUnitsInRectMatching(bj_mapInitialPlayableArea,Condition(function defilter))
    call ForGroup(g, function deact )
    set g = null
endfunction

//===========================================================================
private function I takes nothing returns nothing
    local trigger t = CreateTrigger(  )
    call TriggerRegisterGameStateEventTimeOfDay( t, GREATER_THAN_OR_EQUAL, 18.00 )
    call TriggerRegisterGameStateEventTimeOfDay( t, LESS_THAN, 6.00 )
    call TriggerAddAction( t, function A )
endfunction

endscope

 

[Marcelo Hossomi]

I'd suggest you two things:

1. The groups you create at function A. You should:
- Use GroupUtils to recycle those groups
- (Someone will kill me) destroy g before creating a new one.

2. Repeated GetEnumUnit() in the action functions. Use a local variable to store it, use the variable instead, and null it in the end.

Maybe using ForGroup() function is not recommended too, but I'll have to leave this one to someone more experienced...

 

[Shadow Daemon]

I prefer this code:

scope Night initializer I

globals
  private group gr = CreateGroup()
  private boolexpr agilnact = null
  private boolexpr agildact = null
  private boolexpr nsact    = null
  private boolexpr dsact    = null
  private boolexpr dmact    = null
  private boolexpr deact    = null
endglobals

private function agilnact takes nothing returns boolean
  local unit u = GetFilterUnit()
  if GetUnitAbilityLevel(u, 'A00E') > 0 then
    call UnitRemoveAbility  (u, 'A005')
    call UnitAddAbility     (u, 'A002')
    call SetUnitAbilityLevel(u, 'A002', GetUnitAbilityLevel(u, 'A00E'))
    call UnitAddAbility     (u, 'A00I')
    call SetUnitAbilityLevel(u, 'A00I', GetUnitAbilityLevel(u, 'A00E'))
  endif
  set u = null
  return false
endfunction

private function agildact takes nothing returns boolean
  local unit u = GetFilterUnit()
  if GetUnitAbilityLevel(u, 'A00D') > 0 then
    call UnitRemoveAbility  (u, 'A004')
    call UnitAddAbility     (u, 'A006')
    call SetUnitAbilityLevel(u, 'A006', GetUnitAbilityLevel(u, 'A00D'))
    call UnitRemoveAbility  (u, 'A00I')
  endif
  set u = null
  return false
endfunction

private function nsact takes nothing returns boolean
  local unit u = GetFilterUnit()
  if GetUnitAbilityLevel(u, 'A00B') > 0 then
    call UnitRemoveAbility  (u, 'A007')
    call UnitAddAbility     (u, 'A008')
    call SetUnitAbilityLevel(u, 'A008', GetUnitAbilityLevel(u, 'A00B'))
  endif
  set u = null
  return false
endfunction

private function dsact takes nothing returns boolean
  local unit u = GetFilterUnit()
  if GetUnitAbilityLevel(u, 'A00F') > 0 then
    call UnitRemoveAbility  (u, 'A00A')
    call UnitAddAbility     (u, 'A009')
    call SetUnitAbilityLevel(u, 'A009', GetUnitAbilityLevel(u, 'A00F'))
  endif
  set u = null
  return false
endfunction

private function dmact takes nothing returns boolean
  local unit u = GetFilterUnit()
  if GetUnitAbilityLevel(u, 'A00P') > 0 then
    call UnitRemoveAbility  (u, 'A00N')
    call UnitAddAbility     (u, 'A00O')
    call SetUnitAbilityLevel(u, 'A00O', GetUnitAbilityLevel(u, 'A00P'))
  endif
  set u = null
  return false
endfunction

private function deact takes nothing returns boolean
  local unit u = GetFilterUnit()
  if GetUnitAbilityLevel(u, 'A00Q') > 0 then
    call UnitRemoveAbility  (u, 'A00O')
    call UnitAddAbility     (u, 'A00S')
    call SetUnitAbilityLevel(u, 'A00S', GetUnitAbilityLevel(u, 'A00Q'))
  endif
  set u = null
  return false
endfunction

private function A takes nothing returns nothing
  call GroupEnumUnitsInRect(gr, bj_mapInitialPlayableArea, agiln_bx)
  call GroupEnumUnitsInRect(gr, bj_mapInitialPlayableArea, agild_bx)
  call GroupEnumUnitsInRect(gr, bj_mapInitialPlayableArea, ns_bx)
  call GroupEnumUnitsInRect(gr, bj_mapInitialPlayableArea, ds_bx)
  call GroupEnumUnitsInRect(gr, bj_mapInitialPlayableArea, dm_bx)
  call GroupEnumUnitsInRect(gr, bj_mapInitialPlayableArea, de_bx)
endfunction

//===========================================================================
private function I takes nothing returns nothing
  local trigger t = CreateTrigger()
  call TriggerRegisterGameStateEvent(t, GAME_STATE_TIME_OF_DAY, GREATER_THAN_OR_EQUAL, 18.00)
  call TriggerRegisterGameStateEvent(t, GAME_STATE_TIME_OF_DAY, LESS_THAN,             6.00)
  call TriggerAddAction(t, function A)
  set agiln_bx = Condition(function agilnact)
  set agild_bx = Condition(function agildact)
  set ns_bx    = Condition(function nsact)
  set ds_bx    = Condition(function dsact)
  set dm_bx    = Condition(function dmact)
  set de_bx    = Condition(function deact)
endfunction

endscope

We are just doing all needed actions while picking units.
I'd also recommend to take out abilities' rawcodes into global constants, like that:

globals
  private constant integer ID_SPEED_BUFF = 'A123'
endglobals

to make code more readable and easier to edit.

 

[SanKakU]

that looks very interesting, got a question though. what's the _bx for? is that supposed to be act? also where should i put the global constants, is this triggger fine or should i put them elsewhere. i'm afraid i don't know near as much about this thing called jass as i'd like. i've seen that sort of thing before though so i understand the concept behind using it. now that it's explained in an example like this one it makes a lot of sense. all i need now is to figure out where the global constants would belong about that.

 

[Shadow Daemon]

"_bx" is just suffix to variable names. Boolexpr variables are used to save some time and not to call Condition(.....) function every time the trigger fires. They are like for quick access to evaluation functions.

 

[SanKakU]

hmm...well i'm not sure entirely that i get it yet...you made some globals at the top...and you reference them at the initialization right? but at the top it's not _bx...

i mean, you can't set a variable that isn't declared, right?
set agiln_bx = Condition(function agilnact)

compare it with this one.
private boolexpr agilnact = null

did you mean that putting in agiln_bx it automatically does agilnact because _bx substitutes for act? if so, that's pretty wild.

actually, looking at the spell you proposed again...isn't it a bad idea to do what you're doing? looks like you're grouping every unit in the map into a group. that would be laggy, wouldn't it?

oh... is the part that says return false the part that is key to this? you hit the if and then when it returns true from the if then...yeah wouldn't it have to say else return false rather than endif return false? that or add return true before the endif?

 

[Shadow Daemon]

Okay, let's say I think Condition(function agilnact) is leaking. So the code would be:

private function agilnact takes nothing returns boolean
  // . . .
endfunction

private function A takes nothing returns nothing
  local boolexpr bx = Condition(function agilnact)
  call GroupEnumUnitsInRect(gr, bj_mapInitialPlayableArea, bx)
  call DestroyBoolExpr(bx)
  set bx = null
endfunction

This code would also work, but would be slower, than simple call GroupEnumUnitsInRect(gr, bj_mapInitialPlayableArea, Condition(function agilnact)).

But destroying boolexprs is useless, because, as far as I know, they are precached. I mean, that Condition(function agilnact) first call would create boolean expression in memory, later Condition(function agilnact) calls would just refer to that expression.

So we have to make some global variables, that needed to be filled on initialization with constant values (Condition(function agilnact) wouldn't change, would it?), and then just to use these values as group picking handlers.

By the way, it is the fastest group picking work. GroupEnum***() in this case is picking unit, checking it for conditions and doing some actions (instead of checking only). A lot faster, than picking units matching condition, and then doing some actions to them.

return false returns negative result, so current picked unit would not be added to group.

 

[SanKakU]

so...in other words this code you wrote won't select all the units on the map? ok, that's fine, then. i mean anyway yeah it's pretty much only going to select one unit anyway.