[vJASS] Help me fix this mess I've made.. please

[Molekyle]

Yesterday I decided to learn vJass. I read some tutorials and looked at various approved vJass stuff in the hive's spell section which I felt gave me a pretty good idea of how to do things.

Earlier today I decided to try it out for myself by creating my first vJass spell!
Needless to say, I've come across a few problems...
The spell is supposed to create a (somewhat) random pattern of cracks in the ground that will explode after a set duration, dealing damage to enemy units.

The problems:
1) While the spell itself works fairly well Im having a hard time figuring out a way of making sure that each unit is only damaged ONCE.

I initially wanted to do this by creating an additional group (this.g) in which all units that have been damaged would be stored and thereby excluded from future instances of the spell. This worked... to some extend... as the cleanup for this.g would trigger 8 times towards the end of the spell! I tried to implement some work around by adding an additional condition to the cleanup: if this.g != null but that did not seem to have any effect.

How should I go about fixxing this? I feel that the cause of this problem is somewhat tied to the way that I have coded and structured the spell - which brings me to the next question:

2 Overall code structure.. looking over my own code it feels so incomprehensible and I can't help but feel that I should have approached this spell in an entirely different manner. Did I do something fundamentally wrong?

Here is the code

scope SeismicSlam
    
    globals
        private constant integer ABILITY_ID = 'A01A'
        private constant integer DUMMY_ID = 'h000'
        private constant string MODEL_PATH = "war3mapImported\\DustAndRocks.mdx"
        private constant string MODEL_PATH2 = "Abilities\\Spells\\Other\\Volcano\\VolcanoMissile.mdl"
        private constant string MODEL_PATH3 = "Abilities\\Spells\\Orc\\WarStomp\\WarStompCaster.mdl"
        private constant integer INSTANCES = 9
        private constant real DISTANCE = 100.
        private constant real INTERVAL = 0.10
        private constant real DURATION = 1.00
        private constant real AOE = 175.
        private constant real SPMOD = 1.00
        private constant attacktype A_TYPE = ATTACK_TYPE_HERO
        private constant damagetype D_TYPE = DAMAGE_TYPE_NORMAL
        private constant weapontype W_TYPE = null
    endglobals
    
    private constant function Damage takes integer level returns real
        return 45 * level
    endfunction
    
    private function Spellpower takes unit u returns real
        return GetHeroInt(u, true) * SPMOD
    endfunction
    
    private function UnitFilter takes player source, unit t, group g returns boolean
        return IsUnitEnemy(t, source) and UnitAlive(t) and not IsUnitInGroup(t, g)
    endfunction
    
    //! textmacro_once ALLOCATE
        if thistype(0).prev == 0 then
            set count = count + 1
            set this = count
        else
            set this = thistype(0).prev
            set thistype(0).prev = thistype(0).prev.prev
        endif
        if thistype(0).next == 0 then
            call TimerStart(period, INTERVAL, true, function thistype.periodic)
        else
            set thistype(0).next.prev = this
        endif
        set this.next = thistype(0).next
        set thistype(0).next = this
        set this.prev = thistype(0)
    //! endtextmacro
    
    private struct Spell extends array
        unit u
        unit d
        real angle
        integer instance
        real dur
        effect fx
        group g
        integer s

        static unit t
        static unit nd
        static real x
        static real y
        static real x1
        static real y1
        static real a
        static real damage
        static integer lvl
        static integer split
        static player p
        
        thistype prev
        thistype next
        
        static integer count
        static timer period
        static group filter
                
        method destroy takes nothing returns nothing
            if this.next != 0 then
                set this.next.prev = this.prev
            endif
            set this.prev.next = this.next
            set this.prev = thistype(0).prev
            set thistype(0).prev = this
            if thistype(0).next == 0 then
                call PauseTimer(period)
            endif
            set this.u = null
            set this.d = null
            set this.fx = null
            set this.g = null
        endmethod
        
        static method periodic takes nothing returns nothing
            local thistype this = thistype(0).next
            loop
                exitwhen this == 0
                set this.dur = this.dur - INTERVAL
                set x = GetUnitX(this.d)
                set y = GetUnitY(this.d)
                
                if this.instance > 0 and this.s == 1 then
                    set this.instance = this.instance - 1
                    if this.instance != 7 and this.instance != 3 then
                        set this.angle = this.angle + (GetRandomReal(-20., 20.))
                        set x1 = x + DISTANCE * Cos(this.angle * 0.01745)
                        set y1 = y + DISTANCE * Sin(this.angle * 0.01745)
                        set nd = CreateUnit(GetOwningPlayer(u), DUMMY_ID, x1, y1, this.angle)
                        call SetUnitScale(nd, 0.75, 0.75, 0.75)
                        
                        call Spell.add(this.u,nd,this.angle,this.instance,DURATION,this.g,1)
                        set nd = null
                        set this.s = 0
                    else
                        set split = 0
                        loop
                            exitwhen split == 2
                            set split = split + 1
                            set a = this.angle + (GetRandomReal(75., 105.) * split - 135.)
                            set x1 = x + DISTANCE * Cos(a * 0.01745)
                            set y1 = y + DISTANCE * Sin(a * 0.01745)
                            set nd = CreateUnit(GetOwningPlayer(u), DUMMY_ID, x1, y1, a)
                            call SetUnitScale(nd, 0.75, 0.75, 0.75)
                            set a = this.angle + (90. * split - 135.) / 2
                            call Spell.add(this.u,nd,a,this.instance,DURATION,this.g,1)
                            set nd = null
                        endloop
                        set this.s = 0
                    endif
                endif
                
                if this.dur < 0. then
                    call DestroyEffect(AddSpecialEffectTarget(MODEL_PATH2, this.d, "origin"))
                    call DestroyEffect(this.fx)
                    call UnitApplyTimedLife(this.d, 'BLTF', 0.75)
                    
                    set p = GetOwningPlayer(this.u)
                    set lvl = GetUnitAbilityLevel(this.u, ABILITY_ID)
                    set damage = Damage(lvl) + Spellpower(this.u)
                    set udg_DamageType = 1
                    call GroupEnumUnitsInRange(filter, x, y, AOE, null)
                    loop
                        set t = FirstOfGroup(filter)
                        exitwhen t == null
                        call GroupRemoveUnit(filter,t)
                        if UnitFilter(p,t,this.g) then
                            call GroupAddUnit(this.g, t)
                            call UnitDamageTarget(this.u, t, damage, true, false, A_TYPE, D_TYPE, W_TYPE)
                            call IssueTargetOrder(this.d, ORDER_ID, t)
                        endif
                    endloop
                    set udg_DamageType = 0
                    
                    if this.instance == 0 and FirstOfGroup(this.g) != null then
                        call GroupClear(this.g)
                        call ReleaseGroup(this.g)
                        call DisplayTextToForce(GetPlayersAll(), "this.g cleared")
                        call this.destroy()
                    elseif this.instance >= 0 then
                        call this.destroy()
                        call DisplayTextToForce(GetPlayersAll(), "this.destroy triggered (i >= 0)")
                    endif
                endif
                set this = this.next
            endloop
        endmethod
        
        static method add takes unit u, unit d, real a, integer i, real dur, group g, integer s returns nothing
            local thistype this
            //! runtextmacro ALLOCATE()
            set this.u = u
            set this.d = d
            set this.angle = a
            set this.instance = i
            set this.dur = dur
            set this.fx = AddSpecialEffectTarget(MODEL_PATH, d, "origin")
            set this.g = g
            set this.s = s
        endmethod
        
        static method run takes nothing returns boolean
            local unit u = GetTriggerUnit()
            local real x = GetUnitX(u)
            local real y = GetUnitY(u)
            local real angle = AngleBetweenXY(x, y, GetSpellTargetX(), GetSpellTargetY())
            local group g = NewGroup()
            local unit d
            
            set x = x + DISTANCE * Cos(angle * 0.01745)
            set y = y + DISTANCE * Sin(angle * 0.01745)
            set d = CreateUnit(GetOwningPlayer(u), DUMMY_ID, x, y, angle)
            call SetUnitScale(d, 0.75, 0.75, 0.75)
            call DestroyEffect(AddSpecialEffectTarget(MODEL_PATH3, d, "origin"))
            
            call Spell.add(u,d,angle,INSTANCES,DURATION,g,1)
            
            set u = null
            set d = null
            set g = null
            return false
        endmethod
        
        static method onInit takes nothing returns nothing
            call RegisterSpellEffectEvent(ABILITY_ID, function thistype.run)
            set count = 0
            set period = CreateTimer()
            set filter = CreateGroup()
        endmethod
    endstruct
