JASS Help

12-20-2006, 11:43 PM

First of all, thanks for anyone who can give advice to this. My JASS trigger is a spell, and is suppose to summon 5 shades. It targets 5 random units on the map, and then does a spell to them, which works. However, what doesn't work is that some shades don't move at all. I'm thinking that because the random units can be dead, they don't do anything. However, I'm having trouble adding that condition.

JASS:

function Trig_Destruction_Conditions takes nothing returns boolean
    if ( not ( GetSpellAbilityId() == 'A000' ) ) then
        return false
    endif
    return true
endfunction

function Trig_Destruction_Func takes nothing returns boolean
    return ( GetUnitRace(GetFilterUnit()) == RACE_HUMAN )  then
endfunction

function Trig_Destruction_Actions takes nothing returns nothing
    local unit array target
    local unit array shade
    local location target_point = GetSpellTargetLoc()

    set target[1] = GroupPickRandomUnit(GetUnitsInRectMatching(GetPlayableMapRect(), Condition(function Trig_Destruction_Func)))
    set target[2] = GroupPickRandomUnit(GetUnitsInRectMatching(GetPlayableMapRect(), Condition(function Trig_Destruction_Func)))
    set target[3] = GroupPickRandomUnit(GetUnitsInRectMatching(GetPlayableMapRect(), Condition(function Trig_Destruction_Func)))
    set target[4] = GroupPickRandomUnit(GetUnitsInRectMatching(GetPlayableMapRect(), Condition(function Trig_Destruction_Func)))
    set target[5] = GroupPickRandomUnit(GetUnitsInRectMatching(GetPlayableMapRect(), Condition(function Trig_Destruction_Func)))


    call CreateNUnitsAtLoc( 1, 'u002', Player(8), target_point, bj_UNIT_FACING )
    set shade[1] = GetLastCreatedUnit()

    call CreateNUnitsAtLoc( 1, 'u002', Player(8), target_point, bj_UNIT_FACING )
    set shade[2] = GetLastCreatedUnit()

    call CreateNUnitsAtLoc( 1, 'u002', Player(8), target_point, bj_UNIT_FACING )
    set shade[3] = GetLastCreatedUnit()

    call CreateNUnitsAtLoc( 1, 'u002', Player(8), target_point, bj_UNIT_FACING )
    set shade[4] = GetLastCreatedUnit()

    call CreateNUnitsAtLoc( 1, 'u002', Player(8), target_point, bj_UNIT_FACING )
    set shade[5] = GetLastCreatedUnit()

    call IssueTargetOrderBJ( shade[1], "thunderbolt", target[1] )
    call IssueTargetOrderBJ( shade[2], "thunderbolt", target[2] )
    call IssueTargetOrderBJ( shade[3], "thunderbolt", target[3] )
    call IssueTargetOrderBJ( shade[4], "thunderbolt", target[4] )
    call IssueTargetOrderBJ( shade[5], "thunderbolt", target[5] )

    call TriggerSleepAction(0.5) //sometimes, it didn't spawn because I think it was too close to the removing location part

    call RemoveLocation (target_point)
    set target_point = null

    set shade[1] = null
    set shade[2] = null
    set shade[3] = null
    set shade[4] = null
    set shade[5] = null

    set target[1] = null
    set target[2] = null
    set target[3] = null
    set target[4] = null
    set target[5] = null
endfunction

//===========================================================================
function InitTrig_Destruction takes nothing returns nothing
    set gg_trg_Destruction = CreateTrigger(  )
    call TriggerRegisterAnyUnitEventBJ( gg_trg_Destruction, EVENT_PLAYER_UNIT_SPELL_EFFECT )
    call TriggerAddCondition( gg_trg_Destruction, Condition( function Trig_Destruction_Conditions ) )
    call TriggerAddAction( gg_trg_Destruction, function Trig_Destruction_Actions )
endfunction

For some reason, today the code isn't even working. I try to add and ( IsUnitAliveBJ(GetFilterUnit()) == true ) to the condition part but that doesn't work either. The error I get says "expected ')'", so I try to add those, remove those, etc. But I have no success...so please help! Thanks in advance.

Also, I'm a noob in JASS...so don't be too harsh if I'm making a very obvious mistake, thanks again.

