[vJASS] Unit leak?

[TheGoldenGoblin]

Hello! I recently uploaded my kamehameha spell in the spell section and I wanted to make a GUI and a VJass version. Tho I have a problem with the VJass one since there is always one unit left when the spell ends. And from my testing it's not the .dummy unit..
+rep to anyone trying to help and I appreciate it alot!

Here is the code:

scope Kamehameha

    globals
    
    private constant integer AID = 'A000' //Set AID = your ability id. To get a spells id go into the objekt editor and press ctrl+d.
    
    private constant real aoe    = 200 //How large area around the spell that will take damage.
    private constant real Offset = 50. //How long infront of the caster the unit will spawn.
    
    private constant integer DUMMY  = 'h000' //Set this to the Unit it of your dummy. Press ctrl+d in the objekt editor to find it.
    
    private constant attacktype ATTACK_TYPE  = ATTACK_TYPE_NORMAL
    private constant damagetype DAMAGE_TYPE  = DAMAGE_TYPE_NORMAL
    
    private constant real interval = 0.03
    endglobals
    
    //This is the formula for the damage. You can make your own formula if you want.
    private constant function GetDamage takes integer lvl returns real
      return 500.*lvl
    endfunction
    
    //This is the range of the spell.
    private constant function GetRange takes integer lvl returns real
      return 200.*lvl
    endfunction
    
    //This is how fast the spell will move.
    private constant function GetSpeed takes integer lvl returns real
      return 20. + 0*lvl
    endfunction    
    
    //This checks if a unit is alive so we can deal damage to it or not.
    native UnitAlive takes unit id returns boolean
    
    //This is the formula that filters out which units that should take damage.
    private function FilterUnits takes unit u, player p returns boolean
        return (UnitAlive(u)) and (IsUnitEnemy(u, p)) and (not IsUnitType(u, UNIT_TYPE_MAGIC_IMMUNE))
    endfunction
    
    //DON'T EDIT ANYTHING BELOW
    
    private struct Spell extends array
       unit caster
       unit dummy
       real damage
       location cl
       location sl
       location point
       real angle
       player owner
       real range
       real inRange
       real speed
       integer lvl
       group g
       
       static integer array rn
       static integer ic = 0
    
    
    private static method line takes nothing returns nothing
    local timer t = GetExpiredTimer()
    local thistype this = GetTimerData(t)
    local location l = GetUnitLoc(.dummy)
    local unit u
    local real x = GetUnitX(.dummy)
    local real y = GetUnitY(.dummy)
    local unit u2 = CreateUnit( .owner, DUMMY, x, y, .angle)

    
    set .g       = CreateGroup()

    
    if (UnitAlive(.caster)) and (.inRange < .range) then
      call GroupEnumUnitsInRange( .g, x, y, aoe, null)
      loop
        set u = FirstOfGroup(.g)
        exitwhen u == null
        call GroupRemoveUnit(.g, u)
        if FilterUnits(u, .owner) then
          call UnitDamageTarget(.caster, u, .damage, false, false, ATTACK_TYPE, DAMAGE_TYPE, null)
        endif
      endloop
      call SetUnitPositionLoc( .dummy, PolarProjectionBJ( l, .speed, .angle))
      call UnitApplyTimedLife(u2, 'BTLF', 0.45)
      set u2 = null
      call BJDebugMsg("then")
      set .inRange = .inRange + 5.
    else
      call BJDebugMsg("else")
      call ReleaseTimer(t)
      
      call KillUnit(.dummy)
      call RemoveUnit(.dummy)
      //Deindex the spell
      set rn[this] = rn[0]
      set rn[0] = this
      
      
      //Null all the variables
      set g = null
      set u = null
      set t = null
      set owner = null
      set caster = null
      set l = null
      set t = null
    endif
    
   endmethod
    
    private static method run takes nothing returns nothing
    local thistype this
    
    //Index the spell
    set this = rn[0]
    if this == 0 then
        set ic = ic + 1
        set this = ic
    else
        set rn[0] = rn[this]
    endif
    
    
    set .owner   = GetTriggerPlayer()
    set .caster  = GetTriggerUnit()
    set .lvl     = GetUnitAbilityLevel(.caster, AID)
    set .damage  = GetDamage(lvl)
    set .range   = GetRange(lvl)
    set .cl      = GetUnitLoc(.caster)
    set .sl      = GetSpellTargetLoc()
    set .point   = PolarProjectionBJ(.cl, Offset, .angle)
    set .angle   = AngleBetweenPoints(.cl, .sl)
    set .dummy   = CreateUnitAtLoc( .owner, DUMMY, .point, .angle)
    set .inRange = 0
    set .speed   = GetSpeed(lvl)
    
    call TimerStart(NewTimerEx(this), interval, true, function thistype.line)
    
    set .point = null
    set .sl = null
    set .cl = null
    
    endmethod
    
    private static method onInit takes nothing returns nothing
    call RegisterSpellEffectEvent(AID, function thistype.run)
    endmethod
    
    
    endstruct
