[JASS] Unit Group Problem (Struct Related)

[FlaminG-HunteR]

Having a slight problem with a spell I've created with NewGen here.

It's meant to send the target unit flying backwards.
When it comes to a stop, a bunch of units are grouped within a small radius of that unit,
and are then knocked back in the 'Knockback_Aftershock' function.

All of that works, apart from the last part - the grouped-units aren't knocked back.
All I am able to tell from what is happening here, is that the grouped-units
are moved just once, and then the spell comes to a halt.

I've tried debugging in that non-working function, and it reads 0 for both groups.
Strange though, because when I try debugging in the previous function,
it reads above 0 (if there are units in the group).

So you should've been able to pick up what I'm trying to do here.
I'm using the simple "Loop & exitwhen unit of group = null" method,
with the extra duplicate group adding all removed units back into the one
thats had units removed from it inside the loop.

I've got no idea how to fix this.
Is there any other sort of method I can use that enables me to have access
to a struct's members when looping through a unit group?

scope Knockback

globals
    //Knockback effects config
    private constant integer ID   = 'AOcl'
    private constant real damage  = 4
    private constant real tdelay  = 0.03
    private constant real brake   = 0.975
    private constant real maxdist = 700
    private constant real power   = 25
    private constant string SFX   = "Abilities\\Spells\\Undead\\Impale\\ImpaleHitTarget.mdl"
    private constant string SFX2  = "Abilities\\Spells\\Human\\FlakCannons\\FlakTarget.mdl"

    //Knockback after-shock config
    private constant real damage2  = 325
    private constant real brake2   = 0.9
    private constant real maxdist2 = 445
    private constant real AoE      = 300   
    private constant real power2   = 10 
    private constant string SFX3   = "Abilities\\Spells\\Human\\ThunderClap\\ThunderClapCaster.mdl"
endglobals

struct Knockback_Data
    unit u   
    unit T  
    real ang = 0
    real mxd = maxdist
    real pwr = power
endstruct

struct Knockback_Aftershock_Data
    group g 
    group g2
    real mxd2 = 0
    real pwr2 = 0
    real OX   = 0
    real OY   = 0
endstruct

function Knockback_Filter takes nothing returns boolean
    return IsUnitEnemy(GetFilterUnit(), GetOwningPlayer(GetTriggerUnit())) and GetWidgetLife(GetFilterUnit()) > 0.405
endfunction

function Knockback_Aftershock takes nothing returns nothing
    local timer tt                     = GetExpiredTimer()
    local Knockback_Aftershock_Data d2 = Knockback_Aftershock_Data(GetHandleInt(tt, "d2"))
    local group g                      = d2.g
    local group g2                     = d2.g2
    local real mxd2                    = d2.mxd2
    local real pwr2                    = d2.pwr2
    local real OX                      = d2.OX
    local real OY                      = d2.OY
    local unit p 

        call BJDebugMsg(I2S(CountUnitsInGroup(d2.g)))
        call BJDebugMsg(I2S(CountUnitsInGroup(d2.g2)))

    if mxd2 > 0 then
        loop
            set p      = FirstOfGroup(g)
            exitwhen p == null
            call GroupRemoveUnit(g, p)
            call SetUnitPosition(p, GetUnitX(p) + pwr2 * Cos(Atan2(GetUnitY(p)-OY, GetUnitX(p)-OX)), GetUnitY(p) + pwr2 * Sin(Atan2(GetUnitY(p)-OY, GetUnitX(p)-OX)))
            call DestroyEffect(AddSpecialEffect(SFX2, GetUnitX(p), GetUnitY(p)))
        endloop

        call GroupAddGroup(g2, d2.g)

        set d2.pwr2 = d2.pwr2 * brake2
        set d2.mxd2 = d2.mxd2 - d2.pwr2
    else
        call d2.destroy()
        call PauseTimer(tt)
        call FlushHandleLocals(tt)
        call DestroyTimer(tt)
    endif


    set tt = null
    set g  = null
    set g2 = null
endfunction

