Looking for coding advice (leaks & speed)

[LurkerAspect]

Hi there all

I've been doing JASS for some time, and I've reached a level where I'm comfortable and don't really want to get any more advanced (not planning to be the next Jesus4Lyf!). The spell I have inserted below is what I consider to be the best example of my coding knowledge, I was just wondering where I've gone wrong and what I can do to improve the speed and security of my coding. Help will be rewarded with mention in my current map and a generous wallop of +rep (considering I can give more than 1 rep point these days)

So take a look and tell me what you think! I'm personally concerned about the use of the HIT_GROUP group array, if there's a better MUI way instead of my current MPI (multiple player instanceability!) then please let me know!

//AF_0A_100100_FIRE_BLAST
//Quick synopsis for your convenience: Player casts spell at target point, caster breathes a cone of fire which damages and ignites units (hence the parasite ability)
scope FireBlast initializer init

private keyword FIRE

globals
    private constant integer ABILCODE = 'A00A'
    private constant integer DUMMY_ABILCODE = 'ACpa'
    private constant real CONE_SIZE = 60
    private constant real IMPACT_SIZE = 100
    private constant real DAMAGE = 50
    private constant real SPEED = 900
    private constant real DISTANCE = 500
    private constant string ATTACHMENT_SFX = "Abilities\\Weapons\\RedDragonBreath\\RedDragonMissile.mdl"
    private constant string HIT_SFX = "Abilities\\Weapons\\RedDragonBreath\\RedDragonMissile.mdl"
    
    private integer I = 0
    private FIRE array DATA
    private unit CASTER = null
    private timer TIMER = CreateTimer()
    private group array HIT_GROUP
endglobals

private function GC takes nothing returns boolean
    local unit u = GetFilterUnit()
    local unit d
    local integer p = GetPlayerId(GetOwningPlayer(CASTER))
    if CheckTarget(CASTER,u) and not IsUnitInGroup(u,HIT_GROUP[p]) then
        set d = CreateUnit(Player(p),DUMMY,GetUnitX(u),GetUnitY(u),0)
        call UnitAddAbility(d,DUMMY_ABILCODE)
        call IssueTargetOrder(d,"parasite",u)
        call UnitApplyTimedLife(d,EXPIRE,2)
        call UnitDamageTarget(CASTER,u,DAMAGE,false,false,ATTACK_TYPE_HERO,DAMAGE_TYPE_FIRE,null)
        call DestroyEffect(AddSpecialEffect(HIT_SFX,GetUnitX(u),GetUnitY(u)))
        call GroupAddUnit(HIT_GROUP[p],u)
        set d = null
    endif
    set u = null
    return false
endfunction

private struct FIRE
    unit caster
    unit array dummy[99]
    real array dX[99]
    real array dY[99]
    effect array attachment[99]
    integer dummycount = 0
    real step = 0
    
    static method create takes unit caster, real X, real Y returns FIRE
        local FIRE a = FIRE.allocate()
        local real target_angle = Atan2(Y-GetUnitY(caster),X-GetUnitX(caster))*bj_RADTODEG
        local real min_angle = target_angle-CONE_SIZE/2
        local real max_angle = target_angle+CONE_SIZE/2
        local real angle_step = (((2*bj_PI*DISTANCE)/IMPACT_SIZE)/(2*bj_PI*DISTANCE))*360
        
        set a.caster = caster
        if HIT_GROUP[GetPlayerId(GetOwningPlayer(a.caster))] == null then
            set HIT_GROUP[GetPlayerId(GetOwningPlayer(a.caster))] = CreateGroup()
        endif
        loop
            exitwhen min_angle > max_angle
            set a.dummycount = a.dummycount+1
            set a.dummy[a.dummycount] = CreateUnit(Player(PLAYER_NEUTRAL_PASSIVE),DUMMY,GetUnitX(a.caster),GetUnitY(a.caster),min_angle)
            set a.attachment[a.dummycount] = AddSpecialEffectTarget(ATTACHMENT_SFX,a.dummy[a.dummycount],"head")
            set a.dX[a.dummycount] = SPEED*INTERVAL*Cos(min_angle*bj_DEGTORAD)
            set a.dY[a.dummycount] = SPEED*INTERVAL*Sin(min_angle*bj_DEGTORAD)
            set min_angle = min_angle+angle_step
        endloop
        
        return a
    endmethod
    
    method Step takes nothing returns nothing
        local integer i = 1
        local real X = 0
        local real Y = 0
        
        loop
            exitwhen i > .dummycount
            set X = GetUnitX(.dummy<i>)+.dX<i>
            set Y = GetUnitY(.dummy<i>)+.dY<i>
            call SetUnitPosition(.dummy<i>,X,Y)
            set CASTER = .caster
            call GroupEnumUnitsInRange(GROUP,X,Y,IMPACT_SIZE,Filter(function GC))
            set i = i+1
        endloop
        
        set .step = .step+SPEED*INTERVAL
    endmethod
    
    method destroy takes nothing returns nothing
        local integer i = 1
        loop
            exitwhen i &gt; .dummycount
            call DestroyEffect(.attachment<i>)
            call KillUnit(.dummy<i>)
            set .dX<i> = 0
            set .dY<i> = 0
            set i = i+1
        endloop
        call GroupClear(HIT_GROUP[GetPlayerId(GetOwningPlayer(.caster))])
        set .caster = null
        set .step = 0
        set .dummycount = 0
        call .deallocate()
    endmethod
endstruct

        
private function callback takes nothing returns nothing
    local FIRE a
    local integer i = 1
    
    loop
        exitwhen i &gt; I
        set a = DATA<i>
        if a.step &gt; DISTANCE then
            call a.destroy()
            set DATA<i> = DATA<i>
            set I = I-1
            set i=i-1
        else
            call a.Step()
        endif
        set i = i+1
    endloop
    
    if I == 0 then
        call PauseTimer(TIMER)
    endif
