[JASS] Make this bettah

[Megafyr]

I've been starting to understand more and more jass lately and so I wanted to make my own function. : < How's the performance? Any ways to make it better?

This function is for arrowkey movement.

unction MoveUnit takes integer i returns nothing
    local integer j = 0
    set udg_move_speed[i] = 0
    set udg_move_direction[i] = GetUnitFacing(udg_Player[i])
    if (udg_detect_up[i] == 1) then
        set udg_move_speed[i] = 4
        if (udg_detect_left[i] == 1) then
            set udg_move_direction[i] = udg_move_direction[i] + 8
        elseif (udg_detect_right[i] == 1) then
            set udg_move_direction[i] = udg_move_direction[i] - 8
        endif
    elseif (udg_detect_down[i] == 1) then
        set udg_move_speed[i] = -4
        if (udg_detect_left[i] == 1) then
            set udg_move_direction[i] = udg_move_direction[i] - 8
        elseif (udg_detect_right[i] == 1) then
            set udg_move_direction[i] = udg_move_direction[i] + 8
        endif
    elseif ((udg_detect_left[i] == 1) and (udg_detect_up[i] == 0) and (udg_detect_down[i] == 0))  then
        if (udg_turn_speed[i] < 12) then
            set udg_turn_speed[i] = udg_turn_speed[i] + 0.1
        endif
        set udg_move_direction[i] = udg_move_direction[i] + 8 + udg_turn_speed[i]
    elseif ((udg_detect_right[i] == 1) and (udg_detect_up[i] == 0) and (udg_detect_down[i] == 0))  then
        if (udg_turn_speed[i] > -12) then
            set udg_turn_speed[i] = udg_turn_speed[i] - 0.1
        endif
        set udg_move_direction[i] = udg_move_direction[i] - 8 + udg_turn_speed[i]
    endif
    set udg_tmp_loc = GetUnitLoc(udg_Player[i])
    set udg_tmp_loc2 = PolarProjectionBJ(udg_tmp_loc, udg_move_speed[i], udg_move_direction[i])
    set udg_bool_path = IsTerrainPathingType(GetLocationX(udg_tmp_loc2), GetLocationY(udg_tmp_loc2), TERRAIN_PATHING_WALKABLE)
    if (udg_bool_path == true) then
        call SetUnitPositionLocFacingBJ(udg_Player[i], udg_tmp_loc2, udg_move_direction[i])
        call RemoveLocation(udg_tmp_loc2)
    elseif (udg_bool_path == false) then
        call RemoveLocation(udg_tmp_loc2)
        set j = 1
        loop
        set udg_tmp_loc2 = PolarProjectionBJ(udg_tmp_loc, (udg_move_speed[i] / 2), udg_move_direction[i] + ( 9 * j ))
        set udg_bool_path = IsTerrainPathingType(GetLocationX(udg_tmp_loc2), GetLocationY(udg_tmp_loc2), TERRAIN_PATHING_WALKABLE)
        if (udg_bool_path == true) then
            call SetUnitPositionLocFacingBJ(udg_Player[i], udg_tmp_loc2, udg_move_direction[i])
            call RemoveLocation(udg_tmp_loc2)
            exitwhen true
        elseif (udg_bool_path == false) then
            call RemoveLocation(udg_tmp_loc2)
            set udg_tmp_loc2 = PolarProjectionBJ(udg_tmp_loc, (udg_move_speed[i] / 2), udg_move_direction[i] - ( 9 * j ))
            set udg_bool_path = IsTerrainPathingType(GetLocationX(udg_tmp_loc2), GetLocationY(udg_tmp_loc2), TERRAIN_PATHING_WALKABLE)
            if (udg_bool_path == true) then
                call SetUnitPositionLocFacingBJ(udg_Player[i], udg_tmp_loc2, udg_move_direction[i])
                call RemoveLocation(udg_tmp_loc2)
                exitwhen true
            elseif (udg_bool_path == false) then
                call RemoveLocation(udg_tmp_loc2)
            endif
        endif
        set j = j + 1
        if (j == 20) then
           exitwhen true
        endif
        endloop
    call RemoveLocation(udg_tmp_loc)
    endif
endfunction

 

[WraithSeeker]

First thing, make those udg_prefix GUI variable a local.

 

[Megafyr]

You mean udg_move_direction and udg_move_speed? I thought globals were faster since they aren't created inside the function.

 

[Silvenon]

Well, no, they're just faster to transfer from one function to the other (because they don't have to transfer through a gamecache or whatever, because they're global ).

Locals are faster in this case, better looking and easier to work with. Also, you can use coordinates instead of locations:

local location l = GetUnitLoc(u)
local location lp = PolarProjectionBJ(l, 500, 90)

becomes:

local real x = GetUnitX(u)
local real y = GetUnitY(u)
local real xp = x + 500 * Cos(90 * bj_DEGTORAD)
local real yp = y + 500 * Sin(90 * bj_DEGTORAD)

Coordinates are faster and you don't have to clean them. I converted 90 degrees to radians because Cos and Sin use them. This is just simple trigonometry, you can check the PolarProjectionBJ function in blizzard.j to get the idea.

