[JASS] Trouble with groups

[Nowow]

In order to do map stuff I'm trying to create a creep summoning system which works like this:

Each time a periodic timer ends, a group of creeps spawn in a certain location. The creeps are supposed to patrol though some locations of the map, and to avoid cluttering I limited them to 3 patrol groups. The system checks to see if the patrol groups are empty, and if no group is empty it will create smaller groups that will attack the player directly. If there is a vacant patrol group the newly created units will be assigned to it and will be ordered to their starting location.

The problem is all units accumulate to one group instead of being assigned to a different group.

function Attack_Squad takes rect r, integer i returns group //Attack Squad creation function
   local location tempPoint = GetRectCenter(r)
   
   if i <= 2 then
        call CreateUnitAtLoc(Player(4), 'uske', tempPoint, 90.0)
       call CreateUnitAtLoc(Player(4), 'uske', tempPoint, 90.0)
       
    elseif i == 3 then
       call CreateUnitAtLoc(Player(4), 'uske', tempPoint, 90.0)
       call CreateUnitAtLoc(Player(4), 'n002', tempPoint, 90.0)
       call CreateUnitAtLoc(Player(4), 'n002', tempPoint, 90.0)
           
    elseif i == 4 then
       call CreateUnitAtLoc(Player(4), 'n003', tempPoint, 90.0)
           
    else
        call BJDebugMsg("Faulty switch result: attack group selector")
    endif

   //Cleanup
   call RemoveLocation(tempPoint)
   set tempPoint = null
   return GetUnitsInRectOfPlayer(r, Player(4))
endfunction

function Attack_Pattern takes nothing returns nothing //Attack Squad order function
   call IssueTargetOrderBJ( GetEnumUnit(), "attack", gg_unit_H002_0107 )
endfunction

function Patrol_Squad takes rect r, integer i returns group //Patrol Squad creation
   local location tempPoint = GetRectCenter(r)

       if i == 1 then
           call CreateUnitAtLoc(Player(4), 'uske', tempPoint, 90.0)
           call CreateUnitAtLoc(Player(4), 'uske', tempPoint, 90.0)
           call CreateUnitAtLoc(Player(4), 'uske', tempPoint, 90.0)

       elseif i == 2 then
           call CreateUnitAtLoc(Player(4), 'uske', tempPoint, 90.0)
           call CreateUnitAtLoc(Player(4), 'uske', tempPoint, 90.0)
           call CreateUnitAtLoc(Player(4), 'n002', tempPoint, 90.0)
           call CreateUnitAtLoc(Player(4), 'n002', tempPoint, 90.0)

        elseif i == 3 then
           call CreateUnitAtLoc(Player(4), 'uske', tempPoint, 90.0)
           call CreateUnitAtLoc(Player(4), 'uske', tempPoint, 90.0)
           call CreateUnitAtLoc(Player(4), 'n003', tempPoint, 90.0)

       elseif i == 4 then
           call CreateUnitAtLoc(Player(4), 'uske', tempPoint, 90.0)
           call CreateUnitAtLoc(Player(4), 'uske', tempPoint, 90.0)
           call CreateUnitAtLoc(Player(4), 'n002', tempPoint, 90.0)
           call CreateUnitAtLoc(Player(4), 'n004', tempPoint, 90.0)

       else
           call BJDebugMsg("Faulty switch result: patrol group selector")   
       endif

   //Cleanup
   call RemoveLocation(tempPoint)
   set tempPoint = null
   return GetUnitsInRectOfPlayer(r, Player(4))
endfunction

function Patrol_Check takes nothing returns integer
//This function checks which of the patrol groups are empty, with accordence to their priority.
//If tests are correct, skeletons are removed from groups shortly after their death.
    if (FirstOfGroup(udg_patrolRoad) == null) then
        return 1
    elseif (FirstOfGroup(udg_patrolForest1) == null) then
        return 2
    elseif (FirstOfGroup(udg_patrolForest2) == null) then
        return 3
    else
        return 4
    endif
endfunction