P.S. I know that the target can be the same unit, the spell is suppose to allow that.

12-21-2006, 02:05 AM

PurplePoot

The reason it isnt compiling is

JASS:

function Trig_Destruction_Func takes nothing returns boolean
    return ( GetUnitRace(GetFilterUnit()) == RACE_HUMAN )  then
endfunction

The "then" there is useless, infact, its syntactically not allowed.

replace the above with

JASS:

function Trig_Destruction_Func takes nothing returns boolean
    return GetUnitRace(GetFilterUnit()) == RACE_HUMAN
endfunction

(also removed useless brackets)

Allright, let's do a patchup!

Hmm, first, the condition you wrote... when you first look at it, it might make sense (as this is the way blizzard does it :P), but think about it, and it doesnt. Why do you need a boolean to check if you should return a boolean?

by this logic, we can say that

JASS:

function Trig_Destruction_Conditions takes nothing returns boolean
    if ( not ( GetSpellAbilityId() == 'A000' ) ) then
        return false
    endif
    return true
endfunction

is equal to

JASS:

function Trig_Destruction_Conditions takes nothing returns boolean
    return GetSpellAbilityId() == 'A000'
endfunction

,though the second is faster and runs directly, rather than using a useless if-statement

Next, if you open up JASSCraft (assuming you have it), and look at CreateNUnitsAtLoc(), then you get this

JASS:

function CreateNUnitsAtLoc takes integer count, integer unitId, player whichPlayer, location loc, real face returns group
    call GroupClear(bj_lastCreatedGroup)
    loop
        set count = count - 1
        exitwhen count < 0
        call CreateUnitAtLocSaveLast(whichPlayer, unitId, loc, face)
        call GroupAddUnit(bj_lastCreatedGroup, bj_lastCreatedUnit)
    endloop
    return bj_lastCreatedGroup
endfunction

Which shows that everything that they're doing in this function is specifically do make sure you can get +1 units.

Now, we can call this kind of function a 'BJ' function, along with the many other functions in your script, some which are LITERALLY BJ functions, as they have a BJ suffix.

All of these functions basically just call other functions, sometimes setting a global used for GUI (like Last Created Unit) in the process

Now, thinking about it, calling extra functions is just wasting space.

So, finding every BJ we have (cept TriggerRegisterAnyUnitEventBJ, that one's actually a good one), find their 'native' equivelant, such as native CreateUnit takes player id, integer unitid, real x, real y, real face returns unit, we should replace those with the native equivelants (reordering the parameters as necessary as well), and come out with a much cleaner, more efficient result

example:

JASS:

call CreateNUnitsAtLoc( 1, 'u002', Player(8), target_point, bj_UNIT_FACING )
    set shade[1] = GetLastCreatedUnit()

becomes

JASS:

set shade[1] = CreateUnit( Player( 8 ), 'u002', GetLocationX( target_point ), GetLocationY( target_point ), 0 )

and so on. The same can be changed for all the other BJs as well, such as IssueTargetOrderBJ

Next, the issue of using arrays. Arrays are clunky, since wc3 automatically defines them at 8192 slots. Thus, avoid them in cases, like this, when they are unneccessary.

In this case, we can say

JASS:

local unit target
local unit shade

set target = [...]
set shade = CreateUnit([...])
call IssueTargetOrder( shade, "thunderbolt", target )

should work just fine, repeated 5 times, and nulled at the end

Next, there is no reason not to use loops. Writing out code 15 times can be agonizing, long, and useless, and when there's a way around it that's efficient, then there's no reason not to use it

so, instead of repeating the above lines 5 times, we can say

JASS:

local unit target
local unit shade
local integer counter = 0

loop
     exitwhen counter > 4
     //create unit, etc, issue orders, etc, here
     set counter = counter + 1
endloop

Which works just as well, and is faster to write

Next, leaks

You leak 5 groups, I hope you realize =/

so, to fix this, we can say that since each group is exactly the same, we can make a single variable for them all and destroy it at the end

JASS:

local group g = GetUnitsInRectMatching(bj_mapInitialPlayableArea, Condition(function Trig_Destruction_Func))

//loop opens, etc
set target = GroupPickRandomUnit( g )
//and then, if you dont want the same target to be able to be hit twice, add the following line
call GroupRemoveUnit( g, target )
//shade is created, stormbolts target
//loop ends
//clean up leak here
call DestroyGroup( g )
set g = null

Well, i think that sums it up!

EDIT: gotta go for now, so I'll post the complete script (with the makeover) tomorrow

12-21-2006, 02:34 AM	#3
PipeDream	GroupPickRandomUnit() has to loop through the entire group. You could do this much more efficiently by placing all the units in an array and picking 5 random integers. If you want to sample without replacement, which I suspect you do, if you don't want it to hit the same unit more than twice, then you need a shuffle.

12-22-2006, 12:14 AM	#4
Anopob	Wow thanks. I knew I added something to make it wrong. I'll edit or something if I find that it works. Thanks again! EDIT: I don't understand why array variables are bad still, though.

12-22-2006, 02:11 PM

PurplePoot

Well,
A) there's just no reason to use an array when you dont need them
B) An array is sort of like 8192 variables in one line. For locals it doesnt matter so much, but using tons of global arrays is a death wish
C) Whoops, missed that GetUnitsInRectMatching. time to fix that

