HomeUser Control Panel (unavailable in archive)ForumsTutorialsArt GalleryResourcesMaps

First Jass Spell

10-30-2007, 08:59 AM#1
Chop_tomato
Below is my first trigger written in jass, if anyone has the time to quickly go through and point out any really bad stuff in there it would be greatly appreciated.

Hidden information:
Collapse JASS:
function Trig_Rebellion_Condtions takes nothing returns boolean
    return GetSpellAbilityId() == 'A000' // - checks the spell is the right one
endfunction

//=========================================================

function Trig_Rebellion_Actions takes nothing returns nothing
    local unit c = GetTriggerUnit() // - caster
    local unit x = GetSpellTargetUnit() // - target
    local player p = GetOwningPlayer(x) // - sets p to the owner of the target
    local integer d = 20 + (GetUnitAbilityLevel (c, 'A000') * 5) // - duration
    local unit r // - dummy caster
    local integer b = GetUnitAbilityLevel (x, 'B000') // - sets b to the level of the buff on the target unit
    local timer t = CreateTimer() // - creates the timer
    local real e // - elapsed time of timer
    
    if b == 0 then // - checks the unit is not already affected by the spell
    set r = CreateUnitAtLoc (GetOwningPlayer(r), 'H003', GetUnitLoc (x), 1) // - creates the dummy caster next to the target
    call UnitApplyTimedLife(r, 'B001', 5)
    call SetUnitVertexColor (r, 0, 0, 0, 0) // - makes the dummy caster more invisible
    call IssueTargetOrder (r, "charm", x) // - orders the dummy caster to cast charm
    call TimerStart(t, d, false, null) // - starts the timer for the duration of the "rebellion"
    loop // - loops until the timer has elapsed
        exitwhen e == d
        call TriggerSleepAction(0.1) 
        set e = TimerGetElapsed(t)
        endloop
    set r = CreateUnitAtLoc (GetOwningPlayer(r), 'H003', GetUnitLoc (x), 1) // - same triggers as above,                    // - puts the unit back under control
    call SetUnitVertexColor (r, 0, 0, 0, 0)                               // - puts the unit back under control
    call IssueTargetOrder (r, "charm", x)                                   // - of its original owner
    call UnitApplyTimedLife(r, 'B001', 5)            
    endif
    call PauseTimer(t)
    call DestroyTimer(t)
    set c = null
    set x = null
    set p = null
    set r = null
    set t = null
endfunction

//=========================================================

function InitTrig_Rebellion takes nothing returns nothing
    set gg_trg_Rebellion = CreateTrigger()
    call TriggerRegisterAnyUnitEventBJ( gg_trg_Rebellion, EVENT_PLAYER_UNIT_SPELL_EFFECT )
    call TriggerAddCondition( gg_trg_Rebellion, Condition( function Trig_Rebellion_Conditions ) )
    call TriggerAddAction( gg_trg_Rebellion, function Trig_Rebellion_Actions )
endfunction


Here are the errors I got from the JassCraft syntax checker.

Hidden information:

Line 9: Undeclared variable: gg_trg_Rebellion
Line 10: Undefined function.
Line 11: Undefined function.
Line 20: Undeclared variable: A000
Line 22: Undeclared variable: B000
Line 27: Undeclared variable: H003
Line 30: Undeclared variable: charm
Line 34: Single = in expression, should probably be == <- this one is fixed but still showing error
Line 34: Syntax error
Cbf'd updating the errors, same ones except now has syntax errors on all the leak removing lines


I think there's still some memory leaks in there, not sure how to fix them properly
10-30-2007, 12:10 PM#2
yang
Let me see!!!
10-30-2007, 01:10 PM#3
Fireeye
Collapse JASS:
ffunction Rebellion_Condtions takes nothing returns boolean
    return GetSpellAbilityId() == 'A000' // - checks the spell is the right one
endfunction
you made a typo, remove 1 f in ffunction and this error is fixed.
gg_trg_Rebellion is a global variable created by the world editor, to fix this problem in JASSCraft add this block:
Collapse JASS:
globals
trigger gg_trg_Rebellion = null
endglobals
About those A000,B000,H003 things, you forgot to add ' at the end and beginning, should be: 'A000','B000','H003'
The charm error appears because you forgot to add ", should be "charm"
The next error are actually 2.
Collapse JASS:
    loop // - loops until the timer has elapsed
        exitwhen e = d 
        set e == TimerGetElapsed(t)
    endloop
should be
Collapse JASS:
    loop // - loops until the timer has elapsed
        exitwhen e == d 
        set e = TimerGetElapsed(t)
    endloop
Don't really know what you want to do with this part...
However i hope there won't be any error anymore, were too lazy to take a look on the entire script.
---Edit---
Also move this part
Collapse JASS:

function InitRebellion takes nothing returns nothing
    set gg_trg_Rebellion = CreateTrigger()
    call TriggerRegisterAnyUnitEventBJ( gg_trg_Rebellion, EVENT_PLAYER_UNIT_SPELL_EFFECT )
    call TriggerAddCondition( gg_trg_Rebellion, Condition( function Trig_Rebellion_Conditions ) )
    call TriggerAddAction( gg_trg_Rebellion, function Trig_Rebellion_Actions )
endfunction
to the buttom of the entire script.
10-30-2007, 01:34 PM#4
Silvenon
ffunction Rebellion_Condtions takes nothing returns boolean

Should be:

function Rebellion_Condtions takes nothing returns boolean

This is bad:

