[JASS] Jass Trigger Rewritten - Take a Look

[SanKakU]

so i was sick today and had to stay home, so a couple of days ago i started rewriting one of my map's biggest triggers. i used the broken trigger for reference and started from scratch. anyway today i finished it. doesn't seem to have any bugs and i got all of that in there.

please let me know what you think, jass experts.

and for jass newcomers...if you have questions about it just ask.

scope pickrunnerandchaserrewrite initializer I
 
private function randomedpandaskills takes nothing returns nothing
    call TriggerRegisterUnitEvent( gg_trg_pandaren_flames, GetLastReplacedUnitBJ(), EVENT_UNIT_SPELL_EFFECT )
endfunction
private function randomedmageskills takes nothing returns nothing
    call TriggerRegisterUnitEvent( gg_trg_starfall, GetLastReplacedUnitBJ(), EVENT_UNIT_SPELL_CAST )
    call TriggerRegisterUnitEvent( gg_trg_starfall_off, GetLastReplacedUnitBJ(), EVENT_UNIT_SPELL_ENDCAST )
endfunction 
private function pandaskills takes nothing returns nothing
    call TriggerRegisterUnitEvent( gg_trg_pandaren_flames, GetTrainedUnit(), EVENT_UNIT_SPELL_EFFECT )
endfunction
private function mageskills takes nothing returns nothing
    call TriggerRegisterUnitEvent( gg_trg_starfall, GetTrainedUnit(), EVENT_UNIT_SPELL_CAST )
    call TriggerRegisterUnitEvent( gg_trg_starfall_off, GetTrainedUnit(), EVENT_UNIT_SPELL_ENDCAST )
endfunction 
 
private function randomedtreedemonskills takes nothing returns nothing
    call TriggerRegisterUnitEvent( gg_trg_consume_tree, GetLastReplacedUnitBJ(), EVENT_UNIT_SPELL_EFFECT )
    call TriggerRegisterUnitEvent( gg_trg_teleport_tree, GetLastReplacedUnitBJ(), EVENT_UNIT_SPELL_EFFECT )
endfunction    

private function treedemonskills takes nothing returns nothing
    call TriggerRegisterUnitEvent( gg_trg_consume_tree, GetTrainedUnit(), EVENT_UNIT_SPELL_EFFECT )
    call TriggerRegisterUnitEvent( gg_trg_teleport_tree, GetTrainedUnit(), EVENT_UNIT_SPELL_EFFECT )
endfunction    

private function randomeddemonmageskills takes nothing returns nothing
    call UnitAddItemByIdSwapped( udg_item_Copy[2], GetLastReplacedUnitBJ() )
    call UnitAddItemByIdSwapped( udg_item_Copy[3], GetLastReplacedUnitBJ() )
endfunction

private function demonmageskills takes nothing returns nothing
    call UnitAddItemByIdSwapped( udg_item_Copy[2], GetTrainedUnit() )
    call UnitAddItemByIdSwapped( udg_item_Copy[3], GetTrainedUnit() )
endfunction

 
private function randomchaser takes nothing returns nothing
call ReplaceUnitBJ(GetTrainedUnit(), udg_chasers_random[GetRandomInt(0, 9)], bj_UNIT_STATE_METHOD_DEFAULTS )
if GetUnitTypeId(GetLastReplacedUnitBJ()) == 'Edem' then
    call randomedmageskills()
else
    if GetUnitTypeId(GetLastReplacedUnitBJ()) == 'Ofar' then
        call randomedpandaskills()
    else
    endif
endif
endfunction
 
private function randomrunner takes nothing returns nothing
call ReplaceUnitBJ(GetTrainedUnit(), udg_runners_random[GetRandomInt(0, 25)], bj_UNIT_STATE_METHOD_DEFAULTS )
if GetUnitTypeId(GetLastReplacedUnitBJ()) == 'unec' then
    call randomedtreedemonskills()
else
    if GetUnitTypeId(GetLastReplacedUnitBJ()) == 'hdhw' then
        call randomeddemonmageskills()
    else
    endif
endif
endfunction

private function pnameset takes nothing returns nothing
    call SetPlayerName( GetOwningPlayer(GetTriggerUnit()), udg_strings[( GetConvertedPlayerId(GetOwningPlayer(GetTriggerUnit())) + 12 )] ) 
endfunction 
 
private function setmbiconitemreplaced takes nothing returns nothing
    call MultiboardSetItemIconBJ( udg_mainmultiboard, 1, S2I(udg_mbp[GetConvertedPlayerId(GetOwningPlayer(GetTriggerUnit()))]), udg_icons[GetUnitPointValue(GetLastReplacedUnitBJ())])
endfunction