endfunction

private function Actions takes nothing returns nothing
    local FIRE a = FIRE.create(GetTriggerUnit(),GetLocationX(GetSpellTargetLoc()),GetLocationY(GetSpellTargetLoc()))
    
    set I = I+1
    set DATA<i> = a
    
    if I == 1 then
        call TimerStart(TIMER,INTERVAL,true,function callback)
    endif
endfunction


private function Conditions takes nothing returns boolean
    return GetSpellAbilityId() == ABILCODE
endfunction

private function init takes nothing returns nothing
    local trigger t = CreateTrigger()
    local integer i = 0
    call TriggerRegisterAnyUnitEventBJ(t,EVENT_PLAYER_UNIT_SPELL_CAST)
    call TriggerAddCondition(t,Condition(function Conditions))
    call TriggerAddAction(t,function Actions)
    set t = null
    call DestroyTrigger(t)
endfunction

endscope</i></i></i></i></i></i></i></i></i></i></i></i></i>

For those who are curious, I'm using a blank dummy model with attachment references and adding special effects to them, instead of making hundreds of individualised dummies. Would a recycling system be a good idea? I'm also using an archaic struct system that I dug up somewhere (I can't remember where!) which enables my spells to run on one timer each.

Any tips, advice, and (constructive if possible!) criticism welcome

 

[Frozenhelfir]

So before I look at this if I have it correct, you're trying to trigger a breath of fire (and all the art entailed) that deals damage on hit and then leaves debuff on someone? Just out of curiousity, what does the debuff actually do? Is this all it is supposed to do? I just want to make sure I have what you want correct before looking to deep into this because I see quite a lot of things that I would change if I am correct in what I am thinking.

set t = null
    call DestroyTrigger(t)

for starters, yes... it is the init so it isn't like any optimization is going down, but I don't understand why you would destroy a null trigger >_> Here is my initial perusal of the code. It compiles, but I don't know if it'll still work because I may have typed something wrong somewhere. This should make it SUI unless I botched something. I think your biggest efficiency hit now is to get rid of that timer thing you have going. Aside from that, the debuff information is somewhat pertinent right now.

//AF_0A_100100_FIRE_BLAST
//Quick synopsis for your convenience: Player casts spell at target point, caster breathes a cone of fire which damages and ignites units (hence the parasite ability)
scope FireBlast initializer init

private keyword FIRE

globals
    private constant integer ABILCODE = 'A00A'
    private constant integer DUMMY_ABILCODE = 'ACpa'
    private constant real CONE_SIZE = 60
    private constant real IMPACT_SIZE = 100
    private constant real DAMAGE = 50
    private constant real SPEED = 900
    private constant real DISTANCE = 500
    private constant string ATTACHMENT_SFX = "Abilities\\Weapons\\RedDragonBreath\\RedDragonMissile.mdl"
    private constant string HIT_SFX = "Abilities\\Weapons\\RedDragonBreath\\RedDragonMissile.mdl"
    
    private integer I = 0
    private FIRE array DATA  //what purpose does this serve? i don't see this being very useful for a spell like breath of fire, maybe some bad coding practices took place >_>
    private unit CASTER = null
    private timer TIMER = CreateTimer()
    //private group array HIT_GROUP
    private real array dummyMovePerTickX //going to use dummy indexes for this
    private real array dummyMovePerTickY
    private real array dummyX //instead of GetUnitX/Y every tick, just store it with these if you're being a speed freak
    private real array dummyY
    
    //these are for compilation purposes... you'll want to remove them
    private group GROUP
    private integer DUMMY //I'd use DUMMY_ID for this to be more clear
    private real INTERVAL
endglobals


//remove this function... just using it in place of a unit indexer for compiling it
private function GetUnitIndex takes unit u returns integer
    return 0
endfunction

//GC is a REALLY bad function name, in fact most of your function and variable names are kind of bad for having someone else read them
//if you're looking for some real code security you should make these names more descriptive so when you decide to come back
//and change this code after an undisclosed amount of time you don't have to spend so much time trying to understand what you wrote

//I moved this into the struct as "AffectUnitsNearDummy"
/*private function GC takes nothing returns boolean
    local unit u = GetFilterUnit()
    local unit d
    local integer p = GetPlayerId(GetOwningPlayer(CASTER))
    if CheckTarget(CASTER,u) and not IsUnitInGroup(u,HIT_GROUP[p]) then
        set d = CreateUnit(Player(p),DUMMY,GetUnitX(u),GetUnitY(u),0)
        call UnitAddAbility(d,DUMMY_ABILCODE)
        call IssueTargetOrder(d,"parasite",u)
        call UnitApplyTimedLife(d,EXPIRE,2)
        call UnitDamageTarget(CASTER,u,DAMAGE,false,false,ATTACK_TYPE_HERO,DAMAGE_TYPE_FIRE,null)
        call DestroyEffect(AddSpecialEffect(HIT_SFX,GetUnitX(u),GetUnitY(u)))
        call GroupAddUnit(HIT_GROUP[p],u)
        set d = null
    endif
    set u = null
    return false
endfunction*/

private struct FIRE
    unit caster
    group dummies
    group unitsAffected
    //I really don't like stuff like this where you don't know exactly how many dummies you'll need
    //so you just make a "big enough" number that "should never be reached"... this leads to bad stuff
    //later on when you want to change the spell, and it limits your map to having 81 instances of the spell 
    //even if you never use all 100 dummies. I prefer to use a group if the number of dummies isn't already known.
    //unit array dummy[99]
    //real array dX[99]
    //real array dY[99]
    //Use the object editor for this field. If you're going to attach the effect to the dummy every time... just do it in the object editor
    //if you're looking for speed you want as much stuff done "natively" by the game engine, and putting the effect on the unit in the
    //object editor would do just that =p
    //effect array attachment[99]
    //integer dummycount = 0 //no need to keep track of how many dummies we have with the group method
    real step = 0
    static thistype tempFireStruct
    
    static method create takes unit caster, real X, real Y returns FIRE
        local FIRE a = FIRE.allocate()
        local real target_angle = Atan2(Y-GetUnitY(caster),X-GetUnitX(caster))*bj_RADTODEG
        local real min_angle = target_angle-CONE_SIZE/2
        local real max_angle = target_angle+CONE_SIZE/2
        local real angle_step = (((2*bj_PI*DISTANCE)/IMPACT_SIZE)/(2*bj_PI*DISTANCE))*360
        local unit tempDummy
        local integer dummyIndex
        //I added these at the bottom to try and make my changes easier to see/stick together, when you get your hands on this
        //I'd suggest changing the GetUnitX/Y up in target_angle to use these variables for some extra speed
        local real casterX = GetUnitX(caster)//you GetUnitX the caster every time in the loop, this way will be faster
        local real casterY = GetUnitY(caster)
        set a.caster = caster
        //if you're using any recycling system then i'd suggest changing these
        set a.dummies = CreateGroup()
        set a.unitsAffected = CreateGroup()
        set a.step = 0 //don't know why this wasn't in here, unless setting it to zero up there does it on every creation, otherwise it could cause problems maybe
        loop
            exitwhen min_angle > max_angle
            //set a.dummycount = a.dummycount+1
            set tempDummy = CreateUnit(Player(PLAYER_NEUTRAL_PASSIVE),DUMMY,casterX,casterY,min_angle)
            set dummyIndex = GetUnitIndex(tempDummy)
            call GroupAddUnit(a.dummies,tempDummy)
            //set a.attachment[a.dummycount] = AddSpecialEffectTarget(ATTACHMENT_SFX,a.dummy[a.dummycount],"head")
            set dummyX[dummyIndex] = casterX
            set dummyY[dummyIndex] = casterY
            set dummyMovePerTickX[dummyIndex] = SPEED*INTERVAL*Cos(min_angle*bj_DEGTORAD)
            set dummyMovePerTickY[dummyIndex] = SPEED*INTERVAL*Sin(min_angle*bj_DEGTORAD)
            set min_angle = min_angle+angle_step
        endloop
        set tempDummy = null
        return a
    endmethod
    
    static method AffectUnitsNearDummy takes nothing returns boolean
        local thistype this = thistype.tempFireStruct
        local unit u = GetFilterUnit()
        //local unit d
        //local integer playerId = GetPlayerId(GetOwningPlayer(CASTER))
        //I don't see a CheckTarget in here, so I'm just going to leave this commented for now
        //if CheckTarget(CASTER,u) and not IsUnitInGroup(u,HIT_GROUP[p]) then
        if not IsUnitInGroup(u,this.unitsAffected) then
            //commenting out this debuff pending your response
            //set d = CreateUnit(Player(p),DUMMY,GetUnitX(u),GetUnitY(u),0)
            //call UnitAddAbility(d,DUMMY_ABILCODE)
            //call IssueTargetOrder(d,"parasite",u)
            //call UnitApplyTimedLife(d,EXPIRE,2)
            
            //personally I'd look into a damage detection system if you aren't already using one, just a quick note =p
            call UnitDamageTarget(this.caster,u,DAMAGE,false,false,ATTACK_TYPE_HERO,DAMAGE_TYPE_FIRE,null)
            call DestroyEffect(AddSpecialEffect(HIT_SFX,GetUnitX(u),GetUnitY(u)))
            call GroupAddUnit(this.unitsAffected,u)
            //set d = null
        endif
        set u = null
        return false
    endmethod
    
    static method MoveDummies takes nothing returns nothing
        local unit dummy = GetEnumUnit()
        local integer dummyIndex = GetUnitIndex(dummy)
        set dummyX[dummyIndex] = dummyX[dummyIndex] + dummyMovePerTickX[dummyIndex]
        set dummyY[dummyIndex] = dummyY[dummyIndex] + dummyMovePerTickY[dummyIndex]
        call SetUnitPosition(dummy,dummyX[dummyIndex],dummyY[dummyIndex])
        call GroupEnumUnitsInRange(GROUP,dummyX[dummyIndex],dummyY[dummyIndex],IMPACT_SIZE,Filter(function thistype.AffectUnitsNearDummy))
        set dummy = null
    endmethod
    
    method Step takes nothing returns nothing
        local integer i = 1
       // local real X = 0
       // local real Y = 0
        set thistype.tempFireStruct = this
        call ForGroup(this.dummies,function thistype.MoveDummies)
        /*loop
            exitwhen i > .dummycount
            set dummyX
            //set X = GetUnitX(.dummy<i>)+.dX<i>
            //set Y = GetUnitY(.dummy<i>)+.dY<i>
            call SetUnitPosition(.dummy<i>,X,Y)
            set CASTER = .caster
            call GroupEnumUnitsInRange(GROUP,X,Y,IMPACT_SIZE,Filter(function GC))
            set i = i+1
        endloop**/
        
        set .step = .step+SPEED*INTERVAL
    endmethod
    
    static method KillDummies takes nothing returns nothing
        call KillUnit(GetEnumUnit())
    endmethod
    
    //i usually prefer using an onDestroy method instead, but this may be more optimal.
    method destroy takes nothing returns nothing
        local integer i = 1
        //again, if you're using a group recycler change these
        call ForGroup(this.dummies,function thistype.KillDummies)
        call DestroyGroup(this.dummies)
        call DestroyGroup(this.unitsAffected)
        set this.dummies = null
        set this.unitsAffected = null
        /*loop
            exitwhen i &gt; .dummycount
           //call DestroyEffect(.attachment<i>)
            call KillUnit(.dummy<i>)
            set .dX<i> = 0
            set .dY<i> = 0
            set i = i+1
        endloop*/
        //call GroupClear(HIT_GROUP[GetPlayerId(GetOwningPlayer(.caster))])
        set .caster = null
        set .step = 0
        //set .dummycount = 0
        call .deallocate()
    endmethod
endstruct

        
private function callback takes nothing returns nothing
    local FIRE a
    local integer i = 1
    
    loop
        exitwhen i &gt; I
        set a = DATA<i>
        if a.step &gt; DISTANCE then
            call a.destroy()
            set DATA<i> = DATA<i>
            set I = I-1
            set i=i-1
        else
            call a.Step()
        endif
        set i = i+1
    endloop
    
    if I == 0 then
        call PauseTimer(TIMER)
    endif
endfunction

private function Actions takes nothing returns nothing
    //erm, don't use locations <img src="data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7" class="smilie smilie--sprite smilie--sprite7" alt=":p" title="Stick Out Tongue    :p" data-shortname=":p" /> If you're using jass you shouldn't have to use any functions with Loc in the name that I know of
    //local FIRE a = FIRE.create(GetTriggerUnit(),GetLocationX(GetSpellTargetLoc()),GetLocationY(GetSpellTargetLoc()))
    local FIRE a = FIRE.create(GetTriggerUnit(),GetSpellTargetX(),GetSpellTargetY())
    
    set I = I+1
    set DATA<i> = a
    
    if I == 1 then
        call TimerStart(TIMER,INTERVAL,true,function callback)
    endif
endfunction


private function Conditions takes nothing returns boolean
    return GetSpellAbilityId() == ABILCODE
endfunction

private function init takes nothing returns nothing
    local trigger t = CreateTrigger()
    local integer i = 0
    call TriggerRegisterAnyUnitEventBJ(t,EVENT_PLAYER_UNIT_SPELL_CAST)
    call TriggerAddCondition(t,Condition(function Conditions))
    call TriggerAddAction(t,function Actions)
    set t = null
    call DestroyTrigger(t)
endfunction

endscope</i></i></i></i></i></i></i></i></i></i></i></i></i>

 

[LurkerAspect]

The debuff is based off parasite, and deals 5 damage per second over 5 seconds, with stacking options set to "kill unit".

private FIRE array DATA //is how my struct system works, it's an array that instances of the spell are added to and later called up in a loop in the callback function and the struct methods are called from there (hope that makes sense!)

I also never knew how to make and use a group function within a struct, that'll make my life so much easier in my other spells

Beautifully done and annotated, you sir, are an incredible human being :') I can see you really took time out of your day to check through my entire mess of code and fix it all up +rep is not enough, is there anything else you want as well? (aside from money, because I currently have no way of giving any to you >.<)

 

[luorax]

//i usually prefer using an onDestroy method instead, but this may be more optimal.

onDestroy sucks under every circumstance; it's also only necessary to use it when using polymorphism, interfaces and struct-extending, but all of them sucks as well. Let's just stick to ".destroy()" and ".deallocate()" - they can do the same for you, bot way, way faster.

EDIT:
Very well, I didn't touch that weird thing that handles cone angles, but here is my code:

scope FireBlast

globals
    private constant integer ABILCODE='A00A'
    private constant integer DUMMY_ABILCODE='ACpa'
    private constant real CONE_SIZE=60.
    private constant real IMPACT_SIZE=100.
    private constant real DAMAGE=50.
    private constant real SPEED=900.
    private constant real MAX_DISTANCE=500.
    private constant string ATTACHMENT_SFX="Abilities\\Weapons\\RedDragonBreath\\RedDragonMissile.mdl"
    private constant string HIT_SFX="Abilities\\Weapons\\RedDragonBreath\\RedDragonMissile.mdl"
    private constant integer DUMMY_ID='dumy'
    private constant real INTERVAL=.031250000
    
    private constant group ENUM_GROUP=CreateGroup()
endglobals


private function GetUnitIndex takes unit u returns integer
    return 0
endfunction

private struct FIRE
    unit caster
    group dummies
    group unitsAffected
    real step
    
    private static real array dummyMovePerTickX
    private static real array dummyMovePerTickY
    private static real array dummyX
    private static real array dummyY
    
    private thistype next
    private thistype prev
    
    private static thistype tempInstance
    method destroy takes nothing returns nothing
        local unit u
        loop
            set u=FirstOfGroup(this.dummies)
            exitwhen u==null
            call UnitApplyTimedLife(u,'BTLF',.01)
        endloop
        call DestroyGroup(this.dummies)
        call DestroyGroup(this.unitsAffected)
        set this.prev.next=this.next
        set this.next.prev=this.prev
        call this.deallocate()
    endmethod
    static method moveDummies takes nothing returns nothing
        local thistype this=thistype.tempInstance
        local integer dummyIndex=GetUnitIndex(GetEnumUnit())
        local unit u
        
        set thistype.dummyX[dummyIndex]=thistype.dummyX[dummyIndex]+thistype.dummyMovePerTickX[dummyIndex]
        set thistype.dummyY[dummyIndex]=thistype.dummyY[dummyIndex]+thistype.dummyMovePerTickY[dummyIndex]
        call SetUnitPosition(GetEnumUnit(),thistype.dummyX[dummyIndex],thistype.dummyY[dummyIndex])
        call GroupEnumUnitsInRange(ENUM_GROUP,thistype.dummyX[dummyIndex],thistype.dummyY[dummyIndex],/*
        */   IMPACT_SIZE,null)
        
        loop
            set u=FirstOfGroup(ENUM_GROUP)
            exitwhen u==null
            if not IsUnitInGroup(u,this.unitsAffected) then
                call UnitDamageTarget(this.caster,u,DAMAGE,false,false,ATTACK_TYPE_HERO,DAMAGE_TYPE_FIRE,null)
                call DestroyEffect(AddSpecialEffect(HIT_SFX,GetUnitX(u),GetUnitY(u)))
                call GroupAddUnit(this.unitsAffected,u)
            endif
        endloop
    endmethod
    private static method periodic takes nothing returns nothing
        local thistype this=thistype(0)
        loop
            set this=this.next
            exitwhen this==0
            if this.step<MAX_DISTANCE then
                set thistype.tempInstance=this
                call ForGroup(this.dummies,function thistype.moveDummies)
                set this.step=this.step+SPEED*INTERVAL
            else
                call this.destroy()
            endif
        endloop
    endmethod
    static method create takes unit caster,real x,real y returns thistype
        local thistype this=thistype.allocate()
        local real angle=Atan2(y-GetUnitY(caster),x-GetUnitX(caster))*bj_RADTODEG
        local real minAngle=angle-CONE_SIZE/2
        local real maxAngle=angle+CONE_SIZE/2
        local real angleOffset=(((2*bj_PI*MAX_DISTANCE)/IMPACT_SIZE)/(2*bj_PI*MAX_DISTANCE))*360
        local unit dummy
        local integer dummyIndex
        local real casterX=GetUnitX(caster)
        local real casterY=GetUnitY(caster)
        
        set thistype(0).next.prev=this
        set this.next=thistype(0).next
        set thistype(0).next=this
        set this.prev=thistype(0)
        
        set this.caster=caster
        set this.dummies=CreateGroup()
        set this.unitsAffected=CreateGroup()
        set this.step=0.
        loop
            exitwhen minAngle>maxAngle
            set dummy=CreateUnit(Player(PLAYER_NEUTRAL_PASSIVE),DUMMY_ID,casterX,casterY,minAngle)
            set dummyIndex=GetUnitIndex(dummy)
            call GroupAddUnit(this.dummies,dummy)
            set thistype.dummyX[dummyIndex]=casterX
            set thistype.dummyY[dummyIndex]=casterY
            set thistype.dummyMovePerTickX[dummyIndex]=SPEED*INTERVAL*Cos(minAngle*bj_DEGTORAD)
            set thistype.dummyMovePerTickY[dummyIndex]=SPEED*INTERVAL*Sin(minAngle*bj_DEGTORAD)
            set minAngle=minAngle+angleOffset
        endloop
        set dummy=null
        return this
    endmethod
    private static method onEffect takes nothing returns boolean
        if GetSpellAbilityId()==ABILCODE then
            call thistype.create(GetTriggerUnit(),GetSpellTargetX(),GetSpellTargetY())
        endif
        return false
    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.onEffect))
        
        
        call TimerStart(CreateTimer(),INTERVAL,true,function thistype.periodic)
    endmethod
