[Trigger] Cleaning Up Triggers

[Rufus]

Hi!
My project have a custom movement system and a few other player controlled things.
I would like some help to check through my triggers, find leaks if there are any, and also construct more reliable and efficent triggers.

As they are now, they work. Although, they sometimes feel sluggish and slow, and sometimes, animations aren't playing properly.

Everything under the "Character" category in Tropic Tomb can be updated.


Is there anyone that are up to the task?

 

[KILLCIDE]

Only checking for leaks:

Attack

• This leaks two locations:
  
  • Set Pos1 = ((Position of Char) offset by 70.00 towards (Facing of Char) degrees)
  You need to store (Position of Char) into a variable + use RemoveLocation on Pos1

Knife

• Pos1 leaks
  
• This leaks a unit group
  
  • (Number of units in (Units within 80.00 of Pos2 matching (((Owner of (Matching unit)) Not equal to Player 1 (Red)) and (((Matching unit) is alive) Equal to True)))) Greater than 0

Hookshot Missile

• This leaks a unit group
  
  • (Number of units in (Units within 80.00 of Pos3 matching ((Owner of (Matching unit)) Not equal to Player 1 (Red)))) Greater than 0

Projection

• This leaks a special effect
  
  • Special Effect - Create a special effect attached to the origin of (Last created unit) using war3mapImported\BlueBanishTarget.mdx

Health Bar Update

• You should destroy any floating texts you create

This was a fast read, so I may have missed a lot. An issue I saw is that you cleared locations immediately after use, which is okay, but is useless if you are going to reassign that variable to the same exact location. Leaks will only occur if you overwrite the variable with something else, thus losing reference to that specific object in memory. Just because you use something that leaks, does not mean it leaks.

 

[Dr Super Good]

This was a fast read, so I may have missed a lot. An issue I saw is that you cleared locations immediately after use, which is okay, but is useless if you are going to reassign that variable to the same exact location. Leaks will only occur if you overwrite the variable with something else, thus losing reference to that specific object in memory. Just because you use something that leaks, does not mean it leaks.

???

Leaks (which can be controlled) only occur when no clean up occurs at object (or variable) end of life. Like a plastic bag will only pollute the environment if it is dropped on the ground, so as long as it gets thrown away responsibly at the end of its life it can be used for any number of purposes over any length of time without polluting the environment.

 

[KILLCIDE]

That's literally exactly what I said but reworded with a plastic bag example. I mean I guess it was a bit misleading for me to say that leaks ONLY occur if they are overwritten. My mistake, but I don't understand how that throws off the entire paragraph lol.

 

[Rufus]

Thanks a lot for taking your time, Killcide!

Just to be clear: it is better to remove the floating text and create a new one at every trigger repeat?

 

[Dr Super Good]

Just because you use something that leaks, does not mean it leaks.

It does mean it does leak. Something that leaks will always leak because it will not be called something that leaks if it did not leak. For example units and destructables leak and they always leak because no matter what is done the game leaks memory when they are removed. Sure it is unlikely to ever run out of memory as a result due to how small the amount is (hundreds of thousands of units would need to be created and removed) but it still leaks memory.

A location on the other hand can leak but does not have to. If one loses all references to it before clean-up then it will leak. However if one does clean it up then no leak occurs.

Just to be clear: it is better to remove the floating text and create a new one at every trigger repeat?

Recycle objects when possible. It is far easier for the game engine to change a few properties than make a new object from scratch and then set up all properties. Exception goes for objects which do not need multiple configuration function calls, such as groups and locations, where it is faster to make new ones with a single JASS function call than run a custom JASS function to recycle them due to the inefficiencies of the JASS language.

 

[KILLCIDE]

I believe it would be better to just create it once and then just move / change the text when you have to. I don't play around with Floating Texts that often, so I'm not entirely sure.

