Spell behaves weirdly, help needed!

[LurkerAspect]

Hi there I've just made a long and complex (by my standards) JASS trigger for a custom spell. Basically, I have a spell that jumps between a group of units, starting at the target unit, and finishing with the caster. However, the function behaves quite weirdly;

• First Cast: always goes to the same unit (preplaced on the map) despite which unit I target
• First cast: does not hit all units in group, returns prematurely
• First cast (and possible more) dummy unit does not return to caster, but returns to caster's location when ability was used
• Second cast and beyond: Dummy ignores distances between target completely, damages them almost immediately or at a random point. Also "returns" to caster from any distance.

To make this clear, I am not concerned about the damage function at all. Please don't interfere with it. Currently, I'm looking at a total rewrite, but I'd like to see what's wrong with it right now, so I can learn for future. I can sense it has something to do with the group/s involved. I'm using TimerUtils.

Here is the whole wack of code, enjoy

scope MurderBolt initializer init

globals
    private integer ABILCODE = 'A00M'
    private integer DUMMYID = 'n004'
    private integer HIT_LIMIT = 10
    private real SPEED = 600
    private real RANGE = 2000
    private real IMPACTSIZE = SPEED*INTERVAL+10
    private string SFX = "Abilities\\Spells\\Undead\\DeathCoil\\DeathCoilSpecialArt.mdl"
    real INTERVAL = 0.02 //Not actually here, INTERVAL is a global stated at the beginning of the map, and is used in almost every spell struct
endglobals

private struct MB

    unit caster = null
    unit target = null
    unit dummy = null
    group targetlist = null
    real damage = 0
    real charge = 0
    integer hits = 1
    integer hitlimit = 0
    
    method onDestroy takes nothing returns nothing
        set .caster = null
        set .target = null
        set .dummy = null
        call DestroyGroup(.targetlist)
        set .damage = 0
        set .charge = 0
        set .hits = 1
        set .hitlimit = 0
    endmethod
    
    method Move takes nothing returns nothing
        local real angle = Atan2(GetUnitY(.target)-GetUnitY(.dummy),GetUnitX(.target)-GetUnitX(.dummy))
        local real x = GetUnitX(.dummy)+SPEED*INTERVAL*Cos(angle)
        local real y = GetUnitY(.dummy)+SPEED*INTERVAL*Sin(angle)
        call SetUnitPosition(.dummy,x,y)
        call SetUnitFacingTimed(.dummy,angle*bj_RADTODEG,0.00)
    endmethod
    
    method Hit takes nothing returns nothing
        local real r = 1-(GetUnitState(.target,UNIT_STATE_LIFE)/GetUnitState(.target,UNIT_STATE_MAX_LIFE))
        local real d = .damage*GetUnitState(.target,UNIT_STATE_MAX_LIFE)
        set .charge = .charge+d
        call DestroyEffect(AddSpecialEffect(SFX,GetUnitX(.target),GetUnitY(.target)))
        call UnitDamageTarget(.caster,.target,.damage*GetUnitState(.target,UNIT_STATE_MAX_LIFE),true,true,ATTACK_TYPE_CHAOS,DAMAGE_TYPE_DEATH,WEAPON_TYPE_WHOKNOWS)
        set .damage = r
        //call GroupRemoveUnit(.group,.target)
        //set .target = FirstOfGroup(.group)
        //set .hit = .hit +1
    endmethod
    
    method HitFirst takes nothing returns nothing
        local real r = 1-(GetUnitState(.target,UNIT_STATE_LIFE)/GetUnitState(.target,UNIT_STATE_MAX_LIFE))
        local real d = r*GetUnitState(.target,UNIT_STATE_MAX_LIFE)
        call DebugMessage("first dealt damage was: "+R2S(d)+" to "+GetUnitName(.target))
        set .damage = r
        set .charge = .charge+d
        call DestroyEffect(AddSpecialEffect(SFX,GetUnitX(.target),GetUnitY(.target)))
        call UnitDamageTarget(.caster,.target,d,true,true,ATTACK_TYPE_CHAOS,DAMAGE_TYPE_DEATH,WEAPON_TYPE_WHOKNOWS)
    endmethod
endstruct
        
private function GC takes nothing returns boolean
    return not IsUnitAlly(GetFilterUnit(),GetOwningPlayer(bj_lastCreatedUnit)) and (GetUnitState(GetFilterUnit(),UNIT_STATE_LIFE) > 0) and not IsUnit(GetFilterUnit(),bj_lastCreatedUnit) and GetUnitTypeId(GetFilterUnit()) != DUMMYID and GetOwningPlayer(GetFilterUnit()) != Player(15) and GetUnitAbilityLevel(GetFilterUnit(),'Aloc') == 0 and not IsUnitType(GetFilterUnit(),UNIT_TYPE_STRUCTURE)
endfunction