function Spawn_System takes nothing returns nothing
   //Local variable decleration.
   local integer selector
   local rect downpourRect
   local group tempGroup
   
   //Generates the downpour point from which the units will be spawned.
   set selector = GetRandomInt(1,4)
   //call BJDebugMsg("Downpour point " + I2S(selector)) //Debug Statement

   if selector == 1 then
       set downpourRect = gg_rct_DownpourPoint1
   elseif selector == 2 then
       set downpourRect = gg_rct_DownpourPoint2
   elseif selector == 3 then
       set downpourRect = gg_rct_DownpourPoint3
   elseif selector == 4 then
       set downpourRect = gg_rct_DownpourPoint4
   else
       call BJDebugMsg("Faulty switch result: location selector")   
   endif

   set selector = GetRandomInt(1,4)
   //call BJDebugMsg("Group number " + I2S(selector)) //Debug Statement

   if (Patrol_Check() == 4) then
//If no patrol group is empty, the units will instead be sent to attack the captain.
//If sent to another unit they might kill it then be stuck there not furthering the gameplay mechanic.
       set tempGroup = Attack_Squad(downpourRect,selector)
       call ForGroupBJ(tempGroup, function Attack_Pattern)//Attack function
   
   else
//Patrol groups: temp group is set to global variable and the group is not removed, for future patrol orders.
//The unit group will be assigned to the proper global group then sent to their start location and receive further orders.
       if (Patrol_Check() == 1) then
           set udg_patrolRoad = Patrol_Squad(downpourRect,selector)
           call GroupPointOrder(udg_patrolRoad, "attack", 7150.0, 1530.0)

           call DestroyGroup(tempGroup)
           set downpourRect = null
           set tempGroup = null
            return
           
       elseif (Patrol_Check() == 2) then
           set udg_patrolForest1 = Patrol_Squad(downpourRect,selector)
           call GroupPointOrder(udg_patrolForest1, "attack", -2950.0, -300.0)
           
           call DestroyGroup(tempGroup)
           set downpourRect = null
           set tempGroup = null
           return
           
       elseif (Patrol_Check() == 3) then
           set udg_patrolForest2 = Patrol_Squad(downpourRect,selector)
           call GroupPointOrder(udg_patrolForest2, "attack", -2950.0, -300.0)
           
           call DestroyGroup(tempGroup)
           set downpourRect = null
           set tempGroup = null
           return
           
       else
           call BJDebugMsg("Faulty switch result: patrol assignment")       
       endif
   endif
   
   call DestroyGroup(tempGroup)
   set downpourRect = null
   set tempGroup = null
endfunction

//=====  SPAWN SYSTEM END  =====\\

function Spawn_Timer takes nothing returns nothing
    set udg_spawnTimer = CreateTimer()
    call TimerStart(udg_spawnTimer, 15.0, true, function Spawn_System)
endfunction

//===========================================================================
function InitTrig_Spawn_system takes nothing returns nothing
    set gg_trg_Spawn_system = CreateTrigger(  )
    call TriggerAddAction( gg_trg_Spawn_system, function Spawn_Timer)
endfunction

I know, the trigger isn't optimized at all and I use BJ functions and I should be burned at the stake as a witch and all. You can say that I know very little about JASS so all criticism and advice is welcome.

 

[Emm-A-]

can you check, in which group they end up?

Also a comment to FirstOfGroup().
It is kind of unsafe to use, as it seems to empty groups by picking units from them. This may not be true anymore, but I have read it several times and had problems of that kind in a map of mine as well. So I suggest to never use it with groups, that are meant to be stable for longer.

Please correct me, if this is outdated/wrong!

 

[Pyrogasm]

Why do you set selector = GetRandomInt(1,4) twice in Spawn_System? You can use ForGroup(), IssueTargetOrder(), GroupEnumUnitsInRect(), etc.. Generally speaking a BJ is just a wrapper for a native with the same arguments in a different order. Not all BJs are bad, though; for example BJDebugMsg() is very useful. You may find these helpful:

Common.j functions
Common.j source
Blizzard.j functions
Blizzard.j source

Instead of using locations and their associated function (GetRectCenterLoc(), SetUnitLoc(), CreateUnitLoc()...) you should just use XY coordinates directly. They are faster, easier to transform, and are not handles. This applies to a most of your code, but for a specific example here's Attack_Squad:

function Attack_Squad takes rect r, integer i returns group //Attack Squad creation function
    local real x = GetRectCenterX(r)
    local real Y = GetRectCenterY(r)
    local player p = Player(4) //saving function calls
    local group g = CreateGroup()
  
    if i <= 2 then
        call GroupAddUnit(g, CreateUnit(p, 'uske', x, y, 90.0))
        call GroupAddUnit(g, CreateUnit(p, 'uske', x, y, 90.0))
      
    elseif i == 3 then
        call GroupAddUnit(g, CreateUnit(p, 'uske', x, y, 90.0))
        call GroupAddUnit(g, CreateUnit(p, 'n002', x, y, 90.0))
        call GroupAddUnit(g, CreateUnit(p, 'n002', x, y, 90.0))
          
    elseif i == 4 then
        call GroupAddUnit(g, CreateUnit(p, 'n003', x, y, 90.0))
          
    else
         call BJDebugMsg("Faulty switch result: attack group selector")
    endif

   //Cleanup
   set p = null
   return g
endfunction

You're already using GroupPointOrder(), and you can replace call ForGroupBJ(tempGroup, function Attack_Pattern) (and the Attack_Pattern function entirely) with call GroupTargetOrder(tempGroup, "attack", gg_unit_H002_0107).

As far as I can tell the problem with the groups probably comes from the Spawn_System function destroying and nulling things willy-nilly. The whole function can be simplified and you don't need to use TempGroups at all:

function Spawn_System takes nothing returns nothing
    //Local variable decleration.
    local integer selector
    local integer patcheck
    local rect downpourRect
  
    set selector = GetRandomInt(1,4)
    set patcheck = Patrol_Check()

    if selector == 1 then
        set downpourRect = gg_rct_DownpourPoint1
    elseif selector == 2 then
        set downpourRect = gg_rct_DownpourPoint2
    elseif selector == 3 then
        set downpourRect = gg_rct_DownpourPoint3
    elseif selector == 4 then
        set downpourRect = gg_rct_DownpourPoint4
    else
        call BJDebugMsg("Faulty switch result: location selector")  
    endif

    // ??? set selector = GetRandomInt(1,4)

    if (patcheck == 4) then
        call GroupTargetOrder(Attack_Squad(downpourRect,selector), "attack", gg_unit_H002_0107)
   
    elseif (patcheck == 1) then
        set udg_patrolRoad = Patrol_Squad(downpourRect,selector)
        call GroupPointOrder(udg_patrolRoad, "attack", 7150.0, 1530.0)
          
    elseif (patcheck == 2) then
        set udg_patrolForest1 = Patrol_Squad(downpourRect,selector)
        call GroupPointOrder(udg_patrolForest1, "attack", -2950.0, -300.0)
          
    elseif (patcheck == 3) then
        set udg_patrolForest2 = Patrol_Squad(downpourRect,selector)
        call GroupPointOrder(udg_patrolForest2, "attack", -2950.0, -300.0)
          
    else
        call BJDebugMsg("Faulty switch result: patrol assignment")     
    endif
  
    set downpourRect = null
endfunction

Try with these changes and see how it works.

 

[Wietlol]

Also a comment to FirstOfGroup().
It is kind of unsafe to use, as it seems to empty groups by picking units from them. This may not be true anymore, but I have read it several times and had problems of that kind in a map of mine as well. So I suggest to never use it with groups, that are meant to be stable for longer.

Not really true.

FirstOfGroup() simply returns the first entry of a group.
If you then remove that unit, it will essentially clear the group, iterating over the units one by one.

loop
    set FoG = FirstOfGroup(g)
    exitwhen FoG == null
    call GroupRemoveUnit(g, FoG)
    // iteration
endloop

aka, the famous FirstOfGroup iteration.
(Its real use is that it is much faster than ForGroup() or the filter in the Enum.)

The dangerous part however is that when a unit is removed from the game, the entry in the group is not cleared.
ForGroup() may still iterate over that entry, resulting in a null for PickedUnit(), but it will complete the loop over the entire group.
FirstOfGroup iteration will break at that point.
A way to solve this is by either removing units from the group whenever they are removed from the game or to use the ForGroup iteration to clean the group form null entries.
Hence why FirstOfGroup iterations ussually are only used in groups that are created on the spot and not on groups that already existed for x time.

 