Also, you can replace other BJs. Do you know how to see what a certain BJ does?

EDIT: And what the fuck is this:

        loop
        set udg_tmp_loc2 = PolarProjectionBJ(udg_tmp_loc, (udg_move_speed[i] / 2), udg_move_direction[i] + ( 9 * j ))
        set udg_bool_path = IsTerrainPathingType(GetLocationX(udg_tmp_loc2), GetLocationY(udg_tmp_loc2), TERRAIN_PATHING_WALKABLE)
        if (udg_bool_path == true) then
            call SetUnitPositionLocFacingBJ(udg_Player[i], udg_tmp_loc2, udg_move_direction[i])
            call RemoveLocation(udg_tmp_loc2)
            exitwhen true
        elseif (udg_bool_path == false) then
            call RemoveLocation(udg_tmp_loc2)
            set udg_tmp_loc2 = PolarProjectionBJ(udg_tmp_loc, (udg_move_speed[i] / 2), udg_move_direction[i] - ( 9 * j ))
            set udg_bool_path = IsTerrainPathingType(GetLocationX(udg_tmp_loc2), GetLocationY(udg_tmp_loc2), TERRAIN_PATHING_WALKABLE)
            if (udg_bool_path == true) then
                call SetUnitPositionLocFacingBJ(udg_Player[i], udg_tmp_loc2, udg_move_direction[i])
                call RemoveLocation(udg_tmp_loc2)
                exitwhen true
            elseif (udg_bool_path == false) then
                call RemoveLocation(udg_tmp_loc2)
            endif
        endif
        set j = j + 1
        if (j == 20) then
           exitwhen true
        endif
        endloop

Now I actually learned something.

 

[Megafyr]

I see... I'll fix it. : )
No, how?

Edit:

If (udg_bool_bath == false) then
//Still able to move unit. It moves sideways across whatever is not pathable.
endif

 

[Silvenon]

You can download JassCraft (Jass NewGen Pack would be better). You have JassCraft in the tools on this site. Jass NewGen Pack is on wc3c.net under resources -> tools.