A location on the other hand can leak but does not have to. If one loses all references to it before clean-up then it will leak. However if one does clean it up then no leak occurs.

^ This was what I was talking about. I wasn't talking about leaks in general, I was referring to the way he cleaned up location leaks and immediately resassigned them to the same object.

 

[Rufus]

Just bumpping in case someone can help out further.

 

[pOke]

Only did a very brief skim, but on an efficiency front, you can improve somethings.

If you are going to be using Triggering Unit, Picked Unit, etc, more than once you should save it to a variable and use it.

Abilities that you want to have multiple instances of like knife, could be made less taxing, by writing the code in such a way where you're not picking every unit in the entire map 25 times a second. Also, since Knife Missile never turns off, even when there are no knife instances running, it is still using resources in the background.
I recommend you read how you could accomplish this here: Visualize: Dynamic Indexing

You have a few other triggers that also pick units of a type in the entire map. Probably best to just make a unit group that holds those objects and just iterate through that group checking their status rather than picking the whole map.

You also have quite a few conditions that are similar to what KILLCIDE showed above where you're leaking a group. Whenever you have a condition that checks "number of units in a group", it has to count that for you by making it own group (that doesn't get destroyed, therefore leaking). Instead you should pick the units in the group yourself and increment a counter if you need to know the total. In most cases it appears you just wish to cause damage, so rather than checking to see if its empty, if not deal damage, just deal the damage.

Also, I would recommend filtering in this way, rather having the code picking the units and filtering them for you, this way will let you pick everything, filter out with conditions, and allow for potentially increased efficiency through less function calls.
Convenient Unit Group Filtering in GUI

As it relates to the Move system, there is no doubt that there is a lot of code duplication. When possible be sure that when you are writing something, if it is in both the "then" and "else" you likely are able to just put it outside the if since it's happening regardless.

 

[IcemanBo]

It's cleaner to write using property events instead of periodicly comparing the player's "Food Used" or so.

• KnifeMissile
  • Events
    • Time - Every 0.04 seconds of game time
  • Conditions
  • Actions
    • Set UnitGroup = (Units in (Playable map area) matching ((Unit-type of (Matching unit)) Equal to Projectile Knife))
    • Unit Group - Pick every unit in UnitGroup and do (Actions)
      • Loop - Actions
        • Set Pos1 = (Position of (Picked unit))
        • Set Pos2 = (Pos1 offset by 20.00 towards (Facing of Char) degrees)
        • Set Pos3 = (Position of Char)
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • ((Picked unit) is alive) Equal to True
          • Then - Actions
            • Unit - Move (Picked unit) instantly to Pos2, facing (Facing of Char) degrees
            • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
              • If - Conditions
                • (Number of units in (Units within 80.00 of Pos2 matching (((Owner of (Matching unit)) Not equal to Player 1 (Red)) and (((Matching unit) is alive) Equal to True)))) Greater than 0
              • Then - Actions
                • Unit - Kill (Picked unit)
                • Set UnitGroup = (Units within 80.00 of Pos2 matching ((Owner of (Matching unit)) Not equal to Player 1 (Red)))
                • Unit Group - Pick every unit in UnitGroup and do (Actions)
                  • Loop - Actions
                    • Special Effect - Create a special effect attached to the head of (Picked unit) using Abilities\Spells\Undead\AbsorbMana\AbsorbManaBirthMissile.mdl
                    • Special Effect - Destroy (Last created special effect)
                    • Unit - Cause Char to damage (Picked unit), dealing (CharDamage / 2.00) damage of attack type Spells and damage type Normal
                • Custom script: call DestroyGroup (udg_UnitGroup)
              • Else - Actions
          • Else - Actions
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • (Distance between Pos3 and Pos2) Greater than 1000.00
          • Then - Actions
            • Unit - Remove (Picked unit) from the game
          • Else - Actions
        • Custom script: call RemoveLocation (udg_Pos1)
        • Custom script: call RemoveLocation (udg_Pos2)
        • Custom script: call RemoveLocation (udg_Pos3)
    • Custom script: call DestroyGroup (udg_UnitGroup)