private function setmbiconitemtrained takes nothing returns nothing
    call MultiboardSetItemIconBJ( udg_mainmultiboard, 1, S2I(udg_mbp[GetConvertedPlayerId(GetOwningPlayer(GetTriggerUnit()))]), udg_icons[GetUnitPointValue(GetTrainedUnit())])
endfunction

private function setmbitemname takes nothing returns nothing
    call MultiboardSetItemValueBJ( udg_mainmultiboard, 1, S2I(udg_mbp[GetConvertedPlayerId(GetOwningPlayer(GetTriggerUnit()))]), GetPlayerName(GetOwningPlayer(GetTriggerUnit())) )
endfunction
    
private function rru takes nothing returns nothing
    call UnitAddItemByIdSwapped( 'phlt', GetLastReplacedUnitBJ() )
    call RemoveUnit(GetTriggerUnit())
endfunction
    
private function ru takes nothing returns nothing
    call UnitAddItemByIdSwapped( 'phlt', GetTrainedUnit() )
    call RemoveUnit(GetTriggerUnit())
endfunction

private function spawnflyer takes nothing returns nothing
    call ReplaceUnitBJ(GetTriggerUnit(), 'nnht', bj_UNIT_STATE_METHOD_RELATIVE)
endfunction


private function A takes nothing returns nothing
if GetUnitTypeId(GetTriggerUnit()) == 'ncbf' or GetUnitTypeId(GetTriggerUnit()) == 'ncbe' or GetUnitTypeId(GetTriggerUnit()) == 'ndmg' then
    if GetUnitTypeId(GetTrainedUnit()) == 'edot' then
        call randomrunner()
        call setmbiconitemreplaced()
        call setmbitemname()
        call pnameset()
        call spawnflyer()
    else    
        if GetUnitTypeId(GetTrainedUnit()) == 'unec' then
            call treedemonskills()
            call setmbiconitemtrained()
            call setmbitemname()
            call pnameset()
            call spawnflyer()
        else
            if GetUnitTypeId(GetTrainedUnit()) == 'hdhw' then
                call demonmageskills()
                call setmbiconitemtrained()
                call setmbitemname()
                call pnameset()
                call spawnflyer()
            else
                call setmbiconitemtrained()
                call setmbitemname()
                call pnameset()
                call spawnflyer()
            endif
        endif
    endif
else
if GetUnitTypeId(GetTriggerUnit()) == 'nfnp' then
    if GetUnitTypeId(GetTrainedUnit()) == 'edry' then
        call randomchaser()
        call setmbitemname()
        call setmbiconitemreplaced()
        call pnameset()
        call rru()
    else
        if GetUnitTypeId(GetTrainedUnit()) == 'Edem' then
            call mageskills()
            call setmbitemname()
            call setmbiconitemtrained()
            call pnameset()
            call ru()
        else
            if GetUnitTypeId(GetTrainedUnit()) == 'Ofar' then
                call pandaskills()
                call setmbitemname()
                call setmbiconitemtrained()
                call pnameset()
                call ru()
            else
                call setmbitemname()
                call setmbiconitemtrained()
                call pnameset()
                call ru()
            endif
        endif
    endif
else
endif
endif
endfunction




//===========================================================================
private function I takes nothing returns nothing
    local trigger t = CreateTrigger(  )
    call TriggerRegisterAnyUnitEventBJ( t, EVENT_PLAYER_UNIT_TRAIN_FINISH )
    call TriggerAddAction( t, function A )
endfunction

endscope

 

[Deaod]

Learn to use (and love):

• call TriggerRegisterAnyUnitEventBJ(whichtrigger, whichevent)
• elseif

 

[SanKakU]

i'm not sure what you're suggesting. explain it, please.

 

[xAbysSx]

First things first, if you are using NewGen, youll notice a lot of red writing, why are you using JASS if you use all BJs? BJs is what GUI uses.

Second, I disagree with deaod, as hes also using a BJ, use a loop for that, hold Ctrl + Click the red and look at the alternative route(s).

Also at the bottom you have else, endif, endif, remove the else, theres nothing in it? Its not needed.

In the init part, you are creating trigger t, but its still leaking, use set t = null at the end.

But your main problems are way too many BJs and a bunch of what looks like pointless "else's". If they are not pointless then its just the code is a little sloppy to read, you should align everything.

One last thing, use a global integer for raw codes if you are going to use them more than once or repeatedly.
Example:

scope a initializer b
    globals
        constant integer ID1 = 'RAWCODE'
        constant integer ID2 = 'RAWCODE'
    endglobals
endscope

Besides that I dont see what else you are asking for, other than helping make the code more efficient.

 

[Deaod]

