group problem

[Laiev]

well... what happen?...
the spell run without any problem, but when you cast in a point which got death units, the loop stop (?)...

any help will be welcome

scope HolyLight

globals
    private constant integer AbilId = 'A08F' //ability id
    private constant real Damage = 25.0 //damage per level
    private constant integer Aoe = 400 //area of effect which will grow
    private constant integer AoeGrow = 100 //grow rate of aoe of spell
    private constant string EffectEnemy = "Abilities\\Spells\\Undead\\AnimateDead\\AnimateDeadTarget.mdl" //effect in enemy when damage happen
    private constant string EffectAlly = "Abilities\\Spells\\Human\\HolyBolt\\HolyBoltSpecialArt.mdl" //effect in ally when heal happen
    private constant string Attach = "origin" //position of effect
endglobals

private function HolyLight_Conditions takes nothing returns boolean
    return GetSpellAbilityId() == AbilId
endfunction
    
private function UnitFilter takes nothing returns boolean
    local unit u = GetFilterUnit()
    return IsUnitType(u, UNIT_TYPE_STRUCTURE) == false and IsUnitType(u, UNIT_TYPE_DEAD) == false
endfunction
    
private function HolyLight_Actions takes nothing returns nothing
    local unit u
    local unit cast = GetTriggerUnit()
    local location p = GetSpellTargetLoc()
    local integer i = 0
    local integer n = 1
    local group g = CreateGroup ()
    loop
        exitwhen i >= Aoe
        call CreateEffectCircle (p, i, n, "Abilities\\Spells\\Items\\ResourceItems\\ResourceEffectTarget.mdl", 2)
        call GroupEnumUnitsInRangeOfLoc (g, p, I2R(i), Filter(function UnitFilter))
        loop
            set u = FirstOfGroup (g)
            exitwhen u == null
            if IsUnitEnemy (u, GetOwningPlayer (cast)) == true then
                call DestroyEffect (AddSpecialEffectTarget (EffectEnemy, u, Attach))
                call UnitDamageTarget (GetTriggerUnit(), u, Damage * ( GetUnitAbilityLevel (GetTriggerUnit(), AbilId)), true, false, ATTACK_TYPE_MAGIC, DAMAGE_TYPE_MAGIC, WEAPON_TYPE_WHOKNOWS)
                call GroupRemoveUnit(g,u)
            else
                call DestroyEffect (AddSpecialEffectTarget (EffectAlly, u, Attach))
                call SetWidgetLife(u, GetWidgetLife(u) + Damage * (GetUnitAbilityLevel (GetTriggerUnit(), AbilId)))
                call GroupRemoveUnit(g,u)
            endif
        endloop
        call DestroyGroup (g)
        set g = null
        set i = i + AoeGrow
        set n = n + 10
        call TriggerSleepAction (.3)
    endloop
    call RemoveLocation (p)
    set p = null
endfunction

//===========================================================================
function InitTrig_Holy_Light takes nothing returns nothing
    local trigger t = CreateTrigger()
    call TriggerRegisterAnyUnitEventBJ( t, EVENT_PLAYER_UNIT_SPELL_EFFECT )
    call TriggerAddCondition( t, Condition( function HolyLight_Conditions ) )
    call TriggerAddAction( t, function HolyLight_Actions )
endfunction

endscope

 

[Prozix]

I'm not sure but you create 2 groups

    local group g = CreateGroup () //Create a group
    local unit cast = GetTriggerUnit()
    loop
        exitwhen i >= Aoe
        set g = GetUnitsInRangeOfLocAll(I2R(i), p) //create another group without destroying the previous one

my advice: leave out CreateGroup()

        call DestroyGroup (g)
        set i = i + AoeGrow
        set n = n + 10
        call CreateEffectCircle (p, i, n, "Abilities\\Spells\\Items\\ResourceItems\\ResourceEffectTarget.mdl", 2)
        call TriggerSleepAction (.3)
    endloop
    set i = 0
    set n = 0
    call RemoveLocation (p)
    call DestroyGroup (g) //destroy an already destroyed group > no good

I think you should leave the DestroyGroup(g) out because you already create and destroy it inside the main loop.

I used FirstOfGroup too untill I read this article: How to use groups without leaking by Jesus4Life

 

[Laiev]

@Prozix

Ya, that BJ function create group, so instead it now i use [ljass]GroupEnumUnitsInRangeOfLoc[/ljass]

I removed the [ljass]call DestroyGroup (g)[/ljass] outside loop.

thx

i'll read that tutorial and will update the trigger a bit..

+rep


Any other suggestion are welcome :thup:

 

[Zwiebelchen]

Okay, a couple of things:

1) GetUnitState(u, UNIT_STATE_LIFE) is not a good way to check for living units.
Better use:

IsUnitType(u, UNIT_TYPE_DEAD) == false and GetUnitTypeId(u) != 0

2) As first poster said: Do not create g at local declaration. GetUnitsInRangeOfLocAll already returns a group

3) When you are at it: Do not use GetUnitsInRangeOfLocAll ... use GroupEnumUnitsInRange and use SpellTargetX and SpellTargetY

4) Do not use TriggerSleepAction in spells

5) Don't destroy the same group (g) twice

6) Don't forget to null all handle locals at the end of each function to avoid leaks

7) constant your constants

 

[cryowraith]

What the above poster said. Plus...

I think this line is causing the problems.

GetUnitState(u, UNIT_STATE_LIFE) >= 1

You are using it to check whether unit is alive or not right? If I'm not wrong the more accurate way is

GetUnitState(u, UNIT_STATE_LIFE) >= 0.405

And you do realize that the units nearer to the target location will get damaged/healed multiple times right? Just checking...

 

[Prozix]

an other suggestion would be to use a Timer system. The TriggerSleepAction(.3) is really ugly (because the it will not be .3 seconds before the rest of the code is run most of the time), but using a timer system would complicate your code I think. I don't know which one is the best for your project. I have used Key Timers 2.

Also, maybe you should get a group of units within the maximum range, then do a enum on them with a function that uses a global currentRange variable that you increase. I think this would speed up your code significantly.

@cryowraith
Yeah i noticed this but I don't think this causes the problem: he said that the loop stops when it encounters a dead unit. A unit with 0.672 life would be skipped, just like a dead unit.
Why don't you use the IsUnitDead(unit) (if this doesn't exist : not(IsUnitAlive(unit)) ) function? The magic number 0.405 can be quite confusing and I think the blizzard functions work just fine

Good luck with coding your spell

 

[Laiev]

Okay, a couple of things:

1) GetUnitState(u, UNIT_STATE_LIFE) is not a good way to check for living units.
Better use:

IsUnitType(u, UNIT_TYPE_DEAD) == false and GetUnitTypeId(u) != 0

I just use GetUnitState because i don't know which [ljass]IsUnitType[/ljass] are suppose to use [ljass]UNIT_TYPE_DEAD[/ljass]

2) As first poster said: Do not create g at local declaration. GetUnitsInRangeOfLocAll already returns a group

Fixed in my last post

3) When you are at it: Do not use GetUnitsInRangeOfLocAll ... use GroupEnumUnitsInRange and use SpellTargetX and SpellTargetY

What SpellTargetX/Y suppose to do? Can explain a bit more?

4) Do not use TriggerSleepAction in spells

I just use it because can't understand how Timer's work

5) Don't destroy the same group (g) twice

Fixed in my last post

6) Don't forget to null all handle locals at the end of each function to avoid leaks

Hmm.. I'm not sure if i need to null things which i remove/destroy, thx for it

7) constant your constants

your mean... my globals values?

What the above poster said. Plus...

I think this line is causing the problems.

GetUnitState(u, UNIT_STATE_LIFE) >= 1

You are using it to check whether unit is alive or not right? If I'm not wrong the more accurate way is

GetUnitState(u, UNIT_STATE_LIFE) >= 0.405

And you do realize that the units nearer to the target location will get damaged/healed multiple times right? Just checking...

You're right and yes, it'll heal/damage multiple times in aoe.

 

[Prozix]

Avoid using locations as much as possible... I hate them and they have coused many of the things I had written to fail doing what they should have.

so create two local reals instead of a location
and set the spell target x and y coordinates to them

6) I don't think its nessecary, but it helps avoiding hard to trace and unpredictable bugs.

7) Make things that will not change constant and global>that includes your circle effect for example

 

[Zwiebelchen]

I just use GetUnitState because i don't know which [ljass]IsUnitType[/ljass] are suppose to use [ljass]UNIT_TYPE_DEAD[/ljass]

[ljass]IsUnitType(u, UNIT_TYPE_DEAD)[/ljass] is safer than any GetUnitState, but the 0.405 approach is fine most of the timer.

What SpellTargetX/Y suppose to do? Can explain a bit more?

Just returning the target locations X and Y coordinate. No real advantage with that, but usually you try to avoid locations in spells and use coordinates instead, to make the code shorter and do not need to care about leaks.

I just use it because can't understand how Timer's work

Learn it. Read tutorials. You should learn about timers when creating spells.

Hmm.. I'm not sure if i need to null things which i remove/destroy, thx for it

You do. Removing a handle does not clear the reference to that handle. You need to manually null all handle variables used to avoid leaks.

