[vJASS] Help with code optimization

[Linaze]

Hello. I don't really have a problem as much as a question. I have made a simple ability, and it's the first time I've made an ability using structs, so I wonder how to best improve and optimize my code.

scope RadarScan initializer Init
    //-----------------------------------
    //--! CONFIG CONSTANTS !-------------
    //-----------------------------------
    globals
        private constant integer    ABILITY_ID              = 'A014'
        private constant integer    DETECTION_ABILITY_ID    = 'A00Y'
        
        private constant real       DURATION                = 15.0
        
        private constant string     DETECT_EFFECT           = "Abilities\\Spells\\Other\\Andt\\Andt.mdl"
    endglobals
    
    //-----------------------------------
    //--! CODE !-------------------------
    //-----------------------------------
    struct Scan_Data
        unit u
        real d
    endstruct
    
    globals
        private timer timerExpiration
        private Scan_Data array ar
        private integer totalUnits = 0
    endglobals
    
    private function Counter takes nothing returns nothing
        local Scan_Data sd = Scan_Data.create()
        local integer i = 0
        
        loop
            exitwhen i >= totalUnits
            set sd = ar[i]
            set sd.d = sd.d - 1.0
            
            if sd.d <= 0.0 then
                call UnitRemoveAbility(sd.u,DETECTION_ABILITY_ID)
                set ar[i] = ar[totalUnits - 1]
                set totalUnits = totalUnits - 1
                call sd.destroy()
            endif
            
            set i = i + 1
        endloop
        
        if totalUnits == 0 then
            call PauseTimer(timerExpiration)
        endif
    endfunction
    
    private function OnSpellEffect takes nothing returns nothing
        local Scan_Data sd = Scan_Data.create()
        
        set sd.u = GetTriggerUnit()
        set sd.d = DURATION
        call UnitAddAbility(sd.u,DETECTION_ABILITY_ID)
        call DestroyEffect(AddSpecialEffect(DETECT_EFFECT,GetUnitX(sd.u),GetUnitY(sd.u)))
        if totalUnits == 0 then
            call TimerStart(timerExpiration,1.0,true,function Counter)
        endif
        set totalUnits = totalUnits + 1
        set ar[totalUnits - 1] = sd
    endfunction
    
    private function SpellIdMatch takes nothing returns boolean
        return GetSpellAbilityId() == ABILITY_ID
    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 SpellIdMatch))
        call TriggerAddAction(t,function OnSpellEffect)
        set timerExpiration = NewTimer()
        
        set t = null
    endfunction
endscope

Thanks in advance.

 

[watermelon_1234]

You could actually have the d member in the struct be initially set to DURATION so you don't need to set it in the function OnSpellEffect.

Don't set sd in function Counter to anything; it just wastes creating a struct that you'll never use.

Also, there's really no point in making timerExpiration a NewTimer since you're treating it as a global timer and will never recycle it. Stick with CreateTimer instead.

The rest of this is rather nitpicky.

In function Counter:

                set ar[i] = ar[totalUnits - 1]
                set totalUnits = totalUnits - 1

->

                set totalUnits = totalUnits - 1
                set ar[i] = ar[totalUnits]

In function OnSpellEffect

        set totalUnits = totalUnits + 1
        set ar[totalUnits - 1] = sd

->

set ar[totalUnits] = sd
set totalUnits = totalUnits + 1

 

[mckill2009]

I assumed that you use TU coz of NewTimer(), so where's GetTimerData, SetTimerData and ReleaseTimer???...

Make private function >>> static method, put it under the struct...then use thistype to reffer to the instances of the struct...then you can call TimerStart like...

call SetTimerData(timerExpiration, sd)
call TimerStart(timerExpiration,1.0,true,function thistype.Counter)

 

[Linaze]

Okay, thank you. Is this better, then?

