[Solved] Spell struct

[Arsenal]

I began to study "struct" and the methods of execution. I tried to create spells, but it works partially, I mean that only part of the code is executed. The essence is that a caster shoots an arrow, when hit by an enemy, damage is caused depending on the distance between them, after the damage is inflicted, the enemy begins several times. The trigger only works before the damage is inflicted. That is, a text is created with damage caused and everything, even the text is not destroyed but just flies up. I do not ask you to judge too much, since I suspect there are many mistakes in it, but still I ask you to tell me where the error is and why the trigger is not being executed. Thank you in advance.

library BashArrow

    globals
        private constant integer SPELL_ID = 'A02L'
        private    constant integer DUMMY_SPELL = 'A03S'
        private constant integer BUFF_ID = 'B014'
        private constant integer DUMMY = 'u001'
    endglobals
   
    function BACon takes nothing returns boolean
        return GetSpellAbilityId() == SPELL_ID
    endfunction
   
    struct BashArrow
        unit caster
        unit target
        integer lvl
        location loc
        location loc1
        static method create takes unit u, unit s, integer l, location f1, location f2 returns BashArrow
            local BashArrow this = BashArrow.allocate()
            set this.caster = u
            set this.target = s
            set this.lvl = l
            set this.loc = f1
            set this.loc1 = f2
            return this
        endmethod
        method destroy takes nothing returns nothing
            call this.deallocate()
        endmethod
        method onDestroy takes nothing returns nothing
            set caster = null
            set target = null
            set lvl = 0
            call RemoveLocation(loc)
            call RemoveLocation(loc1)
            set loc = null
            set loc1 = null
        endmethod
        method BaSt takes nothing returns nothing
            local unit d
            local integer i
            local texttag lct
            local real distdmg
            if GetUnitAbilityLevel(this.target, BUFF_ID) > 0 and GetUnitState(this.target, UNIT_STATE_LIFE)>0 then
                call PauseTimer(GetExpiredTimer())
                set distdmg = DistanceBetweenPoints(this.loc, this.loc1)*(.10*this.lvl)
                set lct = CreateTextTagUnit(I2S(R2I(distdmg)), this.target, 0, 50, 70)
                call UnitDamageTarget(this.caster, this.target, distdmg, true, false, ATTACK_TYPE_HERO, DAMAGE_TYPE_MAGIC, null)
                set d = CreateUnit(GetOwningPlayer(this.caster), DUMMY, GetUnitX(this.target), GetUnitY(this.target), 270)
                call UnitAddAbility(d, DUMMY_SPELL)
                call SetUnitAbilityLevel(d, DUMMY_SPELL, this.lvl)
                call UnitApplyTimedLife(d, 'BTLF', 1)
                call SetUnitPathing(d, false)
                call ShowUnit(d, false)
                call IssueTargetOrder(d, "thunderbolt", this.target)
                set d = null
                call PolledWait(1)
                call DestroyTextTag(lct)
                call PolledWait(1)
                if this.lvl > 1 then
                    set d = CreateUnit(GetOwningPlayer(this.caster), DUMMY, GetUnitX(this.target), GetUnitY(this.target), 270)
                    call UnitAddAbility(d, DUMMY_SPELL)
                    call SetUnitAbilityLevel(d, DUMMY_SPELL, this.lvl)
                    call UnitApplyTimedLife(d, 'BTLF', 1)
                    call SetUnitPathing(d, false)
                    call ShowUnit(d, false)
                    call IssueTargetOrder(d, "thunderbolt", this.target)
                    set d = null
                    call PolledWait(2)
                endif
                if this.lvl > 3 then
                    set d = CreateUnit(GetOwningPlayer(this.caster), DUMMY, GetUnitX(this.target), GetUnitY(this.target), 270)
                    call UnitAddAbility(d, DUMMY_SPELL)
                    call SetUnitAbilityLevel(d, DUMMY_SPELL, this.lvl)
                    call UnitApplyTimedLife(d, 'BTLF', 1)
                    call SetUnitPathing(d, false)
                    call ShowUnit(d, false)
                    call IssueTargetOrder(d, "thunderbolt", this.target)
                    set d = null
                    call PolledWait(2)
                endif
                call this.destroy()
                call SX(GetHandleId(GetExpiredTimer()))
                call DestroyTimer(GetExpiredTimer())
                set distdmg = 0
                set lct = null
                set d = null
            elseif GetUnitState(this.target, UNIT_STATE_LIFE)<=0 then
                call PauseTimer(GetExpiredTimer())
                call this.destroy()
                call SX(GetHandleId(GetExpiredTimer()))
                call DestroyTimer(GetExpiredTimer())
            endif
        endmethod
    endstruct
   
    function BALoop takes nothing returns nothing
        local BashArrow data = GetInt(GetExpiredTimer(), "BaSt")
        call data.BaSt()
    endfunction
   
    function BAMain takes nothing returns nothing
        local timer t = CreateTimer()
        local unit u = GetSpellAbilityUnit()
        local unit s = GetSpellTargetUnit()
        local integer lvl = GetUnitAbilityLevel(u, SPELL_ID)
        local location loc = GetUnitLoc(u)
        local location loc1 = GetUnitLoc(s)
        local BashArrow data = BashArrow.create(u, s, lvl, loc, loc1)
        call SaveInt(t, "BaSt", data)
        call TimerStart(t, 0.00001, true, function BALoop)
        set loc = null
        set loc1 = null
        set t = null
        set u = null
        set lvl = 0
    endfunction
           
    function StartTrig_Bash_Arrow takes nothing returns nothing
        set gg_trg_Bash_Arrow = CreateTrigger()
        call TriggerRegisterAnyUnitEventBJ(gg_trg_Bash_Arrow, EVENT_PLAYER_UNIT_SPELL_EFFECT)
        call TriggerAddCondition(gg_trg_Bash_Arrow, Condition(function BACon))
        call TriggerAddAction(gg_trg_Bash_Arrow, function BAMain)
        call SpellPreload(DUMMY_SPELL)
    endfunction