endscope

I realise that asking people to look through that abomination is quite the thing to ask. Im still hoping that someone will take the time and help me out.

 

[TriggerHappy]

So the problem is the units are being damaged multiple times? or have you fixed that.

1. The way you're doing it now seems to be fine (storing units in a group, then removing upon damage). If they're still being damaged multiple times then the problem is elsewhere.
2. The code is not bad for a first vJass script although it could be optimized.

If I have time, and if you need, I can try to optimize it for you.

 

[Molekyle]

Hey TriggerHappy, yes the spell currently only damages each unit once but there is a (BIG) problem with the way I've coded it though.
If the spell never actually hits an enemy FirstOfGroup(this.g) will be null for all instances and thus never cleaned up. My old solution did not use FirstOfGroup(this.g) != null but in return it cleaned up this.g 8 times towards the end of the spell which seemed terribly inefficient.
I thought about creating an additional dummy at the start of the spell and add it to this.g to make sure that there'd always be atleast one unit in the group, but that solution struck me as being kinda newbie-ish.

As for your offer to help me optimize the code: Thank you - I'd love it!

 

[TriggerHappy]

Couldn't you just do the equivalent ofCountUnitsInGroup?

If there's no units, then do nothing.

 

[Molekyle]

Like CountUnitsInGroup(this.g) > 0?

