[JASS] Looking for help with this trigger

[Mono]

Hi everyone, I have recently started to learn JASS by checking the many posts here, reading the tutorials and opening other maps up from this site which use JASS for the custom abilities and whatnot.

I recently made a spell which creates multiple random flame strikes around the caster, and then picks the units within range of the flame strike and deals damage to them, here is the coding for the ability:

scope RandomStrike initializer Init
//===========================================================================   
    globals
        private constant integer ABILITY_ID = 'A001'
        private constant string EFFECT = "Abilities\\Spells\\Human\\FlameStrike\\FlameStrike1.mdl"
        private constant damagetype D_TYPE = DAMAGE_TYPE_NORMAL
        private constant attacktype A_TYPE = ATTACK_TYPE_MAGIC
        group rg = CreateGroup()
        boolexpr filterExpr
    endglobals
    
    private function Damage takes integer level returns real
        return level * 15.
    endfunction
//===========================================================================   
    private function Conditions takes nothing returns boolean
        return GetSpellAbilityId() == ABILITY_ID
    endfunction
//===========================================================================
    private function pick takes nothing returns nothing
        local unit t = GetTriggerUnit()
        local integer level = GetUnitAbilityLevel(t, ABILITY_ID)
        if ( IsUnitType(GetEnumUnit(), UNIT_TYPE_STRUCTURE) == false ) and ( IsUnitType(GetEnumUnit(), UNIT_TYPE_MAGIC_IMMUNE) == false ) and ( IsUnitAlly(GetEnumUnit(), GetOwningPlayer(t)) == false ) then
            call UnitDamageTargetBJ(t, GetEnumUnit(), Damage(level), A_TYPE, D_TYPE)
        endif
        
    endfunction
    
//===========================================================================
    private function Actions takes nothing returns nothing
        local unit t = GetTriggerUnit()
        local integer level = GetUnitAbilityLevel(t,ABILITY_ID)
        local integer loopcount
        local real xc
        local real yc
        local real tx
        local real ty
        local location rl
        set loopcount = 2 * level
        if loopcount >= 0 then
            loop
                exitwhen loopcount <= 0
                set tx = GetUnitX(t)
                set ty = GetUnitY(t)
                set xc = GetRandomReal(-350,350)
                set yc = GetRandomReal(-350,350) 
                set rl = Location((tx+xc), (ty+yc))
                call AddSpecialEffectLoc(EFFECT, rl)
                set loopcount = loopcount - 1
                call TriggerSleepAction(0.15)
                set rg = GetUnitsInRangeOfLocAll(215, rl)
                call ForGroup(rg, function pick)
                call TriggerSleepAction(0.02)
            endloop
        endif
        call DestroyGroup(rg)
        call RemoveLocation(rl)
        set rg = null
        set t = null
    endfunction
//===========================================================================
    private function Init takes nothing returns nothing
        local trigger RandomStrike = CreateTrigger()
        call TriggerRegisterAnyUnitEventBJ(RandomStrike, EVENT_PLAYER_UNIT_SPELL_EFFECT)
        call TriggerAddCondition(RandomStrike, Condition( function Conditions))
        call TriggerAddAction(RandomStrike, function Actions)
    endfunction
endscope

I was wondering if this will leak, and if it will be entirely MUI, I think it is except for the part with the damage being dealt since it is in relation to a GroupBJ. I will just mention the ability works exactly how I would like it to, but again I just want some feedback in regards to the code to make it as efficient, leakfree and MUI as possible.

 

[Silvenon]

You have one leak. Instead of destroying rl location outside, you need to destroy it inside the loop. Because in each cycle you're creating a location, but then you're destroying only the last created one.

