[JASS] Please help me make my code better

[dardas]

Okay.. i've made a code that makes a unit jump, and loose height every 0.01 seconds. its pulled by "gravity", which is a real variable in my game, effecting all units.

My system is VJASS,

i've took the idea of the "Mountain check" from diehard@azeroth,
but i didnt take his system, i made my own.

Tho, diehard@azeroth did teach me how to use it, so credits to him =].

Heres the code:

TheCode

scope RS initializer INIT


    globals
    
        private real Gravity = 0.75 // this will be the gravity for all units in game.
        // Gravity info: @@ Must Read! @@
        // The gravity will be used on all units,
        // 0.75 is quite alot, i'd recommand 0.50 or else it wont be able to count all units.
        // The only problem is that if its not 1, or 0.01, or 0.1, it wont be able to loop thru all numbers.
        // If its 0.75 the starting height(variable X in Test trigger), must be a multiple of 0.75.
        // If you'r good in math, look at how to use other numbers.
        // If the number is higher then 0, the loop will decrease height each 0.01 seconds,
        // The math will do this: CURRENT flying height of unit - gravity = new height.
        // If the number is LOWER then 10, the system will remove the unit from the loop,
        // stop changing his height.
        // Tho will remove his height and restart it to 0.
        // this will be how the system math works:
        // Starting heiht * 0.75 = amount of loops.
        // 150 starting height with 0.75 gravity will be:
        // 150 * 0.75 = 112.5
        // 112.5 loops in total. that means 112.5 / 150 = 0.75
        // 0.75 * 100 (miliseconds in a second), is 75.
        // 75 height loose in a second.
        // hope you understend how to use this,
        // good luck on you'r map =]
        private real x
        private real y
        private location l
        private group Group = CreateGroup()
        private integer c
        private real x2
        private real y2
        public real z1
        public real z2
        private location l2
        //privates decleration 
    
        public boolean UIGU // Unit Is Moving Up-Wards, wont recommand touching.
        public boolean UIGD // Unit Is Moving Down-Wards, wont recommand touching.
        public boolean UISS // Unit Is Staying At Same Terrain, wont recommand touching.
        public real DiffrenceInZ // dont touch, this defines the diffrence in height of terrain. used to jump.
        
    endglobals
    
private function Actions takes nothing returns nothing
local unit u
local real f 

        set u = GetEnumUnit()
        set f = GetUnitFlyHeight(u)
        
    if f > 0 then
            
            call SetUnitFlyHeight( u, f - Gravity, 0 )
                  
    endif
        
        if f <= 10 then
        
            call GroupRemoveUnit(Group, u)
            
        endif
        
        
endfunction

private function GActions takes nothing returns nothing
    
    call ForGroup(Group, function Actions)
    
endfunction

public function Actions2 takes unit u returns nothing
    set x2 = GetUnitX(u)
    set y2 = GetUnitY(u)
    set l2 = Location (x2, y2)
    set z1 = GetLocationZ(l2)
    //----------------------------------------------------------------
    set x = GetLocationX(l2) + 50 * Cos(GetUnitFacing(u) * bj_DEGTORAD)
    set y = GetLocationY(l2) + 50 * Sin(GetUnitFacing(u) * bj_DEGTORAD)
    set l2 = Location(x2, y2)
    set z2 = GetLocationZ(l2)
    
    if z1 > z2 then
        
     set DiffrenceInZ = z1 - z2
     set UIGU = true
     set UIGD = false
     set UISS = false
     
     elseif z1 < z2 then
     
        set DiffrenceInZ = z2 - z1
        set UIGD = true
        set UISS = false
        set UIGU = false
     
        elseif z1 == z2 then
        
            set DiffrenceInZ = 0
            set UISS = true
            set UIGD = false
            set UIGU = false
        
    endif
    call RemoveLocation(l)
    set l = null
endfunction

