[JASS] evaluate my spell trigger please (unit groups and channeling)

[SanKakU]

Notice: This post contains an old script. skip to the third post to see the current script by myself.

so it's a channeling spell...so i made a trigger for when the channeling starts and a trigger for when it ends.

i also register a specific "main unit" for each player and am using that for a group. what do you think? is this a good setup?

the event registration will happen somewhere else, that's not a problem.

i am not entirely sure about unit groups in general...so mostly i'm wondering if i'm handling the unit group properly...i think it is probably right.
i had another setup before this one which was mostly gui but i think maybe it leaked...it is of course an ultimate ability for only one hero so if it did leak it wouldn't be noticeable...but i wanted to go ahead and fix it anyway.

so how do you think i did? i was doing something like pick all the units in map matching unit is a sapper or something like that. now i'm just adding specific units from variable array to the unitgroup and i'm removing them from the unitgroup when the spell stops channeling.

starfall(slowall) on and off triggers

function Trig_starfall_Conditions takes nothing returns boolean
    return GetSpellAbilityId() == 'A00G'
endfunction

function Trig_starfall_slowsapper takes nothing returns nothing
    call SetUnitMoveSpeed( GetEnumUnit(), ( GetUnitDefaultMoveSpeed(GetEnumUnit()) - GetRandomReal(50.00, 150.00) ) )
endfunction

function Trig_starfall_Actions takes nothing returns nothing
call GroupAddUnit(udg_starfallgroup, udg_pickedunit[3])
call GroupAddUnit(udg_starfallgroup, udg_pickedunit[4])
call GroupAddUnit(udg_starfallgroup, udg_pickedunit[5])
call GroupAddUnit(udg_starfallgroup, udg_pickedunit[6])
call GroupAddUnit(udg_starfallgroup, udg_pickedunit[7])
call GroupAddUnit(udg_starfallgroup, udg_pickedunit[8])
call GroupAddUnit(udg_starfallgroup, udg_pickedunit[9])
call GroupAddUnit(udg_starfallgroup, udg_pickedunit[10])
call GroupAddUnit(udg_starfallgroup, udg_pickedunit[12])
call ForGroup(udg_starfallgroup, function Trig_starfall_slowsapper)
endfunction

//===========================================================================
function InitTrig_starfall takes nothing returns nothing
    set gg_trg_starfall = CreateTrigger(  )
    call TriggerAddCondition( gg_trg_starfall, Condition( function Trig_starfall_Conditions ) )
    call TriggerAddAction( gg_trg_starfall, function Trig_starfall_Actions )
endfunction

function Trig_starfall_off_Conditions takes nothing returns boolean
    return GetSpellAbilityId() == 'A00G'
endfunction

function Trig_starfall_off_sappernormalspeed takes nothing returns nothing
    call SetUnitMoveSpeed( GetEnumUnit(), GetUnitDefaultMoveSpeed(GetEnumUnit()) )
endfunction

function Trig_starfall_off_Actions takes nothing returns nothing
call ForGroup(udg_starfallgroup, function Trig_starfall_off_sappernormalspeed)
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[3])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[4])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[5])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[6])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[7])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[8])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[9])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[10])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[12])
endfunction

//===========================================================================
function InitTrig_starfall_off takes nothing returns nothing
    set gg_trg_starfall_off = CreateTrigger(  )
    call TriggerAddCondition( gg_trg_starfall_off, Condition( function Trig_starfall_off_Conditions ) )
    call TriggerAddAction( gg_trg_starfall_off, function Trig_starfall_off_Actions )
endfunction

 

[Silvenon]

Hmm I don't like using ForGroup much, but I think it's the matter of style. This is an example of an alternative which you can use:

function Trig_starfall_off_Actions takes nothing returns nothing
local unit u

loop
    set u = FirstOfGroup(udg_starfallgroup)
    exitwhen u == null
    call GroupRemoveUnit(udg_starfallgroup, u)
    call call SetUnitMoveSpeed( u, GetUnitDefaultMoveSpeed(u) )

call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[3])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[4])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[5])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[6])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[7])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[8])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[9])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[10])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[12])
endfunction

That's more efficient since you don't have to call GetEnumUnit(). Btw, now there's no need to have the Trig_starfall_off_sappernormalspeed function anymore.

Also, I suggest you start using vJass (you can download Jass NewGen Pack from wc3c.net), because it is much easier. Or you don't have to, but either way your naming style really hurts my eyes , try to do something about it.

For example, try to use local variables more, they are much cleaner to use. You can name them with just a single letter (two letters maximum). That would look much nicer.

Also, that spacing is horrible, for example this:

set gg_trg_starfall = CreateTrigger( )


looks too much converted from GUI. I tend to remove those spacings, especially if I (like you, apparently) don't code like that. It's best to keep your code homogeneous.

Also, I would suggest that you use loops, for example this:

call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[3])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[4])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[5])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[6])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[7])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[8])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[9])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[10])
call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[12])

could be done like this:

local integer i = 3
loop
    exitwhen i == 13
    call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[i])
    set i = i + 1
endloop

Also, I suggest you use indentation, the code would look much nicer that way.


Well, that's all, for now. I'm glad to see your conditions coded well

 

[SanKakU]

lol...i've barely tackled group problems at all! i've been avoiding this topic for months. what do the others think about his solution? is this good for improving the spell? are there other ways it can be improved?

just thought i'd mention a couple of things though. one thing, that first box of code of yours has at least two errors in it. secondly. i omitted number 11, but you did not seem to notice that.

ok, so i was thinking of changing the code to something like this:

ah...whoops, nm....too many errors. i keep forgetting you didn't edit much of both triggers, mostly the second one you edited.