Other than that I see no leaks. If you want, I can show you how you can make some things in your code more efficient (some people find that insulting, that's why I said "if you want").

EDIT: Forgot to mention, yeah, it is MUI since it doesn't have any waits.

 

[Mono]

Thanks for the reply, yes if you wouldn't mind either replying or PMing me a way on how to make it more efficient that would be great

 

[Deaod]

Okay... i rewrote that thing to get rid of those horrible, horrible TSA calls. This also had some side effects. No more BJs, more options to adjust, some optimization.

It now uses GroupUtils, TimerUtils and TimedHandles.

library RandomStrike uses GroupUtils, TimerUtils, TimedHandles
//===========================================================================
    globals
        private constant integer ABILITY_ID = 'A001'
        private constant string EFFECT = "Abilities\\Spells\\Human\\FlameStrike\\FlameStrike1.mdl"
        private constant damagetype D_TYPE = DAMAGE_TYPE_NORMAL
        private constant attacktype A_TYPE = ATTACK_TYPE_MAGIC
        private constant real SPAWN_INTERVAL = 0.5
        private constant real FX_STAY = 8
        private constant real DAMAGE_DELAY = 0.25
    endglobals
    
    private function Damage_Amount takes integer level returns real
        return level * 15.
    endfunction
    
    private function Damage_Aoe takes integer level returns real
        return 215
    endfunction
    
    private function NumberOfStrikes takes integer level returns integer
        return 2*level 
    endfunction
    
    private function Radius takes integer level returns real
        return 350
    endfunction
    
    globals
        private Data2 tmps
    endglobals
    
    private struct Data2
        unit c
        real x
        real y
        real level
        
        private static boolexpr DoDamage
        
        private static method DoDamageFunc takes nothing returns boolean
        local unit u=GetFilterUnit()
            if ( IsUnitType(u, UNIT_TYPE_STRUCTURE) == false ) and ( IsUnitType(u, UNIT_TYPE_MAGIC_IMMUNE) == false ) and IsUnitEnemy(u, GetOwningPlayer(tmps.c)) then
                call UnitDamageTarget(tmps.c, u, Damage_Amount(tmps.level), true, false, A_TYPE, D_TYPE, WEAPON_TYPE_WHOKNOWS)
            endif
            set u=null
            return false
        endmethod
        
        private static method Callback takes nothing returns nothing
        local timer t=GetExpiredTimer()
        local Data2 s=GetTimerData(t)
            call ReleaseTimer(t)
            set tmps=s
            call GroupEnumUnitsInRange(ENUM_GROUP, s.x, s.y, Damage_Aoe(s.level), Data2.DoDamage)
            set s.c=null
            call s.destroy()
        endmethod
        
        static method create takes unit c, integer level, real x, real y, real delay returns Data2
        local Data2 s=Data2.allocate()
        local timer t
            set s.c=c
            set s.level=level
            set s.x=x
            set s.y=y
            set t=NewTimer()
            call SetTimerData(t, s)
            call TimerStart(t, delay, false, function Data2.Callback)
            return s
        endmethod
        
        private static method onInit takes nothing returns nothing
            set Data2.DoDamage=Condition(function Data2.DoDamageFunc)
        endmethod
    endstruct
    
    private struct Data
        unit c
        real x
        real y
        integer level
        integer i
        timer t
        
        private method onDestroy takes nothing returns nothing
            call ReleaseTimer(s.t)
            set s.c=null
        endmethod
        
        private method Cond takes nothing returns boolean
            return GetSpellAbilityId()==ABILITY_ID
        endmethod
        
        private method Callback takes nothing returns nothing
        local Data s=GetTimerData(GetExpiredTimer())
        // even distribution of randomness in a circle:
        local real r=SquareRoot(GetRandomReal(0,1))*Radius(s.level)
        local real ang=GetRandomReal(0, 2*bj_PI)
            call DestroyEffectDelayed(AddSpecialEffect(EFFECT, s.x+(Cos(ang)*r), s.y+(Sin(ang)*r)), FX_STAY)
            call Data2.create(s.c, s.level, s.x+(Cos(ang)*r), s.y+(Sin(ang)*r), DAMAGE_DELAY)
            set s.i=s.i-1
            if s.i==0 then
                call s.destroy()
            endif
        endmethod
        
        private method Actions takes nothing returns nothing
        local Data s=Data.allocate()
            set s.c=GetTriggerUnit()
            set s.x=GetUnitX(s.c)
            set s.y=GetUnitY(s.c)
            set s.level=GetUnitAbilityLevel(s.c, ABILITY_ID)
            set s.t=NewTimer()
            set s.i=NumberOfStrikes(s.level)
            call TimerStart(s.t, SPAWN_INTERVAL, true, function Data.Callback)
            call SetTimerData(s.t, s)
        endmethod
        
        private static method onInit takes nothing returns nothing
        local trigger t=CreateTrigger()
            call TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_UNIT_SPELL_EFFECT)
            call TriggerAddCondition(t, Condition(function Data.Cond))
            call TriggerAddAction(t, function Data.Actions)
        endmethod
    endstruct
endscope

On a side note: I havent compiled that script yet, there might be errors.

 

[Silvenon]

First (which is not important, but makes things slightly easier): naming stuff. Instead of naming the local trigger "RandomStrike", you can simply name it "t":

