Trouble with a loop

03-09-2007, 02:51 AM

Hello folks,

After much reading of the forum topics and tutorials I am trying to make the transition from proficient GUI triggering to jass coding. My first attempt is detailed below.

JASS:

function Trig_Juxt_Conditions takes nothing returns boolean
    return GetSpellAbilityId() == 'A005'
endfunction                                       


function Trig_Juxt_Actions takes nothing returns nothing
local group g
local unit cast
local unit swap
local unit u
local real x
local real y
local integer i
local integer gi

set cast = GetTriggerUnit()
set g = GetUnitsInRangeOfLocAll(800.00, GetUnitLoc(cast))
set gi = CountUnitsInGroup(g)
                    
   loop
       set u = FirstOfGroup(g)
       exitwhen i==gi
       if  IsUnitEnemy(u, GetOwningPlayer(cast))==false then
       call GroupRemoveUnit(g,u)
       set i = i+1
      endif
    endloop 

call DisplayTextToPlayer(Player(0),0,0,"Loop1")
set i = 0
set gi = CountUnitsInGroup(g)
set swap = FirstOfGroup(g)
      
 loop
       set u = FirstOfGroup(g)
       exitwhen i==gi
       set x = GetWidgetLife(swap)
       set y = GetWidgetLife(u)
       if  y > x then
       set swap = u
       set i = i+1
       else
       call GroupRemoveUnit(g,u)
       set i = i+1
      
             endif
    endloop 
call DisplayTextToPlayer(Player(0),0,0,"Loop2")
call RemoveUnit(swap)

endfunction

I am sure that it is a bit cumbersome as is, but that can be fixed. My main problem is in the logic of the loop.

As you can see there are debug checkers at the end of each loop. Since neither of them will run when I test the function I can only assume that the loops are not running to completion. Or that the first one is disabling the rest by being infinite.

The intent of the function is to gather all enemy units within 800 units of the triggering unit, determine the one with the highest hit points, and remove that unit.

Suggestions on both logic structures of loops with nested if statements and more efficient ways of achieving my goal would be appreciated. I am checking all code with Jass Craft.

-Fluxx

03-09-2007, 04:33 AM

Pyrogasm

Firstly, use the [jass] tags instead of [php].

Secondly, lets look at the way you're doing things in this function; anything that's hilighted is something that needs to be changed:

JASS:

function Trig_Juxt_Conditions takes nothing returns boolean 
    return GetSpellAbilityId() == 'A005' 
endfunction 

function Trig_Juxt_Actions takes nothing returns nothing 
local group g 
local unit cast
local unit swap 
local unit u 
local real x 
local real y 
local integer i
local integer gi

set cast = GetTriggerUnit()
set g = GetUnitsInRangeOfLocAll(800.00, GetUnitLoc(cast))
set gi = CountUnitsInGroup(g) 
                     
   loop
       set u = FirstOfGroup(g)
       exitwhen i==gi
       if IsUnitEnemy(u, GetOwningPlayer(cast))==false then
           call GroupRemoveUnit(g,u)
           set i = i+1
      endif
    endloop

call DisplayTextToPlayer(Player(0),0,0,"Loop1")
set i = 0 
set gi = CountUnitsInGroup(g)
set swap = FirstOfGroup(g) 
       
 loop 
       set u = FirstOfGroup(g) 
       exitwhen i==gi
       set x = GetWidgetLife(swap) 
       set y = GetWidgetLife(u) 
       if  y > x then 
           set swap = u 
           set i = i+1
       else 
           call GroupRemoveUnit(g,u) 
       set i = i+1
       endif
    endloop  
call DisplayTextToPlayer(Player(0),0,0,"Loop2")
call RemoveUnit(swap)

endfunction

First, let's look at the declaration of locals section. You're declaring "cast", and then setting it first thing in the function. We can always eliminate a line of code by combining those two lines into one like so: local unit cast = GetTriggerUnit(). Any variable may be declared and set to a value in the same line in this manner.

Next, we look at the way you exit your loops. For a general group loop that you want to use without the ForGroup() function, we use a format like this:

JASS:

loop
    set U = FirstOfGroup(G) //U being the unit variable and G being our group
    exitwhen U == null //So we can exit when the group is empty
    //Do some stuff with U
    GroupRemoveUnit(G, U) //Removing it from the group
endloop

You've sort-of got this down, but you're using an integer to equal the number of units in the group, and incrementing another one each iteration of the loop. So, fixing your loops, the whole code now looks something like this:

JASS:

function Trig_Juxt_Conditions takes nothing returns boolean 
    return GetSpellAbilityId() == 'A005' 
endfunction 

function Trig_Juxt_Actions takes nothing returns nothing 
local group g 
local unit cast = GetTriggerUnit()
local unit swap 
local unit u 
local real x 
local real y 

set g = GetUnitsInRangeOfLocAll(800.00, GetUnitLoc(cast))
                     
   loop
       set u = FirstOfGroup(g)
       exitwhen u == null
       if IsUnitEnemy(u, GetOwningPlayer(cast))==false then
           call GroupRemoveUnit(g,u)
       endif
       call GroupRemoveUnit(g,u)
    endloop

call DisplayTextToPlayer(Player(0),0,0,"Loop1")
set swap = FirstOfGroup(g) 
       
 loop 
       set u = FirstOfGroup(g) 
       exitwhen u == null
       set x = GetWidgetLife(swap) 
       set y = GetWidgetLife(u) 
       if  y > x then 
           set swap = u 
       else 
           call GroupRemoveUnit(g,u) 
       endif
       call GroupRemoveUnit(g,u)
    endloop  
call DisplayTextToPlayer(Player(0),0,0,"Loop2")
call RemoveUnit(swap)

endfunction

Immediately you should notice that there are extra GroupRemoveUnit() calls, and the first loop is screwed up. To fix the latter error, we'll introduce a new function that will set the first group accordingly, stop a memory leak, be faster, and remove the need for a filtering loop. To properly use the function GroupEnumUnitsInRange(), a group must first exist. thus, we will set our group like this: local group g = CreateGroup().

Next, we must set the group to the correct value. To do this, we will need a group to set (group "g"), a real x (which you have), a real y (which you also have), a real radius (which would be 800.00), and a boolexpr. We'll set our x and y values using the method outlined above with "cast". What is a boolexpr, you might ask? Here's an explaination of what they are. To create our boolexpr filter, we need another function:

JASS:

function Juxt_Enemy_Filter takes nothing returns boolean
    return IsUnitEnemy(GetFilterUnit(), GetOwningPlayer(udg_Juxt_Caster))
endfunction

Since local variables can't be passed from function to function, I've created a global variable ("Juxt_Caster") to pass the casting unit to the function. Now, we must add the line to set that variable, the filter function, remove the first loop, and re-work our group setting. Now, the trigger will look like this:

JASS:

function Trig_Juxt_Conditions takes nothing returns boolean 
    return GetSpellAbilityId() == 'A005' 
endfunction

function Juxt_Enemy_Filter takes nothing returns boolean
    return IsUnitEnemy(GetFilterUnit(), GetOwningPlayer(udg_Juxt_Caster))
endfunction

function Trig_Juxt_Actions takes nothing returns nothing 
local group g = CreateGroup()
local unit cast = GetTriggerUnit()
local unit swap 
local unit u 
local real x = GetUnitX(cast)
local real y = GetUnitY(cast)

set udg_Juxt_Caster = cast
call GroupEnumUnitsInRange(g, x, y, 800.00, Condition(function Juxt_Enemy_Filter))

//Notice the empty space :D

set swap = FirstOfGroup(g) 
       
 loop 
       set u = FirstOfGroup(g) 
       exitwhen u == null
       set x = GetWidgetLife(swap) 
       set y = GetWidgetLife(u) 
       if  y > x then 
           set swap = u 
       endif
       call GroupRemoveUnit(g,u)
    endloop  
call DisplayTextToPlayer(Player(0),0,0,"Loop2")
call RemoveUnit(swap)