^Very heavy operations. Picking All units In map and internaly again making 2 group creatin + enumeration for each alive unit on map seems costly.
This:

• (Number of units in (Units within 80.00 of Pos2 matching (((Owner of (Matching unit)) Not equal to Player 1 (Red)) and (((Matching unit) is alive) Equal to True)))) Greater than 0

... this line creates a unit group and leaks. (and is unreadable as a 1-liner) And it is used also in other triggers, btw.

Also the location of "Char" is static in the trigger and doesn't need to be reset on each roll.

And filtering the unit for being alive is better before points are created. The filter should be on very top.

• Hookshot Init
  • Events
    • Time - Elapsed game time is 0.00 seconds
  • Conditions
  • Actions
    • Set Pos1 = (Center of (Playable map area))
    • Set Pos2 = (Center of (Playable map area))
    • Lightning - Create a Magic Leash lightning effect from source Pos1 to target Pos2
    • Set HookLightning = (Last created lightning effect)
    • Lightning - Change color of (Last created lightning effect) to (0.70 0.80 1.00) with 1.00 alpha
    • Custom script: call RemoveLocation (udg_Pos1)
    • Custom script: call RemoveLocation (udg_Pos2)

^2 different locations are not needed here. Also you might consider to use a permanent Point "CenterOfMap" which is just never destroyed and used over and over again.

• If (All Conditions are True) then do (Then Actions) else do (Else Actions)
  • If - Conditions
    • Or - Any (Conditions) are true
      • Conditions
        • MoveDown Equal to True
        • MoveRight Equal to True
        • MoveUp Equal to True
        • MoveLeft Equal to True
  • Then - Actions
    • Custom script: call SetUnitAnimationByIndex(udg_Char, 2)
  • Else - Actions

^This part is repeated very often. I think this task should be handled internaly be the move system if possible.

The custom movement trigger:

• Move
  • Events
    • Time - Every 0.02 seconds of game time
  • Conditions
    • MovementDisabled Equal to False
    • CharDead Equal to False
    • (Char has buff Stunned (Pause)) Equal to False
    • Or - Any (Conditions) are true
      • Conditions
        • MoveDown Equal to True
        • MoveRight Equal to True
        • MoveLeft Equal to True
        • MoveUp Equal to True
  • Actions
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • CurrentlyAttacking Equal to False
      • Then - Actions
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • And - All (Conditions) are true
              • Conditions
                • (Char has buff Dash ) Equal to True
                • (Char has buff StopCharDash ) Equal to False
          • Then - Actions
            • Set StatMovementReal = 30.00
            • Custom script: call SetUnitAnimationByIndex(udg_Char, 2)
          • Else - Actions
            • Set StatMovementReal = (StatMovement x 0.02)
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • (Char has buff Slowed) Equal to True
          • Then - Actions
            • Set StatMovementReal = (StatMovementReal / 2.00)
          • Else - Actions
      • Else - Actions
        • Set StatMovementReal = 1.00
    • Set FacingIndicator = (Position of Char)
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • MoveUp Equal to True
      • Then - Actions
        • Set Pos1 = (Position of Char)
        • Set Pos2 = (Pos1 offset by StatMovementReal towards 90.00 degrees)
        • Custom script: call RemoveLocation (udg_Pos1)
        • Set Pos1 = (Pos2 offset by 32.00 towards 90.00 degrees)
        • Unit - Create 1 Path Checker for Neutral Passive at Pos1 facing Default building facing degrees
        • Set Pos3 = (Position of (Last created unit))
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • Or - Any (Conditions) are true
              • Conditions
                • (Terrain type at Pos1) Equal to UnwalkableTerrain
                • (Distance between Pos3 and Pos1) Greater than or equal to 5.00
          • Then - Actions
            • Unit - Remove (Last created unit) from the game
            • Set Pos2 = (Position of Char)
            • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
              • If - Conditions
                • And - All (Conditions) are true
                  • Conditions
                    • MoveUp Equal to True
                    • MoveLeft Equal to False
                    • MoveRight Equal to False
              • Then - Actions
                • Unit - Move Char instantly to Pos2, facing 90.00 degrees
              • Else - Actions
            • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
              • If - Conditions
                • And - All (Conditions) are true
                  • Conditions
                    • MoveLeft Equal to True
                    • MoveUp Equal to True
              • Then - Actions
                • Unit - Move Char instantly to Pos2, facing 135.00 degrees
              • Else - Actions
            • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
              • If - Conditions
                • And - All (Conditions) are true
                  • Conditions
                    • MoveRight Equal to True
                    • MoveUp Equal to True
              • Then - Actions
                • Unit - Move Char instantly to Pos2, facing 45.00 degrees
              • Else - Actions
            • Custom script: call RemoveLocation (udg_Pos1)
            • Custom script: call RemoveLocation (udg_Pos2)
          • Else - Actions
            • Unit - Remove (Last created unit) from the game
            • Unit - Move Char instantly to Pos2, facing (Angle from FacingIndicator to Pos2) degrees
            • Custom script: call RemoveLocation (udg_Pos1)
            • Custom script: call RemoveLocation (udg_Pos2)
      • Else - Actions
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • MoveLeft Equal to True
      • Then - Actions
        • Set Pos1 = (Position of Char)
        • Set Pos2 = (Pos1 offset by StatMovementReal towards 180.00 degrees)
        • Custom script: call RemoveLocation (udg_Pos1)
        • Set Pos1 = (Pos2 offset by 32.00 towards 180.00 degrees)
        • Unit - Create 1 Path Checker for Neutral Passive at Pos1 facing Default building facing degrees
        • Set Pos3 = (Position of (Last created unit))
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • Or - Any (Conditions) are true
              • Conditions
                • (Terrain type at Pos1) Equal to UnwalkableTerrain
                • (Distance between Pos3 and Pos1) Greater than or equal to 5.00
          • Then - Actions
            • Unit - Remove (Last created unit) from the game
            • Set Pos2 = (Position of Char)
            • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
              • If - Conditions
                • And - All (Conditions) are true
                  • Conditions
                    • MoveLeft Equal to True
                    • MoveUp Equal to False
                    • MoveRight Equal to False
              • Then - Actions
                • Unit - Move Char instantly to Pos2, facing 180.00 degrees
              • Else - Actions
            • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
              • If - Conditions
                • And - All (Conditions) are true
                  • Conditions
                    • MoveLeft Equal to True
                    • MoveUp Equal to True
              • Then - Actions
                • Unit - Move Char instantly to Pos2, facing 135.00 degrees
              • Else - Actions
            • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
              • If - Conditions
                • And - All (Conditions) are true
                  • Conditions
                    • MoveLeft Equal to True
                    • MoveDown Equal to True
              • Then - Actions
                • Unit - Move Char instantly to Pos2, facing 225.00 degrees
              • Else - Actions
            • Custom script: call RemoveLocation (udg_Pos1)
            • Custom script: call RemoveLocation (udg_Pos2)
          • Else - Actions
            • Unit - Remove (Last created unit) from the game
            • Unit - Move Char instantly to Pos2, facing (Angle from FacingIndicator to Pos2) degrees
            • Custom script: call RemoveLocation (udg_Pos1)
            • Custom script: call RemoveLocation (udg_Pos2)
      • Else - Actions
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • MoveRight Equal to True
      • Then - Actions
        • Set Pos1 = (Position of Char)
        • Set Pos2 = (Pos1 offset by StatMovementReal towards 0.00 degrees)
        • Custom script: call RemoveLocation (udg_Pos1)
        • Set Pos1 = (Pos2 offset by 32.00 towards 0.00 degrees)
        • Unit - Create 1 Path Checker for Neutral Passive at Pos1 facing Default building facing degrees
        • Set Pos3 = (Position of (Last created unit))
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • Or - Any (Conditions) are true
              • Conditions
                • (Terrain type at Pos1) Equal to UnwalkableTerrain
                • (Distance between Pos3 and Pos1) Greater than or equal to 5.00
          • Then - Actions
            • Unit - Remove (Last created unit) from the game
            • Set Pos2 = (Position of Char)
            • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
              • If - Conditions
                • And - All (Conditions) are true
                  • Conditions
                    • MoveRight Equal to True
                    • MoveDown Equal to False
                    • MoveUp Equal to False
              • Then - Actions
                • Unit - Move Char instantly to Pos2, facing 0.00 degrees
              • Else - Actions
            • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
              • If - Conditions
                • And - All (Conditions) are true
                  • Conditions
                    • MoveRight Equal to True
                    • MoveUp Equal to True
              • Then - Actions
                • Unit - Move Char instantly to Pos2, facing 45.00 degrees
              • Else - Actions
            • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
              • If - Conditions
                • And - All (Conditions) are true
                  • Conditions
                    • MoveRight Equal to True
                    • MoveDown Equal to True
              • Then - Actions
                • Unit - Move Char instantly to Pos2, facing 315.00 degrees
              • Else - Actions
            • Custom script: call RemoveLocation (udg_Pos1)
            • Custom script: call RemoveLocation (udg_Pos2)
          • Else - Actions
            • Unit - Remove (Last created unit) from the game
            • Unit - Move Char instantly to Pos2, facing (Angle from FacingIndicator to Pos2) degrees
            • Custom script: call RemoveLocation (udg_Pos1)
            • Custom script: call RemoveLocation (udg_Pos2)
      • Else - Actions
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • MoveDown Equal to True
      • Then - Actions
        • Set Pos1 = (Position of Char)
        • Set Pos2 = (Pos1 offset by StatMovementReal towards 270.00 degrees)
        • Custom script: call RemoveLocation (udg_Pos1)
        • Set Pos1 = (Pos2 offset by 32.00 towards 270.00 degrees)
        • Unit - Create 1 Path Checker for Neutral Passive at Pos1 facing Default building facing degrees
        • Set Pos3 = (Position of (Last created unit))
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • Or - Any (Conditions) are true
              • Conditions
                • (Terrain type at Pos1) Equal to UnwalkableTerrain
                • (Distance between Pos3 and Pos1) Greater than or equal to 5.00
          • Then - Actions
            • Unit - Remove (Last created unit) from the game
            • Set Pos2 = (Position of Char)
            • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
              • If - Conditions
                • And - All (Conditions) are true
                  • Conditions
                    • MoveDown Equal to True
                    • MoveLeft Equal to False
                    • MoveRight Equal to False
              • Then - Actions
                • Unit - Move Char instantly to Pos2, facing 270.00 degrees
              • Else - Actions
            • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
              • If - Conditions
                • And - All (Conditions) are true
                  • Conditions
                    • MoveDown Equal to True
                    • MoveRight Equal to True
              • Then - Actions
                • Unit - Move Char instantly to Pos2, facing 315.00 degrees
              • Else - Actions
            • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
              • If - Conditions
                • And - All (Conditions) are true
                  • Conditions
                    • MoveDown Equal to True
                    • MoveLeft Equal to True
              • Then - Actions
                • Unit - Move Char instantly to Pos2, facing 225.00 degrees
              • Else - Actions
            • Custom script: call RemoveLocation (udg_Pos1)
            • Custom script: call RemoveLocation (udg_Pos2)
          • Else - Actions
            • Unit - Remove (Last created unit) from the game
            • Unit - Move Char instantly to Pos2, facing (Angle from FacingIndicator to Pos2) degrees
            • Custom script: call RemoveLocation (udg_Pos1)
            • Custom script: call RemoveLocation (udg_Pos2)
      • Else - Actions
    • Custom script: call RemoveLocation (udg_FacingIndicator)

