[JASS] Problem with boolexpr (or group)

[Arrkan]

I've made an ability that is supposed to damage enemy units in an AOE 5 times. However, the 4 last times my allied units (even structures) are damaged aswell, I don't see why as I haven't changed the condition, please explain.

function Trig_Fire_Conditions takes nothing returns boolean
return GetSpellAbilityId()=='A01Q'
endfunction

function burrn takes nothing returns boolean
return (GetWidgetLife(GetFilterUnit())>0) and (IsUnitEnemy(GetFilterUnit(),GetOwningPlayer(GetTriggerUnit()))) and (IsUnitType(GetFilterUnit(),UNIT_TYPE_MECHANICAL)==false)
endfunction

function Trig_Fire_Actions takes nothing returns nothing
local unit caster=GetTriggerUnit()
local location l1=GetUnitLoc(caster)
local location l2=GetSpellTargetLoc()
local integer lvl=GetUnitAbilityLevel(caster,'A01Q')
local real dmg=100
local integer iS=1
local boolexpr b=Condition(function burrn)
local group g
local integer iF
local unit array uUnits
local integer i=1
local integer times=5
call PolledWait((DistanceBetweenPoints(l1,l2)-90)/575)
loop
    exitwhen i>times
        set g=GetUnitsInRangeOfLocMatching(200,l2,b)
        set iF=CountUnitsInGroup(g)
        loop
            exitwhen iS>iF
                set uUnits[iS]=FirstOfGroup(g)
                call UnitDamageTarget(caster,uUnits[iS],dmg,false,false,ATTACK_TYPE_NORMAL,DAMAGE_TYPE_MAGIC,null)
                call GroupRemoveUnit(g,uUnits[iS])
                set uUnits[iS]=null
                set iS=iS+1
            endloop
    call PolledWait(0.1)
    call DestroyGroup(g)
    set iS=1
    set i=i+1
    endloop
call RemoveLocation(l1)
call RemoveLocation(l2)
call DestroyBoolExpr(b)
set l1=null
set l2=null
set b=null
set caster=null
set g=null
endfunction

//===========================================================================
function InitTrig_Fire takes nothing returns nothing
    set gg_trg_Fire = CreateTrigger(  )
    call TriggerRegisterAnyUnitEventBJ( gg_trg_Fire, EVENT_PLAYER_UNIT_SPELL_EFFECT )
    call TriggerAddCondition( gg_trg_Fire, Condition( function Trig_Fire_Conditions ) )
    call TriggerAddAction( gg_trg_Fire, function Trig_Fire_Actions )
endfunction

 

[PurplePoot]

Ugga ugga!

Eww...

First, youre never creating the Group (though you use GetUnitsInRangeOfLocMatching, BJ calls are no excuse )

Second, please use the magic [jass ] tag (without that space at the end)

Third, why are you cycling through an array inside your loop, instead of just using a single unit?

Fourth, PolledWait at low intervals is even MORE inaccurate than TriggerSleepAction

Fourth, locations are ugly, unless theyre needed (like GetSpellTargetLoc())

Fifth, start array indexes at 0

Sixth, when using boolexprs for groups, i suggest using Filter(x) instead of Condition(x)

Seventh, that CountUnitsInGroup call sucks

Eighth, in your condition you should use and not x instead of and x = false

Ninth, there is no reason to use the 'times' variable

Tenth, BJ calls (like GetUnitsInRangeOfLocMatching) automatically destroy the Boolexpr, this is why you are getting the bugs. There is no filter after the first iteration.

Eleventh, here is how to do a custom group iteration like you attempted, only a little cleaner

Here is your attempt, first

function example takes nothing returns nothing
local unit array units
local group g = x//an enum
local integer cycler = 1
local integer groupnum = CountUnitsInGroup( g )

loop
exitwhen cycler > groupnum
set units[cycler] = FirstOfGroup( g )
//do actions with this unit here
call GroupRemoveUnit( g, units[cycler] )
set units[cycler] = null
set cycler = cycler + 1
endloop

