[JASS] Inefficient trigger?

[DistantHaze]

The following trigger has the "smell" of inefficiency, but I can't seem to pin it down. Any suggestions?

function EliteMonsterCreator takes unit creep_unit, integer PointsTotal returns nothing
    local integer PointsPossible
    local integer PointsCurrent


    set udg_NumberOfElites = udg_NumberOfElites + 1
    call SetUnitUserData (creep_unit, 100)
    call SetUnitColor (creep_unit, PLAYER_COLOR_RED)
    call SetUnitVertexColor (creep_unit, 150, 150, 150, 255)
    call SetUnitScalePercent (creep_unit, 140.00, 140.00, 140.00)
    call GroupAddUnit (udg_EliteGroup, creep_unit)


//  Elite Monster Resistances

    if PointsTotal > 4 then
        set PointsPossible = 4
    else
        set PointsPossible = PointsTotal
    endif
    set PointsCurrent = GetRandomInt (0, PointsPossible)
    set PointsTotal = PointsTotal - PointsCurrent
    call UnitAddAbility (creep_unit, udg_EliteMonsterStat [PointsCurrent])


//  Elite Monster Damage

    if PointsTotal > 5 then
        set PointsPossible = 5
    else
        set PointsPossible = PointsTotal
    endif
    set PointsCurrent = GetRandomInt (5, PointsPossible + 5)
    call UnitAddAbility (creep_unit, udg_EliteMonsterStat [PointsCurrent])


//  Elite Monster Life

    if PointsTotal > 8 then
        set PointsPossible = 8
    else
        set PointsPossible = PointsTotal
    endif
    set PointsCurrent = GetRandomInt (11, PointsPossible + 11)
    call UnitAddAbility (creep_unit, udg_EliteMonsterStat [PointsCurrent])


//  Elite Ability

    if GetRandomInt (1, 4) == 4 then
        call UnitAddAbility (creep_unit, 'A084')
        if GetRandomInt (1, 4) == 4 then
            call UnitAddAbility (creep_unit, 'A084')
        endif
    endif
    if GetRandomInt (1, 10) == 10 then
        call UnitAddAbility (creep_unit, 'Apig')
    endif
endfunction


function CreepsCreation_Action01 takes nothing returns nothing
    local integer n_LQualX25         = udg_LevelQualifier * 25
    local integer n_CaveCount        = udg_NumberOfCaveMonsters    
    local integer n_HutNumb          = udg_HutNumber
    local integer n_HutNumb_max      = n_HutNumb + 2
    local integer n_MonsterCap       = udg_MonsterQuantity
    local integer n_EliteCount       = CountUnitsInGroup (udg_EliteGroup)
    local integer n_TrollCount
    local unit creep_spawn
    local real creep_spawnX
    local real creep_spawnY
    local unit creep_unit
    local integer creep_unitE
    local integer R


//  Create Monsters (Cave)

    if n_CaveCount < 40 then
        set R = (GetRandomInt (0, n_LQualX25) / 5) + 200
        set creep_unit = CreateUnit (Player (12), udg_Monster [R], GetRandomReal (-3500.00, 5000.00), GetRandomReal (-11500.00, -10000.00), 0.00)
        call RemoveGuardPosition (creep_unit)

//      ** Create Elite Monster (Cave) **
        if GetRandomInt (1, 30) == 30 then
            set creep_unitE = GetUnitLevel (creep_unit)
            if creep_unitE > 24 and n_EliteCount < 5 then
                call EliteMonsterCreator (creep_unit, creep_unitE / 5)
            endif
        else
            set udg_NumberOfCaveMonsters = n_CaveCount + 1
            call SetUnitUserData (creep_unit, 80)
            call SetUnitVertexColor (creep_unit, 150, 150, 150, 255)
        endif
    endif


//  Create Monsters (Troll)

    loop
        exitwhen n_HutNumb == n_HutNumb_max
        set n_TrollCount = CountUnitsInGroup (udg_TrollGroup [n_HutNumb])
        if n_TrollCount < n_MonsterCap then
            set creep_spawn = udg_TrollHut [n_HutNumb]
            set creep_spawnX = GetUnitX (creep_spawn)
            set creep_spawnY = GetUnitY (creep_spawn)
            loop
                exitwhen n_TrollCount == n_MonsterCap
                set R = GetRandomInt (0, n_LQualX25) / 5
                set creep_unit = CreateUnit (Player (12), udg_Monster [R], creep_spawnX, creep_spawnY, 0.00)
                call SetUnitUserData (creep_unit, n_HutNumb)
                call RemoveGuardPosition (creep_unit)
                call SetUnitVertexColor (creep_unit, 150, 150, 150, 255)
                call GroupAddUnit (udg_TrollGroup [n_HutNumb], creep_unit)
                set n_TrollCount = n_TrollCount + 1
            endloop