public function Add takes unit u, real x returns nothing
        if not IsUnitInGroup(u, Group) then
            call GroupAddUnit(Group, u)
        else
            return
        endif

    
        call UnitAddAbility(u, 'Amrf')
        call UnitRemoveAbility(u, 'Amrf')
        call Actions2(u)
        
        if UIGD == true then
                
            call SetUnitFlyHeight(u, x2 + 50, 0)
            set UIGD = false
                
        elseif UIGU == true then
                
            call SetUnitFlyHeight(u, x2 - 50, 0)
            set UIGU = false
                
        elseif UISS == true then
        
            call SetUnitFlyHeight(u, x2, 0)
            set UISS = false
            
        endif
            
endfunction

private function INIT takes nothing returns nothing
    local trigger JS = CreateTrigger(  )
    call TriggerRegisterTimerEventPeriodic(JS, 0.003 )
    call TriggerAddAction(JS, function GActions )
endfunction

endscope

Again, i remind, this is Vjass, not jass ;p.

Thanks in advance =]

 

[watermelon_1234]

• Your global location should be like this:
  

  private location l = Location(0,0)

  Then, when you want to GetLocationZ of some coordinates, do this:
  

  call MoveLocation(l,x,y)

  You should know that you don't need to use RemoveLocation or set the variable to null with this method.
• You could do
  

  if UnitAddAbility(u, 'Amrf') then
       call UnitRemoveAbility(u, 'Amrf')
  endif

  instead so that it won't bug if the unit already has that ability.
• I suggest doing this for your Init function:
  

  call TimerStart(CreateTimer(),0.003,true,function GActions)

• I would do away with the three booleans and use one integer, like MyInt.
  When UIGD is supposed to be true, MyInt is 1.
  When UIGU is supposed to be true, MyInt is -1.
  When UISS is supposed to be true, MyInt is 0.
  Do you get what I mean? This should simplify that if-then-else in your Add function.
• I see no point in using in l2.
• You need to null u in Actions.
• You could use a bigger timer period, like 0.03.

 

[dardas]

Thank's, i've edited the whole code.. =]
i've changed alot since i updated the code up, but it still was there, so thanks =]

 

[Anachron]

Here is a little update with comments

//: SYSTEM CORE COMMENTS GO HERE!
    // Gravity info: @@ Must Read! @@
    // The gravity will be used on all units,
    // 0.75 is quite alot, i'd recommand 0.50 or else it wont be able to count all units.
    // The only problem is that if its not 1, or 0.01, or 0.1, it wont be able to loop thru all numbers.
    // If its 0.75 the starting height(variable X in Test trigger), must be a multiple of 0.75.
    // If you'r good in math, look at how to use other numbers.
    // If the number is higher then 0, the loop will decrease height each 0.01 seconds,
    // The math will do this: CURRENT flying height of unit - gravity = new height.
    // If the number is LOWER then 10, the system will remove the unit from the loop,
    // stop changing his height.
    // Tho will remove his height and restart it to 0.
    // this will be how the system math works:
    // Starting heiht * 0.75 = amount of loops.
    // 150 starting height with 0.75 gravity will be:
    // 150 * 0.75 = 112.5
    // 112.5 loops in total. that means 112.5 / 150 = 0.75
    // 0.75 * 100 (miliseconds in a second), is 75.
    // 75 height loose in a second.
    // hope you understend how to use this,
    // good luck on you'r map =]
