Spell from DotA to practice vJASS. It's fine?

[Zeth]

Well, I'm learning vJass and i make an spell from DotA, Elune's Arrow of Priestess of the moon.

Does it have any leak? Or something else I should change?

Map download

Here's the code:

scope Elune initializer Init

globals


    private            hashtable ht         = InitHashtable()
    private  constant  integer   Spell_Id   = 'A001' // Spell based on Channel.
    private  constant  integer   Stun_Id    = 'A000' // Stun for the dummy, bassed on thunderbolt.
    private  constant  integer   Arrow_Id   = 'h001' // Arrow.
    private  constant  integer   Dummy_Id   = 'h000' // Dummy that causes the damage and stun.
    private  constant  real      ArrowSpeed = .03    // Velocity of the arrow (basically, callback interval), don't put more than .04 (higger number, lower speed)
    private  constant  real      Collision  = 80.    // Radius of collision of the arrow.
    private  constant  integer   Max_Range  = 3000   // Distance traveled by the arrow
    
    
endglobals

private function Elune_Filter takes nothing returns boolean
    local timer t=GetExpiredTimer()
    local data d=LoadInteger(ht,0,GetHandleId(t))
    local unit stunner
    local unit stunned=GetFilterUnit()
    local player p=GetOwningPlayer(d.caster)
    local integer array D
    if IsUnitEnemy(stunned,p) and GetWidgetLife(stunned)>.405 and not IsUnitType(stunned,UNIT_TYPE_STRUCTURE) then
        set stunner=CreateUnit(p,Dummy_Id,GetUnitX(d.arrow),GetUnitY(d.arrow),0.)
        call UnitApplyTimedLife(stunner,'BTLF',.5)
        
        // Calculating the duration of the stun.
        set D[0]=R2I(d.distance/150)
        set D[1]=10
        if D[0]<D[1] then
            call SetUnitAbilityLevel(stunner,Stun_Id,D[0])
        else
            call SetUnitAbilityLevel(stunner,Stun_Id,D[1])
        endif
        //------------

        call IssueTargetOrder(stunner,"thunderbolt",stunned)
        call UnitDamageTarget(stunner,stunned,GetUnitAbilityLevel(d.caster,Spell_Id)*90.,true,false,ATTACK_TYPE_NORMAL,DAMAGE_TYPE_MAGIC,null)
        call KillUnit(d.arrow)
        //call RemoveUnit(d.arrow) ???
        call RemoveSavedInteger(ht,0,GetHandleId(t))
        call d.destroy()
        call PauseTimer(t)
        call DestroyTimer(t)
    endif
    set t=null
    set stunned=null
    set stunner=null
    set p=null
    return false
endfunction

private function Elune_Callback takes nothing returns nothing
    local timer t=GetExpiredTimer()
    local data d=LoadInteger(ht,0,GetHandleId(t))
    local real nx=GetUnitX(d.arrow)+30.*Cos(d.angle)
    local real ny=GetUnitY(d.arrow)+30.*Sin(d.angle)
    local rect max=bj_mapInitialPlayableArea
    call SetUnitPosition(d.arrow,nx,ny)
    set d.distance=d.distance+30
    call GroupEnumUnitsInRange(d.g,nx,ny,Collision,Filter(function Elune_Filter))
    if d.distance>=Max_Range or GetUnitY(d.arrow)>=GetRectMaxY(max) or GetUnitY(d.arrow)<=GetRectMinY(max) or GetUnitX(d.arrow)>=GetRectMaxX(max) or GetUnitX(d.arrow)<=GetRectMinX(max)  then
        call KillUnit(d.arrow)
        //call RemoveUnit(d.arrow)??
        call RemoveSavedInteger(ht,0,GetHandleId(t))
        call d.destroy()
        call PauseTimer(t)
        call DestroyTimer(t)
    endif
    set max=null
    set t=null
endfunction

struct data

    unit caster
    unit arrow
    real angle
    integer distance=50
    group g=CreateGroup()
    timer t=CreateTimer()
    
    static method create takes nothing returns thistype
        local data this=.allocate()
        local real sx=GetSpellTargetX()
        local real sy=GetSpellTargetY()
        local real cx
        local real cy
        set .caster=GetTriggerUnit()
        set cx=GetUnitX(.caster)
        set cy=GetUnitY(.caster)
        set .angle=Atan2(sy-cy,sx-cx)
        set .arrow=CreateUnit(GetOwningPlayer(.caster),Arrow_Id,cx,cy,.angle*57.2957795)
        call SaveInteger(ht,0,GetHandleId(.t),this)
        call TimerStart(.t,ArrowSpeed,true,function Elune_Callback)
        return this
    endmethod
    
    method onDestroy takes nothing returns nothing
        set .caster=null
        set .arrow=null
        set .g=null
        set .t=null
    endmethod
    