call DestroyGroup( g )
set g = null
endfunction

Here is a much better way to iterate them

function BetterMethod takes nothing returns nothing
local unit u
local group g = CreateGroup()//you have to initialize a group if you're using natives. BJ calls do it for us, but they still suck.

call GroupEnumUnits... //this is a non-BJ call (much better than a BJ call)

loop
set u = FirstOfGroup( g )//define your unit
exitwhen u == null//u will == null if there are no more units in the group when the previous line is called
//do stuff with your unit
call GroupRemoveUnit( g, u )//remove unit
//you don't need to set u to null. it will automatically be set to null when there are no more units in the group to set to u
endloop

call DestroyGroup( g )
set g = null
endfunction

Hope this helps!

 

[Arrkan]

Wow, thanks for your analysis. I'll try what you said. Oh and by the way, I set the times variable to a constant just for easy viewing, it's actually a variable.

 

[PurplePoot]

Aight, no prob

About the times thing, do you mean that it hits a random amount of times?

And again, the boolexpr is being deleted by your lame BJ call (GetUnitsInRangeOfLocMatching). Use GroupEnumUnitsInRange instead, and dont forget to create the group when you do.

 

[Arrkan]

This is what I have now, it works perfectly:

function Trig_Fire_Conditions takes nothing returns boolean
return GetSpellAbilityId()=='A01Q'
endfunction

function burrn takes nothing returns boolean
return (GetWidgetLife(GetFilterUnit())>0.405) and (IsUnitEnemy(GetFilterUnit(),GetOwningPlayer(GetTriggerUnit()))) and not (IsUnitType(GetFilterUnit(),UNIT_TYPE_MECHANICAL))
endfunction

function Trig_Fire_Actions takes nothing returns nothing
local unit caster=GetTriggerUnit()
local location l1=GetUnitLoc(caster)
local location l2=GetSpellTargetLoc()
local integer lvl=GetUnitAbilityLevel(caster,'A01Q')
local real dmg=GetHeroStr(caster,true)
local boolexpr b=Filter(function burrn)
local group g
local unit u
local integer i=1
local integer times=(lvl*3)+2
call PolledWait((DistanceBetweenPoints(l1,l2)-90)/575)
loop
    exitwhen i>times
        set g=CreateGroup()
        call GroupEnumUnitsInRangeOfLoc(g,l2,200,b)
        loop
            set u=FirstOfGroup(g)
            exitwhen u==null
                call UnitDamageTarget(caster,u,dmg,false,false,ATTACK_TYPE_NORMAL,DAMAGE_TYPE_MAGIC,null)
                call GroupRemoveUnit(g,u)
            endloop
    call TriggerSleepAction(0.1)
    call DestroyGroup(g)
    set g=null
    set i=i+1
    endloop
call RemoveLocation(l1)
call RemoveLocation(l2)
call DestroyBoolExpr(b)
set l1=null
set l2=null
set b=null
set caster=null
set g=null
endfunction

//===========================================================================
function InitTrig_Fire takes nothing returns nothing
    set gg_trg_Fire = CreateTrigger(  )
    call TriggerRegisterAnyUnitEventBJ( gg_trg_Fire, EVENT_PLAYER_UNIT_SPELL_EFFECT )
    call TriggerAddCondition( gg_trg_Fire, Condition( function Trig_Fire_Conditions ) )
    call TriggerAddAction( gg_trg_Fire, function Trig_Fire_Actions )
endfunction

So yeah I didn't bother converting locations to coordinates.. is it really that better? Thanks for your support

 

[LordZsar1]

Well, if you can get a location OR two coordinates from something, it is nearly sure to think, that this location was specially created to be returned and has no other purpose...
So, it is a waste of ressources.
(and primitives like "int" and "real" are easier to handle than a... handle ^^ in any situation I have ever seen)

 

[PurplePoot]

A) use GroupEnumUnitsInRange ^^

B) Since you use lvl only once, there is no reason to make its own variable

g2g now, will do some more reviewing later.