function Knockback_Effects takes nothing returns nothing
    local timer t          = GetExpiredTimer()
    local Knockback_Data d = Knockback_Data(GetHandleInt(t, "d"))
    local unit u           = d.u
    local unit T           = d.T
    local real ang         = d.ang
    local real mxd         = d.mxd
    local real pwr         = d.pwr
    local real polarX      = GetUnitX(T) + pwr * Cos(ang)
    local real polarY      = GetUnitY(T) + pwr * Sin(ang)
    local Knockback_Aftershock_Data d2
    local group g
    local boolexpr b
    local timer tt

    if mxd > 0 then
        call SetUnitPosition(T, polarX, polarY)
        call DestroyEffect(AddSpecialEffect(SFX, polarX, polarY))
        call DestroyEffect(AddSpecialEffect(SFX2, polarX, polarY)) 
        call UnitDamageTarget(u, T, damage, false, false, ATTACK_TYPE_NORMAL, DAMAGE_TYPE_NORMAL, null)
        set d.pwr = d.pwr * brake
        set d.mxd = d.mxd - d.pwr
    else
        set g  = CreateGroup()
        set b  = Condition(function Knockback_Filter)
        set d2 = Knockback_Aftershock_Data.create()
        set tt = CreateTimer()

        call GroupEnumUnitsInRange(g, GetUnitX(T), GetUnitY(T), AoE, b)

        set d2.g       = g
        set d2.g2      = g
        set d2.mxd2    = maxdist2
        set d2.pwr2    = power2
        set d2.OX      = GetUnitX(T)
        set d2.OY      = GetUnitY(T) 

        call SetHandleInt(tt, "d2", d2)
        call TimerStart(tt, tdelay, true, function Knockback_Aftershock)

        call DestroyEffect(AddSpecialEffect(SFX3, GetUnitX(T), GetUnitY(T))) 
        
        call DestroyBoolExpr(b)
        set g  = null
        set b  = null
        set tt = null

        call PauseUnit(T, false)
        call d.destroy()
        call PauseTimer(t)
        call FlushHandleLocals(t)
        call DestroyTimer(t)
    endif

    set t = null
    set u = null
    set T = null
endfunction

function Knockback_Actions takes nothing returns nothing 
    local Knockback_Data d = Knockback_Data.create()
    local unit u           = GetTriggerUnit()
    local unit T           = GetSpellTargetUnit()
    local real X1          = GetUnitX(u)
    local real Y1          = GetUnitY(u)
    local real X2          = GetUnitX(T)
    local real Y2          = GetUnitY(T) 
    local real ang         = Atan2(Y2-Y1, X2-X1)
    local timer t          = CreateTimer()

    call PauseUnit(T, true)

    set d.u   = u
    set d.T   = T
    set d.ang = ang

    call SetHandleInt(t, "d", d)
    call TimerStart(t, tdelay, true, function Knockback_Effects)

    set u = null
    set T = null
endfunction

function Knockback_Conditions takes nothing returns boolean
    return GetSpellAbilityId() == ID
endfunction

function InitTrig_Knockback_Copy takes nothing returns nothing
    local trigger t = CreateTrigger(  )
    call TriggerRegisterAnyUnitEventBJ( t, EVENT_PLAYER_UNIT_SPELL_EFFECT )
    call TriggerAddCondition( t, Condition( function Knockback_Conditions ) )
    call TriggerAddAction( t, function Knockback_Actions )
    set t = null
endfunction

endscope

 

[Silvenon]

Wow, a nice piece of code.......very good triggering, nice job.

Why do you use a local trigger in InitTrig_Knockback_Copy function when you can use a global one that is created?

You forgot to nullify the timer in Knockback_Actions. Actually it's better to use Vexorian's CSSafety library which prevents a lot of timer bugs and also you don't have to nullify them.

There is no real need to do this:

local unit u                                   = d.u local
unit T                                          = d.T
local real ang                                = d.ang
local real mxd                                = d.mxd
local real pwr                                = d.pwr

Why don't you just use d.T, d.ang, d.mxd and d.pwr as normal variables? You don't have to make new locals and then equalize them. That way you'd save some space.

Also, it's more efficient to have Cos(ang) and Sin(ang) in a variable so you don't call them every 0.03 seconds (put them in the Knockback_Data struct).