endlibrary

 

[MyPad]

Do you have an initializer? You might have to declare it on the library itself, like this..

library BashArrow initializer StartTrig_Bash_Arrow

You can encapsulate most of the functions, e.g. add a private before the keyword function.

On BAMain, you can get rid of most of the local variables altogether, since you'll only use them once.

On method BAst in struct BashArrow, you use a PolledWait. This is frowned upon and you must use timers instead.

Some functions are not being properly referenced. That means you can possibly cause some compile errors when moving the triggers around.

 

[Wareditor]

after the damage is inflicted, the enemy begins several times

This part isn't clear. The enemy begins what?


I suspect you will need a missile system for what you are trying to do.
However there are other ways to do it. You seem to be using a vanilla spell that inflicts a buff and catching when the buff is added. It should work but what if the unit has already that buff before being hit?

From I have seen, your use of polled wait is the main issue here. You should never use them, especially in periodic timers response.

Let's review your code part by part.

Review

The spell should be in a scope and not a library. Scopes are bellow all libraries so they can use any library without having to declare them.
Spells are put in scopes because you don't need any other code to reference them. So you don't care about their order in the final code.

• Initialization

function StartTrig_Bash_Arrow takes nothing returns nothing
        set gg_trg_Bash_Arrow = CreateTrigger()
        call TriggerRegisterAnyUnitEventBJ(gg_trg_Bash_Arrow, EVENT_PLAYER_UNIT_SPELL_EFFECT)
        call TriggerAddCondition(gg_trg_Bash_Arrow, Condition(function BACon))
        call TriggerAddAction(gg_trg_Bash_Arrow, function BAMain)
        call SpellPreload(DUMMY_SPELL)
    endfunction

Everything seems fine here. I suppose call SpellPreload(DUMMY_SPELL) is not important to the execution of the spell.
If you are going to use a struct for your spell, it would be best to put everything in the struct itself.
You should put this initialization in a static method onInit. This would make it more readable.

private static method onInit takes nothing returns nothing
        local trigger t = CreateTrigger()
        call TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_UNIT_SPELL_EFFECT)
        call TriggerAddCondition(t, Condition(function BACon))
        call TriggerAddAction(t, function BAMain)
        call SpellPreload(DUMMY_SPELL)
    endmethod

Therefore, you would also put the condition and action functions into the struct in forms of static methods.
This would make everything more organised.

• Main

