Vjass Code Problems (Simple)

[Squll2]

Hey everyone. Begginner Vjasser here, with a very simple code that is playing up on me.

Would anyone care to explain to me why those code does not work at all and tell me how I can improve it, and make it faster... cleaner as such?

Thanks in advance!!

The simple aim of the spell, currently, is to create lightning from enemy units around the target point to the caster.

Also, with my GroupEnumUnitsInRange, how do I filter it through a condition? :nuts:

scope CounterEnergy initializer Init

//Changeable / Alterable functions

private function ID takes nothing returns integer
  return 'A000'
endfunction

private function Unit_ID takes nothing returns integer
  return 'h000'
endfunction

private function Boo takes nothing returns boolexpr
   return null
endfunction

private function Radius takes nothing returns real
   return 350.00
endfunction

//Changeable / Alterable functions End.

private function Cons takes nothing returns boolean
   return GetSpellAbilityId() == ID
endfunction

private function Actions takes nothing returns nothing
//Local Setting
 local unit u     = GetTriggerUnit()
 local unit e
 local unit d
 local group g               = CreateGroup()
 local group g2              = CreateGroup()
 local player p              = GetOwningPlayer(u)
 local integer a             = 0
 local integer b             = 0
 local location l            = GetSpellTargetLoc()
 local lightning array Light
 local real x                = GetLocationX(l)
 local real y                = GetLocationY(l)
 local real X
 local real Y
//Local Setting

 call GroupEnumUnitsInRange(g, x, y, 500.00, null)
 call GroupAddGroup(g, g2)
 
loop

 set e = FirstOfGroup(g2)
 exitwhen e == null
 set X = GetUnitX(e)
 set Y = GetUnitY(e)
 
 set Light[a] = AddLightning( "CLPB", true, X, Y, x, y)
 
 set a = a+1
 call GroupRemoveUnit(g2, e)
 
endloop

 call PolledWait(2.00)
 
loop

 exitwhen a < 0
 
 call DestroyLightning(Light[a])
 
 set a = a-1
 
endloop
 
endfunction

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

endscope

 

[kingkingyyk3]

1) remove your leak.
2)

private function ID takes nothing returns integer
  return 'A000'
endfunction

You can make it as constant globals, since variable is faster than calling a function.
3) Create a function like

private function Cons takes nothing returns boolean
   return GetSpellAbilityId() == ID
endfunction

to filter units.

 

[Exide]

Your trigger is quite difficult to read. You should indent it.

Why are these functions?

private function ID takes nothing returns integer
  return 'A000'
endfunction

private function Unit_ID takes nothing returns integer
  return 'h000'
endfunction

private function Boo takes nothing returns boolexpr
   return null
endfunction

private function Radius takes nothing returns real
   return 350.00
endfunction

Why are you using two groups?
Remove g2, and use g only.

EDIT: g should be a private global variable.
If you do this, you don't need to 'call DestroyGroup(g)'

call GroupEnumUnitsInRange(g, x, y, 500.00, null)

instead of 'null', add a function:

function A takes nothing returns boolean
    //return Some Condition Here
endfunction


    //Lots of actions and whatnot..

    call GroupEnumUnitsInRange(g, x, y, 500.00, Filter(function A))

 

[Squll2]

I was told that GroupAddGroup() and using two groups is a better method then just using one group?

Those functions are there, exide, for easy customastion, so I can add them down below and instead of searching for them I will just change their values there, but in this version of the trigger I havn't used them ><

Thanks for the help exide +rep, kingking, when your talking to a noob, please evaluate what you mean.. I know what i have to do I just don't know where to put it and how.. >.>

Edit: Exide what do you mean Why are these functions? Should they be something else?

 

[Exide]

Yea, they should be variables.
Calling a function is unnecessary work.
Using private globals you can declare them at the top of your trigger, so it's equally easy to modify the values.

>I was told that GroupAddGroup() and using two groups is a better method then just using one group?
I never heard anything about that. It sounds silly, though.
Check out this thread: http://www.thehelper.net/forums/showthread.php?t=131720

 

[Zaraf]

For your purposes, you actually don't even need a group.

I've modified your code to show you what I mean

scope CounterEnergy initializer Init
        
        globals
            private constant integer ID      = 'A000'
            private constant integer Unit_ID = 'h000'
            private constant real    Radius  = 350.0
            
            private real Xfilter
            private real Yfilter
            
            private boolexpr LightActions
            private lightning array Light
            
            group ENUM_GROUP = CreateGroup()
        endglobals

private function Cons takes nothing returns boolean
   return GetSpellAbilityId() == ID
endfunction

private function LightActionsAdded takes nothing returns boolean
    local unit f = GetFilterUnit()
    local real x = Xfilter
    local real y = Yfilter
    
    set Light[a] = AddLightning( "CLPB", true, GetUnitX(f), GetUnitY(f), x, y)

    return false
endfunction

private function DestroyLight takes nothing returns nothing
    local integer a = 0
    
    loop
        exitwhen DestroyLightning(Light[a]) == null
        call DestroyLightning(Light[a])
        set Light[a] = null
        set a = a+1
    endloop
    
    call DestroyTimer(GetExpiredTimer())
endfunction

private function Actions takes nothing returns nothing
    //Local Setting
    local unit u                = GetTriggerUnit()
    local player p              = GetOwningPlayer(u)
    local location l            = GetSpellTargetLoc()
    local real x                = GetLocationX(l)
    local real y                = GetLocationY(l)
    local timer t               = CreateTimer()
    //Local Setting
    
    set Xfilter = x
    set Yfilter = y
    call GroupEnumUnitsInRange(ENUM_GROUP, x, y, 500.00, LightActions)

    call TimerStart(t, 2.00, false, function DestroyLight)

    call RemoveLocation(l)
    set t = null
    set l = null
    set u = null
endfunction

//===========================================================================
private function Init takes nothing returns nothing
    local trigger trig = CreateTrigger()
    call TriggerRegisterAnyUnitEventBJ(trig, EVENT_PLAYER_UNIT_SPELL_EFFECT)
    call TriggerAddCondition(trig, Condition( function Cons))
    call TriggerAddAction(trig, function Actions)
    
    set LightActions = Condition(function LightActionsAdded)
endfunction

endscope

Basically, you can perform your actions in the filter function.

The code turned out to be a bit messy since I was trying to avoid using a struct. If you use a struct, then this works flawlessly. But as it is right now, it's not MUI, and there are other problems. But this is more to illustrate to you how you can completely avoid the group altogether, and make your code run faster and better.

I've explained this no group concept in more detail here:

http://www.thehelper.net/forums/showpost.php?p=1076839&postcount=3

I encourage you to learn this technique, and if you'd like, I can speak with you further on it to better explain, and show you how to rewrite your spell using structs to improve it.