endstruct
endscope

 

[Frozenhelfir]

Luorax, I see about the onDestroy, but why the FirstOfGroup loops? Unless I've just been away from Wc3 too long those things are much much worse than onDestroy . Good on you for changing all of the horrible naming conventions. Anyway, if we're getting super speed freak here, I've reverted some of the changes and added some. There are two systems you'll have to pick up in addition to a unit indexer if you don't have one already. They should both be a nice efficiency boost to your map if you use them.

scope FireBlast
//go and get these:
//http://www.thehelper.net/forums/showthread.php/132538-Timer32
//http://www.thehelper.net/forums/showthread.php/123288-GTrigger-Event-System
globals
    private constant integer ABIL_CODE='A00A'
    private constant integer DUMMY_ABILCODE='ACpa'
    private constant real CONE_SIZE=60.
    private constant real IMPACT_SIZE=100.
    private constant real DAMAGE=50.
    private constant real SPEED=900.
    private constant real MAX_DISTANCE=500.
    private constant string ATTACHMENT_SFX="Abilities\\Weapons\\RedDragonBreath\\RedDragonMissile.mdl"
    private constant string HIT_SFX="Abilities\\Weapons\\RedDragonBreath\\RedDragonMissile.mdl"
    private constant integer DUMMY_ID='dumy'
    private constant real INTERVAL=.031250000
    
    private constant group ENUM_GROUP=CreateGroup()