endfunction

Everything else looks in order, and it should run just fine; you just need to nullify those variables. For displaying Debug text, you can just use BJDebugMsg("Text") instead of that whole call DisplayTextToPlayer(Player(0),0,0,"Text") thing. We can also eliminate another unit variable by removing the "u" variable, and reusing "cast".

All cleaned up, the code should look like this:

JASS:

function Trig_Juxt_Conditions takes nothing returns boolean 
    return GetSpellAbilityId() == 'A005' 
endfunction

function Juxt_Enemy_Filter takes nothing returns boolean
    return IsUnitEnemy(GetFilterUnit(), GetOwningPlayer(udg_Juxt_Caster))
endfunction

function Trig_Juxt_Actions takes nothing returns nothing 
local group g = CreateGroup()
local unit cast = GetTriggerUnit()
local unit swap 
local real x = GetUnitX(cast)
local real y = GetUnitY(cast)

set udg_Juxt_Caster = cast
call GroupEnumUnitsInRange(g, x, y, 800.00, Condition(function Juxt_Enemy_Filter))
set swap = FirstOfGroup(g) 
       
 loop 
       set cast = FirstOfGroup(g) 
       exitwhen cast == null
       set x = GetWidgetLife(swap) 
       set y = GetWidgetLife(cast) 
       if  y > x then 
           set swap = cast
       endif
       call GroupRemoveUnit(g,cast)
    endloop  
call BJDebugMsg("Loop2")
call RemoveUnit(swap)

set cast = null
set swap = null
call DestroyGroup(g)
set g = null
endfunction

+1 super-edited JASS code. Total: 2

03-09-2007, 04:45 AM	#3
Fluxx	Pyrogasm. Spectacular. Worked like a charm. Thanks for the help with both the trigger and my assumtions on jass coding. I keep re reading the tutorials here and on other sites, and am moving into compluter and programming logic. Any other suggested readings to improve my code and thought process?

03-09-2007, 04:55 AM

Pyrogasm

Read this thread:

Super-Edited JASS Script #1

Quote:

He is a gentleman and a scholar.

Sweet...

03-09-2007, 05:40 AM	#5
Fluxx	Follow up question. This trigger does not seem to work for multiple units. It will only be run for a single unit regardless of how many are issued the order at the same moment. Is this because of the use of the global variable, and is it possible to allow for multiple effects at the same moment? Ah, it must be choosing the same unit at the same moment.

03-09-2007, 12:02 PM

Rising_Dusk

He reminds me of myself when I first came here.
After awhile, it just gets hard to sit down for an hour and rewrite everything someone does. xD

Also, that code is leaking that boolexpr.

JASS:

function Trig_Juxt_Actions takes nothing returns nothing 
local group g = CreateGroup()
local boolexpr b = Condition(function Juxt_Enemy_Filter)
local unit cast = GetTriggerUnit()
local unit swap 
local real x = GetUnitX(cast)
local real y = GetUnitY(cast)

set udg_Juxt_Caster = cast
call GroupEnumUnitsInRange(g, x, y, 800.00, b)
set swap = FirstOfGroup(g) 
       
 loop 
       set cast = FirstOfGroup(g) 
       exitwhen cast == null
       set x = GetWidgetLife(swap) 
       set y = GetWidgetLife(cast) 
       if  y > x then 
           set swap = cast
       endif
       call GroupRemoveUnit(g,cast)
    endloop  
call BJDebugMsg("Loop2")
call RemoveUnit(swap)

set cast = null
set swap = null
call DestroyGroup(g)
call DestroyBoolExpr(b)
set b = null
set g = null
endfunction

03-09-2007, 05:06 PM	#7
Pyrogasm	I thought BoolExprs only leaked if you used a variable.

03-09-2007, 08:26 PM	#8
Rising_Dusk	They function like any handle that needs to be removed/destroyed. If what you said were true, then we could do Location(x, y) and it would not leak the location. That is not the case though, and that location would need to be destroyed whenever used. This is the same situation with boolexprs.