[JASS] Trigger with recursion not working

Status
Not open for further replies.
Level 2
Joined
Jan 2, 2013
Messages
24
This is the code for an ability that doesn't really work :D Explanation below the functions.

JASS:
scope AID


    globals    
        unit cast = null
        unit targ = null
        real castx
        real casty
        location loc = null
        group grp1
        group grp2
        real dmgcount = 0
        integer i = 0
    endglobals

    
    function Actions2 takes nothing returns nothing
        call ReleaseTimer(GetExpiredTimer())
        call displ("success(or not?)")
    endfunction
    
    
    function Timed1 takes nothing returns nothing
        local timer t = CreateTimer()
        call TimerStart(t, 0.5, false, function Actions2)
        set t = null
    endfunction
    

    // Pauses targets in range=1000 and saves their life + mana for final explosion, one by one every .4 seconds
    function Pick_Act1 takes nothing returns nothing
        //local timer t = CreateTimer()
        //call ReleaseTimer(GetExpiredTimer())
        set targ = GroupPickRandomUnit(grp1)
        if (targ != null) and (i< 3+GetUnitAbilityLevel(cast, 'AID2')) then
            set i = i+1
            set loc = Location(GetUnitX(targ),GetUnitY(targ))
            call CreateNUnitsAtLoc( 1 , 'h004', Player(PLAYER_NEUTRAL_PASSIVE), loc, 0) //special effect dummy
            call SetUnitTimeScalePercent( GetLastCreatedUnit(), 40.00 )
            call UnitApplyTimedLifeBJ( 1.00, 'BTLF', GetLastCreatedUnit() )
            call PauseUnitBJ(true, targ)
            set dmgcount = dmgcount + GetUnitState(targ, UNIT_STATE_LIFE) + GetUnitState(targ, UNIT_STATE_MANA)
            call RemoveLocation(loc)
            call GroupAddUnit(grp2,targ)
            call GroupRemoveUnit(grp1,targ)
            set targ = null
            call displ("pAct1")
            call Pick_Act1()
        else
            call displ("done")
            set targ = null
            call DestroyGroup(grp1)
            call Timed1()
        endif
        //call TimerStart(t, 0.4, false, function Pick_Act1)
        //set t = null
        
    endfunction

    
    //Initial function for this spell. Pauses caster and starts actions on targets
    function Actions1 takes nothing returns nothing
        set cast = GetTriggerUnit()
        set castx= GetUnitX(cast)
        set casty= GetUnitY(cast)
        set loc = Location(castx,casty)
        set grp1 = GetUnitsInRangeOfLocMatching(1000,loc, Filter(function Cond1))
        call RemoveLocation(loc)
        call SetUnitInvulnerable( cast, true )
        call Pick_Act1()
    endfunction

    
    //Simply checks if the spell is the dummy for this ability
    function Condition1 takes nothing returns boolean
        call displ("ok")
        return (GetSpellAbilityId()== 'AID2')
    endfunction


//===========================================================================
function InitTrig_All_is_Dust_2 takes nothing returns nothing
    set gg_trg_All_is_Dust_2 = CreateTrigger(  )
    call TriggerRegisterAnyUnitEventBJ( gg_trg_All_is_Dust_2, EVENT_PLAYER_UNIT_SPELL_CHANNEL )
    call TriggerAddCondition( gg_trg_All_is_Dust_2, Condition(function Condition1 ))
    call TriggerAddAction( gg_trg_All_is_Dust_2, function Actions1 )
endfunction

endscope


