[Trigger] Are there leaks?

[Gumberbunbder]

I've been working on my map for a while now and its nearly done, but I honestly am not the best triggerer. I created several new triggers and compiled the into this one trigger because i though merging them would help reduce lag. Is there a better way to do this and are there leaks? Thanks for your time!

• Time Related Abilities
  • Events
    • Time - Every 3.00 seconds of game time
  • Conditions
  • Actions
    • Set tempUnitGroup = (Units in (Playable map area) matching (((Unit-type of (Matching unit)) Equal to Beorning |c00FFFF00(Elite)|r (Elves)) and (((Matching unit) is alive) Equal to True)))
    • Unit Group - Pick every unit in tempUnitGroup and do (Actions)
      • Loop - Actions
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • ((Picked unit) has buff Night (Night Aura)) Equal to True
          • Then - Actions
            • Unit - Set mana of (Picked unit) to 100.00%
            • Unit - Order (Picked unit) to Night Elf Druid Of The Claw - Bear Form
          • Else - Actions
    • Custom script: call DestroyGroup (udg_tempUnitGroup)
    • Set tempUnitGroup = (Units in (Playable map area) matching (((Unit-type of (Matching unit)) Equal to Beorning |c00FFFF00(Elite)|r (Bear Form)(Elves)) and (((Matching unit) is alive) Equal to True)))
    • Unit Group - Pick every unit in tempUnitGroup and do (Actions)
      • Loop - Actions
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • ((Picked unit) has buff Night (Night Aura)) Equal to False
          • Then - Actions
            • Unit - Set mana of (Picked unit) to 100.00%
            • Unit - Order (Picked unit) to Night Elf Druid Of The Claw - Night Elf Form
          • Else - Actions
    • Custom script: call DestroyGroup (udg_tempUnitGroup)
    • Set tempUnitGroup = (Units in (Playable map area) matching (((Unit-type of (Matching unit)) Equal to Ithilien Ranger |c00FFFF00(Elite)|r (Gondor)) and (((Matching unit) is alive) Equal to True)))
    • Unit Group - Pick every unit in tempUnitGroup and do (Actions)
      • Loop - Actions
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • ((Picked unit) has buff Night (Night Aura)) Equal to True
          • Then - Actions
            • Unit - Add Permanent Invisibility (Ithilien Ranger) to (Picked unit)
          • Else - Actions
    • Custom script: call DestroyGroup (udg_tempUnitGroup)
    • Set tempUnitGroup = (Units in (Playable map area) matching (((Unit-type of (Matching unit)) Equal to Ithilien Ranger |c00FFFF00(Elite)|r (Gondor)) and (((Matching unit) is alive) Equal to True)))
    • Unit Group - Pick every unit in tempUnitGroup and do (Actions)
      • Loop - Actions
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • ((Picked unit) has buff Night (Night Aura)) Equal to False
          • Then - Actions
            • Unit - Remove Permanent Invisibility (Ithilien Ranger) from (Picked unit)
          • Else - Actions
    • Custom script: call DestroyGroup (udg_tempUnitGroup)
    • Set tempUnitGroup = (Units in (Playable map area) matching (((Unit-type of (Matching unit)) Equal to Sindarin Ranger |c00FFFF00(Elite)|r (Elves)) and (((Matching unit) is alive) Equal to True)))
    • Unit Group - Pick every unit in tempUnitGroup and do (Actions)
      • Loop - Actions
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • ((Picked unit) has buff Forest (Forest Aura)) Equal to True
          • Then - Actions
            • Unit - Add Ghost to (Picked unit)
          • Else - Actions
            • Unit - Remove Ghost from (Picked unit)
    • Custom script: call DestroyGroup (udg_tempUnitGroup)
    • Set tempUnitGroup = (Units in (Playable map area) matching ((Unit-type of (Matching unit)) Equal to Siege Tower (All)(Immobile)))
    • Unit Group - Pick every unit in tempUnitGroup and do (Actions)
      • Loop - Actions
        • Set tempPoint = (Position of (Picked unit))
        • Unit - Order (Picked unit) to Unload All At (tempPoint offset by 500.00 towards (Facing of (Picked unit)) degrees)
    • Custom script: call RemoveLocation(udg_tempPoint)
    • Custom script: call DestroyGroup (udg_tempUnitGroup)
    • Set tempUnitGroup = (Units in (Playable map area) matching ((Unit-type of (Matching unit)) Equal to Postern Gate (All)))
    • Unit Group - Pick every unit in tempUnitGroup and do (Actions)
      • Loop - Actions
        • Set tempPoint = (Position of (Picked unit))
        • Unit - Order (Picked unit) to Unload All At (tempPoint offset by 500.00 towards 270.00 degrees)
    • Custom script: call RemoveLocation(udg_tempPoint)
    • Custom script: call DestroyGroup (udg_tempUnitGroup)

 