endstruct

private function Elune_Conditions takes nothing returns boolean
    if GetSpellAbilityId()==Spell_Id then
    call data.create()
    endif
    return false
endfunction
    
private function Init takes nothing returns nothing
    local integer i=0 
    local trigger t=CreateTrigger()
    
    // Bye bye, BJ.
    loop
      call TriggerRegisterPlayerUnitEvent(t, Player(i),EVENT_PLAYER_UNIT_SPELL_EFFECT, null)
      set i=i+1
      exitwhen i==bj_MAX_PLAYER_SLOTS
    endloop
    // ---------
    
    call TriggerAddCondition(t,Condition(function Elune_Conditions))
    set t=null
endfunction

endscope

 

[kingkingyyk3]

1) Use a proper attaching system.
Spamming hashtable is not a good idea.
This causes bad data management, hashtable is not fully utilized.
2) Recycle timers.
3) Use a global dummy to stun unit.
4) Use SetUnitX/Y instead of position, faster by 2700%
5) The formula for stun duration is maxDistance/currentDistance.
6) Not all BJ are evil. BJ function for spell initialization is fine. Your init function is just a simple c'n'p of BJ function, same. Why not just use BJ for better readability.
7) I think the units in range when arrow is collided will be stunned. Just group them up and use FirstOfGroup/GroupPickRandomUnit to get the unit for stunning.
8) Follow proper variable naming convention. GLOBALS, Dynamic globals, structMembers.

 

[Zeth]

1-2) Ok....... I used TimerUtils.
3) Why?
4)Done.
5) It's the same.
6) Ok.....
7) In DotA just hit one unit.
8) Ouch, this was just for practice, anyway, i'll keep that in mind.

    scope Elune initializer Init

    globals


        private  constant  integer   Spell_Id   = 'A001'
        private  constant  integer   Stun_Id    = 'A000'
        private  constant  integer   Arrow_Id   = 'h001'
        private  constant  integer   Dummy_Id   = 'h000'
        private  constant  real      ArrowSpeed = .025   
        private  constant  real      Collision  = 80.    
        private  constant  integer   Max_Range  = 3000   
       
       
    endglobals

    private function Elune_Filter takes nothing returns boolean
        local timer t=GetExpiredTimer()
        local data d=GetTimerData(t)
        local unit stunned=GetFilterUnit()
        local player p=GetOwningPlayer(d.caster)
        local integer array D
        if IsUnitEnemy(stunned,p) and GetWidgetLife(stunned)>.405 and not IsUnitType(stunned,UNIT_TYPE_STRUCTURE) then
            set d.stunner=CreateUnit(p,Dummy_Id,GetUnitX(d.arrow),GetUnitY(d.arrow),0.)
            call UnitApplyTimedLife(d.stunner,'BTLF',.5)
           
            set D[0]=R2I(d.distance/150)
            set D[1]=10
            if D[0]<D[1] then
                call SetUnitAbilityLevel(d.stunner,Stun_Id,D[0])
            else
                call SetUnitAbilityLevel(d.stunner,Stun_Id,D[1])
            endif

            call IssueTargetOrder(d.stunner,"thunderbolt",stunned)
            call UnitDamageTarget(d.stunner,stunned,GetUnitAbilityLevel(d.caster,Spell_Id)*90.,true,false,ATTACK_TYPE_NORMAL,DAMAGE_TYPE_MAGIC,null)
            call KillUnit(d.arrow)
            //call RemoveUnit(d.arrow) ???
            call d.destroy()
            call ReleaseTimer(t)
        endif
        set stunned=null
        set p=null
        return false
    endfunction

    private function Elune_Callback takes nothing returns boolean
        local timer t=GetExpiredTimer()
        local data d=GetTimerData(t)
        local real nx=GetUnitX(d.arrow)+30.*Cos(d.angle)
        local real ny=GetUnitY(d.arrow)+30.*Sin(d.angle)
        local rect max=bj_mapInitialPlayableArea
        call SetUnitX(d.arrow,nx)
        call SetUnitY(d.arrow,ny)
        set d.distance=d.distance+30
        call GroupEnumUnitsInRange(d.g,nx,ny,Collision,Filter(function Elune_Filter))
        if d.distance>=Max_Range or GetUnitY(d.arrow)>=GetRectMaxY(max) or GetUnitY(d.arrow)<=GetRectMinY(max) or GetUnitX(d.arrow)>=GetRectMaxX(max) or GetUnitX(d.arrow)<=GetRectMinX(max)  then
            call KillUnit(d.arrow)
            //call RemoveUnit(d.arrow)??
            call d.destroy()
            call ReleaseTimer(t)
            return true
        endif
        set max=null
        return false
    endfunction

    struct data

        unit caster
        unit arrow
        unit stunner
        real angle
        integer distance=50
        group g=CreateGroup()
        timer t
       
        static method create takes nothing returns thistype
            local data this=.allocate()
            local real sx=GetSpellTargetX()
            local real sy=GetSpellTargetY()
            local real cx
            local real cy
            set .t=NewTimer()
            set .caster=GetTriggerUnit()
            set cx=GetUnitX(.caster)
            set cy=GetUnitY(.caster)
            set .angle=Atan2(sy-cy,sx-cx)
            set .arrow=CreateUnit(GetOwningPlayer(.caster),Arrow_Id,cx,cy,.angle*57.2957795)
            call SetTimerData(.t,this)
            call TimerStart(.t,ArrowSpeed,true,function Elune_Callback)
            return this
        endmethod
       
        method onDestroy takes nothing returns nothing
            set .caster=null
            set .arrow=null
            set .g=null
            set .stunner=null
            set .t=null
        endmethod
       
    endstruct

    private function Elune_Conditions takes nothing returns boolean
        if GetSpellAbilityId()==Spell_Id then
        call data.create()
        endif
        return false
    endfunction
       
    private function Init takes nothing returns nothing
        local trigger t=CreateTrigger()
        call TriggerRegisterAnyUnitEventBJ(t,EVENT_PLAYER_UNIT_SPELL_EFFECT)
        call TriggerAddCondition(t,Condition(function Elune_Conditions))
        set t=null
    endfunction

    endscope

 