Using a dummy ability based on channel, units at random, filtered by the function Cond1 which is not written but has no problem, are supposed to be singularly picked up at 0.4sec intervals between one another. The number of units picked up is dependant on the ability level and should be at least 4. Each of the units picked is paused, a dummy unit for special effect is created on them and then.. the rest is not important, i still have to write it :D
Well it doesn't work, only one unit is picked up and then nothing.
Im guessing the problem(s) is in Pick_Act1, where the recursion is.. It is the first time I try it in jass so there might be something I don't know, or I might be simply stupid and have made an abomination :D All I know is that using the ability only prints "ok" (ability condition check). After 4 casts it prints "ok", "done" and 0.5sec after "success", but i guess that is just because apparently the index i gets increased with each cast. Which is strange, because it means that the part inside the if should have been all executed at least once, but this is not true because "pAct1" is never printed. I really don't get this..
Because the whole thing wasn't working, i took away the timers to make the pick ups directly subsequent, thinking that may be a recursion with delays inside might have been the cause, but that wasn't the problem. Only 1 unit at random is picked up, the special effect dummy is created on it, and then nothing..
What's wrong?
It is also the first time I use a scope, is there something wrong with recursion and scopes?
Thanks in advance!
 
Level 24
Joined
Aug 1, 2013
Messages
4,658
First of all...
you cannot release a timer when you created it yourself.
You need NewTimer() instead of CreateTimer().

Next to that, these recursions are not how they should be used.
In this case, you want a loop.

Also try to use FirstOfGroup() iteration.
And avoid all red functions (except the event one) in this trigger.
It helps.
 
Level 2
Joined
Jan 2, 2013
Messages
24
First of all, the problem was that I didn't initialize the groups is the globals using CreateGroup().. I changed it and now everything works as intended. I need recursion because (unless I'm stupid) I can't see how to make a loop with timers.. Here is the final form:

JASS:
function Pick_Act1 takes nothing returns nothing
        local timer t = CreateTimer()
        call ReleaseTimer(GetExpiredTimer())
        set targ = GroupPickRandomUnit(grp1)
        if (targ != null) and (i< ntarg) then
            set i = i+1
            set loc = Location(GetUnitX(targ),GetUnitY(targ))
            call CreateNUnitsAtLoc( 1 , 'h004', Player(PLAYER_NEUTRAL_PASSIVE), loc, 0) //special effect dummy
            call SetUnitTimeScalePercent( GetLastCreatedUnit(), 40.00 )
            call UnitApplyTimedLifeBJ( 1.00, 'BTLF', GetLastCreatedUnit() )
            call PauseUnitBJ(true, targ)
            call RemoveLocation(loc)
            call GroupAddUnit(grp2,targ)
            call GroupRemoveUnit(grp1,targ)
            set targ = null
            call TimerStart(t, gap, false, function Pick_Act1)
            set t = null
        else
            set i=0
            set targ = null
            call displ(I2S(CountUnitsInGroup(grp1)))
        endif

Which brings the questions:
1) how important is it to use timers insted of triggersleepaction?? Do you use timers EVERYTIME? It forces you to make A LOT of functions.
2) I'll do what you said about the timers but.. What's the difference between newtimer and createtimer? I forgot btw to say that I'm using timerutils, although it's probably obvious.
3) Is recursion generally a good thing to do in jass? Should I prefer loops for some reason?
4) Are there leaks in the script?
5) How does pauseunit work??
6) Why should I not use red functions? Are they slow? Sorry but I wanna learn! :D Also, while using the normal version of create unit, GetLastCreatedUnit doesn't seem to work! Am I doing something wrong?
EDIT: I got why GetLastCreatedUnit doesn't work. If I manually set bj_lastCreatedUnit = CreateUnit(...) to solve it, does it leak??

THAAANKS!
 
