[JASS] JASS spell created with GUI mindset - what to improve

[Maker]

So I decided to try some JASS. I'm used to creating GUI spells so I just went with that logic with this spell.

The spell is a simple ground slam / knockback effetc with simple collision detection. It's just for practise.

My question is how to improve it so the code is better? All comments are appreciated.

Code

scope GrounPound initializer Init_Ground_Pound

    globals
        
        // The ability's rawcode
        private constant integer ABILITYCODE = 'A000'
        // Period for knockback loop
        private constant real LOOP_PERIOD = 0.03
        // AoE of the slam
        private constant real AOE = 350.
        // Base damage
        private constant real BASE_DMG = 50.
        // Bonus damage per ability level, you don't get this at level 1
        private constant real BONUS_DMG = 25.
        // Initial speed for knockback
        private constant real INITIAL_SPEED = 30.
        // Speed reduction per loop, 0,9 means 10 % reduction
        private constant real SPEED_REDUCTION = 0.93
        // Minimum speed limit to prevent long loopin times
        private constant real MIN_SPEED = 1.
        
        private constant attacktype ATTACK_TYPE = ATTACK_TYPE_NORMAL
        private constant damagetype DAMAGE_TYPE = DAMAGE_TYPE_NORMAL
        
        private constant string SLAM_GROUND_EFFECT = "Abilities\\Weapons\\DemolisherFireMissile\\DemolisherFireMissile.mdl"
        
        private item PATH_ITEM
        private timer LOOP_TIMER
        private hashtable HASH
        private group GROUP
        private group KNOCKBACKGROUP
        private player PLAYER
        private real REAL1
        private real REAL2
        private real REAL3
        private unit UNIT
        private integer INTEGER

    endglobals
    
    
    // Damage equation
    private function getDamage takes integer level returns real
        return BASE_DMG + I2R(level - 1) * BONUS_DMG
    endfunction

    
    // Returns the angle between two points
    private function getAngle takes real x1, real x2, real y1, real y2 returns real
        return Atan2(y2-y1 , x2-x1)
    endfunction
    
    
    // Return the distance between points
    private function getDistance takes real x1, real x2, real y1, real y2 returns real
        local real dx = x2 - x1
        local real dy = y2 - y1
        return SquareRoot(dx * dx + dy * dy)
    endfunction
    
    
    // Filters unwanted units from getting knocked back
    // Also damages the units
    private function unitFilter takes nothing returns boolean
        local unit t = GetFilterUnit()
        set INTEGER = GetHandleId(t)
        
        if not(IsUnitType( t , UNIT_TYPE_DEAD )) and IsUnitEnemy( t , PLAYER ) then
            call UnitDamageTarget( UNIT , t , REAL3 , false , false , ATTACK_TYPE , DAMAGE_TYPE , WEAPON_TYPE_WHOKNOWS )
            call GroupAddUnit( KNOCKBACKGROUP , t )
            call SaveReal(HASH , INTEGER, StringHash("speed") , INITIAL_SPEED )
            call SaveReal(HASH , INTEGER, StringHash("reduction") , 1.00 )
            call SaveReal(HASH , INTEGER, StringHash("angle") , getAngle( REAL1 , GetUnitX(t) , REAL2 , GetUnitY(t) ) )
        endif
            
        set t = null
        return  true
    endfunction
    
    
    // Knocks back units
    private function knockback takes nothing returns nothing
        local unit u = GetEnumUnit()
        local integer ID = GetHandleId(u)
        local real r1 = LoadReal( HASH , ID , StringHash("speed") )
        local real r2
        local real r3
        local real r4
        local real r5
        local real r6
        local location l
        
        if not(IsUnitType( u , UNIT_TYPE_DEAD )) and r1 > MIN_SPEED then
        
            set r4 = LoadReal( HASH , ID , StringHash("angle") )
            set r5 = LoadReal( HASH , ID , StringHash("reduction") )
            set r6 = INITIAL_SPEED * (1 - (1 - r5))
            
            set r2 = GetUnitX(u) + r6 * Cos(r4)
            set r3 = GetUnitY(u) + r6 * Sin(r4)
            
            set l = Location(r2, r3)
            
            call SetItemVisible( PATH_ITEM , true )
            call SetItemPositionLoc( PATH_ITEM , l )
            
            if GetItemX(PATH_ITEM) == r2 and GetItemY(PATH_ITEM) == r3 then
                call SetUnitX( u , r2 )
                call SetUnitY( u , r3 )
                call SaveReal( HASH , ID , StringHash("reduction") , r5 * 0.9 )
            else 
                set r6 = 0
            endif
            call SaveReal( HASH , ID , StringHash("speed") , r6 )
            call SetItemVisible( PATH_ITEM , false )
            call RemoveLocation( l )
            
        else
            call GroupRemoveUnit( KNOCKBACKGROUP , u )
            call FlushChildHashtable( HASH , ID )
            if CountUnitsInGroup( KNOCKBACKGROUP ) == 0 then
                call PauseTimer(LOOP_TIMER)
            endif
        endif
        set u = null
        
    endfunction
    
    
    // Periodic timer for knockback trigger
    private function loopExpire takes nothing returns nothing
        call ForGroup( KNOCKBACKGROUP , function knockback )
    endfunction
    
    
    // Checks that the spell casted is the correct one
    private function Conditions takes nothing returns boolean
        return GetSpellAbilityId() == ABILITYCODE
    endfunction

    
    // Sets up the ability
    private function Actions takes nothing returns nothing
        
        set UNIT = GetTriggerUnit()
        set REAL1 = GetUnitX(UNIT)
        set REAL2 = GetUnitY(UNIT)
        set PLAYER = GetOwningPlayer(UNIT)
        set GROUP = CreateGroup()
        set REAL3 = getDamage(GetUnitAbilityLevelSwapped( ABILITYCODE , UNIT))
        
        call GroupEnumUnitsInRange( GROUP, REAL1 , REAL2 , AOE , Filter(function unitFilter) )
        
        if CountUnitsInGroup(KNOCKBACKGROUP) > 0 then
            call TimerStart( LOOP_TIMER , LOOP_PERIOD , true , function loopExpire )
        endif
        
        call TerrainDeformRipple( REAL1 , REAL2, AOE , 100 , 700 , 1 , 2 * 350 / 512 , 2 * 0.7 / 0.7, 32/350 , false )
        call DestroyEffect(AddSpecialEffect( SLAM_GROUND_EFFECT , REAL1 , REAL2 ) )
        call DestroyGroup(GROUP)
        
    endfunction


    function Init_Ground_Pound 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)
        
        // ** SET GLOBALS **
        // Periodic timer for knockback group
        set LOOP_TIMER = CreateTimer()
        // Set global group for units that are knocked back
        set KNOCKBACKGROUP = CreateGroup()
        // Create hashtable
        set HASH = InitHashtable()
        set PATH_ITEM = CreateItem( 'ratc' , 0 , 0 )
        call SetItemVisible( PATH_ITEM , false )
        
    endfunction
    
