Help me clean this spell up

05-19-2009, 06:07 AM

My first attempt at converting my gui spell to jass.

I tried my best to clean it up.

But i am sure its still far from clean, still looks very messy to me.

So would anyone help me clean it up. And explain it to me if its advanced stuffs.

Quote:

The spells creates 6 tornados in the area of the point of the ability casted.

The middle tornado is created outside of the loop, then it is given a dummy monsoon ability and it is ordered to cast it.

5 tornados are created from a loop, using integers to determine the location whereby the tornados are created.

All tornados expires after 15 seconds.

JASS:

scope ThunderArea initializer Init

private function Conditions takes nothing returns boolean
    if ( not ( GetSpellAbilityId() == 'A003' ) ) then
        return false
    endif
    return true
endfunction

private function Actions takes nothing returns nothing
    local location loc = GetSpellTargetLoc()
    local integer int = -72
   
    call CreateNUnitsAtLoc( 1, 'n001', GetOwningPlayer(GetSpellAbilityUnit()), loc, bj_UNIT_FACING )
    call UnitAddAbilityBJ( 'A005', GetLastCreatedUnit() )
    call SetUnitAbilityLevelSwapped( 'A005', GetLastCreatedUnit(), GetUnitAbilityLevelSwapped('A003', GetSpellAbilityUnit()) )
    call UnitAddAbilityBJ( 'A006', GetLastCreatedUnit() )
    call SetUnitAbilityLevelSwapped( 'A006', GetLastCreatedUnit(), GetUnitAbilityLevelSwapped('A003', GetSpellAbilityUnit()) )
    call IssuePointOrderLocBJ( GetLastCreatedUnit(), "monsoon", loc )
    call UnitApplyTimedLifeBJ( 15.00, 'BTLF', GetLastCreatedUnit() )
    call CreateNUnitsAtLoc( 1, 'n002', GetOwningPlayer(GetSpellAbilityUnit()), loc, bj_UNIT_FACING )
    call UnitApplyTimedLifeBJ( 15.00, 'BTLF', GetLastCreatedUnit() )
   
    loop
        exitwhen int >= 360
        set int = ( int + 72 )
        set loc = PolarProjectionBJ(GetSpellTargetLoc(), 400.00, I2R(int))
        call CreateNUnitsAtLoc( 1, 'n001', GetOwningPlayer(GetSpellAbilityUnit()), loc, bj_UNIT_FACING )
        call UnitApplyTimedLifeBJ( 15.00, 'BTLF', GetLastCreatedUnit() )
    endloop
    
    call PlaySoundAtPointBJ( gg_snd_RollingThunder1, 100, loc, 0 )
    call PlaySoundAtPointBJ( gg_snd_LightningBolt1, 100, loc, 0 )
    
    call RemoveLocation(loc)
    
endfunction

//===========================================================================
private function Init takes nothing returns nothing
    local trigger ThunderArea = CreateTrigger(  )
    call TriggerRegisterAnyUnitEventBJ( ThunderArea, EVENT_PLAYER_UNIT_SPELL_EFFECT )
    call TriggerAddCondition( ThunderArea, Condition( function Conditions ) )
    call TriggerAddAction( ThunderArea, function Actions )
endfunction

endscope

05-19-2009, 11:04 PM

Silvenon

Well, there's lot to improve here. For example, the construction of the condition:

JASS:

private function Conditions takes nothing returns boolean
    if ( not ( GetSpellAbilityId() == 'A003' ) ) then
        return false
    endif
    return true
endfunction

This can be done like this:

JASS:

private function Conditions takes nothing returns boolean
    return GetSpellAbilityId() == 'A003'
endfunction

Which is much simpler, faster and cleaner.

Also, I suggest you start using coordinates, they are faster:

JASS:

local location loc = GetSpellTargetLoc()
local real x = GetLocationX(loc)
local real y = GetLocationY(loc)

Next, the function CreateNUnitsAtLoc is horrible, just look at it:

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

And uses CreateUnitAtLocSaveLast:

JASS:

function CreateUnitAtLocSaveLast takes player id, integer unitid, location loc, real face returns unit
    if (unitid == 'ugol') then
        set bj_lastCreatedUnit = CreateBlightedGoldmine(id, GetLocationX(loc), GetLocationY(loc), face)
    else
        set bj_lastCreatedUnit = CreateUnitAtLoc(id, unitid, loc, face)
    endif

    return bj_lastCreatedUnit
endfunction

It's even worse when you're creating only one unit. Notice it uses the CreateUnitAtLoc (generally), but in this case we will use CreateUnit because it uses coordinates.

Also, UnitAddAbilityBJ is a BJ (duh), meaning it calls another function, which is pointless when you can just call the second function in the first place (and that is UnitAddAbility).

I suggest using GetTriggerUnit() instead of GetSpellAbilityUnit(), because it's faster (and shorter to type :P).

SetUnitAbilityLevelSwapped is a BJ too, it calls another function that is practically the same, but has swapped parameters (hence the suffix). You can use the SetUnitAbilityLevel function instead.

IssuePointOrderLocBJ is a BJ....blabla....but we're gonna use the version that takes coordinates as parameters, not the location.

UnitApplyTimedLifeBJ is also a BJ.....blabla...we're gonna use UnitApplyTimedLife function...blabla.

