[JASS] ForGroupBJ and GetUnitsOfTypeIdAll do not work with bj_wantDestroyGroup

[Barade]

Hi,
for my new spell I simply wanted to add all units of type Footman as casters:

• Actions
  • -------- Casters --------
  • Custom script: set bj_wantDestroyGroup = true
  • Unit Group - Pick every unit in (Units of type Footman) and do (Actions)
    • Loop - Actions
      • Custom script: call RewindTime_AddCaster(GetEnumUnit())

This generates something like:

set bj_wantDestroyGroup = true
call ForGroupBJ( GetUnitsOfTypeIdAll('hfoo'), function Trig_Game_Start_Func014A )

I am setting the cleanup flag bj_wantDestroyGroup for ForGroupBJ to avoid a leak of the group from GetUnitsOfTypeIdAll. However this does not work with GetUnitsOfTypeIdAll.
If you look at the function, it uses GroupAddGroup which then will destroy the source group:

function GetUnitsOfTypeIdAll takes integer unitid returns group
    local group   result = CreateGroup()
    local group   g      = CreateGroup()
    local integer index

    set index = 0
    loop
        set bj_groupEnumTypeId = unitid
        call GroupClear(g)
        call GroupEnumUnitsOfPlayer(g, Player(index), filterGetUnitsOfTypeIdAll)
        call GroupAddGroup(g, result)

        set index = index + 1
        exitwhen index == bj_MAX_PLAYER_SLOTS
    endloop
    call DestroyGroup(g)

    return result
endfunction

The source group g being destroyed, everything after the first player will be bugged right which explains that only player 1 units have been added.
This is pretty annoying.
Is there any other way to avoid the leak without using a temporary group? Is this a known issue?

 

[MyPad]

One way to avoid this issue is to filter out the Footman unit type inside the For Group Actions block itself.

• Actions
  • -------- Casters --------
  • Custom script: set bj_wantDestroyGroup = true
  • Unit Group - Pick every unit in (Units in (Playable Map Area)) and do (Actions)
    • Loop - Actions
      • If - If (((Unit-type of (Picked unit)) Equal to Footman) Then do (Custom script: call RewindTime_AddCaster(GetEnumUnit())), Else do (Do Nothing)

 

[Barade]

Is the performance the same? I thought filters might be a bit faster. My main issue is that you can't really use GetUnitsOfTypeIdAll without leaks anymore.
For example, if I have a global variable with a group and clear it before and than use add group to group with GetUnitsOfTypeIdAll, bj_wantDestroyGroup will always have the wrong effect :/. It should have been documented in the trigger action or somewhere.

Any workaround is easy to write, it is just annoying that the standard function has this issue. The function could simply not use a loop and instead of GroupEnumUnitsOfPlayer something like GroupEnumUnitsInRect only once. GroupEnumUnitsOfType uses a name, so there is no native with an ID?

I am just asking myself what would be the fastest way. Maybe the implementation with GroupEnumUnitsOfPlayer per player is somehow faster than GroupEnumUnitsInRec?

 

[Pyrogasm]

It should have been documented in the trigger action or somewhere.

The existence of bj_wantDestroyGroup and how to use it is not documented anywhere in any GUI function aside from the canned line "If the user wants the group destroyed, remember that fact and clear the flag, in case it is used again in the callback.". Your suggestion would actually be the outlier. That it exists at all and works when using most BJ functions is as good as it's ever going to get. Abusing it was never something Blizzard really intended mapmakers to do.

My main issue is that you can't really use GetUnitsOfTypeIdAll without leaks anymore.

I don't understand why you think this. Just... assign the group to a variable and clean it up manually when you're done.

• Set GVar = (Units of type Footmen)
• Unit Group - Pick every unit in GVar and do (Actions)
  • Loop - Actions
    • -------- --------
• Custom script: call DestroyGroup(udg_GVar)

Is the performance the same?

This question is almost meaningless when you are using a bunch of BJ functions (glorified wrapper functions that occasionally have other bonus bits of functionality thrown in) already in your code. Yes, using filterfuncs is preferable when you can.

 

[MyPad]

• Actions
  • -------- Casters --------
  • Custom script: set bj_wantDestroyGroup = true
  • Unit Group - Pick every unit in (Units in (Playable Map Area)) and do (Actions)
    • Loop - Actions
      • If - If (((Unit-type of (Picked unit)) Equal to Footman) Then do (Custom script: call RewindTime_AddCaster(GetEnumUnit())), Else do (Do Nothing)

1.) The performance of the above snippet should be similar, if not faster due to avoiding the repetitive execution of Filter functions. Regardless, if it doesn't execute all too frequently, performance shouldn't matter too much.

2.) There exists no ID equivalent for GroupEnumUnitsOfType.