endscope

I added the test map using the VJass version if anyone wanna check it out

 

[PurplePoot]

In the else, you kill and remove .dummy but not u2; you should move creation of u2 into the if statement. You also leak locations all over the place.

 

[TheGoldenGoblin]

Thanks man that fixed it! Do I need to use call RemoveLocation? because I thought I didn't need to use that in VJass since I'm nulling the variables

 

[PurgeandFire]

You still need to use RemoveLocation().

Nulling just reassigns the local so that it is no longer pointing to the handle. This lets the handle ID get recycled. But if the object isn't destroyed, it'll just remain in memory.

Thus, you should first remove the location, and then assign the local to null. e.g.:

local location temp = Location(0, 0)
call RemoveLocation(temp)
set temp = null

Although, in JASS we usually just use coordinates, which aren't stored in memory that way (and don't need to be removed).

 

[PurplePoot]

Nulling is in addition to the normal process of removal. Furthermore, there is no real reason to use locations at all in this instance. Here's a cleaned up version of the code: I also removed the custom indexing system junk because yeah, just don't.

scope Kamehameha

    globals
        private constant integer AID = 'A000' //Set AID = your ability id. To get a spells id go into the objekt editor and press ctrl+d.
    
        private constant real aoe    = 200. //How large area around the spell that will take damage.
        private constant real Offset = 50. //How long infront of the caster the unit will spawn.
    
        private constant integer DUMMY  = 'h000' //Set this to the Unit it of your dummy. Press ctrl+d in the objekt editor to find it.
    
        private constant attacktype ATTACK_TYPE  = ATTACK_TYPE_NORMAL
        private constant damagetype DAMAGE_TYPE  = DAMAGE_TYPE_NORMAL
    
        private constant real interval = 0.03
    endglobals
    
    //This is the formula for the damage. You can make your own formula if you want.
    private constant function GetDamage takes integer lvl returns real
      return 500.*lvl
    endfunction
    
    //This is the range of the spell.
    private constant function GetRange takes integer lvl returns real
      return 200.*lvl
    endfunction
    
    //This is how fast the spell will move.
    private constant function GetSpeed takes integer lvl returns real
      return 20. + 0*lvl
    endfunction    
    
    //This checks if a unit is alive so we can deal damage to it or not.
    native UnitAlive takes unit id returns boolean
    
    //This is the formula that filters out which units that should take damage.
    private function FilterUnits takes unit u, player p returns boolean
        return UnitAlive(u) and IsUnitEnemy(u, p) and not IsUnitType(u, UNIT_TYPE_MAGIC_IMMUNE)
    endfunction
    
    //DON'T EDIT ANYTHING BELOW

        
    
    private struct Spell
        unit caster
        unit dummy
        real damage
        real angle
        player owner
        real range
        real inRange
        real speed
        integer lvl
       
        static group g = CreateGroup()
        static thistype carry
        
        private static method Callback takes nothing returns nothing
            if FilterUnits(GetEnumUnit(),.carry.owner) then
                call UnitDamageTarget(.carry.caster, GetEnumUnit(), .carry.damage, false, false, ATTACK_TYPE, DAMAGE_TYPE, null)
            endif
        endmethod
    
        private static method line takes nothing returns nothing
            local timer t = GetExpiredTimer()
            local thistype this = GetTimerData(t)
            local real x = GetUnitX(.dummy)
            local real y = GetUnitY(.dummy)
            local unit u2
    
            if UnitAlive(.caster) and .inRange < .range then
                call GroupEnumUnitsInRange( .g, GetUnitX(.dummy), GetUnitY(.dummy), aoe, null)
                set .carry = this
                call ForGroup(.g, function Spell.Callback)
                set u2 = CreateUnit(.owner, DUMMY, GetUnitX(.dummy), GetUnitY(.dummy), .angle*bj_RADTODEG)
                call SetUnitX(.dummy, GetUnitX(.dummy)+.speed*Cos(.angle))
                call SetUnitY(.dummy, GetUnitY(.dummy)+.speed*Sin(.angle))
                call UnitApplyTimedLife(u2, 'BTLF', .45)
                call SetUnitExploded(u2, true)
                set u2 = null
                set .inRange = .inRange + 5.
            else
                call ReleaseTimer(t)
                call SetUnitExploded(.dummy, true)
                call KillUnit(.dummy)
                set .dummy = null
                set .caster = null
                set .owner = null
                call this.destroy()
            endif
        endmethod
        private static method run takes nothing returns nothing
            local thistype this = thistype.create()
            set .caster  = GetTriggerUnit()
            set .owner   = GetOwningPlayer(.caster)
            set .lvl     = GetUnitAbilityLevel(.caster, AID)
            set .damage  = GetDamage(.lvl)
            set .range   = GetRange(.lvl)
            set .angle   = Atan2(GetSpellTargetY()-GetUnitY(.caster),GetSpellTargetX()-GetUnitX(.caster))
            set .dummy   = CreateUnit(.owner,DUMMY,GetUnitX(.caster) + Offset*Cos(.angle),GetUnitY(.caster) + Offset*Sin(.angle),.angle*bj_RADTODEG)
            set .inRange = 0
            set .speed   = GetSpeed(.lvl)
            call TimerStart(NewTimerEx(this), interval, true, function thistype.line)
        endmethod
    
        private static method onInit takes nothing returns nothing
            call RegisterSpellEffectEvent(AID, function thistype.run)
        endmethod
    endstruct