function BAMain takes nothing returns nothing
        local timer t = CreateTimer()
        local unit u = GetSpellAbilityUnit()
        local unit s = GetSpellTargetUnit()
        local integer lvl = GetUnitAbilityLevel(u, SPELL_ID)
        local location loc = GetUnitLoc(u)
        local location loc1 = GetUnitLoc(s)
        local BashArrow data = BashArrow.create(u, s, lvl, loc, loc1)
        call SaveInt(t, "BaSt", data)
        call TimerStart(t, 0.00001, true, function BALoop)
        set loc = null
        set loc1 = null
        set t = null
        set u = null
        set lvl = 0
    endfunction

- GetSpellAbilityUnit() should be GetTriggerUnit()
- It's better to not use locations but (x,y) coordinates, but they are fine if used correctly. From what I can gather you seem to be cleaning the leak properly.
- The periodic timer will be inaccurate and resource heavy. it would be better to have a 0.04 period to increase accuracy and optimize resources.
- I guess you attach some data to the timer using call SaveInt(t, "BaSt", data). It would be better to use an efficient system such as TimerUtilsEx.
- You don't have to null non handle variables (so integers don't have to be nulled).

The coordinates getters, the timer creation and the attachment of data to the timer should be all put into the create static method. There is no point in putting it outside.
The main function is only here to get the event units.

So this is what I have for the moment:

        private static method onEffect takes nothing returns nothing
            call thistype.create(GetTriggerUnit(),GetSpellTargetUnit())
        endmethod
       
         private static method condition takes nothing returns boolean
            return GetSpellAbilityId() == SPELL_ID
        endmethod
           
        private 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.condition))
            call TriggerAddAction(t, function thistype.onEffect)
            //call SpellPreload(DUMMY_SPELL)
        endmethod

thistype refers to the struct type.
For instance local BashArrow this is the same as local thistype this when written inside the struct. Using thistype allow you to change the struct's name easily and make it more readable.

• create and onPeriod

Following the instructions written for main, create and onPeriod should look like this:

private static method onPeriod takes nothing returns nothing
            //replace the following with better timer data attachement system 
            local thistype this = GetInt(GetExpiredTimer(), "BaSt")
            call this.onLoop()
             /* using the TimerUtilsEx API it would look like this
            local thistype this = GetTimerData(GetExpiredTimer())
            call this.onLoop()
            */
        endmethod
       
        private static constant real period = 0.04
       
        static method create takes unit u, unit s returns thistype
            local thistype this = thistype.allocate()
           
            set this.caster = u
            set this.target = s
           
            set this.lvl = GetUnitAbilityLevel(u,SPELL_ID)
           
            //storing caster and target coordinates and not locations
            set this.cx = GetUnitX(u)
            set this.cx = GetUnitY(u)
            set this.tx = GetUnitX(s)
            set this.ty = GetUnitY(s)
           
            //replace the following with better timer data attachement system 
            set this.timer = CreateTimer()
            //call SaveInt(this.timer "BaSt", this)
            call TimerStart(this.timer, period, true, function thistype.onPeriod)
           
            /* using the TimerUtilsEx API it would look like this
            set this.timer = NewTimerEx(this)
            call TimerStart(this.timer,period, true, function thistype.onPeriod)
            */
           
            return this
        endmethod

the timer is stored because you are gonna want to remove it once the spell is executed.
The timer's period is put into a constant to be configurable easily.
I also renamed some methods for readability.

• onLoop

This is where the core of the spell is.
From what I understand the timer is supposed to run until the target get hits by the missile and get the spell's buff.

- As I said, you should never use polled wait in a periodic response function.
Only use it in functions that are only supposed to run once.
- Checking the target's life isn't a good way to check if it's still in game and alive. You need to check it's unit's Id (to check if it's not removed) and if it's not considered dead.
Something like that: if GetUnitTypeId(this.target) != 0 and not IsUnitType(this.target,UNIT_TYPE_DEAD) then
- All those checks should be in the onPeriod method. The onLoop method should just be for the spell to actually execute when the requirements are met. onLoop should be then named onExecute.
- Don't use onDestroy, just put it in destroy.

Hence we have something like this:

private method destroy takes nothing returns nothing
            set this.caster = null
            set this.target = null
           
            if this.timer != null then
                call SX(GetHandleId(this.timer))
                call DestroyTimer(this.timer)
               
                //with TimerUtilsEx API
                //call ReleaseTimer(this.timer)
                set this.timer = null
            endif   
           
            call this.deallocate()
        endmethod
       
        private method onExecute takes nothing returns nothing
        //...
        endmethod
 
   
        private static method onPeriod takes nothing returns nothing
            //replace the following with better timer data attachement system 
            local thistype this = GetInt(GetExpiredTimer(), "BaSt")
           
             /* using the TimerUtilsEx API it would look like this
            local thistype this = GetTimerData(GetExpiredTimer())
            */
           
            if GetUnitTypeId(this.target) != 0 and not IsUnitType(this.target,UNIT_TYPE_DEAD) then
                if GetUnitAbilityLevel(this.target, BUFF_ID) > 0 then
                    call this.onExecute()
                endif
            else
                call this.destroy()
            endif
        endmethod

For the onExecute method:
- distance is calulated using the (x,y) coordinates stored. The formula to get the damage is externalised for easier configuration.
- Actually you don't need to store the coordinates because you want to have the distance at the time of impact not at the time of cast. We change that accordingly.
- I don't understand what are you doing with that dummy, you need to explain yourself more so I can help.

Here is the final draft :

scope BashArrow
    globals
        private constant integer SPELL_ID = 'A02L'
        private constant integer DUMMY_SPELL = 'A03S'
        private constant integer BUFF_ID = 'B014'
        private constant integer DUMMY = 'u001'
    endglobals
    struct BashArrow
        private unit caster
        private unit target
       
        private integer lvl
       
        private timer timer
       
        private method destroy takes nothing returns nothing
            set this.caster = null
            set this.target = null
           
            if this.timer != null then
                call SX(GetHandleId(this.timer))
                call DestroyTimer(this.timer)
               
                //with TimerUtilsEx API
                //call ReleaseTimer(this.timer)
                set this.timer = null
            endif   
           
            call this.deallocate()
        endmethod
       
        private static method getDistanceDamage takes real dist, integer level returns real
            return dist*0.10*level
        endmethod
       
        private method onExecute takes nothing returns nothing
            //calculate distance 
            local real dx = GetUnitX(this.target) - GetUnitX(this.caster)
            local real dy = GetUnitY(this.target) - GetUnitY(this.caster)
            local real distance = SquareRoot(dx*dx + dy*dy)
            //calculate dmg
            local real dmg = getDistanceDamage(distance,this.lvl)
         
            call UnitDamageTarget(this.caster, this.target, dmg, true, false, ATTACK_TYPE_HERO, DAMAGE_TYPE_MAGIC, null)
           
            //Texttag stuff to be added
            //Dummy stuff to be added
           
            call this.destroy()
        endmethod
 
   
        private static method onPeriod takes nothing returns nothing
            //replace the following with better timer data attachement system 
            local thistype this = GetInt(GetExpiredTimer(), "BaSt")
           
             /* using the TimerUtilsEx API it would look like this
            local thistype this = GetTimerData(GetExpiredTimer())
            */
           
            if GetUnitTypeId(this.target) != 0 and not IsUnitType(this.target,UNIT_TYPE_DEAD) then
                if GetUnitAbilityLevel(this.target, BUFF_ID) > 0 then
                    call this.onExecute()
                endif
            else
                call this.destroy()
            endif
        endmethod
       
        private static constant real period = 0.04
       
        static method create takes unit u, unit s returns thistype
            local thistype this = thistype.allocate()
           
            set this.caster = u
            set this.target = s
           
            set this.lvl = GetUnitAbilityLevel(u,SPELL_ID)
           
            //replace the following with better timer data attachement system 
            set this.timer = CreateTimer()
            call SaveInt(this.timer "BaSt", this)
            call TimerStart(this.timer, period, true, function thistype.onPeriod)
           
            /* using the TimerUtilsEx API it would look like this
            set this.timer = NewTimerEx(this)
            call TimerStart(this.timer,period, true, function thistype.onPeriod)
            */
           
            return this
        endmethod
       
        private static method onEffect takes nothing returns nothing
            call thistype.create(GetTriggerUnit(),GetSpellTargetUnit())
        endmethod
       
         private static method condition takes nothing returns boolean
            return GetSpellAbilityId() == SPELL_ID
        endmethod
           
        private 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.condition))
            call TriggerAddAction(t, function thistype.onEffect)
            call SpellPreload(DUMMY_SPELL)
        endmethod
    endstruct
endscope

Hope I was of help

 

[Arsenal]

Hope I was of help

Thank you for explaining everything in detail) I'll try myself further