endscope

 

[watermelon_1234]

The main code fixes

• You don't need I2R in the getDamage function since there are already two reals there.
• Don't use locations when you can use coordinates. Instead of SetItemPositionLoc use SetItemPosition
• Use GetUnitAbilityLevel instead of the BJ.
• You do not need to destroy global groups. Just set GROUP to CreateGroup in Init_Ground_Pound

Other misc.

• You can avoid using CountUnitsInGroup by having a global integer variable store the amount of units in KNOCKBACKGROUP
• INTEGER isn't really useful since you could easily use a local variable in the unitFilter function.
• Generally, only constant globals are put in all caps.
• You can use GetTriggerPlayer() instead of GetOwningPlayer(UNIT)
• You're not using getDistance.
• Init_Ground_Pound could be private.

 

[Maker]

Thank you.

I made most of the suggested changes. Maybe I'll add some things to the spell and use it in the campaign I'm making afterall. More custom spells never hurt anyone.

 

[Adiktuz]

@watermelon_1234: GetTriggerPlayer() also works for Player unit events?

@Maker: is the trigger in the first post the edited one already?

you can already do this things in the globals block rather than make it in the init trigger...

// ** SET GLOBALS **
        
        set LOOP_TIMER = CreateTimer()
        // Set global group for units that are knocked back
        set KNOCKBACKGROUP = CreateGroup()
        // Create hashtable
        set HASH = InitHashtable()