just because i use a BJ, you dont have to disagree with me.
There are some BJs you can use without too much penalty. TriggerRegisterAnyUnitEventBJ is one of them. Why? Because the leak caused by the null boolexpr is negligible.

On the other hand, you dont have to null t in the Init function, since the trigger is static (which is a good thing).

i spent a few minutes rewriting your code, and this is what i got:

scope pickrunnerandchaserrewrite initializer Init

private function setmbiconitem takes unit t, unit u returns nothing
    call MultiboardSetItemIconBJ( udg_mainmultiboard, 1, S2I(udg_mbp[GetConvertedPlayerId(GetOwningPlayer(t))]), udg_icons[GetUnitPointValue(u)])
endfunction

private function Actions takes nothing returns nothing
local unit u
local real x
local real y
local real a
local player p
local integer uid
if GetUnitTypeId(GetTriggerUnit()) == 'ncbf' or GetUnitTypeId(GetTriggerUnit()) == 'ncbe' or GetUnitTypeId(GetTriggerUnit()) == 'ndmg' then
    set u=GetTrainedUnit()
    if GetUnitTypeId(GetTrainedUnit()) == 'edot' then
        set x=GetUnitX(u)
        set y=GetUnitY(u)
        set a=GetUnitFacing(u)
        set p=GetOwningPlayer(u)
        set uid=udg_runners_random[GetRandomInt(0, 25)]
        call RemoveUnit(u)
        set u=CreateUnit(p,uid,x,y,a)
    endif
    if GetUnitTypeId(u)=='hdhw' then
        call UnitAddItemByIdSwapped( udg_item_Copy[2], u )
        call UnitAddItemByIdSwapped( udg_item_Copy[3], u )
    endif
    call setmbiconitem(GetTriggerUnit(), u)
    call MultiboardSetItemValueBJ( udg_mainmultiboard, 1, S2I(udg_mbp[GetConvertedPlayerId(GetOwningPlayer(GetTriggerUnit()))]), GetPlayerName(GetOwningPlayer(GetTriggerUnit())) )
    call SetPlayerName( GetOwningPlayer(GetTriggerUnit()), udg_strings[( GetConvertedPlayerId(GetOwningPlayer(GetTriggerUnit())) + 12 )] )
    call ReplaceUnitBJ(GetTriggerUnit(), 'nnht', bj_UNIT_STATE_METHOD_RELATIVE)
elseif GetUnitTypeId(GetTriggerUnit()) == 'nfnp' then
    set u=GetTrainedUnit()
    if GetUnitTypeId(GetTrainedUnit()) == 'edry' then
        set x=GetUnitX(u)
        set y=GetUnitY(u)
        set a=GetUnitFacing(u)
        set p=GetOwningPlayer(u)
        set uid=udg_chasers_random[GetRandomInt(0, 9)]
        call RemoveUnit(u)
        set u=CreateUnit(p, uid, x, y, a)
    endif
    call UnitAddItemByIdSwapped( 'phlt', u )
    call setmbiconitem(GetTriggerUnit(), u)
    call SetPlayerName( GetOwningPlayer(GetTriggerUnit()), udg_strings[( GetConvertedPlayerId(GetOwningPlayer(GetTriggerUnit())) + 12 )] )
    call MultiboardSetItemValueBJ( udg_mainmultiboard, 1, S2I(udg_mbp[GetConvertedPlayerId(GetOwningPlayer(GetTriggerUnit()))]), GetPlayerName(GetOwningPlayer(GetTriggerUnit())) )
    call RemoveUnit(GetTriggerUnit())
endif
set u=null
endfunction




//===========================================================================
private function Init takes nothing returns nothing
    local trigger t = CreateTrigger()
    call TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_UNIT_TRAIN_FINISH)
    call TriggerAddAction(t, function Actions)
    // register spells // remember to verify a spells AID in the conditions of those triggers
    call TriggerRegisterAnyUnitEventBJ(gg_trg_pandaren_flames, EVENT_UNIT_SPELL_EFFECT)
    call TriggerRegisterAnyUnitEventBJ(gg_trg_starfall, EVENT_UNIT_SPELL_CAST)
    call TriggerRegisterAnyUnitEventBJ(gg_trg_starfall_off, EVENT_UNIT_SPELL_ENDCAST)
    call TriggerRegisterAnyUnitEventBJ(gg_trg_consume_tree, EVENT_UNIT_SPELL_EFFECT)
    call TriggerRegisterAnyUnitEventBJ(gg_trg_teleport_tree, EVENT_UNIT_SPELL_EFFECT)
endfunction

endscope

Theres more room for improvements (like not using udg_ globals. Write your own library for stuff like Multiboards)

 

[Livirus]

Well... as Deaod stated.

instead of