The same problem occurs, if the spell never hits anything this.g will never be cleaned up. All instances will be cleaned/destroyed with the elseif this.instance >= 0

                    if this.instance == 0 and CountUnitsInGroup(this.g) > 0 then
                        call GroupClear(this.g)
                        call ReleaseGroup(this.g)
                        call DisplayTextToForce(GetPlayersAll(), "this.g cleared")
                        call this.destroy()
                    elseif this.instance >= 0 then
                        call this.destroy()
                        call DisplayTextToForce(GetPlayersAll(), "this.destroy triggered (i >= 0)")
                    endif

Which is why I feel that I've done something fundamentally wrong with the way I have approached the spell. I simply can't think of a way to check whether this.g has already been cleaned up - It feels way too hard /sadface

If there's no units, then do nothing.

But even if there are no units I still need to clean up the instance with this.destroy, otherwise this.u, this.d will leak?

 

[Molekyle]

I managed to fix the spell by taking the 'newbie-ish' approach that i mentioned in #3... it now functions as I'd like it to and there aren't any leaks (as far as I can tell). That being said I still can't help but feel that my code looks like a bloated abomination, but I guess that is something that will improve gradually as I become more proficient with vJass.

Thank you TriggerHappy for the help you provided. Even though it did not fix the problem directly your interest and suggestions were much appreciated.

 

[watermelon_1234]

I know you've solved your problem already but I would like to add a tip about the code structure.

Your current code uses the struct Spell twice for different reasons: when the spell is actually casted and handling cracks. The way I would approach this would be to separate the code handling cracks into another struct, let's say Crack, so that Spell would just be concerned with handling the spell cast information and when it should create Cracks. In this way, Spell would also know what units are already hit by having the group g and it can pass that information to Crack. Spell would clean up that information at the very end of the spell.