scope RS initializer init //Initializers are ALWAYS small-case

    //: You check my indention? That is good indention
    globals
        //: Next time please use speaking names. Group isn't self-explaining
        //: Always give variables a default value
        private real        Gravity         = 0.75 // this will be the gravity for all units in game.
        private real        x               = 0. 
        private real        y               = 0.
        private location    l               = null
        private group       Group           = CreateGroup()
        private integer     c               = 0
        private real        x2              = 0.
        private real        y2              = 0.
        private real        z1              = 0.     
        private real        z2              = 0.
        private location    l2              = null
        //privates decleration
    
        //: Again, these variables are not self-explaining!
        private boolean      UIGU            = false // Unit Is Moving Up-Wards, wont recommand touching.
        private boolean      UIGD            = false // Unit Is Moving Down-Wards, wont recommand touching.
        private boolean      UISS            = false // Unit Is Staying At Same Terrain, wont recommand touching.
        private real         DiffrenceInZ    = false // dont touch, this defines the diffrence in height of terrain. used to jump.
    endglobals
    
    //: Try to use structs!
    private struct gravity
    endstruct
    
    //: Actions? Use better names.
    private function Actions takes nothing returns nothing
        local unit u = GetEnumUnit()
        local real f  = GetUnitFlyHeight(u) //: Yes, you can do this
        
        if f > 0 then
            call SetUnitFlyHeight( u, f - Gravity, 0 )
        endif
        if f >= 10 then //: make 10 configureable
            call GroupRemoveUnit(Group, u)
        endif
        
        //: Don't forget to remove leaks
        set u = null
    endfunction

    //: Again, better names
    private function GActions takes nothing returns nothing
        call ForGroup(Group, function Actions) //: Removed useless spacing
    endfunction

    public function Actions2 takes unit u returns nothing
        set x2  = GetUnitX(u)
        set y2  = GetUnitY(u)
        set l2  = Location (x2, y2)
        set z1  = GetLocationZ(l2)
        //----------------------------------------------------------------
        //: When you make long lines, at least explain why you did it
        set x   = GetLocationX(l2) + 50 * Cos(GetUnitFacing(u) * bj_DEGTORAD)
        set y   = GetLocationY(l2) + 50 * Sin(GetUnitFacing(u) * bj_DEGTORAD)
        set l2  = Location(x2, y2)
        set z2  = GetLocationZ(l2)
        
        //: This is a correct if/then/else construct
        //: MISSING COMMENTS!
        if z1 > z2 then
            set DiffrenceInZ = z1 - z2
            set UIGU = true
            set UIGD = false
            set UISS = false
            //: Because bla bla bla
        elseif z1 < z2 then
            set DiffrenceInZ = z2 - z1
            set UIGD = true
            set UISS = false
            set UIGU = false
            //: Now the unit is blargh...
        elseif z1 == z2 then
            set DiffrenceInZ = 0
            set UISS = true
            set UIGD = false
            set UIGU = false
        endif
        
        //: Cleanup area
        call RemoveLocation(l)
        set l = null
    endfunction

    //: speaking names?!
    public function Add takes unit u, real x returns nothing
        //: We don't want the same unit to be in here twice
        if not IsUnitInGroup(u, Group) then
            call GroupAddUnit(Group, u)
        else
            return
        endif

        call UnitAddAbility(u, 'Amrf')
        call UnitRemoveAbility(u, 'Amrf')
        call Actions2(u) //: Action2? You kidding. Use speaking names.
        
        if UIGD == true then
            call SetUnitFlyHeight(u, x2 + 50, 0)
            set UIGD = false
        elseif UIGU == true then
            call SetUnitFlyHeight(u, x2 - 50, 0)
            set UIGU = false
        elseif UISS == true then
            call SetUnitFlyHeight(u, x2, 0)
            set UISS = false
        endif
    endfunction

    //: Every block inside a library/scope is indented!
    private function init takes nothing returns nothing
        //local trigger   trig    = CreateTrigger() //: NO SPACES BETWEEN BRACKETS >.<
        local timer     tim     = CreateTimer()
        
        //: We don't need triggers, we can use timerstart instead
        call TimerStart(tim, 0.003 /*CONFIGUREABLE!*/, true, function GAction)
        //call TriggerRegisterTimerEvent(trig, 0.003 /*MAKE THIS CONFIGUREABLE*/, true)
        //call TriggerAddAction(tim, function GActions )
    endfunction

endscope

 

[dardas]

well.. the only problem is that i updated since i gave you code :/ it'll be a mess if i add those comments atm.. cuz i changed UIGD, UIGU, UISS to integer, removed/added stuff, edited names.. etc ;p

 

[Anachron]

Then why the hell didn't you update it? What do I know. I thought it was the latest version.

 

[dardas]

1.. i thought you wasnt gonna update it =]

2. heres the code:

TheCode