private function Init takes nothing returns nothing
        local trigger t = CreateTrigger()
        call TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_UNIT_SPELL_EFFECT)
        call TriggerAddCondition(t, Condition( function Conditions))
        call TriggerAddAction(t, function Actions)
    endfunction

I suggest you name all your locals with two letters max. It's faster and easier to read that way (I know you still got your GUI variables naming method in mind, I did too ).

Next, the cool thing about JASS is that you can use coordinates. I see you're using coordinates, but why are you using functions that require locations? You can use the XY functions....oops, you have another leak, I didn't notice it. It's that group rg variable, you're creating a new group, then you're creating a new one again by calling GetUnitsInRangeOfLocAll function. See how that function looks like:

function GetUnitsInRangeOfLocAll takes real radius, location whichLocation returns group

    return GetUnitsInRangeOfLocMatching(radius, whichLocation, null)

endfunction

It's calling GetUnitsInRagneOfLocMatching function, which looks like this:

function GetUnitsInRangeOfLocMatching takes real radius, location whichLocation, boolexpr filter returns group

    local group g = CreateGroup()

    call GroupEnumUnitsInRangeOfLoc(g, whichLocation, radius, filter)

    call DestroyBoolExpr(filter)

    return g

endfunction

See? It's creating a new group. You're then leaking a group in each loop cycle, because you're creating groups and not destroying them (I will show you a workaround further).

I suggest using "enum" functions for groups instead of "get"s:

call GroupEnumUnitsInRange(rg, tx+xc, ty+yc, 215, null)


Now you don't need your location variable anymore.

Oh, wait, this isn't MUI, I didn't notice the wait:

call TriggerSleepAction(0.02)


I suggest you don't use those anymore, they're bad. Use timers instead. Here's the tutorial.

Also, instead of ForGroup, you can loop through a group in the JASS way:

loop
    set u = FirstOfGroup(rg)
    exitwhen u = null
    call GroupRemoveUnit(rg, u)
    call UnitDamageTargetBJ(t, u, Damage(level), A_TYPE, D_TYPE)
endloop

But, we forgot about the condition, we can use boolexprs. We can first create the filter function:

private function filter takes nothing returns boolean
        return not IsUnitType(GetFilterUnit(), UNIT_TYPE_STRUCTURE) and not IsUnitType(GetEnumUnit(), UNIT_TYPE_MAGIC_IMMUNE)
    endfunction