just do this

// ** SET GLOBALS **
        // Periodic timer for knockback group
        private timer LOOP_TIMER = CreateTimer()
        // Set global group for units that are knocked back
        private group KNOCKBACKGROUP = CreateGroup()
        // Create hashtable
        private hashtable HASH = InitHashtable()

 

[watermelon_1234]

@watermelon_1234: GetTriggerPlayer() also works for Player unit events?

Yes. It's something neat I learned from Bribe who learned it from Pharaoh_. =P

 

[Maker]

I've got a problem with this. It works in a normal map, but when I try to test a campaign map, it throws me to the game menu just when it starts loading the map.

This is the code now.

Code

scope GroundPound initializer InitTrig_GroundPound

    globals
        
        // The ability's rawcode
        private constant integer ABILITYCODE = 'AHfs'
        // Period for knockback loop
        private constant real LOOP_PERIOD = 0.03
        // AoE of the slam
        private constant real AOE = 350.
        // Base damage
        private constant real BASE_DMG = 50.
        // Bonus damage per ability level, you don't get this at level 1
        private constant real BONUS_DMG = 25.
        // Initial speed for knockback
        private constant real INITIAL_SPEED = 30.
        // Speed reduction per loop, 0,9 means 10 % reduction
        private constant real SPEED_REDUCTION = 0.9
        // Minimum speed limit to prevent long loopin times
        private constant real MIN_SPEED = 1.
        
        private constant attacktype ATTACK_TYPE = ATTACK_TYPE_NORMAL
        private constant damagetype DAMAGE_TYPE = DAMAGE_TYPE_NORMAL
        
        private constant string SLAM_GROUND_EFFECT = "Abilities\\Weapons\\DemolisherFireMissile\\DemolisherFireMissile.mdl"
        
        // Periodic timer for knockback group
        private timer LOOP_TIMER = CreateTimer()
        // Set global group for units that are knocked back
        private group KNOCKBACKGROUP = CreateGroup()
        // Create hashtable
        private hashtable HASH = InitHashtable()
        
        private item PATH_ITEM
        private group GROUP
        private player PLAYER
        private real REAL1
        private real REAL2
        private real REAL3
        private unit UNIT
        private integer INTEGER

    endglobals
    
    
    // Damage equation
    private function getDamage takes integer level returns real
        return BASE_DMG + (level - 1) * BONUS_DMG
    endfunction

    
    // Returns the angle between two points
    private function getAngle takes real x1, real x2, real y1, real y2 returns real
        return Atan2(y2-y1 , x2-x1)
    endfunction
    
    
    // Return the distance between points
    private function getDistance takes real x1, real x2, real y1, real y2 returns real
        local real dx = x2 - x1
        local real dy = y2 - y1
        return SquareRoot(dx * dx + dy * dy)
    endfunction
    
    
    // Filters unwanted units from getting knocked back
    // Also damages the units
    private function unitFilter takes nothing returns boolean
        local unit t = GetFilterUnit()
        set INTEGER = GetHandleId(t)
        
        if not(IsUnitType( t , UNIT_TYPE_DEAD )) and IsUnitEnemy( t , PLAYER ) then
            call UnitDamageTarget( UNIT , t , REAL3 , false , false , ATTACK_TYPE , DAMAGE_TYPE , WEAPON_TYPE_WHOKNOWS )
            call GroupAddUnit( KNOCKBACKGROUP , t )
            call SaveReal(HASH , INTEGER, StringHash("speed") , INITIAL_SPEED )
            call SaveReal(HASH , INTEGER, StringHash("reduction") , 1.00 )
            call SaveReal(HASH , INTEGER, StringHash("angle") , getAngle( REAL1 , GetUnitX(t) , REAL2 , GetUnitY(t) ) )
        endif
            
        set t = null
        return  true
    endfunction
    
    
    // Knocks back units
    private function knockback takes nothing returns nothing
        local unit u = GetEnumUnit()
        local integer ID = GetHandleId(u)
        local real r1 = LoadReal( HASH , ID , StringHash("speed") )
        local real r2
        local real r3
        local real r4
        local real r5
        local real r6
        
        if not(IsUnitType( u , UNIT_TYPE_DEAD )) and r1 > MIN_SPEED then
        
            set r4 = LoadReal( HASH , ID , StringHash("angle") )
            set r5 = LoadReal( HASH , ID , StringHash("reduction") )
            set r6 = INITIAL_SPEED * (1 - (1 - r5))
            
            set r2 = GetUnitX(u) + r6 * Cos(r4)
            set r3 = GetUnitY(u) + r6 * Sin(r4)
            
            call SetItemVisible( PATH_ITEM , true )
            call SetItemPosition( PATH_ITEM , r2 , r3 )
            
            if GetItemX(PATH_ITEM) == r2 and GetItemY(PATH_ITEM) == r3 then
                call SetUnitX( u , r2 )
                call SetUnitY( u , r3 )
                call SaveReal( HASH , ID , StringHash("reduction") , r5 * SPEED_REDUCTION )
            else 
                set r6 = 0
            endif
            call SaveReal( HASH , ID , StringHash("speed") , r6 )
            call SetItemVisible( PATH_ITEM , false )
            
        else
            call GroupRemoveUnit( KNOCKBACKGROUP , u )
            call FlushChildHashtable( HASH , ID )
            if CountUnitsInGroup( KNOCKBACKGROUP ) == 0 then
                call PauseTimer(LOOP_TIMER)
            endif
        endif
        set u = null
        
    endfunction
    
    
    // Periodic timer for knockback trigger
    private function loopExpire takes nothing returns nothing
        call ForGroup( KNOCKBACKGROUP , function knockback )
    endfunction
    
    
    // Checks that the spell casted is the correct one
    private function Conditions takes nothing returns boolean
        return GetSpellAbilityId() == ABILITYCODE
    endfunction

    
    // Sets up the ability
    function GP_Actions takes nothing returns nothing
        
        set UNIT = GetTriggerUnit()
        set REAL1 = GetUnitX(UNIT)
        set REAL2 = GetUnitY(UNIT)
        set PLAYER = GetOwningPlayer(UNIT)
        set GROUP = CreateGroup()
        set REAL3 = getDamage(GetUnitAbilityLevel( UNIT , ABILITYCODE))
        
        call GroupEnumUnitsInRange( GROUP, REAL1 , REAL2 , AOE , Filter(function unitFilter) )
        
        if CountUnitsInGroup(KNOCKBACKGROUP) > 0 then
            call TimerStart( LOOP_TIMER , LOOP_PERIOD , true , function loopExpire )
        endif
        
        call TerrainDeformRipple( REAL1 , REAL2, AOE , 100 , 700 , 1 , 2 * 350 / 512 , 2 * 0.7 / 0.7, 32/350 , false )
        call DestroyEffect(AddSpecialEffect( SLAM_GROUND_EFFECT , REAL1 , REAL2 ) )
        call DestroyGroup(GROUP)
        
    endfunction


    function InitTrig_GroundPound 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 GP_Actions)

        set PATH_ITEM = CreateItem( 'ratc' , 0 , 0 )
        call SetItemVisible( PATH_ITEM , false )
        
    endfunction
    
endscope

There's nothing in the campaign except one map with this spell in it.

Syntax check gives me an error at line 1 already, "Syntax error" and then it gives "Statement outside of function" for just about every line.

 

[watermelon_1234]

You'll need to somehow parse the campaign's maps with vJass another way since simply saving the campaign in the campaign editor won't work. (JassHelper can't compile the vJass...)

This thread has a thread if you still want to use vJass in campaigns.