scope JS initializer INIT


    globals
        
        private real Gravity = 0.75 // READ BELOW BEFORE EDITING. // THIS EFFECTS ALL UNITS IN GAME!.
        
        // MUST READ BELOW!
        
        // Gravity info: @@ Must Read! @@
        // The gravity will be used on all units,
        // The only problem is that if the gravity is not 1, or 0.01, or 0.1, it wont be able to loop thru all numbers.
        // If its 0.75 the starting height(variable X in Test trigger), must be a multiple of 0.75.
        // This is how to out-smart the system:
        // If the number is higher then 0, the loop will decrease height each 0.01 seconds,
        // The math will do this: CURRENT flying height of unit - gravity = new height.
        // If the number is LOWER then 10, the system will remove the unit from the loop,
        // stop changing his height.
        // Tho will remove his height and restart it to 0.
        // this will be how the system math works:
        // Starting heiht * 0.75 = amount of loops.
        // 150 starting height with 0.75 gravity will be:
        // 150 * 0.75 = 112.5
        // 112.5 loops in total. that means 112.5 / 150 = 0.75, do this check if you'r unsure of your math!
        // 0.75 * 100 (100 = amount of miliseconds in a second), is 75.
        // 75 height loose in a second.
        // Another last thing: Change how much times the system runs in a second. 0.01 = 100 times, that means 0.75*100, if its 0.05, then its 0.75 * 20 = 15 height lost in a second.
        // hope you understend how to use this,
        // good luck on you'r map =]
        
           //~~~~~~~~~~~~~~~~~~~\\
          //~~~System Made By~~~~\\
         //~~~~~~~~Dardas~~~~~~~~~\\
        //~~~~~Enjoy it :D~~~~~~~~~\\
       //~~~~~~~~~~~~~~~~~~~~~~~~~~~\\
       
       //How much time delay between each run of the jumping code.
        private real SystemRun = 0.01 // i'd recommand changing this. ~ if you change this, make sure you change gravity! ~
       //How much time delay between each run of the jumping code.
        
        // Privates Declerations
        // Do not edit below.
        private group Group = CreateGroup() // Creating the group which holds the units that are currently flying.
        private real x // Read function Mountain for more info.
        private real y // Read function Mouintain for more info.
        private real z1 // Read function Mountain for more info.
        private real z2 // Read function Mountain for more info.
        private location l = Location(0,0) // read function Mountain for more info.
    
        public integer UIG // Read function Mountain for more info.
        public real DiffrenceInZ // Read function Mountain for more info.
        //Privates Decelartion End.
    endglobals
    
private function Actions takes nothing returns nothing
local unit u
local real f 

        set u = GetEnumUnit()
        set f = GetUnitFlyHeight(u)
        
    if f > 0 then // checking if unit is in mid air.
            
            call SetUnitFlyHeight( u, f - Gravity, 0 ) // decreasing height every time this is called
                  
    endif
        
        if f <= 10 then // checks if unit is 10 height before touching the ground, or not.
        
            call GroupRemoveUnit(Group, u) // removing the unit from the group, stopping reducing his height.
            
        endif
        
        
endfunction

public function Mountain takes unit u returns nothing

    set x = GetUnitX(u) // Setting the coordinate X of the unit.
    
    set y = GetUnitY(u) // Setting the coordinate Y of the unit.
    
    call MoveLocation(l, x, y) // Setting location l as coordinate X, and Y.
    
    set z1 = GetLocationZ(l) // This is used to check if unit is on a high level terrain, low level terrain, or in same terrain.
    
    set x = GetLocationX(l) + 50 * Cos(GetUnitFacing(u) * bj_DEGTORAD) // This is the next coordinate X the unit will step on.
    
    set y = GetLocationY(l) + 50 * Sin(GetUnitFacing(u) * bj_DEGTORAD) // This is the next coordinate Y the unit will step on.
    
    call MoveLocation(l, x, y) // Moving Location l to the coordinates X, and Y.
    
    set z2 = GetLocationZ(l) // Setting the check if unit is going up a mountain, down a mountain, or stays in same terrain.
    
    
    if z1 > z2 then // checking if the terrain height in the new location is HIGHER then the first location.
        // UIG = checking if unit is walking up a mountain, down a mountain, or in same height.
        set DiffrenceInZ = z1 - z2 // Sets the diffrence in height to: old location height - new location height. 
        set UIG = 1  // Thisis a check that says its going UP
        
    elseif z1 < z2 then // Checking if the terrain height in the new location is LOWER then the first location.
        set DiffrenceInZ = z2 - z1 // sets the diffrence in height to: new location height - old location height.
        set UIG = -1 // This is a check that says its going DOWN.
        
    elseif z1 == z2 then // Checking if the terrain height in the new location is SAME as the first location.
        set DiffrenceInZ = 0 // sets the diffrence in heigt to 0, since the unit walks in same terrain height.
        set UIG = 0 // This is a check that says its going in same height.
        
    endif