^"DRY concept" : don't repeat yourself. Similar code is written again and again. The code must check for pressed keys once, and then do appropriate actions.
Don't have many seperated action blocks for each pressed key seperatly.

Also, creating so many dummy units to keep checking the path is too costly and unefficient. One dummy that is used over and over again, or even better would be something like this: Check Walkability

You might want to use SetUnitX/Y over MoveUnit, because it doesn't interrupt the current animation.


At those triggers:

• Actions
  • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
    • If - Conditions
      • Or - Any (Conditions) are true
        • Conditions
          • MoveDown Equal to True
          • MoveRight Equal to True
          • MoveLeft Equal to True
    • Then - Actions
      • Set MoveUp = False
    • Else - Actions
      • Unit - Remove Ability: Dash from Char
      • Animation - Reset Char's animation
      • Set MoveUp = False

->

• If (All Conditions are True) then do (Then Actions) else do (Else Actions)
  • If - Conditions
    • MoveDown Equal to False
    • MoveRight Equal to False
    • MoveLeft Equal to False
  • Then - Actions
    • Unit - Remove Ability: Dash from Char
    • Animation - Reset Char's animation
  • Else - Actions
• Set MoveUp = False

Periodic triggers should only run if needed. For example the regen trigger, and the floating text trigger.