endglobals


private function GetUnitIndex takes unit u returns integer
    return 0
endfunction

private struct FIRE
    unit caster
    group dummies
    group unitsAffected
    real step
    
    private static real array dummyMovePerTickX
    private static real array dummyMovePerTickY
    private static real array dummyX
    private static real array dummyY
    
    //private thistype next
    //private thistype prev
    private static thistype tempInstance
    
    implement T32xs
    
    static method KillDummies takes nothing returns nothing
        call KillUnit(GetEnumUnit())
    endmethod
    
    method destroy takes nothing returns nothing
        //local unit u
        /*loop
            set u=FirstOfGroup(this.dummies)
            exitwhen u==null
            call UnitApplyTimedLife(u,'BTLF',.01)
        endloop*/
        call ForGroup(this.dummies, function thistype.KillDummies)
        call this.stopPeriodic()
        //remember the group recycler, this technically leaks if I remember correctly
        call DestroyGroup(this.dummies)
        call DestroyGroup(this.unitsAffected)
        //set this.prev.next=this.next
        //set this.next.prev=this.prev
        call this.deallocate()
    endmethod
    
    static method AffectUnitsNearDummy takes nothing returns boolean
        local thistype this = thistype.tempInstance
        local unit u = GetFilterUnit()
        //local unit d
        //local integer playerId = GetPlayerId(GetOwningPlayer(CASTER))
        //I don't see a CheckTarget in here, so I'm just going to leave this commented for now
        //if CheckTarget(CASTER,u) and not IsUnitInGroup(u,HIT_GROUP[p]) then
        if not IsUnitInGroup(u,this.unitsAffected) then
            //commenting out this debuff pending your response
            //set d = CreateUnit(Player(p),DUMMY,GetUnitX(u),GetUnitY(u),0)
            //call UnitAddAbility(d,DUMMY_ABILCODE)
            //call IssueTargetOrder(d,"parasite",u)
            //call UnitApplyTimedLife(d,EXPIRE,2)
            
            //personally I'd look into a damage detection system if you aren't already using one, just a quick note =p
            call UnitDamageTarget(this.caster,u,DAMAGE,false,false,ATTACK_TYPE_HERO,DAMAGE_TYPE_FIRE,null)
            call DestroyEffect(AddSpecialEffect(HIT_SFX,GetUnitX(u),GetUnitY(u)))
            call GroupAddUnit(this.unitsAffected,u)
            //set d = null
        endif
        set u = null
        return false
    endmethod
    
    static method moveDummies takes nothing returns nothing
        //doing this on every dummy is pretty suboptimal, do it before the forgroup