endfunction

private function GActions takes nothing returns nothing
    
    call ForGroup(Group, function Actions) // running the "Actions" trigger.
    
endfunction

public function Jump takes unit u, real x returns nothing
        if not IsUnitInGroup(u, Group) then // Checks if the unit is allready in that group or not.
            call GroupAddUnit(Group, u) // Adds the unit to the group if he IS NOT inside the group allready.
        else
            return // Just another thing needed.. no't much use to it tho.
        endif

    
        call UnitAddAbility(u, 'Amrf') // adding the crow form ability, ask in [url]www.hiveworkshop.com[/url] for info.
        call UnitRemoveAbility(u, 'Amrf') // removing the crow form ability, ask in [url]www.hiveworkshop.com[/url] for info.
        
        call Mountain(u) // calling the code that checks if unit is on a mountain or not.
        
        if UIG == -1 then
                
            call SetUnitFlyHeight(u, x + 50, 0) // Unit is going down-wards a mountain, increasing his height.
                
        elseif UIG == 1 then
                
            call SetUnitFlyHeight(u, x - 50, 0) // Unit is going up-wards a mountain, decreasing his height.
                
        elseif UIG == 0 then
        
            call SetUnitFlyHeight(u, x, 0) // Unit is going on same height, keeping his height regular.
            
        endif
            
endfunction

private function INIT takes nothing returns nothing
    local trigger JS = CreateTrigger(  )
    call TimerStart(CreateTimer(),SystemRun,true,function GActions)
    call TriggerAddAction(JS, function GActions )
endfunction
endscope

3. Thanks =]

 

[Anachron]

You should still read my comments.

 

[dardas]

What are structs actually?.

 

[Anachron]

In other programming languages they are called classes.

 

[dardas]

so.. its something like scope, library.. that are used to..?

 

[Anachron]

Omg no.
Hell, you should read the things more carefully.
You should google it. (Object orientated programming, classes)

 

[watermelon_1234]

Hm, you seemed to have ignored some of my advice or misinterpreted it.

• You could do
  

  if UnitAddAbility(u, 'Amrf') then
       call UnitRemoveAbility(u, 'Amrf')
  endif

  instead so that it won't bug if the unit already has that ability.

I'm talking about replacing this:

        call UnitAddAbility(u, 'Amrf') // adding the crow form ability, ask in [url]www.hiveworkshop.com[/url] for info.
        call UnitRemoveAbility(u, 'Amrf') // removing the crow form ability, ask in [url]www.hiveworkshop.com[/url] for info.

• I suggest doing this for your Init function:
  

  call TimerStart(CreateTimer(),0.003,true,function GActions)

You don't need a trigger anymore.

• I would do away with the three booleans and use one integer, like MyInt.
  When UIGD is supposed to be true, MyInt is 1.
  When UIGU is supposed to be true, MyInt is -1.
  When UISS is supposed to be true, MyInt is 0.
  Do you get what I mean? This should simplify that if-then-else in your Add function.

There was a reason I made the booleans give a certain number. It was so that you could simplify this:

        if UIGD == true then
                
            call SetUnitFlyHeight(u, x2 + 50, 0)
            set UIGD = false
                
        elseif UIGU == true then
                
            call SetUnitFlyHeight(u, x2 - 50, 0)
            set UIGU = false
                
        elseif UISS == true then
        
            call SetUnitFlyHeight(u, x2, 0)
            set UISS = false
            
        endif

into this:

call SetUnitFlyHeight(u, x2 + 50*UIG, 0)

You can't use it like this now though since you have UIG as -1 when it needs to be 1 and 1 when it needs to be -1.

• You need to null u in Actions.

Fix this please. I'm talking about the local unit variable. Anachron also mentioned this.

Also, in your Mountain actions, there's no reason to use GetLocationX/Y when you already have the variables set to that. Use those instead. (It doesn't seem like you'll need to store those later on.)