scope RadarScan initializer Init
    //-----------------------------------
    //--! CONFIG CONSTANTS !-------------
    //-----------------------------------
    globals
        private constant integer    ABILITY_ID              = 'A014'
        private constant integer    DETECTION_ABILITY_ID    = 'A00Y'
        
        private constant real       DURATION                = 15.0
        
        private constant string     DETECT_EFFECT           = "Abilities\\Spells\\Other\\Andt\\Andt.mdl"
    endglobals
    
    //-----------------------------------
    //--! CODE !-------------------------
    //-----------------------------------
    struct Scan_Data
        unit u
        real d = DURATION
    endstruct
    
    globals
        private timer timerExpiration
        private Scan_Data array ar
        private integer totalUnits = 0
    endglobals
    
    private function Counter takes nothing returns nothing
        local integer i = 0
        
        loop
            exitwhen i >= totalUnits
            set ar[i].d = ar[i].d - 1.0
            
            if ar[i].d <= 0.0 then
                call UnitRemoveAbility(ar[i].u,DETECTION_ABILITY_ID)
                set totalUnits = totalUnits - 1
                set ar[i] = ar[totalUnits]
                call ar[i].destroy()
            endif
            
            set i = i + 1
        endloop
        
        if totalUnits == 0 then
            call PauseTimer(timerExpiration)
        endif
    endfunction
    
    private function OnSpellEffect takes nothing returns nothing
        local Scan_Data sd = Scan_Data.create()
        
        set sd.u = GetTriggerUnit()
        call UnitAddAbility(sd.u,DETECTION_ABILITY_ID)
        call DestroyEffect(AddSpecialEffect(DETECT_EFFECT,GetUnitX(sd.u),GetUnitY(sd.u)))
        
        if totalUnits == 0 then
            call TimerStart(timerExpiration,1.0,true,function Counter)
        endif
        
        set ar[totalUnits] = sd
        set totalUnits = totalUnits + 1
    endfunction
    
    private function SpellIdMatch takes nothing returns boolean
        return GetSpellAbilityId() == ABILITY_ID
    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 SpellIdMatch))
        call TriggerAddAction(t,function OnSpellEffect)
        set timerExpiration = CreateTimer()
        
        set t = null
    endfunction
endscope

@mckill, I'm afraid I have no idea what you're talking about.

 

[Justify]

There is still a lot of stuff to do:
Make the struct private, just for the case.
Insert your globals into the struct, now they are named "private static".
Insert your functions into the struct, now they are named "methods".
Take the advantage of using methods and static methods.
Don't create a timer, recycle it dynamicly.

 

[Magtheridon96]

Don't use actions, just use conditions..
Put all your actions code inside the condition with an if/then/endif that evaluates the condition and then return false at the end.

 

[Linaze]

There is still a lot of stuff to do:
Make the struct private, just for the case.
Insert your globals into the struct, now they are named "private static".
Insert your functions into the struct, now they are named "methods".
Take the advantage of using methods and static methods.
Don't create a timer, recycle it dynamicly.

Could you elaborate on this? I have never done anything like what you mention.

Don't use actions, just use conditions..
Put all your actions code inside the condition with an if/then/endif that evaluates the condition and then return false at the end.

I could, of course, but is it really faster? I've seen a lot of people use both actions and conditions.

 

[Justify]

Static variables are normal globals. However, it looks neater as it's more structured.
Methods are functions that can only be used with a struct instance and will automaticly have acces to it's members. Static methods are structured functions. At your code it would be this:

scope RadarScan initializer Init
    //-----------------------------------
    //--! CONFIG CONSTANTS !-------------
    //-----------------------------------
    globals
        private constant integer    ABILITY_ID              = 'A014'
        private constant integer    DETECTION_ABILITY_ID    = 'A00Y'
        
        private constant real       DURATION                = 15.0
        
        private constant string     DETECT_EFFECT           = "Abilities\\Spells\\Other\\Andt\\Andt.mdl"
    endglobals
    
    //-----------------------------------
    //--! CODE !-------------------------
    //-----------------------------------
    struct Scan_Data
        unit u
        real d = DURATION
    

        private static timer timerExpiration
        private static Scan_Data array ar
        private static integer totalUnits = 0
    
        static method Counter takes nothing returns nothing
            local integer i = 0
            local Scan_Data this
            
            loop
                exitwhen i >= totalUnits
                set this = ar[i]
                set .d = .d - 1.0
                
                if .d <= 0.0 then
                    call UnitRemoveAbility(.u,DETECTION_ABILITY_ID)
                    set totalUnits = totalUnits - 1
                    set ar[i] = ar[totalUnits]
                    call .destroy()
                endif
                
                set i = i + 1
            endloop
            
            if totalUnits == 0 then
                call ReleaseTimer(timerExpiration)
            endif
        endmethod
    
        static method OnSpellEffect takes nothing returns boolean
            local Scan_Data this
            
            if GetSpellAbilityId() == ABILITY_ID then
                set this = Scan_Data.create()
                set .u = GetTriggerUnit()
                call UnitAddAbility(.u,DETECTION_ABILITY_ID)
                call DestroyEffect(AddSpecialEffect(DETECT_EFFECT,GetUnitX(.u),GetUnitY(.u)))
                
                if totalUnits == 0 then
                    set timerExpiration = NewTimer()
                    call TimerStart(timerExpiration,1.0,true,function thistype.Counter)
                endif
                
                set ar[totalUnits] = this
                set totalUnits = totalUnits + 1
            endif
            
            return false
        endmethod
        
        static method onInit takes nothing returns nothing
            local trigger t = CreateTrigger()
            
            call TriggerRegisterAnyUnitEventBJ(t,EVENT_PLAYER_UNIT_SPELL_EFFECT)
            call TriggerAddCondition(t,Condition(function thistype.OnSpellEffect))
        endmethod
    endstruct
