My Spell

[GIMLI_2]

Hi,
So, i made my first "spell" which uses timer. Basically, its just a suicide spell that deals AoE damage. It uses a 0.00 second timer because otherwise the unit will die before finishing the cast and it wont start the cooldown.
The ability itself works fine, but my code seems a bit long for such a simple ability. Any suggestions? What could be improved?
Thanks

Code

struct Suicide
     unit       u_pointer
     integer    id_pointer
endstruct

constant function DamageOutputSuicide takes integer id returns real
    if id == 'A010' then
        return 500.00
    else
        return 300.00
    endif
endfunction

constant function RadiusSuicide takes integer id returns real
    if id == 'A010' then
        return 350.00
    else
        return 220.00
    endif
endfunction

function Suicide_Timed takes nothing returns nothing
    local timer     t       = GetExpiredTimer()
    local Suicide   data    = GetTimerData(t)
    local unit      u       = data.u_pointer
    local unit      l
    local integer   id      = data.id_pointer
    local real      x       = GetUnitX(u)
    local real      y       = GetUnitY(u)
    local group     g       = CreateGroup()
    local effect    e
    call GroupEnumUnitsInRange(g, x, y, RadiusSuicide(id), null)
    loop
        set l = FirstOfGroup(g)
        exitwhen l == null
        if (ValidTarget(l, GetOwningPlayer(u), true, false) == true) then
            call UnitDamageTarget(u, l, DamageOutputSuicide(id), true, false, ATTACK_TYPE_NORMAL, DAMAGE_TYPE_NORMAL, WEAPON_TYPE_WHOKNOWS)
        endif
        call GroupRemoveUnit(g, l)
    endloop
    call UnitDamageTarget(u, u, 1000.00, true, false, ATTACK_TYPE_NORMAL, DAMAGE_TYPE_NORMAL, WEAPON_TYPE_WHOKNOWS)
    set e = AddSpecialEffect("Objects\\Spawnmodels\\Other\\NeutralBuildingExplosion\\NeutralBuildingExplosion.mdl", x, y)
    call DestroyGroup(g)
    call DestroyEffect(e)
    call ReleaseTimer(t)
    set u = null
    set g = null
    set e = null
    set t = null
endfunction

function Trig_Suicide_Conditions takes nothing returns boolean
    local Suicide   data
    local timer     t
    local integer   id      = GetSpellAbilityId()
    if (id == 'A00R') or (id == 'A010') then
        set data = Suicide.create()
        set t = NewTimer()
        set data.u_pointer = GetTriggerUnit()
        set data.id_pointer = id
        call SetTimerData(t, data)
        call TimerStart(t, 0.00, false, function Suicide_Timed)
        set t = null
    endif
    return false
endfunction

//===========================================================================
function InitTrig_Suicide takes nothing returns nothing
    local integer index
    set index = 0
    set gg_trg_Suicide = CreateTrigger()
    loop
        call TriggerRegisterPlayerUnitEvent(gg_trg_Suicide, Player(index), EVENT_PLAYER_UNIT_SPELL_EFFECT, null)
        set index = index + 1
        exitwhen index == bj_MAX_PLAYER_SLOTS
    endloop
    call TriggerAddCondition(gg_trg_Suicide, Condition(function Trig_Suicide_Conditions))
endfunction

 

[Wietlol]

Just add a 0.01s expiration timer to the caster.
You dont need a timer for this.

DestroyEffect and AddEffect can be done in the same line

 

[GIMLI_2]

Just add a 0.01s expiration timer to the caster.
You dont need a timer for this.

yep, i forgot about that
but if that wouldnt be an option, is the trigger ok like this?

DestroyEffect and AddEffect can be done in the same line

call DestroyEffect(AddSpecialEffect("Objects\\Spawnmodels\\Other\\NeutralBuildingExplosion\\NeutralBuildingExplosion.mdl", x, y)

like that?

 

[Wietlol]

Ye exactly like that.

When you use structs like that, you should use "extends array" to remove the allocate and deallocate functions.
Then you use the unit index as index of the variables.

"unit l" can be changed to "unit FoG" which is often used in those cases.
Just for readability.

Also in FoG iterations, you should do it in this order:
-loop
----set FoG = FirstOfGroup(g)
----exitwhen FoG == null
----call GroupRemoveUnit(g, FoG)
---- (empty line)
----//dostuff
-endloop

Be aware that "ATTACK_TYPE_NORMAL" is Attack Type Spells.
"ATTACK_TYPE_MELEE" is Attack Type Normal.

"call SetTimerData(t, data)"
Can be included in "NewTimer()" as "NewTimerEx(data)".

 

[GIMLI_2]

Ok, i'll change that.

When you use structs like that, you should use "extends array" to remove the allocate and deallocate functions.
Then you use the unit index as index of the variables.

I dont get it

 

[Wietlol]

Instead of using "Suicide.create()"
You can also do:

struct Suicide extends array
     unit       caster
     integer    abilityId
endstruct

function Trig_Suicide_Conditions takes nothing returns boolean
    local Suicide   data
    local unit      u
    local integer   id      = GetSpellAbilityId()
    
    if (id == 'A00R') or (id == 'A010') then
        set u = GetTriggerUnit()
        set data = GetUnitUserData(u)
        set data.caster = u
        set data.abilityId = id
        call TimerStart(NewTimerEx(data), 0.00, false, function Suicide_Timed)
        
        set u = null
    endif
    
    return false
endfunction

Assuming you have a unit indexer that uses the unit user data.

Most unit indexers already have a unit array that contains all units in the list sorted by the indices.
So in that case, you dont need the caster variable.

 

[GIMLI_2]

aaah ok, thanks.

call TimerStart(NewTimerEx(data), 0.00, false, function Suicide_Timed)

so you can put everything in one line? does that create a timer?

 

[Wietlol]

It creates a new timer via timer utils with the TimerData of "data".
Then it starts that timer.

It is making actual use of return types.