private function Callback takes nothing returns nothing
    local timer t = GetExpiredTimer()
    local MB a = GetTimerData(t)
    local real d = SquareRoot((GetUnitX(a.dummy)-GetUnitX(a.target))*(GetUnitX(a.dummy)-GetUnitX(a.target))-(GetUnitY(a.dummy)-GetUnitY(a.target))*(GetUnitY(a.dummy)-GetUnitY(a.target)))
    
    if GetUnitState(a.dummy,UNIT_STATE_LIFE) <= 0 then
        call DestroyGroup(a.targetlist)
        call a.destroy()
        call ReleaseTimer(t)
        call PauseTimer(t)
        call DestroyTimer(t)
    elseif d <= IMPACTSIZE and a.hits == 1 then
        call a.HitFirst()
        call GroupRemoveUnit(a.targetlist,a.target)
        set a.target = FirstOfGroup(a.targetlist)
        set a.hits = a.hits +1
        call DebugMessage("first hit successful, next target proof: "+GetUnitName(a.target))
        call PingMinimap(GetUnitX(a.target),GetUnitY(a.target),1)
    elseif d <= IMPACTSIZE and a.hits != 1 and a.hits < a.hitlimit then
        call a.Hit()
        call GroupRemoveUnit(a.targetlist,a.target)
        set a.target = FirstOfGroup(a.targetlist)
        set a.hits = a.hits +1
        call DebugMessage("hit registered")
    elseif d <= IMPACTSIZE and a.hits == a.hitlimit then
        call a.Hit()
        set a.target = a.caster
        call DebugMessage("final hit accomplished")
        set a.hits = a.hits +1
    elseif d <= IMPACTSIZE and a.hits >= a.hitlimit and a.caster == a.target then
        call DestroyGroup(a.targetlist)
        call KillUnit(a.dummy)
        call SetUnitState(a.caster,UNIT_STATE_LIFE,GetUnitState(a.caster,UNIT_STATE_LIFE)+a.charge)
        call SetUnitState(a.caster,UNIT_STATE_MANA,GetUnitState(a.caster,UNIT_STATE_MANA)+a.charge)
        call DestroyGroup(a.targetlist)
        call a.destroy()
        call ReleaseTimer(t)
        call PauseTimer(t)
        call DestroyTimer(t)
        call DebugMessage("dummy returned succesfully")
    else
        call a.Move()
    endif
    
    set d = 0
    set t = null
endfunction

private function Actions takes nothing returns nothing
    local MB a = MB.create()
    local timer t = CreateTimer()
    local integer i

    set a.dummy = null
    set a.target = null
    set a.caster = GetSpellAbilityUnit()
    set a.target = GetSpellTargetUnit()
    set a.targetlist = CreateGroup()
    set a.hits = 1
    set a.dummy = CreateUnit(Player(bj_PLAYER_NEUTRAL_EXTRA),DUMMYID,GetUnitX(a.caster),GetUnitY(a.caster),0)
    set bj_lastCreatedUnit = a.caster
    call DebugMessage("before groupenum")
    call GroupEnumUnitsInRange(a.targetlist,GetUnitX(a.caster),GetUnitY(a.caster),RANGE,Filter(function GC))
    call DebugMessage("after groupenum")
    set bj_lastCreatedUnit = null
    call GroupRemoveUnit(a.targetlist,a.target)
    set i = CountUnitsInGroup(a.targetlist)
    if i < 10 then
        set a.hitlimit = i
    else
        set a.hitlimit = 10
    endif
    call DebugMessage("group count: "+I2S(i))
    call DebugMessage("hit limit: "+I2S(a.hitlimit))
    call SetTimerData(t,a)
    call TimerStart(t,INTERVAL,true,function Callback)
    
    set t = null
endfunction
    

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

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

endscope

Feel free to ask for any more information you may need

Any useful help is greatly appreciated and will be handsomely rewarded

EDIT: Other coding tips are also appreciated, if you have anything to say about my coding criticism is welcome

 

[Rllulium]

I don't have time to find the problem in your trigger atm... but i will check it out later.
As for now, you should probably check out these:
Timer32
Leakless Groups

 

[NeuroToxin]

GetUnitState(.target,UNIT_STATE_LIFE)
//Can be
GetWidgetLife(.target)

private function GC takes nothing returns boolean
    return not IsUnitAlly(GetFilterUnit(),GetOwningPlayer(bj_lastCreatedUnit))
 and (GetUnitState(GetFilterUnit(),UNIT_STATE_LIFE) > 0)// Can be, IsUnitType(GetFilterUnit(), UNIT_TYPE_ALIVE)
and not IsUnit(GetFilterUnit(),bj_lastCreatedUnit)
 and GetUnitTypeId(GetFilterUnit()) != DUMMYID and GetOwningPlayer(GetFilterUnit()) != Player(15) and GetUnitAbilityLevel(GetFilterUnit(),'Aloc') == 0//Group Enum functions do not pick up locusted units anyways.
 and not IsUnitType(GetFilterUnit(),UNIT_TYPE_STRUCTURE)
endfunction

Also, make a local unit f, and replace all those filter units with f.

 

[emjlr3]

First Cast: always goes to the same unit (preplaced on the map) despite which unit I target

[ljass]FirstOfGroup[/ljass] will do that with a fixed random seed. Use [ljass]GroupPickRandomUnit[/ljass] instead

*will update later

 

[LurkerAspect]

Re-wrote spell, now works perfectly, even after I had some problems with functions not completing themselves, but thanks to a thread I made last year I was able to fix it. Always remember kids, initialize those variables!

@NeuroToxin: Thanks for the GetWidgetLife tip, but I think that your f idea leaks that unit variable, because you can't set it to null before you return the variable. I might be wrong, I read in a post somewhere that units leak a long time ago :S please correct me if I'm wrong!

 

[Laiev]

@da1nOnlyEd

You're right, if you use an unit variable to pass the FilterUnit, you'll leak that variable because you can't do something after the return.

 

[Rllulium]

@da1nOnlyEd

You're right, if you use an unit variable to pass the FilterUnit, you'll leak that variable because you can't do something after the return.

Now, I don't know what the his new trigger looks like, but if he followed that tutorial I linked, there should be no need for that.
Just move any group actions into the filter and always return false.