Why isnt my Holy Nova working?

[Roku]

Hey, i just finished this holy nova spell of mine, wich basically has 10% chance to trigger if a unit has a specific item and takes damage. I made the trigger in JASS and i can't quite understand what's wrong. Im using ABCT by Cohadar, and PUI by Cohadar (great systems ), and as you can see i inserted a debug message, wich isnt showing at all.
Here's my trigger:

scope HolyNova

globals
    private unit array caster    
    private integer pc = 0
endglobals

struct eff
    unit caster
    integer dummycount = 0
endstruct

private function Conds takes nothing returns boolean
    if ( not ( UnitHasItemOfTypeBJ(GetTriggerUnit(), 'I009') == true ) ) then
        return false
    endif
    if GetRandomInt(1,10) > 1 then
        return false
    endif
    
    return true
endfunction
        

private function Unit_Filter takes nothing returns boolean
    local unit f = GetFilterUnit()
    
    if(IsUnitType(f,UNIT_TYPE_MAGIC_IMMUNE)) then
       return false
    endif
    
    if(IsUnitType(f,UNIT_TYPE_STRUCTURE))then
	return false
    endif
	    
    set f = null
    
    return true
endfunction

private function Effects takes nothing returns boolean
    local eff data = ABCT_GetData()
    local unit u = data.caster
    local location p = GetUnitLoc(u)
    local unit d = CreateUnitAtLoc(GetOwningPlayer(u), 'h005', p,  bj_UNIT_FACING)
    
    if data.dummycount == 3 then
        call data.destroy()
        set data.dummycount = 0
        return true
    else
    
    call UnitAddAbility(d, 'A033')
    call IssuePointOrder(d, "detonate", GetLocationX(p), GetLocationY(p))
    call UnitApplyTimedLife(d, 'BTLF', 2.00)
    set data.dummycount = data.dummycount + 1
    endif
    
    return false
endfunction    

private function Heal takes nothing returns nothing
    local unit u = GetEnumUnit()
    local unit c = caster[pc]
    local real i = I2R(GetHeroInt(c, true))
    
    if IsUnitAlly(u, GetOwningPlayer(c)) then
        call SetUnitState(u, UNIT_STATE_LIFE, GetUnitState(u, UNIT_STATE_LIFE) + i)
    else
        call UnitDamageTarget(c, u, i / 2, true, false, ATTACK_TYPE_CHAOS, DAMAGE_TYPE_UNIVERSAL , WEAPON_TYPE_WHOKNOWS)
    endif
    
    set u = null
    set c = null    
endfunction

function Act takes nothing returns nothing
    local eff data = eff.create()
    local unit u = GetTriggerUnit()
    local location p = GetUnitLoc(u)
    local group healed
    
    call BJDebugMsg("HOLY NOVA!")
    set data.dummycount = 0
    set pc = GetUnitIndex(u)
    set caster[pc] = u
    set data.caster = u
    call ABCT_Start(function Effects, data, 0.10)
    set healed = GetUnitsInRangeOfLocMatching(600, p, Condition(function Unit_Filter))
    call ForGroup(healed, function Heal)
    
endfunction

//===========================================================================
function InitTrig_Holy_Nova takes nothing returns nothing
    local trigger t = CreateTrigger()
    call TriggerAddCondition(t, Condition(function Conds))
    call TriggerAddAction( t, function Act )
endfunction

endscope