endscope

Didn't test it, hope I did no mistakes.

 

[Linaze]

Okay, that answers several questions while raising a few new ones.
What does the prefix "static" do?
Won't the compiler complain about the lack of an "Init" function?
What do I win by using your method instead of the one I used, speed?

 

[mckill2009]

IMO, you should index a unit so that the ability will not dissapear when the first duration runs out...

EDIT: btw, set the Custom value of your hero to zero first...

EDIT 2: Justify already explained what I mean above...

scope RadarScan initializer Init
    //-----------------------------------
    //--! CONFIG CONSTANTS !-------------
    //-----------------------------------
    globals
        private constant integer    ABILITY_ID              = 'A014'
        private constant integer    DETECTION_ABILITY_ID    = 'A00Y'
        
        private constant real       DURATION                = 15.0
        
        private constant string     DETECT_EFFECT           = "Abilities\\Spells\\Other\\Andt\\Andt.mdl"
    endglobals
    
    //-----------------------------------
    //--! CODE !-------------------------
    //-----------------------------------
    struct Scan_Data
        unit u
        real d = DURATION
    endstruct
    
    globals
        private timer timerExpiration
        private Scan_Data array ar
        private integer totalUnits = 0
    endglobals
    
    private function Counter takes nothing returns nothing
        local integer i = 0
        
        loop
            exitwhen i >= totalUnits
            if ar[i].d > 0.0 then
                set ar[i].d = ar[i].d - 1.0
            else
                call SetUnitUserData(ar[i].u, GetUnitUserData(ar[i].u)-1)
                if GetUnitUserData(ar[i].u)==0 then 
                    call UnitRemoveAbility(ar[i].u,DETECTION_ABILITY_ID)
                endif
                set totalUnits = totalUnits - 1
                set ar[i] = ar[totalUnits]
                call ar[i].destroy()
            endif
            
            set i = i + 1
        endloop
        
        if totalUnits == 0 then
            call PauseTimer(timerExpiration)
        endif
    endfunction
    
    private function OnSpellEffect takes nothing returns nothing
        local Scan_Data sd = Scan_Data.create()
        
        set sd.u = GetTriggerUnit()
        call SetUnitUserData(sd.u, GetUnitUserData(sd.u)+1) 
        call UnitAddAbility(sd.u,DETECTION_ABILITY_ID)
        call DestroyEffect(AddSpecialEffect(DETECT_EFFECT,GetUnitX(sd.u),GetUnitY(sd.u)))
        
        if totalUnits == 0 then
            call TimerStart(timerExpiration,1.0,true,function Counter)
        endif
        
        set ar[totalUnits] = sd
        set totalUnits = totalUnits + 1
    endfunction
    
    private function SpellIdMatch takes nothing returns boolean
        return GetSpellAbilityId() == ABILITY_ID
    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 SpellIdMatch))
        call TriggerAddAction(t,function OnSpellEffect)
        set timerExpiration = CreateTimer()
        
        set t = null
    endfunction
endscope

 

[Justify]

Okay, that answers several questions while raising a few new ones.
What does the prefix "static" do?
Won't the compiler complain about the lack of an "Init" function?
What do I win by using your method instead of the one I used, speed?

In theory static is the most dumb keyword ever, as it makes the compiler ignore structs. Normaly each variable is "created" once for each instance. Methods have the possibility of using it for each instance.

struct bla
   int a

   method xyz takes nothing returns nothing
   endmethod
endstruct

Now "a" is created for each instance. "xyz" has to be used with an instance, for example: myInstance.xyz() what automaticly calls the method (function) with a reference towards myInstance called "this".
Static makes the compiler ignore the fact that you use structs. A static variable is a normal gobal and a static method is a normal funtion.
However, it looks better.

Yes, he will complain about the Init function. Forgot to delete it. Smart guy

The speed is nearly the same. However, when perfectionating the way I use you will see that in some situations coding will be a LOT easier. The only speed improvement is the killed action as actions are realy useless, just a call more.

 

[Linaze]

So how does this look?

