[JASS] Feedback for a spell

[Abovegame]

I'm interested if this spell will be buggy.If you find anything out of the ordinary please tell me.

scope ShadowIncineration
    globals
        private group g = CreateGroup()
        private unit caster
        private string effect_path = "Abilities\\Spells\\Undead\\RaiseSkeletonWarrior\\RaiseSkeleton.mdl"
    endglobals

function Trig_AoESpell_Conditions takes nothing returns boolean
    if GetSpellAbilityId() =='A00K' then
        return true
    endif
    return false
endfunction

function TimerEnds takes nothing returns nothing
    local timer t = GetExpiredTimer()
    local integer hnd_id = GetHandleId(t)
    local integer unitID = LoadInteger(udg_ghash,hnd_id,0)
    local real unit_default_speed
    local real unit_default_turn_speed
    local unit u
   
    loop
        set u = FirstOfGroup(g)
        exitwhen(u == null)
        if GetUnitTypeId(u) == unitID then
            set unit_default_speed = GetUnitDefaultMoveSpeed(u)
            set unit_default_turn_speed = GetUnitDefaultTurnSpeed(u)
            call SetUnitMoveSpeed(u,unit_default_speed)
            call SetUnitTurnSpeed(u,unit_default_turn_speed)
            call UnitRemoveAbility(u,'A00A')
            call GroupRemoveUnit(g,u)
        endif
    endloop
   
    call PauseTimer(t)
    call DestroyTimer(t)
    call FlushChildHashtable(udg_ghash,hnd_id)
    set u = null
    set t = null
endfunction


function Pick takes nothing returns nothing
    local unit u = GetFilterUnit()
    local integer unitID = GetUnitTypeId(u)
    local real unit_life = GetUnitState(u,UNIT_STATE_LIFE)
    local real unit_max_life = GetUnitState(u,UNIT_STATE_MAX_LIFE)
    local real unit_default_speed = GetUnitDefaultMoveSpeed(u)
    local real unit_default_turn_speed = GetUnitDefaultTurnSpeed(u)
    local real new_speed = unit_default_speed * 0.3
    local real new_turn_speed = unit_default_turn_speed * 0.3
    local effect damage_effect
    local timer t = CreateTimer()
    local integer hnd_id = GetHandleId(t)
    local real damage = 25.0 * GetUnitAbilityLevel(caster,'A00K') + GetHeroAgi(caster,true) * 0.75

    if  unit_life <= unit_max_life * 0.3 and IsUnitEnemy(u,Player(0)) == true then
        call SetUnitMoveSpeed(u,new_speed )
        call SetUnitTurnSpeed(u,new_turn_speed )
        call UnitAddAbility(u,'A00A')

        call GroupAddUnit(g,u)
        call SaveInteger(udg_ghash, hnd_id, 0, unitID)
        call TimerStart(t,15.0,false, function TimerEnds)
    elseif IsUnitEnemy(u,Player(0)) == true then
        set damage_effect = AddSpecialEffectTarget(effect_path,u,"origin")
        call DestroyEffect(damage_effect)
        call UnitDamageTarget(caster,u,damage,false,false,ATTACK_TYPE_MAGIC,DAMAGE_TYPE_MAGIC,null)
    endif
    set damage_effect = null
    set t = null
    set u = null
endfunction

function Trig_AoESpell_Actions takes nothing returns nothing
    local location loc = GetSpellTargetLoc()
    set caster = GetTriggerUnit()
    call GroupEnumUnitsInRangeOfLoc(g,loc,200.0,Filter(function Pick))
   
    set loc = null
endfunction

//===========================================================================
function InitTrig_ShadowIncineration takes nothing returns nothing
    set gg_trg_ShadowIncineration = CreateTrigger(  )
    call TriggerRegisterAnyUnitEventBJ( gg_trg_ShadowIncineration, EVENT_PLAYER_UNIT_SPELL_EFFECT )
    call TriggerAddCondition( gg_trg_ShadowIncineration, Condition( function Trig_AoESpell_Conditions ) )
    call TriggerAddAction( gg_trg_ShadowIncineration, function Trig_AoESpell_Actions )
endfunction

endscope

 

[Jampion]

function Trig_AoESpell_Conditions takes nothing returns boolean
    if GetSpellAbilityId() =='A00K' then
        return true
    endif
    return false
endfunction

->

function Trig_AoESpell_Conditions takes nothing returns boolean
     return GetSpellAbilityId() =='A00K'
endfunction

Trig_AoESpell_Actions:

You can probably use GetSpellTargetX/Y instead of GetSpellTargetLoc(). GetSpellTargetX/Y is not in my functions list in JNGP, but it is correctly highlighted as native function.

you should remove loc

Pick:

this is personal preference, but it saves you a variable

        set damage_effect = AddSpecialEffectTarget(effect_path,u,"origin")
        call DestroyEffect(damage_effect)

->

 call DestroyEffect(AddSpecialEffectTarget(effect_path,u,"origin"))

TimerEnds :

the loop is infinite in most cases.
call GroupRemoveUnit(g,u) must always be executed regardless of unit type
otherwise the loop will be infinte

If understand it correctly, a unit below 30% health is slowed for 15 seconds and a unit above 30% health is damaged.

You should use the scope more. Using an initializer is easier in my opinion than using the default jass initialization, because it is independent from the trigger name.
Pick and TimerEnds are very generic names, so it is likely to have the same function in another trigger as well. Use the keyword private to avoid this problem:
private function TimerEnds takes nothing returns nothing
You could make the global effect_path constant.

The way you iterate through the group seems a bit weird to me, but as long as it works it's ok. I mean the fact that you are using a filter func and add the unit in the filter func to the group. Usually the unit would be automatically added to the group by returning true.

 

[IcemanBo]

If the spell is designed for public usage then it might be good to have some configuration block for things like damage, for example.

 

[Abovegame]

So i tweaked around a little bit and ended up doing this:

    globals
        private group g = CreateGroup()
        private unit caster
        private integer idx = 0
        private unit array units_picked
        private constant string effect_path = "Abilities\\Spells\\Undead\\RaiseSkeletonWarrior\\RaiseSkeleton.mdl"
    endglobals

private function TimerEnds takes nothing returns nothing
    local timer t = GetExpiredTimer()
    local integer hnd_id = GetHandleId(t)
    local integer index = LoadInteger(udg_ghash,hnd_id,0)
    local real unit_default_speed
    local real unit_default_turn_speed
    local unit u = units_picked[index]
    local integer i = 0
  
    set unit_default_speed = GetUnitDefaultMoveSpeed(u)
    set unit_default_turn_speed = GetUnitDefaultTurnSpeed(u)
    call SetUnitMoveSpeed(u,unit_default_speed)
    call SetUnitTurnSpeed(u,unit_default_turn_speed)
    call UnitRemoveAbility(u,'A00A')
  
    call PauseTimer(t)
    call DestroyTimer(t)
    call FlushChildHashtable(udg_ghash,hnd_id)
    set u = null
    set t = null
endfunction


private function Pick takes nothing returns nothing
    local unit u = GetFilterUnit()
    local real unit_life = GetUnitState(u,UNIT_STATE_LIFE)
    local real unit_max_life = GetUnitState(u,UNIT_STATE_MAX_LIFE)
    local real unit_default_speed = GetUnitDefaultMoveSpeed(u)
    local real unit_default_turn_speed = GetUnitDefaultTurnSpeed(u)
    local real new_speed = unit_default_speed * 0.3
    local real new_turn_speed = unit_default_turn_speed * 0.3

    local timer t = CreateTimer()
    local integer hnd_id = GetHandleId(t)
    local real damage = 25.0 * GetUnitAbilityLevel(caster,'A00K') + GetHeroAgi(caster,true) * 0.75
  
    if  unit_life <= unit_max_life * 0.3 and IsUnitEnemy(u,Player(0)) == true then
        call SetUnitMoveSpeed(u,new_speed )
        call SetUnitTurnSpeed(u,new_turn_speed )
        call UnitAddAbility(u,'A00A')
        call SaveInteger(udg_ghash, hnd_id, 0, idx)
        call TimerStart(t,15.0,false, function TimerEnds)
        set units_picked[idx] = u
        set idx = idx + 1
    elseif IsUnitEnemy(u,Player(0)) == true then
        call DestroyEffect(AddSpecialEffectTarget(effect_path,u,"origin"))
        call UnitDamageTarget(caster,u,damage,false,false,ATTACK_TYPE_MAGIC,DAMAGE_TYPE_MAGIC,null)
    endif
    set t = null
    set u = null
endfunction

function Trig_AoESpell_Actions takes nothing returns nothing
    local location loc = GetSpellTargetLoc()
    set caster = GetTriggerUnit()
    set idx = 0
    call GroupEnumUnitsInRangeOfLoc(g,loc,200.0,Filter(function Pick))
    call RemoveLocation(loc)
    call GroupClear(g)
    set loc = null
endfunction

This seems to work perfectly only if the hero can't cast it before the timer ends, i'll leave it at this for now.But originally i wanted it to work somewhat like Spirit Link, where it clears the last affected units and places the buff on the newly casted.

On a side note, i had to use

GroupAddUnit()

because it wouldn't add the units to a group by itself for whatever reason.

 

[Chaosy]

Wrap it all into a library. Way cleaner init function that way.