[DracoL1ch]

Hence why FirstOfGroup iterations ussually are only used in groups that are created on the spot and not on groups that already existed for x time.

actually how 'bout simply not using buggy "RemoveUnit" at all? that suddenly fix whole this sitution, and there's very few cases when you would like to really remove unit

 

[KILLCIDE]

there's very few cases when you would like to really remove unit

I can't ever imagine a situation where RemoveUnit is ever good other than a map having some stupid black hole mechanic that removes units from play.

 

[DracoL1ch]

I can't ever imagine a situation where RemoveUnit is ever good other than a map having some stupid black hole mechanic that removes units from play.

to get rid of dead hero icon for instance

 

[KILLCIDE]

The more you know ¯\_(ツ)_/¯

 

[Troll-Brain]

Talking about the FOG loop that can be broken :

I don't think RemoveUnit is the culprit here, or is it ?

I mean that should be the same with an unit removed of the game by the game engine itself, let's say the end of decay.

 

[DracoL1ch]

I mean that should be the same with an unit removed of the game by the game engine itself, let's say the end of decay.

indeed, had a story when illusions been kept "alive" after their death just because ghost links in the group

 

[Nowow]

The dangerous part however is that when a unit is removed from the game, the entry in the group is not cleared.
ForGroup() may still iterate over that entry, resulting in a null for PickedUnit(), but it will complete the loop over the entire group.
FirstOfGroup iteration will break at that point.

So what you're saying is that units which are already dead and decayed are not removed from the group? I always thought that FoG == null means that the group itself is empty.
So what happens to those null group entries, memorywise, when I set the group to a new set of units? Are they flushed completely or is there that pesky handle leak (the 0.04kb thingy. that's how it's called, right?) to worry about?

 

[Wietlol]

I don't think RemoveUnit is the culprit here, or is it ?

Indeed, when a unit dies or whatever, it starts to decay, and eventually be removed from the game.
If there are still pointers pointing at it, the handle will not be recycled, but the unit is still a null reference at that moment.

Anyway, I think there is another function that checks if said group is empty.

So what you're saying is that units which are already dead and decayed are not removed from the group? I always thought that FoG == null means that the group itself is empty.
So what happens to those null group entries, memorywise, when I set the group to a new set of units? Are they flushed completely or is there that pesky handle leak (the 0.04kb thingy. that's how it's called, right?) to worry about?

It essentially means that the first entry is pointing to null.
Wether that is because the first entry is outside of the group bounds and returns a default value of null, or if the actual entry is null, that is something you do not know.

 

[Dr Super Good]

but the unit is still a null reference at that moment.

It is not a null reference, it just operates like null in most natives. Comparing a removed unit handle to null returned false last time a checked.

Units are not removed from groups automatically, which can be a potential leak source as well as degrading performance of operations on the group. I believe first of group loops can have problems with null units as FirstOfGroup returns null if the unit in the group is not valid anymore. However ForGroup loops do not, and will only run the function on existing units. One can use a ForGroup loop to sanitize a group by adding all picked units to another clean group. Clearing a group with the appropriate native will return it to default state, removing all units from it both existing and not as well as resetting operation complexity to initial values.

The problem is all units accumulate to one group instead of being assigned to a different group.

In the original Attack_Squad trigger there is no guarantee that the group returned contained all created units as well as only created units. This is because unit creation is subject to displacement and so it is possible the units were created outside the intended rect and that other units belonging to the player but not part of the wave may occupy the rect. Solution is to explicitly add every created unit to a group instead of trying to guess them all.

Same issue with Patrol_Squad. Must explicitly add created units to the group instead of trying to guess which units were created.

Patrol_Check probably bugs due to the groups never being sanitized of dead units. Hence why all units end up going to patrol 1 as it forever thinks patrol 1 is dead. Counting the units in the group with a simple ForGroup loop is probably the most reliable way, although is slightly wasteful (but still trivial).

DestroyGroup is called on tempGroup even though it may not have a value. This would cause a thread crash and hence handle leaks due to the local variable nulls not running below it. Only use DestroyGroup on a variable with a non null and non destroyed group, anything else is wasteful and almost certainly avoidable with more sensible object life cycle management.