//        local thistype this=thistype.tempInstance
        //if you have to call GetEnumUnit() more than once make a local for it
        local unit enumUnit = GetEnumUnit()
        local integer dummyIndex=GetUnitIndex(enumUnit)
        //local unit u
        
        set thistype.dummyX[dummyIndex]=thistype.dummyX[dummyIndex]+thistype.dummyMovePerTickX[dummyIndex]
        set thistype.dummyY[dummyIndex]=thistype.dummyY[dummyIndex]+thistype.dummyMovePerTickY[dummyIndex]
        call SetUnitPosition(enumUnit,thistype.dummyX[dummyIndex],thistype.dummyY[dummyIndex])
        call GroupEnumUnitsInRange(ENUM_GROUP,thistype.dummyX[dummyIndex],thistype.dummyY[dummyIndex],/*
        */   IMPACT_SIZE,Filter(function thistype.AffectUnitsNearDummy))
        set enumUnit = null
        /*loop
            set u=FirstOfGroup(ENUM_GROUP)
            exitwhen u==null
            if not IsUnitInGroup(u,this.unitsAffected) then
                call UnitDamageTarget(this.caster,u,DAMAGE,false,false,ATTACK_TYPE_HERO,DAMAGE_TYPE_FIRE,null)
                call DestroyEffect(AddSpecialEffect(HIT_SFX,GetUnitX(u),GetUnitY(u)))
                call GroupAddUnit(this.unitsAffected,u)
            endif
        endloop*/
    endmethod
    
    method periodic takes nothing returns nothing
        if this.step<MAX_DISTANCE then
            set thistype.tempInstance=this
            call ForGroup(this.dummies,function thistype.moveDummies)
            set this.step=this.step+SPEED*T32_PERIOD
        else
            call this.destroy()
        endif
    endmethod
    
    static method onEffect takes nothing returns nothing
        local thistype this=thistype.allocate()
        local real x = GetSpellTargetX()
        local real y = GetSpellTargetY()
        local unit caster = GetTriggerUnit()
        //local real angle=Atan2(y-GetUnitY(caster),x-GetUnitX(caster))*bj_RADTODEG
        //local real minAngle=angle-CONE_SIZE/2
        //local real maxAngle=angle+CONE_SIZE/2
        local real angleOffset=(((2*bj_PI*MAX_DISTANCE)/IMPACT_SIZE)/(2*bj_PI*MAX_DISTANCE))*360
        local unit dummy
        local integer dummyIndex
        local real casterX=GetUnitX(caster)
        local real casterY=GetUnitY(caster)
        local real angle=Atan2(y-casterY,x-casterX)*bj_RADTODEG
        local real minAngle=angle-CONE_SIZE/2
        local real maxAngle=angle+CONE_SIZE/2
        
       /* set thistype(0).next.prev=this
        set this.next=thistype(0).next
        set thistype(0).next=this
        set this.prev=thistype(0)*/
        
        set this.caster=caster
        //remember the recycler :]
        set this.dummies=CreateGroup()
        set this.unitsAffected=CreateGroup()
        set this.step=0.
        loop
            exitwhen minAngle>maxAngle
            set dummy=CreateUnit(Player(PLAYER_NEUTRAL_PASSIVE),DUMMY_ID,casterX,casterY,minAngle)
            set dummyIndex=GetUnitIndex(dummy)
            call GroupAddUnit(this.dummies,dummy)
            set thistype.dummyX[dummyIndex]=casterX
            set thistype.dummyY[dummyIndex]=casterY
            set thistype.dummyMovePerTickX[dummyIndex]=SPEED*T32_PERIOD*Cos(minAngle*bj_DEGTORAD)
            set thistype.dummyMovePerTickY[dummyIndex]=SPEED*T32_PERIOD*Sin(minAngle*bj_DEGTORAD)
            set minAngle=minAngle+angleOffset
        endloop
        call this.startPeriodic()
        set dummy=null
        set caster = null
    endmethod
    
   /*private static method onEffect takes nothing returns nothing
        call thistype.create(GetTriggerUnit(),GetSpellTargetX(),GetSpellTargetY())
    endmethod*/
    
    private static method onInit takes nothing returns nothing
        local trigger t=CreateTrigger()
        call GT_RegisterStartsEffectEvent(t,ABIL_CODE)
        call TriggerAddAction(t,function thistype.onEffect)
        
        
        //call TimerStart(CreateTimer(),INTERVAL,true,function thistype.periodic)
    endmethod