Something that use brackets, like (Picked Unit) does mean it is a function call.
Function calls are much slower than variable lookups.
So if something like (Picked Unit) or something similar is used multiple times, it makes sense to store it into a temp variable.

It's not easy to read other's map code and interpret everything, so some things are not easy to state out.
But there are several confusing lines for an person from outside, for example:

• Push Box
  • Events
    • Time - Every 0.02 seconds of game time
  • Conditions
  • Actions
    • Unit Group - Pick every unit in PushedBox and do (Actions)
      • Loop - Actions
        • Set Pos1 = (Position of (Picked unit))
        • Set Pos2 = (Pos1 offset by 5.00 towards (Mana of (Picked unit)) degrees)
        • Unit - Move (Picked unit) instantly to Pos2
        • Custom script: call RemoveLocation (udg_Pos1)
        • Custom script: call RemoveLocation (udg_Pos2)
    • Wait 0.25 seconds
    • Unit Group - Remove all units from PushedBox
    • Trigger - Turn off (This trigger)

I have no idea why (Mana of (Picked unit)) as offset reference for example.

Or also the life check for destructables in range in the interact trigger.

 

[Rufus]

Thanks a lot, both of you!
Skimming your posts through, this seems to be what I was looking for. There is a whole lot of new content for me, but I'll make sure I understand it!

Hey, Iceman! I'd love some more help if you can spare the time!
I've read somewhere that the wait trigger is not always accurate. What is the best way to go around this? I need to know for the attack and stun trigger.

 

[pOke]

Rather than using waits you can either use timers, or you can simulate a timer via a periodic event that increments a counter.

I'd also recommend this guide, as it contains a lot of tips and information that is invaluable for GUI work. Things You Should Know When Using Triggers / GUI

 

[IcemanBo]

MUI Spells Using Artificial Waits that's the good-standard GUI way for accurate time outs.

You may use it for spells how explained, but this approach of course actually works for everything else, too.

How ever, you often don't really want to have 100% accurate results, but just a small timeout, always, when you do "Wait 0.5" seconds or so.
You don't have to care about small inaccuracies often, so using waits for this is totaly fine in GUI and actually a true life saver.

So for most of the stuff I saw I in the triggers I would not make the hustle to create a 2nd trigger for each single wait. Only maybe for very important things, which also maybe last longer.