scope RadarScan
    //=====================================================
    //CONFIG CONSTANTS:
    //
    //ABILITY_ID            -   Raw ID of the triggering ability
    //DETECTION_ABILITY_ID  -   Raw ID of the detection-yielding ability
    //DURATION              -   The duration of the ability
    //DETECT_EFFECT         -   The path to the effect model
    //
    //=====================================================
    globals
        private constant integer    ABILITY_ID              = 'A014'
        private constant integer    DETECTION_ABILITY_ID    = 'A00Y'
        
        private constant real       DURATION                = 15.0
        
        private constant string     DETECT_EFFECT           = "Abilities\\Spells\\Other\\Andt\\Andt.mdl"
    endglobals
    
    //=====================================================
    //CODE:
    //Don't touch anything below unless you know what you're doing.
    //
    //=====================================================
    struct Scan_Data
        unit u
        real d = DURATION

        static timer timerExpiration
        static Scan_Data array ar
        static integer totalUnits = 0

        static method Counter takes nothing returns nothing
            local integer i = 0
            local Scan_Data this 
            
            loop
                exitwhen i >= totalUnits
                set this = ar[i]
                set .d = .d - 1.0
                
                if .d <= 0.0 then
                    call UnitRemoveAbility(.u,DETECTION_ABILITY_ID)
                    set totalUnits = totalUnits - 1
                    set ar[i] = ar[totalUnits]
                    call destroy()
                endif
                
                set i = i + 1
            endloop
            
            if totalUnits == 0 then
                call ReleaseTimer(timerExpiration)
            endif
        endmethod
        
        static method OnSpellEffect takes nothing returns boolean
            local Scan_Data this = Scan_Data.create()
        
            if GetSpellAbilityId() == ABILITY_ID then
                set this = Scan_Data.create()
                set .u = GetTriggerUnit()
                
                call UnitAddAbility(.u,DETECTION_ABILITY_ID)
                call DestroyEffect(AddSpecialEffect(DETECT_EFFECT,GetUnitX(.u),GetUnitY(.u)))
                
                if totalUnits == 0 then
                    set timerExpiration = NewTimer()
                    call TimerStart(timerExpiration,1.0,true,function thistype.Counter)
                endif
                
                set ar[totalUnits] = this
                set totalUnits = totalUnits + 1
            endif
            
            return false
        endmethod

        static method onInit takes nothing returns nothing
            local trigger t = CreateTrigger()
            
            call TriggerRegisterAnyUnitEventBJ(t,EVENT_PLAYER_UNIT_SPELL_EFFECT)
            call TriggerAddCondition(t,Condition(function thistype.OnSpellEffect))
            
            set t = null
        endmethod
    endstruct
endscope

Also, a question: Should I code like this every time I use a struct in the code or are there cases where I shouldn't? And when a struct is not used, should I just for for regular functions and globals then?

 

[watermelon_1234]

It's better but the struct should be declared as private (just to avoid conflicting if you have another struct somewhere named Scan_Data).

In OnSpellEffect, do not initialize this or else you'll end up wasting a struct instance.

In the static method Counter, I'm pretty sure you need to have a set i = i - 1 at the end of the if .d <= 0.0 or else it wouldn't check the struct instance that was set.


The way you've coded your struct, you're using one timer to check all the instances of your struct.
It can be done another way with TimerUtils where there's one timer for each struct instance. (You can learn more about that here)
If your code depends on precision with its timers, you'll probably want to use the latter way. If it doesn't, the way you've coded it will be fine.
When a struct isn't needed, it's fine to stick with globals and functions.

 

[Linaze]

I made the struct private now. I'm not exactly sure what you mean by not initailizing this, and as for having to reduce the value of i by one I don't know, the spell seems to be functioning just as intended when testing.

I'll look into that about timers.

 

[watermelon_1234]

I meant

local Scan_Data this = Scan_Data.create()

Just don't assign it to anything.

Basically, the struct that replaced the destroyed one gets skipped in the loop.
So, the bug with not reducing i would be that it would delay .d = .d - 1.0 by one second for that struct instance, making the unit have the ability longer by one second.

 

[Linaze]

I still don't quite get how I can just reduce i by 1, wouldn't that mess up the loop or something?

 

[watermelon_1234]

Reducing i by 1 doesn't mess up the loop, it just resets the loop so that the struct instance it will be properly looped. You only need to reduce i by 1 when a struct gets destroyed.
This should make it clearer.

 

[Linaze]

Okay, I read that tutorial and I think I've grasped the basic idea now, but I'm still a bit confused. In that tutorial, the author uses ".allocate", which I never did in my code, and no one mentioned that either. So if you or someone else could elaborate on that, I'd be very happy.

 

[watermelon_1234]

Okay, so basically "allocate" is a static method that all structs have and use to get a private id for their instances. It's pretty much a basic form of "create" since it just gives an instance of the struct. In your code, you used "create," but since you didn't make a custom create method, it would actually call "allocate."

Generally, "create" is used to make a new instance of the struct since we're able to override that method. Usually this would be done to set members of the struct instance.

For example, you could have done something like this:

static method create takes unit u returns thistype // This method must return the struct's type.
    local thistype this = thistype.allocate() // Get a struct instance with a private id. You have to use allocate for this.
    set .u = u
    return this
endmethod

...

set this = Scan_Data.create(GetTriggerUnit())