3.) If I were to go for the most appropriate (not necessarily the fastest) approach for getting all footmen, I'd use GroupEnumUnitsInRect, then manually filter out the desired unit type and perform all relevant actions on them, but at that point, I would be better off coding it entirely in JASS.

---

Spoiler: Code Dissection

Regardless of that, I've found out what causes the block of code above to misbehave. Inspecting Blizzard.j, we get the following:

function GroupAddGroup takes group sourceGroup, group destGroup returns nothing
    // If the user wants the group destroyed, remember that fact and clear
    // the flag, in case it is used again in the callback.
    local boolean wantDestroy = bj_wantDestroyGroup
    set bj_wantDestroyGroup = false
    set bj_groupAddGroupDest = destGroup
    call ForGroup(sourceGroup, function GroupAddGroupEnum)
    // If the user wants the group destroyed, do so now.
    if (wantDestroy) then
        call DestroyGroup(sourceGroup)
    endif
endfunction

function GetUnitsOfTypeIdAll takes integer unitid returns group
    local group   result = CreateGroup()
    local group   g      = CreateGroup()
    local integer index
    set index = 0
    loop
        set bj_groupEnumTypeId = unitid
        call GroupClear(g)
        call GroupEnumUnitsOfPlayer(g, Player(index), filterGetUnitsOfTypeIdAll)
        call GroupAddGroup(g, result)
        set index = index + 1
        exitwhen index == bj_MAX_PLAYER_SLOTS
    endloop
    call DestroyGroup(g)
    return result
endfunction

function ForGroupBJ takes group whichGroup, code callback returns nothing
    // If the user wants the group destroyed, remember that fact and clear
    // the flag, in case it is used again in the callback.
    local boolean wantDestroy = bj_wantDestroyGroup
    set bj_wantDestroyGroup = false
    call ForGroup(whichGroup, callback)
    // If the user wants the group destroyed, do so now.
    if (wantDestroy) then
        call DestroyGroup(whichGroup)
    endif
endfunction

As it turns out, bj_wantDestroyGroup is overwritten in the function GroupAddGroup. That means by the time the function ForGroupBJ is actually called, bj_wantDestroyGroup would always be false.

To show how the trigger block would look like in JASS (Note: The name of the callback function is not similar).

// In the actual trigger editor, this would be the trigger name plus some arbitrary number.
// Also, a voiceline from Necromancers
function RightClickForHotUndeadAction takes nothing returns nothing
    call RewindTime_AddCaster(GetEnumUnit())
endfunction

// function header
set bj_wantDestroyGroup = true
call ForGroupBJ(GetUnitsOfTypeByIdAll('hfoo'), function RightClickForHotUndeadAction)
// function footer

and the flow of the code...

// function header ...
// Assume we have a local group variable
local group enumGroup
  
// The starting point of the block of code
set bj_wantDestroyGroup = true

// GetUnitsOfTypeByIdAll('hfoo')
set enumGroup = CreateGroup()

local group   g       = CreateGroup()
local integer index = 0
loop
    set bj_groupEnumTypeId = 'hfoo'
    call GroupClear(g)  // Blizzard being Blizzard.
    call GroupEnumUnitsOfPlayer(g, Player(index), filterGetUnitsOfTypeIdAll)

    // GroupAddGroup(g, enumGroup)
    // Remember that bj_wantDestroyGroup == true
    local boolean wantDestroy = bj_wantDestroyGroup
    set bj_wantDestroyGroup = false
    set bj_groupAddGroupDest = enumGroup
    call ForGroup(g, function GroupAddGroupEnum)
    if (wantDestroy) then
        // This block is unintentionally executed.
        call DestroyGroup(g)
    endif

    // At the first iteration, the dummy group "g" would have already been destroyed,
    // making it useless for subsequent enumerations.
    set index = index + 1
    exitwhen index == bj_MAX_PLAYER_SLOTS
endloop
call DestroyGroup(g)

// By this point, bj_wantDestroyGroup has been overwritten.
call ForGroupBJ(enumGroup, function RightClickForHotUndeadAction)
// Since the enumGroup handle cannot be reached at this point, it cannot be manually destroyed.
// Moreover, what should've destroyed it didn't, because bj_wantDestroyGroup was false at the
// point when ForGroupBJ was called.

Edit:
1. Elaborated more on the flow of the code, unwrapping and exposing the function calls.
2. Tucked away the latter chunk of my post in a spoiler, should it prove useful to future users.
Also, edited the former chunk to better answer the follow-up questions asked.

 

[Barade]

thx. I like to use bj_wantDestroyGroup with GUI triggers since I don't need a global variable and no DestroyGroup call but I think it's clear now which possibilities I have.

 

[IcemanBo]

Might think to use bj_lastCreatedGroup instead of bj_wantDestroyGroup if you don't want create new variable.