endstruct
endscope

 

[LurkerAspect]

Hi Frozenhelfir, I'm still processing your first post I have one major problem with the spell; the group filter method doesn't run for some arbitrary reason. Could you help me out with this? I've incorporated every suggestion you made, but for some reason I can't call that one method. These two methods are the culprits:

static method HitUnit takes nothing returns boolean
        local thistype this = thistype.tempFIRE
        local unit u = GetFilterUnit()
        call BJDebugMsg("marker") //debug messages that I placed to try and let me know if the method is being called AT ALL
        if not IsUnitInGroup(u,this.hitGroup) then
            call UnitDamageTarget(this.caster,u,DAMAGE,false,false,ATTACK_TYPE_HERO,DAMAGE_TYPE_FIRE,null)
            call DestroyEffect(AddSpecialEffect(HIT_SFX,GetUnitX(u),GetUnitY(u)))
            call GroupAddUnit(this.hitGroup,u)
        endif
        call BJDebugMsg(GetUnitName(this.caster))
        set u = null
        return false
    endmethod
        
    static method MoveDummies takes nothing returns nothing
        local unit dummy = GetEnumUnit()
        local integer dummyIndex = GetUnitIndex(dummy)
        set dummyX[dummyIndex] = dummyX[dummyIndex]+dummyMoveX[dummyIndex]
        set dummyY[dummyIndex] = dummyY[dummyIndex]+dummyMoveY[dummyIndex]
        call SetUnitPosition(dummy,dummyX[dummyIndex],dummyY[dummyIndex])
        call GroupEnumUnitsInRange(GROUP,dummyX[dummyIndex],dummyY[dummyIndex],IMPACT_SIZE,Filter(function thistype.HitUnit))
        set dummy = null
    endmethod