[IcemanBo]

Yes, you leak locations in your last two group enumerations.
You create a location each time a unit is picked, but only destroy
one location after enumeration ends. You should destroy inside the enumeration.

Furthermore you leak insde your last two group enumerations because you do:
" (tempPoint offset by 500.00 towards 270.00 degrees)",
which creates an other location.
You need help of an other location variable to prevent this leak.

Finaly it's not to prevent leaks, but to code more efficient - better
only PickAllUnitsInMap only once, and the use multiple
"If - statements" inside it. Currently you pick AllUnitsInMap very often,
which is kind of useless.

 

[KILLCIDE]

If your unit group function only picks one unit, then no, you do not leak (; (mostly):

Unit - Order (Picked unit) to Unload All At (tempPoint offset by 500.00 towards (Facing of (Picked unit)) degrees)

tempPoint offset by X creates a new location, so you will need to remove it.

When your unit group function does end up picking more than 1 unit, then you're only destroying the most recent iteration of tempPoint.

Set tempUnitGroup = (Units in (Playable map area) matching ((Unit-type of (Matching unit)) Equal to Siege Tower (All)(Immobile))) Unit Group - Pick every unit in tempUnitGroup and do (Actions) Loop - Actions Set tempPoint = (Position of (Picked unit)) Set tempPoint2 = (Position of (Picked unit)) Unit - Order (Picked unit) to Unload All At (tempPoint offset by 500.00 towards (Facing of (Picked unit)) degrees) Custom script: call RemoveLocation(udg_tempPoint) Custom script: call DestroyGroup (udg_tempUnitGroup) Set tempUnitGroup = (Units in (Playable map area) matching ((Unit-type of (Matching unit)) Equal to Postern Gate (All))) Unit Group - Pick every unit in tempUnitGroup and do (Actions) Loop - Actions Set tempPoint = (Position of (Picked unit)) Unit - Order (Picked unit) to Unload All At (tempPoint offset by 500.00 towards 270.00 degrees) Custom script: call RemoveLocation(udg_tempPoint) Custom script: call DestroyGroup (udg_tempUnitGroup)

Move the custom script INSIDE the unit group loop, as well as removing the new location you create with polar offset, and you will have no leaks.

Set tempUnitGroup = (Units in (Playable map area) matching ((Unit-type of (Matching unit)) Equal to Siege Tower (All)(Immobile))) Unit Group - Pick every unit in tempUnitGroup and do (Actions) Loop - Actions Set tempPoint = (Position of (Picked unit)) Unit - Order (Picked unit) to Unload All At (tempPoint offset by 500.00 towards (Facing of (Picked unit)) degrees) Custom script: call RemoveLocation(udg_tempPoint) Custom script: call DestroyGroup (udg_tempUnitGroup) Set tempUnitGroup = (Units in (Playable map area) matching ((Unit-type of (Matching unit)) Equal to Postern Gate (All))) Unit Group - Pick every unit in tempUnitGroup and do (Actions) Loop - Actions Set tempPoint = (Position of (Picked unit)) Set tempPoint2 = (tempPoint offset by 500.00 towards (Facing of (Picked unit)) degrees) Unit - Order (Picked unit) to Unload All At tempPoint2 Custom script: call RemoveLocation(udg_tempPoint) Custom script: call RemoveLocation(udg_tempPoint2) Custom script: call DestroyGroup (udg_tempUnitGroup)

On a side note I recommend to not use (Matching unit) to filter out your units. It looks ugly, makes it hard to read, and is a pain in the ass if you wanted to add more filters.

This is an extremely better way:

Custom script: set bj_wantDestroyGroup = true Unit Group - Pick every unit in (Units within range of location) and do (Actions) Loop - Actions Set PickedUnit = (Picked unit) If (All Conditions are True) then do (Then Actions) else do (Else Actions) If - Conditions (PickedUnit is alive) Equal to True (PickedUnit is Magic Immune) Equal to False (PickedUnit is A structure) Equal to False -------- you can add as much filters as you want -------- Then - Actions -------- run actions -------- Else - Actions -------- PickedUnit did not pass filters, do nothing--------

I use bj_wantDestroyGroup because if you are destroying the group right after its creation, there is no need to store it to a variable. I also store (Picked Unit) into a variable because it is slightly more efficient then having to refer to the function call all the time

 

[Gumberbunbder]

Wow you gys responded very quickly! Thanks! I'm going to move the custom scripts inside the unit group loop and also take care of that extra point reference.

I didn't know that making a variable for picked unit is more efficient, i'll do that.

And also, i have no idea about custom scripts except removing group etc. So the set bj_wantDestroyGroup = true thing, i don't know what that is actually doing. something to do with destroying group i see, but before rather than after.

 

[IcemanBo]

Using (Picked Unit) is a function call.
Using a variable is no function call.

Function calls are slow, and so if you use (Picked Unit) more often (like more than 2 times),
it is used to store it in a variable and therefor use the variable instead.

If you set "bj_wantDestroyGroup" boolean = true via custom script, it will automatically destroy the next unit group creation, that is done via GUI.
(you can read the tutorial about memory leaks in my signature)

 

[KILLCIDE]

I didn't know that making a variable for picked unit is more efficient, i'll do that.

Every set of parantheses is a function call. The more parantheses you have inside other parantheses, the slower it will be Since you are using a unit group loop, storing the unit into a variable saves you a ton of function calls every iteration.

And also, i have no idea about custom scripts except removing group etc. So the set bj_wantDestroyGroup = true thing, i don't know what that is actually doing. something to do with destroying group i see, but before rather than after.

Take a look at this thread

EDIT: Sigh.. IcemanBo beat me to it again.

 

[Gumberbunbder]

Thanks a lot guys! Lol beaten again eh? I'm going to post this trigger again once I'm done fixing it entirely and you can see if it looks right. I'm going to do all the things you guys said

Edit: This look good?

• Time Related Abilities
  • Events
    • Time - Every 3.00 seconds of game time
  • Conditions
  • Actions
    • Custom script: set bj_wantDestroyGroup = true
    • Unit Group - Pick every unit in (Units in (Playable map area)) and do (Actions)
      • Loop - Actions
        • Set PickedUnit = (Picked unit)
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • And - All (Conditions) are true
              • Conditions
                • (Unit-type of PickedUnit) Equal to Beorning |c00FFFF00(Elite)|r (Elves)
                • (PickedUnit has buff Night (Night Aura)) Equal to True
          • Then - Actions
            • Unit - Set mana of PickedUnit to 100.00%
            • Unit - Order PickedUnit to Night Elf Druid Of The Claw - Bear Form
          • Else - Actions
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • And - All (Conditions) are true
              • Conditions
                • (Unit-type of PickedUnit) Equal to Beorning |c00FFFF00(Elite)|r (Bear Form)(Elves)
                • (PickedUnit has buff Night (Night Aura)) Equal to False
          • Then - Actions
            • Unit - Set mana of PickedUnit to 100.00%
            • Unit - Order PickedUnit to Night Elf Druid Of The Claw - Night Elf Form
          • Else - Actions
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • And - All (Conditions) are true
              • Conditions
                • (Unit-type of PickedUnit) Equal to Ithilien Ranger |c00FFFF00(Elite)|r (Gondor)
                • (PickedUnit has buff Night (Night Aura)) Equal to True
          • Then - Actions
            • Unit - Add Permanent Invisibility (Ithilien Ranger) to PickedUnit
          • Else - Actions
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • And - All (Conditions) are true
              • Conditions
                • (Unit-type of PickedUnit) Equal to Ithilien Ranger |c00FFFF00(Elite)|r (Gondor)
                • (PickedUnit has buff Night (Night Aura)) Equal to False
          • Then - Actions
            • Unit - Remove Permanent Invisibility (Ithilien Ranger) from PickedUnit
          • Else - Actions
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • And - All (Conditions) are true
              • Conditions
                • (Unit-type of PickedUnit) Equal to Sindarin Ranger |c00FFFF00(Elite)|r (Elves)
                • (PickedUnit has buff Forest (Forest Aura)) Equal to True
          • Then - Actions
            • Unit - Add Ghost to PickedUnit
          • Else - Actions
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • And - All (Conditions) are true
              • Conditions
                • (Unit-type of PickedUnit) Equal to Sindarin Ranger |c00FFFF00(Elite)|r (Elves)
                • (PickedUnit has buff Forest (Forest Aura)) Equal to False
          • Then - Actions
            • Unit - Remove Ghost from PickedUnit
          • Else - Actions
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • And - All (Conditions) are true
              • Conditions
                • (Unit-type of PickedUnit) Equal to Siege Tower (All)(Immobile)
          • Then - Actions
            • Set tempPoint = (Position of PickedUnit)
            • Set tempPoint2 = (tempPoint offset by 500.00 towards (Facing of PickedUnit) degrees)
            • Unit - Order (Picked unit) to Unload All At tempPoint2
            • Custom script: call RemoveLocation(udg_tempPoint)
            • Custom script: call RemoveLocation(udg_tempPoint2)
          • Else - Actions
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • And - All (Conditions) are true
              • Conditions
                • (Unit-type of PickedUnit) Equal to Postern Gate (All)
          • Then - Actions
            • Set tempPoint = (Position of PickedUnit)
            • Set tempPoint2 = (tempPoint offset by 500.00 towards 270.00 degrees)
            • Unit - Order (Picked unit) to Unload All At tempPoint2
            • Custom script: call RemoveLocation(udg_tempPoint)
            • Custom script: call RemoveLocation(udg_tempPoint2)
          • Else - Actions

 

[KILLCIDE]

The default If - Conditions is treated as "And - All (Conditions) are true", so there is no need to add that extra function. Also, make sure to not use (Picked unit) if you already have it saved to a variable. You still use the function call at a few places. My only gripe is the extreme amount of If/Then/Elses you have in a 3 second periodic loop. May I ask what the purpose of this loop is so I can optimize it?

 

[Gumberbunbder]

There are several units that this trigger involves. The first one is the Beorning unit which becomes a bear during the night and regular form during the day. I made a unit that spawns during the night with a map wide aura. This trigger checks when the Beorning gets the buff from the night aura and then causes it to transform.

Then theres the Ithilien Ranger which has the same function, becomes invisible at night. The trigger checks that they have the night buff as well.

Theres also the Sindarin Ranger which becomes invisible near trees. When they get near trees they get a buff which this trigger checks for.

The last is for siege towers to make sure units are unloaded automatically.

I fixed those other "picked unit" functions

Edit:
I used to have a separate trigger for each unit, but i merged them because i thought that was more efficient.

 

[IcemanBo]

" And - All (Conditions) are true" is not needed here.
If you simply add conditions without them it is exactly the same result.

The only case "And - All (Conditions) are true" may be useful,
is insde a " Or - Any (Conditions) are true" block.