Last edited:
  1. Timers are used for precision, mostly. TriggerSleepAction() has the restriction that it can only be used in trigger actions/ExecuteFunc callbacks, it can't reliably wait under ~0.25 seconds, and timers perform waits in actual game-time. I'll sometimes use TriggerSleepAction() for long waits (e.g. waits greater than 1 second), but if it can be easily implemented with a timer--you may want to opt for the timer route. TriggerSleepAction has some unpredictability to it. For example--with cinematics, you'll almost always want to use timers. If you're trying to time animations in particular moments, then the slight imprecision of TriggerSleepAction could lead to animations being a little off in timing.
    .
  2. TimerUtils is a timer attachment and recycling system. NewTimer() and ReleaseTimer() work in tandem--NewTimer() will fetch a new timer from the system for you to use, and ReleaseTimer(t) will release a timer. The system expects that the timer you are releasing was created through NewTimer(). Technically it'll still work with CreateTimer(), but ReleaseTimer() just stores the timer for later use--but if you don't use NewTimer(), you're not taking advantage of those stored timers.
    .
  3. Recursion is slow in JASS. Go for loops and iterative methods when possible. I mean, it isn't going to be a huge difference--probably just nanoseconds of computation. But if you expect the number of recursive calls to be high (e.g. a sorting algorithm), then you should try to find an iterative solution.
    .
  4. . I don't see any clear leaks. But the script isn't MUI (Multiple-Unit Instance-able). That means that if two units were to channel the spell at the same time, it would experience a bug. Even though timers are incredibly useful, the main drawback is that we can't attach data to it. So over the years, many people have developed timer attachment systems to allow you to save data and load it later on. This tutorial is pretty good to give you a quick timer overview:
    http://www.thehelper.net/threads/how-to-use-timers-in-jass.145499/
    Some of the systems are a bit dated, but you really can't go wrong with using TimerUtils. TimerUtils kind of revolves around using structs, so learn that soon if you can. It is a bit difficult to understand at first, but it'll become a bit clearer as you use it more and more. They are similar to classes in programming languages (don't be fooled by the name).
    .
  5. Pause unit will prevent attacks, movement, abilities, etc. on a unit. The downside is that it will also pause buff timers, so it isn't really recommended. More info is here:
    http://www.hiveworkshop.com/forums/submissions-414/snippet-pauseunit-266681/
    .
  6. Red functions are usually just wrappers for GUI. They tend to just swap arguments to follow GUI's description. So yes, they are slower. Example:
    JASS:
    function SetUnitAbilityLevelSwapped takes integer abilcode, unit whichUnit, integer level returns integer
        return SetUnitAbilityLevel(whichUnit, abilcode, level)
    endfunction
    It is just a roundabout way of running a function. So we tend to inline them by using the native equivalent. native functions are defined in the common.j in wc3's data files, and they run the fastest. The "red" functions are known as BJ's (not that kind), since they are defined in the Blizzard.j file. Some BJ's are fine to use, such as ModuloInteger or RAbsBJ, or more commonly: TriggerRegisterAnyUnitEventBJ. If you are unsure whether to inline a BJ, just look it up in the Blizzard.j (see my signature)--or if you are using JassNewGenPack, then you can just look it up in the "Function List" within your trigger editor.

    As for the last created unit issue--most of the GetLastCreated... things are just there for GUI. In GUI, we'll usually assign things to variables like so:
    • Unit - Create 1 footman for Player 1 (Red) at ...
    • Set TempUnit = (Last created unit)
    But in JASS, we don't need to take that extra step. Shizam:
    JASS:
    set tempUnit = CreateUnit(Player(0), 'hfoo', 0, 0, 0)

If there is anything you need explained, let me know.
 
Level 2
Joined
Jan 2, 2013
Messages
24
Those were.. ridiculously good answers!
I guess i have one last question: if I use bj_lastCreatedUnit = CreateUnit(..) so that I don't have to make an extra local unit, will it leak if i don't null it, just like every other unit reference? Or is it "special"? I don't even know if nulling bj_lastCreatedUnit makes sense :D
Thanks a lot man
 
You only really need to null local variables. bj_lastCreatedUnit is a global, so you don't have to worry about it.

In case you're curious as to why, here is an explanation:

We have to manage our own memory in Warcraft 3, which is why we have functions like DestroyGroup(), RemoveLocation(), etc. Each object is associated with a particular ID (you can think of it as being similar to a memory address). The DestroyGroup(), RemoveLocation(), etc. functions will release the main chunk of that object's memory. However, the ID is released only when its local reference count is 0.

What is a "local reference count"? When you assign a local variable to point to an object, that count goes up by 1. When you assign the local to a different object or assign it to null, it goes down by 1. If you don't assign locals to null, then the objects' local reference counts will never reach 0, so that ID just stays used up forever. It isn't as major as forgetting to use DestroyGroup() or RemoveLocation(), but it is still a good habit to develop.

A few notes:
  • What about globals? Objects have a global reference count as well. But globals are global--they are reassigned all the time so it shouldn't matter. Also, the handle ID can still be released when the global reference count is > 0.
    .
  • What type of variables should you null? There are five primitive types that you do not need to null: integer, string, real, code, boolean. Those are stored differently in memory--they don't count references. Only types that extend agent will count references (you can see the common.j to see a list of all the types). There are quite a few handles that aren't agents--those don't need to be nulled. But it doesn't hurt to null things. I'll usually null any local variable of any type besides those primitives.
 
Level 24
Joined
Aug 1, 2013
Messages
4,658
Which brings the questions:
1) how important is it to use timers insted of triggersleepaction?? Do you use timers EVERYTIME? It forces you to make A LOT of functions.
2) I'll do what you said about the timers but.. What's the difference between newtimer and createtimer? I forgot btw to say that I'm using timerutils, although it's probably obvious.
3) Is recursion generally a good thing to do in jass? Should I prefer loops for some reason?
4) Are there leaks in the script?
5) How does pauseunit work??
6) Why should I not use red functions? Are they slow? Sorry but I wanna learn! :D Also, while using the normal version of create unit, GetLastCreatedUnit doesn't seem to work! Am I doing something wrong?
EDIT: I got why GetLastCreatedUnit doesn't work. If I manually set bj_lastCreatedUnit = CreateUnit(...) to solve it, does it leak??