It seems like boolexprs that are not generated with And(), Or() or Not() don't, so there's not need in destroying them in this case.

Now here's the problem, I think:

loop
    set p = FirstOfGroup(g)
    exitwhen p == null
    call GroupRemoveUnit(g, p)
    call SetUnitPosition(p, GetUnitX(p) + pwr2 * Cos(Atan2(GetUnitY(p)-OY, GetUnitX(p)-OX)), GetUnitY(p) + pwr2 * Sin(Atan2(GetUnitY(p)-OY, GetUnitX(p)-OX)))
    call DestroyEffect(AddSpecialEffect(SFX2, GetUnitX(p), GetUnitY(p)))
endloop
call GroupAddGroup(g2, d2.g)

This line messes that up:

set d2.g = g set d2.g2 = g

It should be like this:

local group g2 = CreateGroup()
// ...
// ...
call GroupAddGroup(d2.g, g2)
loop
    set p = FirstOfGroup(g2)
    exitwhen p == null
    call GroupRemoveUnit(g2, p)
    call SetUnitPosition(p, GetUnitX(p) + pwr2 * Cos(Atan2(GetUnitY(p)-OY, GetUnitX(p)-OX)), GetUnitY(p) + pwr2 * Sin(Atan2(GetUnitY(p)-OY, GetUnitX(p)-OX)))
    call DestroyEffect(AddSpecialEffect(SFX2, GetUnitX(p), GetUnitY(p)))
endloop
call DestroyGroup(g2)

I hope you understand what stuff you have to change, because I don't have time to explain. You did it too complicated when it could be much simpler.

Crap gotta go, I'll finish later

 

[PurplePoot]

You can easily get rid of all the locals in the init. (except the struct, and the timer if you don't want to put it inside the struct)

By the way, you can clean up that group loop a lot, and storing only 1 group;

        loop
            set p      = FirstOfGroup(g)
            exitwhen p == null
            call GroupRemoveUnit(g, p)
            call SetUnitPosition(p, GetUnitX(p) + pwr2 * Cos(Atan2(GetUnitY(p)-OY, GetUnitX(p)-OX)), GetUnitY(p) + pwr2 * Sin(Atan2(GetUnitY(p)-OY, GetUnitX(p)-OX)))
            call DestroyEffect(AddSpecialEffect(SFX2, GetUnitX(p), GetUnitY(p)))
        endloop

        call GroupAddGroup(g2, d2.g)

Becomes (removing g2 altogether) (also faster and cleaner than silvenon's code)

//g is equal to dg.g
//local group g = d2.g
        set d2.g = CreateGroup()
        loop
            set p      = FirstOfGroup(g)
            exitwhen p == null
            call GroupRemoveUnit(g, p)
            call GroupAddUnit(d2.g,p)
            call SetUnitPosition(p, GetUnitX(p) + pwr2 * Cos(Atan2(GetUnitY(p)-OY, GetUnitX(p)-OX)), GetUnitY(p) + pwr2 * Sin(Atan2(GetUnitY(p)-OY, GetUnitX(p)-OX)))
            call DestroyEffect(AddSpecialEffect(SFX2, GetUnitX(p), GetUnitY(p)))
        endloop
        call DestroyGroup(g)

---

Silvenon, boolexprs still "leak" in a sense, (without And/Or/Not) but they're stored in a similar way to strings, and the same boolexpr can be reused if it wasn't destroyed.

 

[Silvenon]

(also faster and cleaner than silvenon's code)

I was in a real hurry and I didn't have time to understand the whole code and think of a most efficient way.

Silvenon, boolexprs still "leak" in a sense, (without And/Or/Not) but they're stored in a similar way to strings, and the same boolexpr can be reused if it wasn't destroyed.

Somebody said that destroying boolexprs is "dangerous", whatever that meant.

 

[PurplePoot]

If you're using them over time anywhere else, the boolexprs tend to die there too (due to the way they're stored)

 

[FlaminG-HunteR]

Thanks guys, that helped, and it now works.

My only problem now, is that, for some reason rather the filter isn't working properly.
Units are grouped regardless whether they're an enemy to the trig unit or not.

Is there a way to reference a struct in a filter function?
If not, is there a way to fix this?

 

[Silvenon]

Ah, here we have the problem:

function Knockback_Filter takes nothing returns boolean
    return IsUnitEnemy(GetFilterUnit(), GetOwningPlayer(GetTriggerUnit())) and GetWidgetLife(GetFilterUnit()) > 0.405
endfunction

GetTriggerUnit() is null in this case, because you're creating the boolexpr in a function that is not _Actions, so there's no event response. The easy way out of that would be something like this:

function Knockback_Filter takes nothing returns boolean
    return IsUnitEnemy(GetFilterUnit(), GetOwningPlayer(bj_ghoul[100])) and GetWidgetLife(GetFilterUnit()) > 0.405
endfunction

// ...
// ...
// Knockback_Actions function
set bj_ghoul[100] = GetTriggerUnit()

bj_ghoul is a global unit variable already existing, so you don't have to create a new one. It's MUI, because the boolexpr is created instantly, so you don't have to worry about that.

 

[PurplePoot]

No, silvenon, GetTriggerUnit() and such pass through functions that aren't threaded.

However, since the timer creates a new thread, GetTriggerUnit() isn't automatically passed.

Oh, by the way, you can use Filter() instead of Condition()

 

[Silvenon]

No, silvenon, GetTriggerUnit() and such pass through functions that aren't threaded.

However, since the timer creates a new thread, GetTriggerUnit() isn't automatically passed.

I don't know what 'threaded' means, but what I wanted to say is that in this case GetTriggerUnit() is null. In what other cases the event responses do pass?

Oh, by the way, you can use Filter() instead of Condition()

........but you said that there is no difference

 

[PurplePoot]

1) Threaded - in a new thread; runs parallel to the old one, on its own stack.

Meaning things like waits don't interfere between threads, and thread crashes only affect the thread they crash in.

2) Filter saves map size