//          ** Create Elite Monster (Troll) **
            if GetRandomInt (1, 50) == 50 then
                set creep_unitE = GetUnitLevel (creep_unit)
                if GetUnitLevel (creep_unit) > 24 and n_EliteCount < 5 then
                    call EliteMonsterCreator (creep_unit, creep_unitE / 5)
                endif
            endif
        endif
        if n_HutNumb == 4080 then
            set n_HutNumb = 4001
            set n_HutNumb_max = 4001
        else
            set n_HutNumb = n_HutNumb + 1
        endif
    endloop
    set udg_HutNumber = n_HutNumb

    set creep_unit = null
    set creep_spawn = null
endfunction

function InitTrig_CreepsCreation takes nothing returns nothing
    set gg_trg_CreepsCreation = CreateTrigger ()
    call TriggerAddAction (gg_trg_CreepsCreation, function CreepsCreation_Action01)
endfunction

 

[Arhowk]

whats the purpose of this

 

[mckill2009]

- No event...
- Uses SetUnitUserData, when it can conflict with other systems like UnitIndexer, use hashtables instead...
- Remove unit from the group when it dies...
- Avoid using BJ's, like SetUnitScalePercent >>> SetUnitScale...
- CountUnitsInGroup should be made simple by using ForGroup and a global integer...
- If this is a system, you should put configurables in a function on top...
- There are many but that's all for now...

 

[Arhowk]

also, why do youse plain functions and locals with prefixes? use structs, private functions, and private locals

 

[DistantHaze]

- 1. No event...
- 2. Uses SetUnitUserData, when it can conflict with other systems like UnitIndexer, use hashtables instead...
- 3. Remove unit from the group when it dies...
- 4. Avoid using BJ's, like SetUnitScalePercent >>> SetUnitScale...
- 5. CountUnitsInGroup should be made simple by using ForGroup and a global integer...
- 6. If this is a system, you should put configurables in a function on top...
- 7. There are many but that's all for now...

1. This trigger is ran periodically via another trigger, and thus needs no event.
2. UnitUserData is read when a unit dies; I am unfamiliar with UnitIndexer.
3. Units are removed from the unit group when they die.
4. Thank you; I shall change the ScalePercent function.
5. I could use ForGroup, etc., but that doesn't seem more efficient.
6. Please explain what you mean.
7. Feel free to go on with other examples.

 

[DistantHaze]

also, why do youse plain functions and locals with prefixes? use structs, private functions, and private locals

I don't know the vJass or NexGen coding.

 

[PurgeandFire]

Well, the only part that can be considered a bit heavy in terms of operations is the loop within the loop. However, it is pretty optimized so I don't think you need to fix that.

One optimization will save you a bit of performance and that is moving this:
set n_TrollCount = CountUnitsInGroup (udg_TrollGroup [n_HutNumb])
Out of the main loop. Since you already set n_TrollCount+1 when you add units to a group, you don't need to recalculate it every time.

Aside from that, further optimizations aren't super necessary. The code appears to work well and is pretty good in terms of optimizations. The main optimizations you could make would just lead to obfuscation and they wouldn't really have a noticeable effect anyway.

 

[DistantHaze]

One optimization will save you a bit of performance and that is moving this:
set n_TrollCount = CountUnitsInGroup (udg_TrollGroup [n_HutNumb])
Out of the main loop. Since you already set n_TrollCount+1 when you add units to a group, you don't need to recalculate it every time.

Well, the only problem is that CountUnitsInGroup (udg_TrollGroup [n_HutNumb]) is going to return a different value depending on which udg_TrollGroup is being counted, i.e. udg_TrollGroup [4001], udg_TrollGroup [4002], and udg_TrollGroup [4003]. Basically, each troll hut (valued from 4001 to 4080) will maintain up to [udg_MonsterQuantity] monsters at any one time.

 

[mckill2009]

2. UnitUserData is read when a unit dies; I am unfamiliar with UnitIndexer.
5. I could use ForGroup, etc., but that doesn't seem more efficient.
6. Please explain what you mean.