Notice that "matching units" are here referred to with GetFilterUnit. Also, instead of == false you can use "not", which is a bit faster I think. Also, if you have == true (which you don't in this code, but still) you can remove it, because "return bool == true" is equal to "return bool".

Now, lets add the boolexpr:

call GroupEnumUnitsInRange(rg, tx+xc, ty+yc, 215, Filter(function filter))


Notice that Filter function is apparently the same as Condition function (no difference has been found yet). You can see the usage of Condition function when converting a condition from GUI to JASS. It's the same thing here.

I thought that boolexprs leak, but apparently they don't because they're based off strings (which don't leak). So destroying boolexprs is dangerous. For example, if you're basing two boolexprs on a single function, destroying one boolexpr (with DestroyBoolExpr) will destroy the other one too!

Btw, you don't need your "pick" function anymore.

Ok, let's return to that group looping. Notice the BJ function:

function UnitDamageTargetBJ takes unit whichUnit, unit target, real amount, attacktype whichAttack, damagetype whichDamage returns boolean

    return UnitDamageTarget(whichUnit, target, amount, true, false, whichAttack, whichDamage, WEAPON_TYPE_WHOKNOWS)

endfunction

So you can just call that function directly:

call UnitDamageTarget(t, u, Damage(level), true, false, A_TYPE, D_TYPE, null)


Instead of WEAPON_TYPE_WHOKNOWS you can just put null, it's the same.

Finally, you can leave this line outside the loop:

call DestroyGroup(rg)


Because the looping through the group I showed you above clears the group from all the units, so now you will achieve the effect you wanted: you will add units in the loop, then clear them, then add them again, and so on, and in the end you'll destroy it.

You don't need to null rg because it's a global variable.

Ok, that's all, I hope I didn't make a mistake anywhere, I was kinda in a hurry.

EDIT: Screw you, Deaod. That didn't teach him anything. You just rewrote his script in your way without explaining anything.

 

[Deaod]

Heh, i found getting rid of the TSA a little difficult (hence why i use two structs instead of only one).
Also, that FirstOfGroup() loop is a little inefficient. Its more efficient to do all things inside the boolexpr of the enum.

I think you missed the effect leak. He only calls AddSpecialEffectLoc, but never calls DestroyEffect. WC3 may clean those up, but im not too sure about that.

 

[Silvenon]

I think you missed the effect leak. He only calls AddSpecialEffectLoc, but never calls DestroyEffect.

Yeah, I missed that, thanks.

WC3 may clean those up, but im not too sure about that.

Huh? No, you must destroy all special effects. I had an effect leak in my map and it wasn't pretty.

 

[Mono]

Wow, thanks for both those replies, I'll scan through them both a few times to try and take it all in so I can use it for upcoming triggers and abilities. I'll see if I can manage to edit my trigger with the above help and if I can't I'll post back asking for some more help.

 

[Mono]

Ok, so here is what I have now for the trigger, I read the timer tutorial, but am still unsure how to implement it, also how can I implement the destroying of the special effect?

scope t initializer Init
//===========================================================================   
    globals
        private constant integer ABILITY_ID = 'A001'
        private constant string EFFECT = "Abilities\\Spells\\Human\\FlameStrike\\FlameStrike1.mdl"
        private constant damagetype D_TYPE = DAMAGE_TYPE_NORMAL
        private constant attacktype A_TYPE = ATTACK_TYPE_MAGIC
        group rg = CreateGroup()
        boolexpr filterExpr
    endglobals
    
    private function Damage takes integer level returns real
        return level * 15.
    endfunction
//===========================================================================   
    private function Conditions takes nothing returns boolean
        return GetSpellAbilityId() == ABILITY_ID
    endfunction
//===========================================================================
    function filter takes nothing returns boolean
        return not IsUnitType(GetFilterUnit(), UNIT_TYPE_STRUCTURE) and not IsUnitType(GetEnumUnit(), UNIT_TYPE_MAGIC_IMMUNE) and not IsUnitAlly(GetEnumUnit(), GetOwningPlayer(GetTriggerUnit()))
    endfunction
    
//===========================================================================
    private function Actions takes nothing returns nothing
        local unit t = GetTriggerUnit()
        local integer level = GetUnitAbilityLevel(t,ABILITY_ID)
        local integer loopcount
        local real xc
        local real yc
        local real tx
        local real ty
        local location rl
        local unit = u
        local timer ti
        set loopcount = 2 * level
        if loopcount >= 0 then
            loop
                exitwhen loopcount <= 0
                set tx = GetUnitX(t)
                set ty = GetUnitY(t)
                set xc = GetRandomReal(-350,350)
                set yc = GetRandomReal(-350,350) 
                set rl = Location((tx+xc), (ty+yc))
                call AddSpecialEffectLoc(EFFECT, rl)
                set loopcount = loopcount - 1
                call GroupEnumUnitsInRange(rg, tx+xc, ty+yc, 215, Filter(function filter))
                 loop            
                    set u = FirstOfGroup(rg)
                    exitwhen u == null
                    call GroupRemoveUnit(rg, u)
                    call UnitDamageTarget(t, u, Damage(level), true, false, A_TYPE, D_TYPE, null)
                endloop
            endloop
        endif
        //call DestroyGroup(rg) Is this the correct spot for the DestroyGroup, because when I keep it here, the trigger only fires once.
       //  If I use the ability a second or third time, nothing triggers.
        call RemoveLocation(rl)
        set rl = null
        set t = null
    endfunction
//===========================================================================
    private function Init takes nothing returns nothing
        local trigger r = CreateTrigger()
        call TriggerRegisterAnyUnitEventBJ(r, EVENT_PLAYER_UNIT_SPELL_EFFECT)
        call TriggerAddCondition(r, Condition( function Conditions))
        call TriggerAddAction(r, function Actions)
    endfunction
endscope

If I could get some help implementing the timer so the flamestrike + damage goes off every .15 seconds that would be good, I tried to do this, but had no idea where to start to get it implemented. There is also a problem of sometimes I think it isnt actually creating the 8 effects, and if it does, they all seem to be stacking in the exact same spot, if there is a way around this could you please explain how.

Thanks

 

[Silvenon]

//call DestroyGroup(rg) Is this the correct spot for the DestroyGroup, because when I keep it here, the trigger only fires once.
// If I use the ability a second or third time, nothing triggers.

Oops, my bad, don't destroy the group at all. But I suggest that, if you want to make it MUI, create a local group:

local group rg = CreateGroup()


Then it's alright to leave call DestroyGroup(rg) there. Then you would also have to nullify the group.

About destroying the effect, you can do this:

call DestroyEffect(AddSpecialEffectLoc(EFFECT, rl))


But test it if it looks right. Also, don't use the location version, use the XY one:

call DestroyEffect(AddSpecialEffect(EFFECT, tx+xc, ty+yc))


Now you don't need the location anymore. Locations are pretty bad because you have to clean them and because they're slow.

Also, a really minor thing, for easier readability, you can put set loopcount = loopcount - 1 on the bottom of the loop.

About the timer, well, you have to have an attaching system. Also, you have to create a struct:

private struct Data
    unit t
    group rg
    integer level
    integer loopcount
endstruct

Then you can use ABC, there's an explanation there how to use it.

// ...
// ...

local timer ti = CreateTimer()
local Data dat = Data.create()

// ...

set dat.t = t // I tend to name them exactly like the locals I'm attaching, to make things easier
set dat.rg = rg // don't forget to change it so the group is local now
set dat.level = level
set dat.loopcount = loopcount

call SetTimerStructA(ti, dat)

call TimerStart(ti, 0.15, true, callback)

// ...

Notice the syntax. You can read about it in the jasshelper manual (you have it in your jasshelper folder which is located in your Jass NewGen Pack folder).

Btw, remove the loop now, we're going to create a new function for that (as you can see from the TimerStart line):

private function callback takes nothing returns nothing
    local timer ti = GetExpiredTimer()
endfunction

This is the way to get the expired timer, the timer that started the function. Now you can detach your Data:

private function callback takes nothing returns nothing
    local timer ti = GetExpiredTimer()
    local Data dat = GetTimerStructA(ti)
endfunction

Then you can do everything else:

private function callback takes nothing returns nothing
    local timer ti = GetExpiredTimer()
    local Data dat = GetTimerStructA(ti)
    local real tx = GetUnitX(dat.t)
    local real ty = GetUnitY(dat.t)
    local real xc = GetRandomReal(-350,350)
    local real yc = GetRandomReal(-350,350)
    local unit u
    call AddSpecialEffect(EFFECT, tx+xc, ty+yc)
    call GroupEnumUnitsInRange(dat.rg, tx+xc, ty+yc, 215, Filter(function filter))
    loop
        set u = FirstOfGroup(dat.rg)
        exitwhen u == null
        call GroupRemoveUnit(dat.rg, u)
        call UnitDamageTarget(dat.t, u, Damage(dat.level), true, false, A_TYPE, D_TYPE, null)
    endloop
    set dat.loopcount = dat.loopcount - 1
endfunction

All you have to do now is stop this when loopcount <= 0:

private function filter takes nothing returns boolean  // make sure this function is above the callback function
    // ...
endfunction

private function callback takes nothing returns nothing
    local timer ti = GetExpiredTimer()
    local Data dat = GetTimerStructA(ti)
    local real tx = GetUnitX(dat.t)
    local real ty = GetUnitY(dat.t)
    local real xc = GetRandomReal(-350,350)
    local real yc = GetRandomReal(-350,350)
    local unit u
    call AddSpecialEffect(EFFECT, tx+xc, ty+yc)
    call GroupEnumUnitsInRange(dat.rg, tx+xc, ty+yc, 215, Filter(function filter))
    loop
        set u = FirstOfGroup(dat.rg)
        exitwhen u == null
        call GroupRemoveUnit(dat.rg, u)
        call UnitDamageTarget(dat.t, u, Damage(dat.level), true, false, A_TYPE, D_TYPE, null)
    endloop
    set dat.loopcount = dat.loopcount - 1
    
    if dat.loopcount <= 0 then
        call DestroyGroup(dat.rg)
        call ClearTimerStructA(t)
        call dat.destroy()
        call PauseTimer(t) // I paused it because it can malfunction when destroyed (in rare situations
        call DestroyTimer(t)
        set t = null
    endif
endfunction

This is how your Actions function should look like this now:

    private function Actions takes nothing returns nothing
        local unit t = GetTriggerUnit()
        local integer level = GetUnitAbilityLevel(t,ABILITY_ID)
        local integer loopcount = 2 * level
        local group rg = CreateGroup()
        local timer ti = CreateTimer()
        local Data dat = Data.create()

        set dat.t = t
        set dat.rg = rg
        set dat.level = level
        set dat.loopcount = loopcount
        
        call SetTimerStructA(ti, dat)
        call TimerStart(ti, 0.15, true, callback)
        
        set t = null
        set ti = null
    endfunction

And that's everything (for now). I think I did it right. Let me know if something's not working.