I just personally have a habit of using Condition for Condition and Filter for Filter, like they're intended to.

 

[FlaminG-HunteR]

Thanks for the bj_ghoul thing Silvenon, it now works perfectly,
and I can flex the trigger more.

And thanks for helping out too, PurplePoot : )

 

[Silvenon]

2) Filter saves map size

Saves map size for unknown reason or saves map size because it has less letters?

Anyways, I'm gonna have to start using Filter then.....thanks for the advice

 

[PurplePoot]

Less letters, silvenon

 

[Silvenon]

Omg, ok Pooty, then I'm gonna be careful so I don't have 3 extra letters in my scripts, that will greatly decrease my map size.

If you're so worried about the map size, why don't you name your functions this way:

function PootyRulz takes nothing returns boolean
    return true
endfunction

Into:

function PR takes nothing returns boolean
    return true
endfunction

This way I saved a "lot" of space

I hope you were kidding.....

 

[PurplePoot]

No, I was actually serious. (If you use a lot of things like that, not just Filter vs Condition but other stuff, you can actually save quite a few KB or so on)

And you didn't save any space that way, thanks to old man optimizer

It's the same with GetSpellAbilityUnit vs GetTriggerUnit, and so on, and so forth.

It doesn't make enough of a difference that you should relearn that unless you're really scratching your 4mb limit, though.

 

[Silvenon]

"Serious" and "" don't go together.

And you didn't save any space that way, thanks to old man optimizer

What? I don't get it.....

It doesn't make enough of a difference that you should relearn that unless you're really scratching your 4mb limit, though.

Nah, maybe you weren't kidding, but I certanly was

 

[PurplePoot]

Optimizer reduces function names to shortest possible, remember, silv?

And is also sort of a "So there!" in a friendly way, IMO anyways

 

[Silvenon]

So there!

Optimizer reduces function names to shortest possible, remember, silv?

I can't remember something that I never new .
I don't use optimizers so I don't know everything they do

 

[PurplePoot]

Well, only Vexorian's does as far as I know, but it's why all the function names are weird when you try and open some protected maps.

Anyways, I think this thread has been hijacked -.-

I dunno if it matters too much since the problem is solved, but meh...