2) UnitIndexer sets the index(custom value) of a unit automatically, if you change it manually, then it will conflict the inidex...what if for example there are people who likes your system and uses UnitIndexer?...

5) CountUnitsInGroup makes useless calls including wantDestroyGroup, imo it should be like this...

function CountInGrp takes group g returns integer 
    local integer count = 0
    loop
        call GroupRemoveUnit(g, FirstOfGroup(g)) 
        exitwhen FirstOfGroup(g)==null
        set count = count + 1               
    endloop
    return count
endfunction

6) Sample;
this local integer n_LQualX25 = udg_LevelQualifier * 25
can be this...

function LQual takes integer level returns integer
   return level * 25
endfunction

 

[DistantHaze]

5) CountUnitsInGroup makes useless calls including wantDestroyGroup, imo it should be like this...

function CountInGrp takes group g returns integer 
    local integer count = 0
    loop
        call GroupRemoveUnit(g, FirstOfGroup(g)) 
        exitwhen FirstOfGroup(g)==null
        set count = count + 1               
    endloop
    return count
endfunction

Wouldn't this function actually empty the group, via GroupRemoveUnit ?

 

[shadowvzs]

Wouldn't this function actually empty the group, via GroupRemoveUnit ?

but its empty but locally, so only in that function but in main function where u call it, there dont will be changed until u dont use global unit group variable

 

[DistantHaze]

but its empty but locally, so only in that function but in main function where u call it, there dont will be changed until u dont use global unit group variable

So would the actual line be written something like the following?

set n_TrollCount = function CountInGrp (udg_TrollGroup [n_HutNumb])

 

[shadowvzs]

So would the actual line be written something like the following?

set n_TrollCount = function CountInGrp (udg_TrollGroup [n_HutNumb])

if trollcount is integer and nhutnumb group then yes

 

[mckill2009]

Yes, the global group will be empty as well, I purposely did that function to avoid unnecessary destroying of groups, if you want efficiency, then make your own function to reduce number of calls...

 

[PurgeandFire]

@DistantHaze: Ah I see, my mistake. Then you probably shouldn't worry too much about the efficiency as there isn't really anything you can do to the code to make a noticeable difference.

Do you actually notice speed loss or are you just concerned about the efficiency in general?

 

[DistantHaze]

@DistantHaze: Do you actually notice speed loss or are you just concerned about the efficiency in general?

Just concerned about overall efficiency; I'd like to get all my triggers as trim as possible.

 

[DistantHaze]

Well, here's what I have. Any other suggestions?

function EliteMonsterCreator takes unit creep_unit, integer PointsTotal returns nothing
    local integer PointsPossible
    local integer PointsCurrent


    set udg_NumberOfElites = udg_NumberOfElites + 1
    call SetUnitUserData (creep_unit, 100)
    call SetUnitColor (creep_unit, PLAYER_COLOR_RED)
    call SetUnitVertexColor (creep_unit, 150, 150, 150, 255)
    call SetUnitScale (creep_unit, 1.40, 1.40, 1.40)
    call GroupAddUnit (udg_EliteGroup, creep_unit)


//  Elite Monster Resistances

    if PointsTotal > 4 then
        set PointsPossible = 4
    else
        set PointsPossible = PointsTotal
    endif
    set PointsCurrent = GetRandomInt (0, PointsPossible)
    set PointsTotal = PointsTotal - PointsCurrent
    call UnitAddAbility (creep_unit, udg_EliteMonsterStat [PointsCurrent])


//  Elite Monster Damage

    if PointsTotal > 5 then
        set PointsPossible = 5
    else
        set PointsPossible = PointsTotal
    endif
    set PointsCurrent = GetRandomInt (5, PointsPossible + 5)
    call UnitAddAbility (creep_unit, udg_EliteMonsterStat [PointsCurrent])


//  Elite Monster Life

    if PointsTotal > 8 then
        set PointsPossible = 8
    else
        set PointsPossible = PointsTotal
    endif
    set PointsCurrent = GetRandomInt (11, PointsPossible + 11)
    call UnitAddAbility (creep_unit, udg_EliteMonsterStat [PointsCurrent])


//  Elite Ability

    if GetRandomInt (1, 4) == 4 then
        call UnitAddAbility (creep_unit, 'A084')
        if GetRandomInt (1, 4) == 4 then
            call UnitAddAbility (creep_unit, 'A084')
        endif
    endif
    if GetRandomInt (1, 10) == 10 then
        call UnitAddAbility (creep_unit, 'Apig')
    endif