As you're also a beginning vJasser too, I wouldn't recommend those crazy efficient strategies recommended by those tutorials (I'm referring to that textmacro_once ALLOCATE). I would recommend you to become more comfortable with scripting and use other people's libraries for functionality (like TimerUtils or Timer32). If and only if your script is causing Warcraft 3 to lag so horribly, then you could try those strategies, though it's much more likely that graphics would be the cause of your lag.

You should also definitely add comments since they'll help you out when you need to look at your code again. Related to that is avoiding short variable names that don't carry any meaning in itself, like your member integer variable s.

 

[Molekyle]

Hi watermelon, thank you for your input. Your suggestion to seperate the spell into 2 structs rather than 1 does indeed seem like the better approach (and I reckon that the code would look alot more orderly and manageable using this method). I'll definitely keep that in mind.

As for the allocation method - I had some issues with the text macro thing which made me change to a different template. I've linked the template below just in case you'd be curious:
http://www.hiveworkshop.com/forums/jass-ai-scripts-tutorials-280/efficient-clean-vjass-spell-models-216166/

When I started the map I considered using something like T32, but I ultimately decided against it as quite alot of the spells use different timer intervals. As far as I could tell Timer32 only supported one timer interval?

TimerUtils would probably be a good addition? But just out of curiosity - what is the benefit of using TimerUtils rather than just having a set timer for each spell with a periodic effect?

I've also posted the 'fixxed' version of spell below, although I might re-fix it soon with your suggestions:

scope ColossusSmash
    
    globals
        private constant integer ABILITY_ID = 'A01A'
        private constant integer DABILITY_ID = 'A017'
        private constant integer DUMMY_ID = 'h000'
        private constant string MODEL_PATH = "war3mapImported\\DustAndRocks.mdx"
        private constant string MODEL_PATH2 = "Abilities\\Spells\\Other\\Volcano\\VolcanoMissile.mdl"
        private constant string MODEL_PATH3 = "Abilities\\Spells\\Orc\\WarStomp\\WarStompCaster.mdl"
        private constant integer INSTANCES = 10
        private constant real DISTANCE = 100.
        private constant real INTERVAL = 0.10
        private constant real DURATION = 1.50
        private constant real AOE = 175.
        private constant real SPMOD = 1.00
        private constant attacktype A_TYPE = ATTACK_TYPE_HERO
        private constant damagetype D_TYPE = DAMAGE_TYPE_NORMAL
    endglobals
    
    private constant function Damage takes integer level returns real
        return 47 * level * level - 47 * level + 188.
    endfunction
    
    private function Spellpower takes unit u returns real
        return GetHeroInt(u, true) * SPMOD
    endfunction
    
    private function UnitFilter takes player source, unit t, group g returns boolean
        return IsUnitEnemy(t, source) and UnitAlive(t) and not IsUnitInGroup(t, g)
    endfunction
    
    private struct Spell extends array

        private static unit array u
        private static unit array d
        private static real array a
        private static real array temp
        private static integer array instance
        private static integer array s
        private static effect array fx
        private static group array g
        
        //ALLOCATION\\
        private static integer array rn
        private static integer ic = 0

        private static integer array next
        private static integer array prev

        private static timer iterator = CreateTimer()
        private static integer count = 0
        private static group fg = CreateGroup()
        //END\\
                
        method destroy takes nothing returns nothing
            set rn[this] = rn[0]
            set rn[0] = this

            set prev[next[this]] = prev[this]
            set next[prev[this]] = next[this]

            set count = count - 1
            if count == 0 then
                call PauseTimer(iterator)
            endif
            
            //CLEAN UP\\
            set u[this] = null
            set d[this] = null
            set fx[this] = null
            set g[this] = null
            //END\\
        endmethod
        
        static method periodic takes nothing returns nothing
            //INSTANCE SPECIFIC VARIABLES\\
            local unit nd
            local unit t
            local real x
            local real x1
            local real y
            local real y1
            local real angle
            local real damage
            local integer split
            local integer lvl
            local player p
            //END\\
            
            local thistype this = next[0]
            loop
                exitwhen this == 0
                set x = GetUnitX(d[this])
                set y = GetUnitY(d[this])
                
                if temp[this] < DURATION and instance[this] > 0 and s[this] == 1 then
                    set instance[this] = instance[this] - 1
                    if instance[this] != 6 and instance[this] != 2 then
                        set angle = a[this] + (GetRandomReal(-30., 30.))
                        set x1 = x + DISTANCE * Cos(angle * 0.01745)
                        set y1 = y + DISTANCE * Sin(angle * 0.01745)
                        set nd = CreateUnit(GetOwningPlayer(u[this]), DUMMY_ID, x1, y1, angle)
                        call SetUnitScale(nd, 0.75, 0.75, 0.75)
                        
                        call Spell.add(u[this], nd, a[this], DURATION, instance[this], 1, g[this])
                    
                        if instance[this] == 9 then
                            set temp[this] = DURATION + INSTANCES * INTERVAL
                            call DestroyEffect(AddSpecialEffectTarget(MODEL_PATH3, nd, "origin"))
                        endif
                        
                        set nd = null
                        set s[this] = 0
                    else
                        set split = 0
                        loop
                            exitwhen split == 2
                            set split = split + 1
                            set angle = a[this] + (GetRandomReal(75., 105.) * split - 135.)
                            set x1 = x + DISTANCE * Cos(angle * 0.01745)
                            set y1 = y + DISTANCE * Sin(angle * 0.01745)
                            set nd = CreateUnit(GetOwningPlayer(u[this]), DUMMY_ID, x1, y1, angle)
                            call SetUnitScale(nd, 0.75, 0.75, 0.75)
                            set angle = a[this] + (90. * split - 135.) / 2
            
                            call Spell.add(u[this], nd, angle, DURATION, instance[this], 1, g[this])
                            
                            set nd = null
                        endloop
                        set s[this] = 0
                    endif
                endif
                
                set temp[this] = temp[this] - INTERVAL
                
                if temp[this] < 0. then
                    if instance[this] < 9 then
                        call DestroyEffect(AddSpecialEffectTarget(MODEL_PATH2, d[this], "origin"))
                        call DestroyEffect(fx[this])
                        call UnitApplyTimedLife(d[this], 'BLTF', 0.75)
                    
                        set p = GetOwningPlayer(u[this])
                        set lvl = GetUnitAbilityLevel(u[this], ABILITY_ID)
                        set damage = Damage(lvl) + Spellpower(u[this])
                        set udg_DamageType = 1
                        call GroupEnumUnitsInRange(fg, x, y, AOE, null)
                        loop
                            set t = FirstOfGroup(fg)
                            exitwhen t == null
                            call GroupRemoveUnit(fg,t)
                            if UnitFilter(p,t,g[this]) then
                                call GroupAddUnit(g[this], t)
                                call UnitDamageTarget(u[this], t, damage, true, false, A_TYPE, D_TYPE, null)
                            endif
                        endloop
                        set udg_DamageType = 0
                        
                        set p = null
                        call this.destroy()
                        
                    elseif instance[this] == 9 and FirstOfGroup(g[this]) != null then
                        call KillUnit(d[this])
                        call DestroyEffect(fx[this])
                        call GroupClear(g[this])
                        call ReleaseGroup(g[this])
                        call this.destroy()
                    endif
                endif
                set this = next[this]
            endloop
        endmethod
        
        static method add takes unit caster, unit dummy, real angle, real dur, integer i, integer split, group dgroup returns nothing
            //ALLOCATE INSTANCE\\
            local thistype this = rn[0]
            set this = rn[0]
            if this == 0 then
                set ic = ic + 1
                set this = ic
            else
                set rn[0] = rn[this]
            endif

            set next[this] = 0
            set prev[this] = prev[0]
            set next[prev[0]] = this
            set prev[0] = this

            set count = count + 1
            if count == 1 then
                call TimerStart(iterator, INTERVAL, true, function thistype.periodic)
            endif
            //END\\
            
            set u[this] = caster
            set d[this] = dummy
            set a[this] = angle
            set temp[this] = dur
            set instance[this] = i
            set s[this] = split
            set fx[this] = AddSpecialEffectTarget(MODEL_PATH, dummy, "origin")
            set g[this] = dgroup
        endmethod
        
        static method run takes nothing returns boolean
            //LOCAL VARIABLES\\
            local unit caster = GetTriggerUnit()
            local real x = GetUnitX(caster)
            local real y = GetUnitY(caster)
            local real angle = 57.29582 * Atan2(GetSpellTargetY() - y, GetSpellTargetX() - x)
            local unit dummy = CreateUnit(GetOwningPlayer(caster), DUMMY_ID, x, y, angle)
            local group dgroup = NewGroup()
            //END\\
            
            //ALLOCATE INSTANCE\\
            local thistype this = rn[0]
            set this = rn[0]
            if this == 0 then
                set ic = ic + 1
                set this = ic
            else
                set rn[0] = rn[this]
            endif

            set next[this] = 0
            set prev[this] = prev[0]
            set next[prev[0]] = this
            set prev[0] = this

            set count = count + 1
            if count == 1 then
                call TimerStart(iterator, INTERVAL, true, function thistype.periodic)
            endif
            //END\\
            
            set u[this] = caster
            set d[this] = dummy
            set a[this] = angle
            set temp[this] = DURATION
            set instance[this] = INSTANCES
            set s[this] = 1
            set fx[this] = AddSpecialEffectTarget(".mdl", dummy, "origin")
            set g[this] = dgroup

            call GroupAddUnit(dgroup, dummy)
            
            set caster = null
            set dummy = null
            set dgroup = null
            
            return false
        endmethod
        
        static method onInit takes nothing returns nothing
            call RegisterSpellEffectEvent(ABILITY_ID, function thistype.run)
        endmethod
    endstruct
endscope