In JassCraft you can just double click the function and it appears at the bottom (in Jass NewGen Pack it's ctrl + left click), showing itself as a native or as a BJ. For example, if I double click CreateUnit function, this line appears at the bottom:

native CreateUnit takes player id, integer unitid, real x, real y, real face returns unit


This is obviously a native. But if I double click on CreateNUnitsAtLoc, I get this:

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

This is not a native, because it does a lot of other stuff and calls some other functions (everything that doesn't have a native in it and isn't a single line, it's a BJ ).

We can see that this function creates a lot of unnecessary stuff that we could have done more easily and more efficiently. We can also see here that the CreateUnitAtLocSaveLast is also a BJ. If you copy it above and double click it, you'll get:

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

Only in JASS you can really see how awful GUI is. This is even worse when you take into account that you use the CreateNUnitsAtLoc function mostly to create just one unit. CreateUnitAtLocSaveLast function allows you to get the created unit with GetLastCreatedUnit function, which is also a BJ and actually returns the global variable where the created unit was stored. If you double click it, you'll see what I mean:

function GetLastCreatedUnit takes nothing returns unit

    return bj_lastCreatedUnit

endfunction

So instead of all this, you can just do:

local unit u = CreateUnit(...)

It's much faster, cleaner and it's more efficient.

Now, lets convert something in your code, for example, that PolarProjectionBJ function. If you double click it, you'll get this:

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

So that is what is actually happening. What you can do is simply copy that (but without having to use locations. Lets find an example in your code:

set udg_tmp_loc = GetUnitLoc(udg_Player[i])
set udg_tmp_loc2 = PolarProjectionBJ(udg_tmp_loc, udg_move_speed[i], udg_move_direction[i])

A location is consisted of two coordinates, x and y, which are real numbers. You can get the position of udg_Player[i] like by declaring x and y as reals at the beginning and using GetUnitX and GetUnitY functions:

local real x = GetUnitX(udg_Player[i])
local real y = GetUnitY(udg_Player[i])

Then you can convert PolarProjectionBJ to natives according to what we saw when we double clicked it. We can create two additional coordinates which will belong to the "projected" point:

local real x = GetUnitX(udg_Player[i])
local real y = GetUnitY(udg_Player[i])
local real x1
local real y1
// ...
// ...
set x1 = x + udg_move_speed[i] * Cos(udg_move_direction[i] * bj_DEGTORAD)
set y1 = y + udg_move_speed[i] * Sin(udg_move_direction[i] * bj_DEGTORAD)(

The Cos and Sin functions take radians as angle values, not degrees, so you can simply convert degrees to radians by multiplying with bj_DEGTORAD, which is actually a constant global (for some people it's easier to remember bj_DEGTORAD than bj_PI/180.0, and there is no change in efficiency so it doesn't matter). You can also convert radians to degrees by multiplying with bj_RADTODEG. You probably noticed that DEGTORAD is short for "degrees to radians" and RADTODEG is short for "radians to degrees".

Now, to get back to the example lines, further, instead of this:

set udg_bool_path = IsTerrainPathingType(GetLocationX(udg_tmp_loc2), GetLocationY(udg_tmp_loc2), TERRAIN_PATHING_WALKABLE)


You can pass the coordinates directly:

set udg_bool_path = IsTerrainPathingType(x1, y1, TERRAIN_PATHING_WALKABLE)


So we can conclude that using coordinates is much faster, simpler and more efficient than using locations. Though sometimes you have to use them, but you can immediately extract its coordinates and destroy it afterward. One example is GetSpellTargetLoc function, which doesn't have a version with coordinates, but you can just do this:

function Blah takes nothing returns nothing
    local location l = GetSpellTargetLoc()
    local real x = GetLocationX(l)
    local real y = GetLocationY(l)
    
    call RemoveLocation(l)
    
    // ...
    // do something with x and y...
    // ...
    
    set l = null
endfunction

I hope things are more clear now.

 

[Megafyr]

Yeah. Thank you for helping me. Here's the code now:

function MoveUnit takes integer i returns nothing
    local integer j = 0
    local real n = 0
    local real m = GetUnitFacing(udg_Player[i])
    local real x = GetUnitX(udg_Player[i])
    local real y = GetUnitY(udg_Player[i])
    local real x1
    local real y1
    if (udg_detect_up[i] == 1) then
        set n = 4
        if (udg_detect_left[i] == 1) then
            set m = m + 8
        elseif (udg_detect_right[i] == 1) then
            set m = m - 8
        endif
    elseif (udg_detect_down[i] == 1) then
        set n = -4
        if (udg_detect_left[i] == 1) then
            set m = m - 8
        elseif (udg_detect_right[i] == 1) then
            set m = m + 8
        endif
    elseif ((udg_detect_left[i] == 1) and (udg_detect_up[i] == 0) and (udg_detect_down[i] == 0))  then
        if (udg_turn_speed[i] < 12) then
            set udg_turn_speed[i] = udg_turn_speed[i] + 0.1
        endif
        set m = m + 8 + udg_turn_speed[i]
    elseif ((udg_detect_right[i] == 1) and (udg_detect_up[i] == 0) and (udg_detect_down[i] == 0))  then
        if (udg_turn_speed[i] > -12) then
            set udg_turn_speed[i] = udg_turn_speed[i] - 0.1
        endif
        set m = m - 8 + udg_turn_speed[i]
    endif
    set x1 = x + n * Cos(m * bj_DEGTORAD)
    set y1 = y + n * Sin(m * bj_DEGTORAD)
    set udg_bool_path = IsTerrainPathingType(x1, y1, TERRAIN_PATHING_WALKABLE)
    if (udg_bool_path == true) then
        call SetUnitPosition(udg_Player[i], x1, y1 )
        call SetUnitFacing(udg_Player[i], m)
    elseif (udg_bool_path == false) then
        set j = 2
        loop
        set x1 = x + (n / 2) * Cos((m + ( 9 * j ))* bj_DEGTORAD)
        set y1 = y + (n / 2) * Sin((m + ( 9 * j ))* bj_DEGTORAD)
        set udg_bool_path = IsTerrainPathingType(x1, y1, TERRAIN_PATHING_WALKABLE)
        if (udg_bool_path == true) then
            call SetUnitPosition(udg_Player[i], x1, y1)
            call SetUnitFacing(udg_Player[i], m)
            exitwhen true
        elseif (udg_bool_path == false) then
            set x1 = x + (n / 2) * Cos((m + ( -9 * j ))* bj_DEGTORAD)
            set y1 = y + (n / 2) * Sin((m + ( -9 * j ))* bj_DEGTORAD)
            set udg_bool_path = IsTerrainPathingType(x1, y1, TERRAIN_PATHING_WALKABLE)
            if (udg_bool_path == true) then
                call SetUnitPosition(udg_Player[i], x1, y1)
                call SetUnitFacing(udg_Player[i], m)
                exitwhen true
            endif
        endif
        set j = j + 1
        if (j == 20) then
           exitwhen true
        endif
        endloop
    endif
endfunction

 

[Silvenon]

Better, but still the globals look awful, try to replace them with locals. Or if you can't, please use JNGP way, for example:

scope Whatever
globals
    real detect_down = ...
endglobals

// ...
// ...

if detect_down == 1 then
    // ...
endif

// ...
endscope

At least you'll get rid of the udg_s. But I suggest you replace them with locals, if you can't you can post the whole code (where you've set those globals) and I can show you how.

Do you use Jass NewGen Pack (JNGP)?

 

[Megafyr]

I don't think that I'm gonna do that just yet, cause I have made my whole map in GUI until now, so all the udg_variables are used in alot of GUI triggers aswell. If I start changing them I have to change almost everything in my entire map which will take too much time. : < But thanks anyway.

I use the NewGen WE, isn't JNGP included?

 

[Silvenon]

Ok then.

Yeah, then it's ok, I was just wondering if you can use vJass syntax.

Well, I think that's everything.