endfunction

function CaveCounter takes nothing returns nothing
    set udg_CaveMonsterCount = udg_CaveMonsterCount +1
endfunction

function EliteCounter takes nothing returns nothing
    set udg_EliteMonsterCount = udg_EliteMonsterCount +1
endfunction

function TrollCounter takes nothing returns nothing
    set udg_TrollMonsterCount = udg_TrollMonsterCount +1
endfunction

function CreepsCreation_Action01 takes nothing returns nothing
    local integer n_LQual            = udg_LevelQualifier
    local integer n_LQualX25         = n_LQual * 25
    local integer n_CaveCount
    local integer n_HutNumb          = udg_HutNumber
    local integer n_HutNumb_max      = n_HutNumb + 2
    local integer n_MonsterCap       = udg_MonsterQuantity
    local integer n_EliteCount
    local integer n_TrollCount
    local unit creep_spawn
    local real creep_spawnX
    local real creep_spawnY
    local unit creep_unit
    local integer creep_unitE
    local integer R


//  Determine Number of Elite Monsters

    set udg_EliteMonsterCount = 0
    call ForGroup (udg_EliteGroup, function EliteCounter)
    set n_EliteCount = udg_EliteMonsterCount


//  Create Monsters (Cave)

    set udg_CaveMonsterCount = 0
    call ForGroup (udg_CaveGroup, function CaveCounter)
    set n_CaveCount = udg_CaveMonsterCount
    if n_CaveCount < 40 then
        set R = (GetRandomInt (0, n_LQualX25) / 5) + 200
        set creep_unit = CreateUnit (Player (12), udg_Monster [R], GetRandomReal (-3500.00, 5000.00), GetRandomReal (-11500.00, -10000.00), 0.00)
        call RemoveGuardPosition (creep_unit)

//      ** Create Elite Monster (Cave) **
        if GetRandomInt (1, 30) == 30 then
            set creep_unitE = GetUnitLevel (creep_unit)
            if creep_unitE > 24 and n_EliteCount < 5 then
                call EliteMonsterCreator (creep_unit, creep_unitE / 5)
            endif
        else
            call GroupAddUnit (udg_CaveGroup, creep_unit)
            call SetUnitUserData (creep_unit, 80)
            call SetUnitVertexColor (creep_unit, 150, 150, 150, 255)
        endif
    endif


//  Create Monsters (Troll)

    loop
        exitwhen n_HutNumb == n_HutNumb_max
        set udg_TrollMonsterCount = 0
        call ForGroup (udg_TrollGroup [n_HutNumb], function TrollCounter)
        set n_TrollCount = udg_TrollMonsterCount
        if n_TrollCount < n_MonsterCap then
            set creep_spawn = udg_TrollHut [n_HutNumb]
            set creep_spawnX = GetUnitX (creep_spawn)
            set creep_spawnY = GetUnitY (creep_spawn)
            loop
                exitwhen n_TrollCount == n_MonsterCap
                set R = GetRandomInt (0, n_LQualX25) / 5
                set creep_unit = CreateUnit (Player (12), udg_Monster [R], creep_spawnX, creep_spawnY, 0.00)
                call SetUnitUserData (creep_unit, n_HutNumb)
                call RemoveGuardPosition (creep_unit)
                call SetUnitVertexColor (creep_unit, 150, 150, 150, 255)
                call GroupAddUnit (udg_TrollGroup [n_HutNumb], creep_unit)
                set n_TrollCount = n_TrollCount + 1
            endloop

//          ** Create Elite Monster (Troll) **
            if GetRandomInt (1, 50) == 50 then
                set creep_unitE = GetUnitLevel (creep_unit)
                if GetUnitLevel (creep_unit) > 24 and n_EliteCount < 5 then
                    call EliteMonsterCreator (creep_unit, creep_unitE / 5)
                endif
            endif
        endif
        if n_HutNumb == 4080 then
            set n_HutNumb = 4001
            set n_HutNumb_max = 4001
        else
            set n_HutNumb = n_HutNumb + 1
        endif
    endloop
    set udg_HutNumber = n_HutNumb

    set creep_unit = null
    set creep_spawn = null
endfunction

function InitTrig_CreepsCreation takes nothing returns nothing
    set gg_trg_CreepsCreation = CreateTrigger ()
    call TriggerAddAction (gg_trg_CreepsCreation, function CreepsCreation_Action01)
endfunction