Any (or at least almost any) GetUnits[...] call tends to be a BJ call, so we can replace this with

JASS:

local group g = CreateGroup()
local boolexpr b = Filter( function Trig_Destruction_Func )
call GroupEnumUnitsInRect( g, bj_mapInitialPlayableArea, b )
call DestroyBoolExpr( b )
set b = null

and finally, here's the finished product (still uses GroupPickRandomUnit

)

JASS:

function Trig_Destruction_Conditions takes nothing returns boolean
    return GetSpellAbilityId() == 'A000'
endfunction

function Trig_Destruction_Func takes nothing returns boolean
    return GetUnitRace(GetFilterUnit()) == RACE_HUMAN
endfunction

function Trig_Destruction_Actions takes nothing returns nothing
    local unit target
    local unit shade
    local location target_point = GetSpellTargetLoc()
    local integer counter = 0
    local group g = CreateGroup()
    local boolexpr b = Filter( function Trig_Destruction_Func )
    call GroupEnumUnitsInRect( g, bj_mapInitialPlayableArea, b )
    call DestroyBoolExpr( b )
    set b = null

    loop
        exitwhen counter > 4
        set target = GroupPickRandomUnit( g )
        set shade = CreateUnit( Player( 8 ), 'u002', GetLocationX( target_point ), GetLocationY( target_point ), 0 )
        call IssueTargetOrder( shade, "thunderbolt", target )
        call GroupRemoveUnit( g, target )
        set counter = counter + 1
    endloop
    call RemoveLocation (target_point)
    set target_point = null
    set shade = null
    set target = null
    call DestroyGroup( g )
    set g = null
endfunction

//===========================================================================
function InitTrig_Destruction takes nothing returns nothing
    set gg_trg_Destruction = CreateTrigger(  )
    call TriggerRegisterAnyUnitEventBJ( gg_trg_Destruction, EVENT_PLAYER_UNIT_SPELL_EFFECT )
    call TriggerAddCondition( gg_trg_Destruction, Condition( function Trig_Destruction_Conditions ) )
    call TriggerAddAction( gg_trg_Destruction, function Trig_Destruction_Actions )
endfunction

12-22-2006, 08:06 PM	#6
Anopob	Thanks, will try it now. What's a local "boolexpr"?

12-22-2006, 08:23 PM	#7
Captain Griffen	An array takes up roughly 32KBytes. A hundred will take up 3.2MBytes. In other words, unless you start using thousands, it won't matter.

12-22-2006, 08:41 PM	#8
Anopob	Hm, when I test it sometimes all the shades do their job, while others don't :( EDIT: Oh okay, thanks for the response.

12-23-2006, 05:38 PM	#9
PurplePoot	boolexpr = boolean expression Basically, a filterfunc or a conditionfunc Filter( function x ) and Condition( function x ) return filterfunc and conditionfunc, respectively

12-23-2006, 06:46 PM	#10
Anopob	So why sometimes the shades don't move? Should I add a condition checking if "target" == alive?