endscope

Feel free to ask any questions you have about changes I made. As in the other thread, I didn't actually compile it so I may've made a typo somewhere.

I also don't understand the insistence with putting everything in a struct in the first place, but I left it that way this time for the sake of not screwing with things too much.

 

[TheGoldenGoblin]

The callback function doesn't work because the struct Spell doesn't allow .syntax. And I get alot of other errors like this.destroy and this.create.
The reason I'm using structs is because every spell I have looked at and tried to learn from have used structs. So I don't really know how to make spells without the thistype thingy.
The thing in the other thread worked perfectly, Thank you!

Edit: I looked in the VJass spell section and every single spell there uses structs..

 

[PurplePoot]

Right, made some dumb errors due to not having coded Warcraft in 4 years and forgetting some of vJass's quirks. Updated (and actually compiled in the editor this time).

About structs, it's just an insistence that a lot of people on hive seem to have to make java-style code where everything is static methods in an object for... well, no good reason, to be frank. Having a basic struct to hold stuff is nice, but there is no need to dump a bunch of methods in it. Consider this alternative example which works exactly the same way as the above code but discards methods:

Code

scope Kamehameha initializer init

    globals
        private constant integer AID = 'A000' //Set AID = your ability id. To get a spells id go into the objekt editor and press ctrl+d.
    
        private constant real aoe    = 200. //How large area around the spell that will take damage.
        private constant real Offset = 50. //How long infront of the caster the unit will spawn.
    
        private constant integer DUMMY  = 'h000' //Set this to the Unit it of your dummy. Press ctrl+d in the objekt editor to find it.
    
        private constant attacktype ATTACK_TYPE  = ATTACK_TYPE_NORMAL
        private constant damagetype DAMAGE_TYPE  = DAMAGE_TYPE_NORMAL
    
        private constant real interval = 0.03
    endglobals
    
    //This is the formula for the damage. You can make your own formula if you want.
    private constant function GetDamage takes integer lvl returns real
      return 500.*lvl
    endfunction
    
    //This is the range of the spell.
    private constant function GetRange takes integer lvl returns real
      return 200.*lvl
    endfunction
    
    //This is how fast the spell will move.
    private constant function GetSpeed takes integer lvl returns real
      return 20. + 0*lvl
    endfunction    
    
    //This checks if a unit is alive so we can deal damage to it or not.
    native UnitAlive takes unit id returns boolean
    
    //This is the formula that filters out which units that should take damage.
    private function FilterUnits takes unit u, player p returns boolean
        return UnitAlive(u) and IsUnitEnemy(u, p) and not IsUnitType(u, UNIT_TYPE_MAGIC_IMMUNE)
    endfunction
    
    //DON'T EDIT ANYTHING BELOW
	private struct Spell
        unit caster
        unit dummy
        real damage
        real angle
        player owner
        real range
        real inRange
        real speed
        integer lvl
	endstruct
    globals
		private group g = CreateGroup()
		private Spell carry
	endglobals
    
    private function Callback takes nothing returns nothing
        if FilterUnits(GetEnumUnit(),carry.owner) then
            call UnitDamageTarget(carry.caster, GetEnumUnit(), carry.damage, false, false, ATTACK_TYPE, DAMAGE_TYPE, null)
        endif
    endfunction
    
    private function loop takes nothing returns nothing
        local timer t = GetExpiredTimer()
        local Spell dat = GetTimerData(t)
        local real x = GetUnitX(dat.dummy)
        local real y = GetUnitY(dat.dummy)
        local unit dumm
    
        if UnitAlive(dat.caster) and dat.inRange < dat.range then
            call GroupEnumUnitsInRange( g, GetUnitX(dat.dummy), GetUnitY(dat.dummy), aoe, null)
            set carry = dat
            call ForGroup(g, function Callback)
            set dumm = CreateUnit(dat.owner, DUMMY, GetUnitX(dat.dummy), GetUnitY(dat.dummy), dat.angle*bj_RADTODEG)
            call SetUnitX(dat.dummy, GetUnitX(dat.dummy)+dat.speed*Cos(dat.angle))
            call SetUnitY(dat.dummy, GetUnitY(dat.dummy)+dat.speed*Sin(dat.angle))
            call UnitApplyTimedLife(dumm, 'BTLF', .45)
            call SetUnitExploded(dumm, true)
            set dumm = null
            set dat.inRange = dat.inRange + 5.
        else
            call ReleaseTimer(t)
            call SetUnitExploded(dat.dummy, true)
            call KillUnit(dat.dummy)
            set dat.dummy = null
            set dat.caster = null
            set dat.owner = null
            call dat.destroy()
        endif
    endfunction
    private function run takes nothing returns nothing
        local Spell dat = Spell.create()
        set dat.caster  = GetTriggerUnit()
        set dat.owner   = GetOwningPlayer(dat.caster)
        set dat.lvl     = GetUnitAbilityLevel(dat.caster, AID)
        set dat.damage  = GetDamage(dat.lvl)
        set dat.range   = GetRange(dat.lvl)
        set dat.angle   = Atan2(GetSpellTargetY()-GetUnitY(dat.caster),GetSpellTargetX()-GetUnitX(dat.caster))
        set dat.dummy   = CreateUnit(dat.owner,DUMMY,GetUnitX(dat.caster) + Offset*Cos(dat.angle),GetUnitY(dat.caster) + Offset*Sin(dat.angle),dat.angle*bj_RADTODEG)
        set dat.inRange = 0
        set dat.speed   = GetSpeed(dat.lvl)
        call TimerStart(NewTimerEx(dat), interval, true, function loop)
    endfunction
    
    private function init takes nothing returns nothing
        call RegisterSpellEffectEvent(AID, function run)
    endfunction
endscope

Ultimately the most important thing is identifying a style you're comfortable with and using that. Just don't feel constrained by what people here tend to do, since most of them have a very singular-minded view of how to approach problems like this which is by no means the only, nor necessarily the best, one. In this particular case it's mostly just whatever style you prefer given that they both compile into the same thing. I personally argue that keeping stuff out of the struct makes it a little more readable (mostly because it forces people not to abuse the .x shortcut for this.x, which is really horrible for readability) but if you prefer methods then by all means go with that.

 

[BPower]

There is nothing wrong with using structs. Like mentioned it basically depends on what you are comfortable with.
However structs come along with very nice functionalities i.e. methods. Note that methods are not equal to static methods.

For some reasons I think the . is very neat when it comes to readability.
Btw the moment you decide to use a struct members for your array variables, you create all the struct associated code. (Creators, Destructors, ... )
Unfortunately the tutorial showing what code structs generate is gone :/.

(mostly because it forces people not to abuse the .x shortcut for this.x, which is really horrible for readability)

I did so too in the past and I agree with you today it's ugly, but this.x also equals x which I think looks very clean.