Im using another trigger, with the event "unit enters playable map" to add the event "Unit - Takes Damage" to this trigger. Would appreciate your help, +rep.
(Ignore the leaks, i'll solve them later.)

 

[T.s.e]

Could it be because you make all the other functions private?

 

[SFilip]

> Im using another trigger, with the event "unit enters playable map" to add the event "Unit - Takes Damage" to this trigger
That's not really possible, you used a local variable in your InitTrig function so there's no way to refer to this trigger and that event never gets added.
No idea where that stupid local triggers "trend" came from, but you should always use the gg_ version in your InitTrig function.

 

[Trollvottel]

...or make a global public trigger to add the event.

i think this global trigger "trend" is because auf better readability.

 

[Roku]

Ok thanks it works now (cheers) but there is another issue...Whenever the nova procs, the special effect is created at the affected units position as well. I mean i have my hero with the item, another unit is damaging him, the nova goes off with its effect, but not only on my main hero but on the other affected unit also. So i have like 2 nova effects (explosions), but it only heals once wich is good.

Maybe it has something to do with the fact that the ability is based of detonate? I even renamed some local units, so they wont have the same name but that didnt seem to fix the problem.

 

[GoGo-Boy]

Might I add some optimization stuff that comes to my mind when I look at this?

private function Conds takes nothing returns boolean
    if ( not ( UnitHasItemOfTypeBJ(GetTriggerUnit(), 'I009') == true ) ) then
        return false
    endif
    if GetRandomInt(1,10) > 1 then
        return false
    endif
    
    return true
endfunction

Should be this: (cleaner and faster)

private function Conds takes nothing returns boolean
    if  UnitHasItemOfTypeBJ(GetTriggerUnit(), 'I009') then
        if GetRandomInt(1,100) <= 10 then   // using GetRandomInt(1,100) might be better because the wc3 random system doesn't work
        return true      // properly which shows of stronger when you use little random possibilities
    endif
endif
    
return false
endfunction

private function Unit_Filter takes nothing returns boolean
    local unit f = GetFilterUnit()
    
    if(IsUnitType(f,UNIT_TYPE_MAGIC_IMMUNE)) then
       return false
    endif
    
    if(IsUnitType(f,UNIT_TYPE_STRUCTURE))then
	return false
    endif
	    
    set f = null
    
    return true
endfunction

Is better this:

private function Unit_Filter takes nothing returns boolean
    local unit f = GetFilterUnit()
    
    loop
        exitwhen IsUnitType(f,UNIT_TYPE_MAGIC_IMMUNE) == true      // IsUnitType without a == true comparison cause bugs (which are solved by vex's optimizer though)
        exitwhen IsUnitType(f,UNIT_TYPE_STRUCTURE) == true
        set f = null
        return true 
    endloop
    
    set f = null
    
    return false
endfunction

And to be honest I think this is very unnecessary work:

private function Effects takes nothing returns boolean
    local eff data = ABCT_GetData()
    local unit u = data.caster
    local location p = GetUnitLoc(u)
    local unit d = CreateUnitAtLoc(GetOwningPlayer(u), 'h005', p,  bj_UNIT_FACING)
    
    if data.dummycount == 3 then
        call data.destroy()
        set data.dummycount = 0
        return true
    else
    
    call UnitAddAbility(d, 'A033')
    call IssuePointOrder(d, "detonate", GetLocationX(p), GetLocationY(p))
    call UnitApplyTimedLife(d, 'BTLF', 2.00)
    set data.dummycount = data.dummycount + 1
    endif
    
    return false
endfunction

You use a unit as an effect and add the ability detonate to get the death animation?? How about this:

call DestroyEffect(AddSpecialEffectLoc(*modelstring*, *location*))

 

[Roku]

>>And to be honest I think this is very unnecessary work:

private function Effects takes nothing returns boolean
    local eff data = ABCT_GetData()
    local unit u = data.caster
    local location p = GetUnitLoc(u)
    local unit d = CreateUnitAtLoc(GetOwningPlayer(u), 'h005', p,  bj_UNIT_FACING)
    
    if data.dummycount == 3 then
        call data.destroy()
        set data.dummycount = 0
        return true
    else
    
    call UnitAddAbility(d, 'A033')
    call IssuePointOrder(d, "detonate", GetLocationX(p), GetLocationY(p))
    call UnitApplyTimedLife(d, 'BTLF', 2.00)
    set data.dummycount = data.dummycount + 1
    endif
    
    return false
endfunction

No, not exactly. I have downloaded a model, wich looks like the detonate animation but its yellow. So i made a custom detonate, and added my model to it. The problem with the method you suggested (create a special effect) would fail, since i cant modify it's size to suite my heal AoE. This function has nothing to do with the units death animation, and im thinking of changing the dummy spell to another one beside detonate, because detonate dispells, which i do not want.

By the way, thanks for the improvements you suggested, ill correct them. And i used GetRandomInt(1,10) on purpose so my spell would proc more frequently, once every 10 hits, and not 10x every 100 hits.

 

[GoGo-Boy]

GetRandomInt(1,100) <= 10 is the same as GetRandomInt(1,10) < 1.
You're right though, it should be GetRandomInt(1,100) >= 11

 

[phyrex1an]

private function Conds takes nothing returns boolean
    if  UnitHasItemOfTypeBJ(GetTriggerUnit(), 'I009') then
        if GetRandomInt(1,100) <= 10 then   // using GetRandomInt(1,100) might be better because the wc3 random system doesn't work
        return true      // properly which shows of stronger when you use little random possibilities
    endif
endif
    
return false
endfunction

private function Conds takes nothing returns boolean
    return UnitHasItemOfTypeBJ(GetTriggerUnit(), 'I009') and GetRandomInt(1,100) <= 10
endfunction

private function Unit_Filter takes nothing returns boolean
    local unit f = GetFilterUnit()
    
    loop
        exitwhen IsUnitType(f,UNIT_TYPE_MAGIC_IMMUNE) == true      // IsUnitType without a == true comparison cause bugs (which are solved by vex's optimizer though)
        exitwhen IsUnitType(f,UNIT_TYPE_STRUCTURE) == true
        set f = null
        return true 
    endloop
    
    set f = null
    
    return false
endfunction

1. IsUnitType does only bug when you use it as only return from a function used as a boolexpr and only when you use something else than UNIT_TYPE_HERO.
2. return not (IsUnitType(GetFilterUnit(), UNIT_TYPE_MAGIC_IMMUNE) or IsUnitType(GetFilterUnit(), UNIT_TYPE_STRUCTURE))

 

[GoGo-Boy]

private function Conds takes nothing returns boolean
     if  UnitHasItemOfTypeBJ(GetTriggerUnit(), 'I009') then
         if GetRandomInt(1,100) <= 10 then   // using GetRandomInt(1,100) might be better because the wc3 random system doesn't work
             return true      // properly which shows of stronger when you use little random possibilities
         endif
     endif
      
return false
endfunction

I've asked for optimized conditions once here. Cohadar told me that this is the best way too do it. Nesting or how it is called.

 

[Terrabull]

Go-Go:
Nesting is practically the same as using an AND statement, it comes down to personal prefrence. Typing all that out is not desirable to me, especially if I can place it all in a "return" statement and just have one line.
That nesting strategy was given because you asked w/ a statement that had tons of conditions, nesting improves readability when there are more conditions than fit on a single line. It is wasted space and time if you are only using two.

 

[GoGo-Boy]

Hmm yeah, just thought about it being an improvement of reading. I just accustomed to it and I gotta say it improves the codes "look" quite a lot. You're right though, it unnecessary for 2 conditions.