THAAANKS!

1, very important
2, difference between using a library and not
3, yes
4, no
5, it doesnt
6, because they are slower and sometimes require data types that you dont want to use

Sorry, just have to keep my reputation: "Toxic as hell"

But to be a bit helpfull:
Your ReleaseTimer() or DestroyTimer() in the case of not using NewTimer(), should be placed in the else part of the if statement.
You remove timer "t" and you do not create a new one before you use it agian.
You should not remove it until it will not be used again.

On the other hand, you can do
call TimerStart(t, gap, true, function Pick_Act1)
inside Actions1() if you do not care about the first "gap" and then use that.
But lets save that for later on.

Also, "dmgcount" is counting total health and mana of all picked units... is that right?

One last thing...
I dont know how many units can cast this ability and in what frequency, but it is not MUI.
However, making it MUI requires a little change.
I wrote this small library that allows me to use the index intention of TimerUtils without structs.
JASS:
library timerIndex uses TimerUtils
//*********************************************************************
//* TimerIndex 1.0
//* ----------
//*
//*  This library creates a unique index for the TimerData of timerUtils.
//*  A timer that is created with an index must also be released as an indexed timer.
//*
    
    globals
        integer udg_NextTimerIndex = 0
        boolean array udg_TimerIndex_Occupied
    endglobals
    
    function ReleaseIndexedTimer takes timer t returns nothing
        set udg_TimerIndex_Occupied[GetTimerData(t)] = false
        call ReleaseTimer(t)
    endfunction
    function NewIndexedTimer takes nothing returns timer
        loop
            set udg_NextTimerIndex = udg_NextTimerIndex + 1
            if udg_NextTimerIndex > 8191 then
                set udg_NextTimerIndex = 1
            endif
            
            exitwhen not udg_TimerIndex_Occupied[udg_NextTimerIndex]
        endloop
        
        set udg_TimerIndex_Occupied[udg_NextTimerIndex] = true
        return NewTimerEx(udg_NextTimerIndex)
    endfunction
    
endlibrary
With this, you can store the data like caster, ability level, target group, damage value, targeted location, etc, etc, etc in arrays.
The first three are the ones that you pretty much want.

It is simply replacing NewTimer() with NewIndexedTimer() and ReleaseTimer() with ReleaseIndexedTimer().
Then you can use GetTimerData() (timerutils' standard) to get a unique index between 1 and 8191.
Then you make the values that can change for each individual cast an array global.
The rest is self explaining... also null those values when you remove the timer ;)
 
Level 2
Joined
Jan 2, 2013
Messages
24
Well, that was toxic enough :D
I really forgot about the boolean in the timer, making it periodic makes sense!
Thank you!

EDIT: Oh and yeah, only one unit will ever cast this so it doesn't need to be mui!
 
Status
Not open for further replies.
Top