[muzk]

3) Globadummy is more efficient that just spawn create/kill or remove unit. With a global dummy you can do everything like stuns slows cripples etc using 1 DUMMY ONLY.

hmm isnt necesary but its like a convention: use capital letters for global variables.


30.*Cos(d.angle)
30.*Sin(d.angle)
You can save in your struct these values
eg

set d.cos = 30*Cos(angle)
set d.sin= 30*Sin(angle)

You are spamming GetOwningPlayer(d.caster) in your filter , I recomment to save it in your struct too.

For these periods T32 or KT2 is good (as timer attach)

Sae it in your data: GetUnitAbilityLevel(d.caster,Spell_Id)*90

 

[Laiev]

7) Ya, right, what Kingking said is to you group enum units when missile hit someunit and get one unit of that group (just one) and then stun that unit...

in dota, this really happen

 

[kingkingyyk3]

hmm isnt necesary but its like a convention: use capital letters for global variables.

Better readability, less trouble when editing the spell in future.

 

[DioD]

CAPITALS is for constants, this let you know, this global declared elsewhere and its same for every system. (like PI, ECF, ARRAYSIZE and soo on)

First Capital for Globals, let you see,Global is Soo Global.

no capitals for params, you dont need to null params

sEcond cApital for lOcals, yOu sHoud nUll lOcals

any naming rule is just for you, but, it provide wide range of bonuses if used, since you always see status of variable.

For code, i suggest to use single timer for all instances of spell, this is truly vJass style, your current implementation is not vJass at all.

 

[Zeth]

7) Ya, right, what Kingking said is to you group enum units when missile hit someunit and get one unit of that group (just one) and then stun that unit...

in dota, this really happen

Is not more optimal if units never enter to group?

 

[muzk]

Also, you used ArrowSpeed like a period (time), and speed is distance/time. You should so something like

private constant real SPEED = 700.00
private constant real PERIOD = 0.025

You are leaking a group, you didnt destroy or recycled it. Put it in a global, I think everyone use a global group for things like that.

 

[Laiev]

@Zeth

well, maybe, maybe not... in my map, i found a bug (with a line missile, like Elune Arrow)

when it hit a hero, it should be destroyed but it can hit 2 heroes before the project be removed.

this is the problem :/ it can hit 2 heroes at the same time before the project be removed.

 

[muzk]

@Laiev
Add a boolean (X) to your struct or w/e you was using, put it in your group filter (if not .X) and when your proj hit a unit, set it to true (set .X = true).
I think that would solve your problem.

EDIT
or if you kill your proj, kill it when it hit a unit, so add in your filter the condition proj is alive.