if () then
     //...
else
     if () then
          //...
     endif
endif

do this:

if () then
     //...
elseif () then
     //...
endif

 

[SanKakU]

as far as i can tell your rewrite of my code messes up when trying to add events to the triggers for the spells. - maybe you just forgot to code it in there? anyway, when you get the random runner, won't you run into a problem with replacing the unit that you already removed? other than that, it seems to be pretty good code i suppose. but basically you ignored 3/4 of the units with special needs besides the obvious randomer units. that's what it looks like.
as for the BJ functions that i used...i didn't know how to use non BJ for multiboards and no one has explained it to me through tutorials or otherwise. i don't know how to write the field information for any of the multiboard actions in jass. that, or, as the other guy mentioned, these BJ aren't inefficient...it's not like he posted anything better and neither did you, heh heh.

what's the difference between using else if and elseif? obviously they look different, but, other than that? having one less endif? is that good?

is replacing the unit leaking? i was under the impression that it didn't. that's the only reason i can see why you would suggest using those local variables for that.

anyway i think maybe my inclusion of all of those else is good because once you hit an if, with my coding you hit all those functions and then you just go straight to the end of the trigger. but with your code you hit if again and again while trying to get to all the functions. or maybe i'm wrong...it could be the opposite.

oh and btw i got rid of the item variable and am just using the item id now.

 

[Deaod]

as far as i can tell your rewrite of my code messes up when trying to add events to the triggers for the spells.[...]

Then look at the Init function.

[...]won't you run into a problem with replacing the unit that you already removed?[...]

If i could still access a removed units values, i wouldnt need to use locals in the first place.

[...]it seems to be pretty good code i suppose.[...]

if it was good code, i wouldnt say theres room for improvement (like using 0-based arrays).

[...]with my coding you hit all those functions and then you just go straight to the end of the trigger.[...]

Thats what happens when you use elseif. It is the same, only that using elseif keeps indentation at a minimum and improves code readability.

 

[SanKakU]

your init function does not do the event add properly though. it makes the event read a unit has casted a spell when it's supposed to read this unit here has casted a spell that means the trigger fires every time a unit casts a spell instead of just everytime that unit casts a spell. that takes too much memory. that's why it's messed up.

If i could still access a removed units values, i wouldnt need to use locals in the first place.

ok i understand what you mean here, i guess what i'm asking is why is what you wrote better than just using the function that does it for you that i used?
i'm not saying i'm not going to change the code, i just want to make sure i'm changing it for the better is all...

the only thing that comes to mind is perhaps your method replaces the unit at the exact location, whereas mine might not, but then again who knows...after all it's a flying unit so i don't think it matters...if that's the reason.

 

[Deaod]

[...]that's why it's messed up.[...]

It's not messed up. It actually takes much less memory (a single event handle vs. a bunch of event handles, one for each unit) than your original way, and is a LOT easier.

The ReplaceUnitBJ function was made for a bunch of cases. The specific case where you just want to remove the old unit and create a new one is so simple i wouldnt use a BJ for that. Plus its inlined code, so i minimized overhead.

 

[Livirus]

...what's the difference between using else if and elseif?

It's more readable, I think that's all as far as I know.
Easier to read, better chance to find errors.

Ex. compare this:

function example takes integer a, integer b returns integer
     if (a < 0) then
          return a

     else
          if (b < 0) then
               return b

          else
               if (a < b) then
                    return a+b

               else
                    if ((a > 3) or (b > 10)) then
                         return a*b

                    endif
               endif
          endif
     endif
endfunction

...with this:

function example takes integer a, integer b returns integer
     if (a < 0) then
          return a

     elseif (a < b) then
          return b

     elseif (a > 3) then
          return a+b

     elseif ((a > 3) or (b > 10)) then
          return a*b

     endif
endfunction

 

[SanKakU]

It's not messed up. It actually takes much less memory (a single event handle vs. a bunch of event handles, one for each unit) than your original way, and is a LOT easier.

The ReplaceUnitBJ function was made for a bunch of cases. The specific case where you just want to remove the old unit and create a new one is so simple i wouldnt use a BJ for that. Plus its inlined code, so i minimized overhead.

edit: hmm it looks like perhaps the only thing that happens everytime the trigger is going off is that it's going off because of the first event, and then it does those actions...i was reading it wrong, nevermind. but i still think doing it this other way that i had in mind is better.

the memory might be a problem because any unit doing an ability will be checked by every trigger with this event.

as for the BJ function...i'm still uncertain about it. i know red functions are bad but i like using purple functions to replace them. i'm not convinced the bothersome implementation of variables is worth it.

To Livirus: I might start doing that. I guess it depends on whether that makes it easier for me to read it or not.