What's bugging me is when I use your code, the method is called, despite the fact they're almost exactly the same. What've I done wrong?
and here's my entire code, which incorporates all your suggestions:

//AF_0A_100100_FIRE_BLAST
scope FireBlast initializer init

private keyword FIRE

globals
    private constant integer ABILCODE = 'A00A'
    private constant integer DUMMY_ABILCODE = 'ACpa'
    private constant integer DUMMY_ID = 'n002'
    private constant real CONE_SIZE = 60
    private constant real IMPACT_SIZE = 100
    private constant real DAMAGE = 50
    private constant real SPEED = 900
    private constant real DISTANCE = 500
    private constant string HIT_SFX = "Abilities\\Weapons\\RedDragonBreath\\RedDragonMissile.mdl"
    
    private integer I = 0
    private FIRE array DATA
    private unit CASTER = null
    private timer TIMER = CreateTimer()
    //private group array HIT_GROUP
    private real array dummyMoveX 
    private real array dummyMoveY
    private real array dummyX
    private real array dummyY

    private group GROUP = CreateGroup()
    private real INTERVAL = 0.02
endglobals

//private function GC takes nothing returns boolean
//    local unit u = GetFilterUnit()
//    local unit d
//    local integer p = GetPlayerId(GetOwningPlayer(CASTER))
//    if CheckTarget(CASTER,u) and not IsUnitInGroup(u,HIT_GROUP[p]) then
//        set d = CreateUnit(Player(p),DUMMY,GetUnitX(u),GetUnitY(u),0)
//        call UnitAddAbility(d,DUMMY_ABILCODE)
//        call IssueTargetOrder(d,"parasite",u)
//        call UnitApplyTimedLife(d,EXPIRE,2)
//        call UnitDamageTarget(CASTER,u,DAMAGE,false,false,ATTACK_TYPE_HERO,DAMAGE_TYPE_FIRE,null)
//        call DestroyEffect(AddSpecialEffect(HIT_SFX,GetUnitX(u),GetUnitY(u)))
//        call GroupAddUnit(HIT_GROUP[p],u)
//        set d = null
//    endif
//    set u = null
//    return false
//endfunction

private struct FIRE
    unit caster
    group dummies
    group hitGroup
    //unit array dummy[99]
    //real array dX[99]
    //real array dY[99]
    //effect array attachment[99]
    real step = 0
    static thistype tempFIRE
    
    static method create takes unit caster, real X, real Y returns FIRE
        local FIRE a = FIRE.allocate()
        local real target_angle = Atan2(Y-GetUnitY(caster),X-GetUnitX(caster))*bj_RADTODEG
        local real min_angle = target_angle-CONE_SIZE/2
        local real max_angle = target_angle+CONE_SIZE/2
        local real angle_step = (((2*bj_PI*DISTANCE)/IMPACT_SIZE)/(2*bj_PI*DISTANCE))*360
        local unit tempDummy
        local integer dummyIndex
        local real casterX = GetUnitX(caster)
        local real casterY = GetUnitY(caster)
        
        set a.caster = caster
        set a.dummies = CreateGroup()
        set a.hitGroup = CreateGroup()
        set a.step = 0
        //if HIT_GROUP[GetPlayerId(GetOwningPlayer(a.caster))] == null then
        //    set HIT_GROUP[GetPlayerId(GetOwningPlayer(a.caster))] = CreateGroup()
        //endif
        loop
            exitwhen min_angle > max_angle
            set tempDummy = CreateUnit(Player(PLAYER_NEUTRAL_PASSIVE),DUMMY_ID,casterX,casterY,min_angle)
            set dummyIndex = GetUnitIndex(tempDummy)
            call GroupAddUnit(a.dummies,tempDummy)
            set dummyX[dummyIndex] = casterX
            set dummyY[dummyIndex] = casterY
            set dummyMoveX[dummyIndex] = SPEED*INTERVAL*Cos(min_angle*bj_DEGTORAD)
            set dummyMoveY[dummyIndex] = SPEED*INTERVAL*Sin(min_angle*bj_DEGTORAD)
            
            set min_angle = min_angle+angle_step
        endloop
        set tempDummy = null
        return a
    endmethod
    
    static method HitUnit takes nothing returns boolean
        local thistype this = thistype.tempFIRE
        local unit u = GetFilterUnit()
        call BJDebugMsg("marker")
        if not IsUnitInGroup(u,this.hitGroup) then
            call UnitDamageTarget(this.caster,u,DAMAGE,false,false,ATTACK_TYPE_HERO,DAMAGE_TYPE_FIRE,null)
            call DestroyEffect(AddSpecialEffect(HIT_SFX,GetUnitX(u),GetUnitY(u)))
            call GroupAddUnit(this.hitGroup,u)
        endif
        call BJDebugMsg(GetUnitName(this.caster))
        set u = null
        return false
    endmethod
        
    static method MoveDummies takes nothing returns nothing
        local unit dummy = GetEnumUnit()
        local integer dummyIndex = GetUnitIndex(dummy)
        set dummyX[dummyIndex] = dummyX[dummyIndex]+dummyMoveX[dummyIndex]
        set dummyY[dummyIndex] = dummyY[dummyIndex]+dummyMoveY[dummyIndex]
        call SetUnitPosition(dummy,dummyX[dummyIndex],dummyY[dummyIndex])
        call GroupEnumUnitsInRange(GROUP,dummyX[dummyIndex],dummyY[dummyIndex],IMPACT_SIZE,Filter(function thistype.HitUnit))
        set dummy = null
    endmethod        
    
    method Step takes nothing returns nothing
        //local real X = 0
        //local real Y = 0
        
        call ForGroup(.dummies,function thistype.MoveDummies)
        //loop
            //exitwhen i > .dummycount
            //set X = GetUnitX(.dummy<i>)+.dX<i>
            //set Y = GetUnitY(.dummy<i>)+.dY<i>
            //call SetUnitPosition(.dummy<i>,X,Y)
            //set CASTER = .caster
            //call GroupEnumUnitsInRange(GROUP,X,Y,IMPACT_SIZE,Filter(function GC))
            //set i = i+1
        //endloop
        
        set .step = .step+SPEED*INTERVAL
    endmethod
    
    static method KillDummies takes nothing returns nothing
        call KillUnit(GetEnumUnit())
    endmethod
    
    method destroy takes nothing returns nothing
        call ForGroup(.dummies,function thistype.KillDummies)
        call DestroyGroup(.dummies)
        call DestroyGroup(.hitGroup)
        set .dummies = null
        set .hitGroup = null
        //local integer i = 1
        //loop
            //exitwhen i &gt; .dummycount
            //call DestroyEffect(.attachment<i>)
            //call KillUnit(.dummy<i>)
            //set .dX<i> = 0
            //set .dY<i> = 0
            //set i = i+1
        //endloop
        set .caster = null
        set .step = 0
        call .deallocate()
    endmethod