your mean... my globals values?

Yes

6) I don't think its nessecary, but it helps avoiding hard to trace and unpredictable bugs.

It IS neccesary to clear the reference. Without a cleared reference, the variable remains and will leak. This has nothing to do with bugs at all.

 

[Prozix]

Well.. that absolutely makes no sense :banghead:
so this leaks

local group g = CreateGroup()
call DestroyGroup(g)

and this doesn't :O

local group g = CreateGroup()
call DestroyGroup(g)
set g = null

isn't the whole purpose of DestroyGroup(g) that it destroys the group

are you absolutely sure that this is the inevitable truth?

 

[_whelp]

A global group is better, since you don't use any waits between group actions... and just clear your group, so you don't have to keep making new handles.

 

[Laiev]

Well.. that absolutely makes no sense :banghead:
so this leaks

local group g = CreateGroup()
call DestroyGroup(g)

and this doesn't :O

local group g = CreateGroup()
call DestroyGroup(g)
set g = null

isn't the whole purpose of DestroyGroup(g) that it destroys the group

are you absolutely sure that this is the inevitable truth?

Ya, this make no sense to me too... also you can't 'null' integer/real, right?


~~~~~~

Upgraded the trigger in the first post and other problem, now just happen one wave lol... not exactly why... and the trigger still stopping to run when a death unit is in area of effect

 

[Prozix]

A global group is better, since you don't use any waits between group actions... and just clear your group, so you don't have to keep making new handles.

True, but not if Laiev is gonna do what I suggested, getting the in maxrange units and then do stuff with that group over time. The downside of that is that units that enter the range over time are not taken into account because they aren't in the initial group. It depends on what your spell should do ^^.

Ya, this make no sense to me too... also you can't 'null' integer/real, right?

There is no need to. Integers and reals are not handles to objects > they are destroyed automaticly just like handles. If you don't destroy the object that a handle points to, and the handle goes out of scope and gets destroyed automatically, the still uses memory but it cant be accessed anymore > memory leak.

 

[Prozix]

private function True takes nothing returns boolean
    return true
endfunction

IsUnitType(u, UNIT_TYPE_STRUCTURE) == false and IsUnitType(u, UNIT_TYPE_DEAD) == false

hmmm...
hmmm..

You might wanna do this

private function UnitFilter takes nothing returns boolean
    local unit u = GetFilterUnit()
    return IsUnitType(u, UNIT_TYPE_STRUCTURE) == false and IsUnitType(u, UNIT_TYPE_DEAD) == false
endfunction

 

[Laiev]

the True function is just to prevent boolexpr leak


EDIT: you mean use this filter instead of True filter?

 

[Prozix]

that's pretty much what I tried to instill in your mind yes

 

[Laiev]

still not working

updating the first post with lastest trigger

 

[Prozix]

nulling handles, I have searched for it:

Handle Reference Leak

Agents in Jass are reference counted. Whenever a variable is set to an agent, that agent's reference count increases. When reset, variables lower the reference count of their previously held agent, and naturally increases the reference counter of their new value. Because local variables don't lower their value's reference count when the variables stop existing (i.e. when the function instance ends), they must be manually set to point to something else before that in order to lower the reference count. Null can be used as the new value, as its reference count won't increase nor will it be destroyed, because it isn't an existing object. If an agent's handle count never reaches 0 after destruction, its handle slot will never be recycled.

so yes, you have to null variables that point to objects :O. Meaning that I have written lots of crappy things in my life X3

I will try to find why your trigger isn't working.

 

[Laiev]

little question about this trigger.....

for some reason, global unit u = GetTriggerUnit() can't be used?

 

[Romek]

private function UnitFilter takes nothing returns boolean
    local unit u = GetFilterUnit()
    return IsUnitType(u, UNIT_TYPE_STRUCTURE) == false and IsUnitType(u, UNIT_TYPE_DEAD) == false
endfunction

You were actually better off simply using [ljass]GetFilterUnit()[/ljass] twice; at least it doesn't leak.

You also don't need to null 'u' at the end of the function, as it's already null (this is a requirement for the loop to end).

> [ljass]private constant unit cast = GetTriggerUnit()[/ljass]
[ljass]GetTriggerUnit()[/ljass] isn't a constant function. That line is a big no-no.

Also, [ljass]GetUnitState(u, UNIT_STATE_LIFE)[/ljass] and [ljass]SetUnitState(u, UNIT_STATE_LIFE, int)[/ljass] can be replaced with [ljass]GetWidgetLife(u)[/ljass] and [ljass]SetWidgetLife(u, int)[/ljass].

Could you provide more detail as to when the spell doesn't work? Perhaps try debugging a little to see at which point it breaks too.