[vJASS] Checking Code in Struct

[FUJI]

Hello hivers, I'm practicing now a Structs in vJASS and I understand them, but Is this code is right?

Tell me I did miss something in the code or mistakes in this code:
Demo Code:

library FireBlow initializer Init


globals
    private hashtable hash = InitHashtable ( )
endglobals
    
globals
    
    private constant integer ABILITY_ID = 'A000'
    private constant integer MISSILE_ID = 'u000'
    private constant string  EXPLOSION_EFFECT = "Abilities\\Spells\\Human\\FlakCannons\\FlakTarget.mdl"
    
endglobals


private function GetMissileDamage takes integer lvl returns real
    return 100. * lvl
endfunction


private function GetMissileSpeed takes integer lvl returns real
    return 10. + 5. * lvl
endfunction


private function GetMissileDetectTarget takes nothing returns real
    return 50.00
endfunction



private struct SpellData


    unit caster
    unit target
    unit missile
    real speed
    real detect
    real damage
    real castX
    real castY
    integer level
    
    // Spell Loop //
    
    static method Loop takes nothing returns nothing
        
        local timer tmr = GetExpiredTimer ( )
        local SpellData this = LoadInteger ( hash, 0, GetHandleId ( tmr ) )
        local real missileX = GetUnitX ( this.missile )
        local real missileY = GetUnitY ( this.missile )
        local real targX = GetUnitX ( this.target )
        local real targY = GetUnitY ( this.target )
        local real angle = Atan2( targY - missileY, targX - missileX )
        local real distance = SquareRoot ( ( targX - missileX ) * ( targX - missileX ) + ( targY - missileY ) * ( targY - missileY ) )         
        local real mx = missileX + this.speed * Cos ( angle )
        local real my = missileY + this.speed * Sin ( angle )
        call SetUnitX ( this.missile, mx )
        call SetUnitY ( this.missile, my )
        call SetUnitFacing ( this.missile, angle * bj_RADTODEG )
        if distance <= this.detect then
            call KillUnit ( this.missile )
            call UnitDamageTarget ( this.caster, this.target, this.damage, true, false, ATTACK_TYPE_CHAOS, DAMAGE_TYPE_NORMAL, null )
            call this.destroy()
            call PauseTimer(tmr)
            call DestroyTimer(tmr)
            set tmr = null
        endif
        set tmr = null
    endmethod
    
    
    static method create takes unit u, unit targ, integer lvl returns SpellData
        
        local timer tmr = CreateTimer()
        local SpellData this = SpellData.allocate()
        local real angle2
        local real targX2
        local real targY2
        set this.caster = u
        set this.target = targ
        set this.level = lvl
        set this.castX = GetUnitX ( this.caster )
        set this.castY = GetUnitY ( this.caster )
        set this.speed = GetMissileSpeed(this.level)
        set this.damage = GetMissileDamage(this.level)
        set this.detect = GetMissileDetectTarget()
        set targX2 = GetUnitX ( this.target )
        set targY2 = GetUnitY ( this.target )
        set angle2 = Atan2 ( targY2 - this.castY, targX2 - this.castX )
        set this.missile = CreateUnit ( GetOwningPlayer ( this.caster ), MISSILE_ID, this.castX, this.castY, angle2 * bj_RADTODEG )
        call SaveInteger ( hash, 0, GetHandleId(tmr), this )
        call TimerStart ( tmr, 0.03125000, true, function SpellData.Loop )
        set tmr = null
        return this
    endmethod


endstruct

// Spell Execution //

private function Action takes nothing returns boolean
    local unit cast
    local unit targ
    if GetSpellAbilityId() == ABILITY_ID then
        
        set cast = GetTriggerUnit ( )
        set targ = GetSpellTargetUnit ( )
        call SpellData.create(cast,targ,GetUnitAbilityLevel(cast,ABILITY_ID))
        set cast = null
        set targ = null
    
    endif
    
    return false
endfunction



// Spell Initialization //
private function Init takes nothing returns nothing
    local trigger trig = CreateTrigger(  )
    
    call TriggerRegisterAnyUnitEventBJ( trig, EVENT_PLAYER_UNIT_SPELL_EFFECT )
    
    call TriggerAddCondition( trig, Condition ( function Action ) )
endfunction

endlibrary

Suggestions is appreciated ^^

 

[Maker]

• Action doesn't need unit variables and could be a method inside the struct
• function Init could be method onInit inside the struct
• Apparently GetWidgetX/Y is faster than the unit counterparts
• You potentially null tmr twice in Loop

Register spell effect event -> Spell Effect Event
Uses a hashtable -> Table
Uses multiple timers with 0.03125 period -> CTL
Jass Proper Application Guide

But yeah, basically you got it right.

 

[FUJI]

^ Thanks Maker .

 

[Lender]

change this
if distance <= this.detect then
to
if (targX - missileX)*(targX - missileX)+(targY - missileY)*(targY - missileY)<=this.detect*this.detect then
it's way faster

also u can simply use .anithing instead of this.anithing

saves u from a lot writing

and inline these
local real mx = missileX + this.speed * Cos ( angle ) local real my = missileX + this.speed * Sin ( angle )

 

[TriggerHappy]

Looks good so far

• libraryshould be scope, unless this spell requires other libraries.
• I would suggest TimerUtils (for timer recycling)

 

[muzzel]

• libraryshould be scope, unless this spell requires other libraries.

Why? As far as i can see it doesnt really make a difference, i use libraries for everything...

 

[TriggerHappy]

Libraries are placed above scopes IIRC when jasshelper compiles, so it would be best to use scopes for spells which require no other libraries.

 

[Maker]

Yeah, libs are compiled before scopes.

 

[muzzel]

i still dont understand the benefit

 

[PurgeandFire]

There is no benefit to using a scope over a library (at least, there is no *practical* benefit. technically scope inits are called directly as a function call whereas lib inits are called via ExecuteFunc()).

It is just a matter of preference. A few years ago it was common practice to place any non-system in a scope (since you usually only needed encapsulation and the initializer), and it would be compiled after libraries. Nowadays it goes either way, but people tend to use libraries for everything (scopes are more-or-less deprecated) since libraries have the advantage of explicit requirements. I'm guessing this came about because some ppl suck at documenting.

tl;dr use either, no practical difference (although if you use a scope, then please add documentation of what libraries the spell requires).

 

[FUJI]

Thanks for all feedbacks!, I appreciate it ^^. +Rep

 

[Malhorne]

Use thistype instead of the name of the struct when calling static methods or when you're typing
local SpellData this -> local thistype this

 

[FUJI]

^ Thanks

 

[Malhorne]

You're welcome dude