endstruct

private function callback takes nothing returns nothing
    local FIRE a
    local integer i = 1
    
    loop
        exitwhen i &gt; I
        set a = DATA<i>
        if a.step &gt; DISTANCE then
            call a.destroy()
            set DATA<i> = DATA<i>
            set I = I-1
            set i=i-1
        else
            call a.Step()
        endif
        set i = i+1
    endloop
    
    if I == 0 then
        call PauseTimer(TIMER)
    endif
endfunction

private function Actions takes nothing returns nothing
    local FIRE a = FIRE.create(GetTriggerUnit(),GetLocationX(GetSpellTargetLoc()),GetLocationY(GetSpellTargetLoc()))
    
    set I = I+1
    set DATA<i> = a
    
    if I == 1 then
        call TimerStart(TIMER,INTERVAL,true,function callback)
    endif
endfunction


private function Conditions takes nothing returns boolean
    return GetSpellAbilityId() == ABILCODE
endfunction

private function init takes nothing returns nothing
    local trigger t = CreateTrigger()
    local integer i = 0
    call TriggerRegisterAnyUnitEventBJ(t,EVENT_PLAYER_UNIT_SPELL_CAST)
    call TriggerAddCondition(t,Condition(function Conditions))
    call TriggerAddAction(t,function Actions)
    set t = null
    call DestroyTrigger(t)
endfunction

endscope</i></i></i></i></i></i></i></i></i></i></i></i></i>

I'm starting to wonder if my Newgen or vJass are outdated, because I don't have the GetSpellTargetX/Y native loaded in my function library :S

If I've made any stupid mistakes please forgive me, It's 2:42am and I'm bloody tired!

 

[Frozenhelfir]

You aren't setting thistype.tempFire before the ForGroup(movedummies) thing, so when you go into the filter it tries to use an uninitialized int and probably stops/crashes the thread.

 

[luorax]

Luorax, I see about the onDestroy, but why the FirstOfGroup loops? Unless I've just been away from Wc3 too long those things are much much worse than onDestroy .

Well, as far as I know it's way faster than ForGroup calls, especially with a null filter, because:
-ForGroup opens a new thread for each enumed unit;
-when filtering units, another useless thread is being opened.

Opening thread is a very slow function, with my method you can do the enums really fast.

Yea, I agree on adding extra snippets, but I wanted to get the best out of the no-snippet concept.
I'd recommend using SpellEffectEvent tho'; all the other stuff that GTrigger knows is neglible, just increases the map size for no reason at all.

EDIT:

if you have to call GetEnumUnit() more than once make a local for it

You wanted to say "more than three times", right? Because using a local handle variable over one or two native calls is actually slower, and I think it also looks worse.

 

[Frozenhelfir]

I've found great use out of the other GT events. It doesn't really add extra size to your map. The real size in maps comes from models and skins. I don't know why map size is really an argument when it comes to jass. Using filter functions instead of a firstofgroup was the overwhelming standard and widely accepted to be the fastest/most efficient method of doing groups back in the day in which I mapped. If you can point me to the benchmark/proof/whatever that firstofgroup is faster I'd like to see it. The firstofgroup loops also run into some stability problems when units get prematurely removed from the group via RemoveUnit() among other things, which is another reason why we used the filters/forgroup when possible. You also don't have to re-enum the group every time you want to use it. I also remember someone showing that local variables are indeed faster for more than one function call. Most of what I'm working on could very well be legacy and irrelevant and I'd be willing to change my mind pending the proofs.