the globals btw are in a different trigger, but that's not mattering since they're globals.
anyway, what i'm wondering is if the units that might be nonexisting units (of which there will most likely be some) in the unit array are going to be going into the unit group as null, and if that's going to be a problem. i would guess yes, so, how are we going to work around this? or is it fine?


idk, i think mostly, i'm just confused about how to use groups, lol.

do i have to ADD the units to the group and then remove them with each trigger? that's what i'm guessing. so i think i'll do that. but i mean is it going to be better to use local or global group or is it the same and what are the two going to look like?

ok so they will look something like this:

starfall

scope starfall initializer I

private function C takes nothing returns boolean
    return GetSpellAbilityId() == 'A00G'
endfunction

private function A takes nothing returns nothing
local unit u = null
local integer index = 2
loop
call GroupAddUnit(GROUPRUNNERS, RUNNER[index])
set index = index + 1
exitwhen index == 9
endloop
call GroupAddUnit(GROUPRUNNERS, RUNNER[11])
loop
set u = FirstOfGroup(GROUPRUNNERS)
exitwhen u == null
call SetUnitMoveSpeed(u, GetUnitDefaultMoveSpeed(u) - GetRandomReal(50.00, 150.00))
call GroupRemoveUnit(GROUPRUNNERS, u)
endloop
endfunction

//===========================================================================
private function I takes nothing returns nothing
    local trigger t = CreateTrigger(  )
    call TriggerRegisterPlayerUnitEvent( t, Player(0),EVENT_PLAYER_UNIT_SPELL_CAST, Filter(function FilterDebug) )
    call TriggerRegisterPlayerUnitEvent( t, Player(1),EVENT_PLAYER_UNIT_SPELL_CAST, Filter(function FilterDebug) )
    call TriggerRegisterPlayerUnitEvent( t, Player(10),EVENT_PLAYER_UNIT_SPELL_CAST, Filter(function FilterDebug) )
    call TriggerAddCondition( t, Condition( function C ) )
    call TriggerAddAction( t, function A )
    set bj_lastCreatedUnit = CreateUnit(Player(PLAYER_NEUTRAL_PASSIVE), 'earc', -80, -600, 0)    
    call UnitAddAbility(bj_lastCreatedUnit, 'A00G')        
    call KillUnit(bj_lastCreatedUnit)
endfunction
endscope

starfalloff

scope starfalloff initializer I

private function C takes nothing returns boolean
    return GetSpellAbilityId() == 'A00G'
endfunction

private function A takes nothing returns nothing
local unit u = null
local integer index = 2
loop
call GroupAddUnit(GROUPRUNNERS, RUNNER[index])
set index = index + 1
exitwhen index == 9
endloop
call GroupAddUnit(GROUPRUNNERS, RUNNER[11])
loop
set u = FirstOfGroup(GROUPRUNNERS)
exitwhen u == null
call SetUnitMoveSpeed(u, GetUnitDefaultMoveSpeed(u))
call GroupRemoveUnit(GROUPRUNNERS, u)
endloop
endfunction

//===========================================================================
private function I takes nothing returns nothing
    local trigger t = CreateTrigger(  )
    call TriggerRegisterPlayerUnitEvent( t, Player(0),EVENT_PLAYER_UNIT_SPELL_ENDCAST, Filter(function FilterDebug) )
    call TriggerRegisterPlayerUnitEvent( t, Player(1),EVENT_PLAYER_UNIT_SPELL_ENDCAST, Filter(function FilterDebug) )
    call TriggerRegisterPlayerUnitEvent( t, Player(10),EVENT_PLAYER_UNIT_SPELL_ENDCAST, Filter(function FilterDebug) )
    call TriggerAddCondition( t, Condition( function C ) )
    call TriggerAddAction( t, function A )
endfunction

endscope

global variables in a scope in another trigger

globals
unit array RUNNER
unit array CHASER
group GROUPRUNNERS
endglobals

is that ok?

should i add something like
if RUNNER[index] == null then
loop
set index = index + 1
exitwhen index == 9
endloop
endif
lol...the above would not work i think...i don't know what to write. i'm really bad at loops.
i would do that in order to skip to the next number while adding the units to the group.
that way i wouldn't add nulled units to the group. would this be a good idea??

 

[Vulcano]

You can do this for the omitted unit:

loop

local integer i = 3
loop
    exitwhen i == 13
    call GroupRemoveUnit(udg_starfallgroup, udg_pickedunit[i])
    set i = i + 1
    if i = 11 then
        i = i + 1
    endif
endloop

You can also add more tabs to make your code easier to read:

Function A

private function A takes nothing returns nothing
    local unit u = null
    local integer index = 2
    loop
        call GroupAddUnit(GROUPRUNNERS, RUNNER[index])
        set index = index + 1
        exitwhen index == 9
    endloop
    call GroupAddUnit(GROUPRUNNERS, RUNNER[11])
    loop
        set u = FirstOfGroup(GROUPRUNNERS)
        exitwhen u == null
        call SetUnitMoveSpeed(u, GetUnitDefaultMoveSpeed(u))
        call GroupRemoveUnit(GROUPRUNNERS, u)
    endloop
endfunction

 

[SanKakU]

ok, that helps with loops, thanks for that tip. now since i didn't ask about loops to begin with...and since everything else has pretty much been answered already...what about this group thing, is the way i got it set up a problem or not? so basically there would be 9 runners, if some players are not playing, or if simply some runners have been captured...then some of those 9 that i'm adding are going to be......incorrect additions to the group...so, i ask again, is this a problem?

maybe the way i explained it this time makes it more clear.

 

[Vulcano]

I still don't understand what you mean, i think you want to remove units owned by not playing players from group?