Now, you can put constant values (like rawcodes) in constant functions, the look and behave almost the same as a normal function, but they are slightly faster and can return only a constant value (meaning they can't call other functions).

JASS:

constant function GetUnitId1 takes nothing returns integer
    return 'n001'
endfunction

constant function GetUnitId2 takes nothing returns integer
    return 'n002'
endfunction

constant function GetAbilityId1 takes nothing returns integer
    return 'A005'
endfunction

constant function GetAbilityId2 takes nothing returns integer
    return 'A005'
endfunction

constant function GetAbilityId3 takes nothing returns integer
    return 'A006'
endfunction

There, we set the constant values. Btw, we can declare a player variable also, which will store the owning player of the caster. That way you don't have to call it each time.

We can do that with the level too.

This is how it should look when all this is changed:

JASS:

    local location loc = GetSpellTargetLoc()
    local real x = GetLocationX(loc)
    local real y = GetLocationY(loc)
    local unit c = GetTriggerUnit()
    local unit u
    local player p = GetOwningPlayer(c)
    local integer lev = GetUnitAbilityLevel(GetAbilityId1(), c)
    local integer int = -72
    set u = CreateUnit(p, GetUnitId1(), x, y, 0)
    call UnitAddAbility(u, GetAbilityId2())
    call SetUnitAbilityLevel(u, GetAbilityId2(), lev)
    call UnitAddAbility(u, GetAbilityId3())
    call SetUnitAbilityLevel(u, GetAbilityId3(), lev)
    call IssuePointOrder(u, "monsoon", x, y)
    call UnitApplyTimedLife(u, 'BTLF', 15)
    
    set u = CreateUnit(p, GetUnitId2(), x, y, 0)
    call UnitApplyTimedLife(u, 'BTLF', 15)

If you're wondering why is there a unit u variable, it will be used for the loop later. Btw, we stored the caster in a variable also, to prevent calling the GetTriggerUnit() function multiple times.

Now lets move on. PolarProjectionBJ looks like this:

JASS:

function PolarProjectionBJ takes location source, real dist, real angle returns location
    local real x = GetLocationX(source) + dist * Cos(angle * bj_DEGTORAD)
    local real y = GetLocationY(source) + dist * Sin(angle * bj_DEGTORAD)
    return Location(x, y)
endfunction

It uses coordinated. Just what we need. We can rewrite it like this:

JASS:

set x = x + 400 * Cos(I2R(int) * bj_DEGTORAD)
set y = y + 400 * Sin(I2R(int) * bj_DEGTORAD)

Anyways, we can change the loop part like this:

JASS:

    loop
        exitwhen int >= 360
        set int = ( int + 72 )
        set x = x + 400 * Cos(I2R(int) * bj_DEGTORAD)
        set y = y + 400 * Sin(I2R(int) * bj_DEGTORAD)
        set u = CreateUnit(p, GetUnitId1(), x, y, 0)
        call UnitApplyTimedLife(u, 'BTLF', 15)
    endloop

And in the end we need to clean leaks. Note that there are two things you need to keep an eye on, destroying what needs to be destroyed and nulling the handles (handle is every variable type except integer, real, boolean, string (and code, but that's not important that much)). Not nulling causes really minor leaks, but you should avoid it anyways, because it will eventually cause you problems. This is what you should do:

JASS:

    call RemoveLocation(loc)
    
    set loc = null
    set c = null
    set u = null
    set p = null

And that's that, pretty much.

I probably made some mistakes, I was typing fast.

P.S. It's best to leave PlaySoundAtPointBJ, because it has a lot of functions called in it, so it's better to leave this way if we don't want extra 10 lines of code.

05-20-2009, 07:04 AM

Tyrande_ma3x

JASS:

constant function GetUnitId1 takes nothing returns integer
    return 'n001'
endfunction

constant function GetUnitId2 takes nothing returns integer
    return 'n002'
endfunction

constant function GetAbilityId1 takes nothing returns integer
    return 'A005'
endfunction

constant function GetAbilityId2 takes nothing returns integer
    return 'A005'
endfunction

constant function GetAbilityId3 takes nothing returns integer
    return 'A006'
endfunction

This is totally useless and although it gets inlined by jasshelper, it should be in a global block since it's much better to read and configure.

JASS:

// Goes after scope.
globals
// You can remove the private keyword if you want to access these things from another script.
    private constant integer GetUnitId1 = 'n001'
    private constant integer GetUnitId2 = 'n002'
    private constant integer GetAbilityId1 = 'A005'
    private constant integer GetAbilityId2 = 'A005' // ? 
    private constant integer GetAbilityId3 = 'A006'
endglobals

Of course, when using globals you will remove those ( ), so instead of

JASS:

call SetUnitAbilityLevel(u, GetAbilityId2(), lev)

it is going to be

JASS:

call SetUnitAbilityLevel(u, GetAbilityId2, lev)

The other stuff was already explained. :)

05-20-2009, 11:57 AM

0zyx0

JASS:

    loop
        exitwhen int >= 360
        set int = ( int + 72 )
        set x = x + 400 * Cos(I2R(int) * bj_DEGTORAD)
        set y = y + 400 * Sin(I2R(int) * bj_DEGTORAD)
        set u = CreateUnit(p, GetUnitId1(), x, y, 0)
        call UnitApplyTimedLife(u, 'BTLF', 15)
    endloop

This wouldn't create units in a circle, because the projection is applied from the last location. This would be a better option:

JASS:

    loop
        exitwhen angle >= 6.
        call UnitApplyTimedLife(CreateUnit(p, GetUnitId1, x + 400 * Cos(angle), y + 400 * Sin(angle), 0), 'BTLF', 15)
        set angle = angle + 1.256636
    endloop

Which uses radians directly. The variable angle should be a real variable with the initial value 0. The reason it is compared to 6 is because real variables can sometimes be inaccurate, so if it was 2*PI, it could create a unit too much.

05-21-2009, 11:25 AM	#5
Silvenon	If you're gonna use global blocks like Tyrande says, you will need Jass NewGen Pack, you can find it on this site. I suggest you move to vJass as soon as possible, because it's easier. Whoops, you're right, 0zyx0. My bad.