Collapse JASS:
function InitRebellion takes nothing returns nothing
    set gg_trg_Rebellion = CreateTrigger()
    call TriggerRegisterAnyUnitEventBJ( gg_trg_Rebellion, EVENT_PLAYER_UNIT_SPELL_EFFECT )
    call TriggerAddCondition( gg_trg_Rebellion, Condition( function Trig_Rebellion_Conditions ) )
    call TriggerAddAction( gg_trg_Rebellion, function Trig_Rebellion_Actions )
endfunction

This function should be at the bottom, at least below the action/condition functions used in the trigger.

This kinda sucks:

Collapse JASS:
call CreateUnitAtLoc (GetOwningPlayer(r), H003, GetUnitLoc (x), 1) // - creates the dummy caster next to the target
set r = GetLastCreatedUnit() // - sets r to the dummy caster

Because you used CreateUnitAtLoc which doesn't store the created unit in bj_lastCreatedUnit, so using GetLastCreatedUnit() won't retrieve it. You have two options:
  1. Instead of CreateUnitAtLoc, you can use CreateUnitAtLocSaveLast, then you can use GetLastCreatedUnit()
  2. You can do this instead:
set r = CreateUnitAtLoc (GetOwningPlayer(r), H003, GetUnitLoc (x), 1)

I suggest the second option, it's faster anyways. Also, H003 must be an integer, so wrap it in '', like this:

call CreateUnitAtLoc (GetOwningPlayer(r), @'H003'@, GetUnitLoc (x), 1)

Same mistake here:

call UnitApplyTimedLife(r, B001, 5)

Should be:

call UnitApplyTimedLife(r, 'B001', 5)

Again (this should go at the top, but meh):

local integer d = 20 + (GetUnitAbilityLevel (c, A000) * 5)

Should be:

local integer d = 20 + (GetUnitAbilityLevel (c, 'A000') * 5)

Again:

local integer b = GetUnitAbilityLevel (x, B000)

Should be:

local integer b = GetUnitAbilityLevel (x, 'B000')

Almost the same mistake here:

call IssueTargetOrder (r, charm, x)

In this case you must wrap charm in "", so:

call IssueTargetOrder (r, "charm", x)

This loop is a wtf:

Collapse JASS:
loop
    exitwhen e = d
    set e == TimerGetElapsed(t)
endloop

This will crash the map for sure because the loop is executing an infinite number of times in a single moment, and the exitwhen you're using requires some time interval, so this:

Collapse JASS:
loop
    exitwhen e = d
    call TriggerSleepAction(0.1)
    set e == TimerGetElapsed(t)
endloop

Won't crash the map. It's checking every 0.1 seconds if the timer has elapsed.

Also, this:

call Destroy

Doesn't do anything. What the hell is it supposed to be??? :)

I thought I've seen them all, but I guess there's always something I haven't seen:

Collapse JASS:
set unit c = null
set unit x = null
set player p = null
set unit r = null
set timer t = null

First of all: Hahahaha!! Sorry :P

This is correct:

set <variable name> = <value>

This is a wtf (meaning wrong):

set <variable type> <variable name> = <value>

It doesn't work that way, so that part should look like this:

Collapse JASS:
set c = null
set x = null
set p = null
set r = null
set t = null

EDIT: How I hate when somebody finishes before me, but mine is better anyways :P

Btw, Fireeye, you didn't change anything in that loop part, you just posted two identical codes.
10-30-2007, 01:48 PM#5
Fireeye
i did, the condition had only 1 = and the set had 2 =.
Mine got 2 = in the condition and 1 in the set, which is still wrong in your loop.
10-30-2007, 08:26 PM#6
Silvenon
Oh, I didn't see :), a single "=" is hard to notice. Sorry, my bad.
10-30-2007, 10:26 PM#7
Chop_tomato
thanks for all the help guys, the call destroy at the end was just something i forgot to get rid of after (hopefully)fixing leaks. Ill get working on the rest now

ok, updated with all the fixes, turns out the ffunction was just from c&p
10-31-2007, 03:50 AM#8
Pyrogasm
None of you noticed that he put "InitRebellion" instead of "InitTrig_Rebellion". That's a pretty big error right there.
10-31-2007, 06:20 AM#9
Chop_tomato
Quote:
Originally Posted by Pyrogasm
None of you noticed that he put "InitRebellion" instead of "InitTrig_Rebellion". That's a pretty big error right there.

I read a tutorial that said you dont need the Trig_ part in the function names (cant remember where), so i got rid of it because i think it looks bad, but ill put it back in so i dont have to worry about it. Maybe it meant in functions that get called from other functions, not functions that have init, conditions, and actions... /confused

updated again
10-31-2007, 09:15 AM#10
Av3n
Shouldn't you hide the unit, not change the vertex colouring?

call ShowUnit(r,false)
-Av3n
10-31-2007, 02:14 PM#11
Silvenon
Quote:
Maybe it meant in functions that get called from other functions, not functions that have init, conditions, and actions... /confused

Yes, it meant about the functions that are used from InitTrig_ function

Quote:
None of you noticed that he put "InitRebellion" instead of "InitTrig_Rebellion". That's a pretty big error right there.

And that too :), but it still can't compete with set unit u = null
10-31-2007, 07:34 PM#12
Pyrogasm
InitTrig functions are essential. They are executed when the map initializes and it sets up the triggers. With the NewGen pack you can remove the need for InitTrigs, but then they won't be executed at init.

So what you've done is replace the InitTrig_Rebellion function with one called "InitRebellion" that will never be called and thus never set up the spell's trigger.
11-01-2007, 10:28 AM#13
Silvenon
I was kidding with that "it's not so important" tone, of course it's essential.
11-01-2007, 10:58 PM#14
Pyrogasm
I was posting in response to Chop_tomato, not you Silvenon. :D
11-02-2007, 09:14 AM#15
Silvenon
I thought you were referring to both of us. My bad