[Trigger] Ideas on how to Improve / Leak Management

[WatashiwaleBaer]

Hey Guys - I just finished my first 2 Heroes I concept on my own.

My Question is now - Have you any Ideas on how to improve the Triggers (Less Triggers with same result for example)

And are there any Memory Leaks?

Can you locate the error why Aizen turns his Vector Coloring to 100% Transparency out of random sometimes?

And is the Ammunition System I used for Sion stable?

Thanks in advance

 

[Meticulous]

Just looked over it quickly, but here are some general problems I spotted.

Stuff

• Level Up System
  • Events
    • Player - Player 1 (Red) skips a cinematic sequence
  • Conditions
  • Actions
    • Unit Group - Pick every unit in (Units currently selected by Player 1 (Red)) and do (Actions)
      • Loop - Actions
        • Hero - Set (Picked unit) Hero-level to ((Hero level of (Picked unit)) + 1), Show level-up graphics
        • Unit - Reset ability cooldowns for (Picked unit)
        • Unit - Set mana of (Picked unit) to 100.00%
        • Unit - Set life of (Picked unit) to 100.00%

The unit group leaks, though I guess this is just a debug trigger so it doesn't really matter.

• Unit - Move ShunpoStrikeInvisDummy instantly to (Position of ShunpoStrikeEnemy)

Location leak.

• EtherliteLevelUp
  • Events
    • Unit - A unit Gains a level
  • Conditions
    • (Unit-type of (Triggering unit)) Equal to Daughter of the Eltnam Family
  • Actions
    • -------- Level up "Etherlite" every 10 levels --------
    • If ((Real((Level of (Triggering unit)))) Equal to 10.00) then do (Unit - Increase level of Etherlite for (Triggering unit)) else do (Do nothing)
    • If ((Real((Level of (Triggering unit)))) Equal to 20.00) then do (Unit - Increase level of Etherlite for (Triggering unit)) else do (Do nothing)
    • If ((Real((Level of (Triggering unit)))) Equal to 30.00) then do (Unit - Increase level of Etherlite for (Triggering unit)) else do (Do nothing)
    • If ((Real((Level of (Triggering unit)))) Equal to 40.00) then do (Unit - Increase level of Etherlite for (Triggering unit)) else do (Do nothing)
    • If ((Real((Level of (Triggering unit)))) Equal to 50.00) then do (Unit - Increase level of Etherlite for (Triggering unit)) else do (Do nothing)

Triggers like this are very inefficient for a couple reasons,

1. Every time you see something in (brackets), that's a function call. The more you can minimize function calls the more efficient your trigger will be. So, for example, instead of getting ((Real((Level of (Triggering unit)))) every time, you can just set a temporary real variable so you only have to call those 4 functions once.

2. Running a bunch of separate single-function if/then/else like this in a row causes every one of them to run whenever the trigger is executed, whereas if you nested multi-function if/then/else blocks in eachother (e.g. if the unit isn't level 10, then check if it's level 20, otherwise level the ability up), the later would only run if the former came up false. Also, I'm pretty sure (Do nothing) is an absolutely meaningless function call which you can avoid using with the multi-function if/then/else. There's really no reason to ever use a single-function.

 

[WatashiwaleBaer]

Just looked over it quickly, but here are some general problems I spotted.

Stuff

• Level Up System
  • Events
    • Player - Player 1 (Red) skips a cinematic sequence
  • Conditions
  • Actions
    • Unit Group - Pick every unit in (Units currently selected by Player 1 (Red)) and do (Actions)
      • Loop - Actions
        • Hero - Set (Picked unit) Hero-level to ((Hero level of (Picked unit)) + 1), Show level-up graphics
        • Unit - Reset ability cooldowns for (Picked unit)
        • Unit - Set mana of (Picked unit) to 100.00%
        • Unit - Set life of (Picked unit) to 100.00%

The unit group leaks, though I guess this is just a debug trigger so it doesn't really matter.

• Unit - Move ShunpoStrikeInvisDummy instantly to (Position of ShunpoStrikeEnemy)

Location leak.

• EtherliteLevelUp
  • Events
    • Unit - A unit Gains a level
  • Conditions
    • (Unit-type of (Triggering unit)) Equal to Daughter of the Eltnam Family
  • Actions
    • -------- Level up "Etherlite" every 10 levels --------
    • If ((Real((Level of (Triggering unit)))) Equal to 10.00) then do (Unit - Increase level of Etherlite for (Triggering unit)) else do (Do nothing)
    • If ((Real((Level of (Triggering unit)))) Equal to 20.00) then do (Unit - Increase level of Etherlite for (Triggering unit)) else do (Do nothing)
    • If ((Real((Level of (Triggering unit)))) Equal to 30.00) then do (Unit - Increase level of Etherlite for (Triggering unit)) else do (Do nothing)
    • If ((Real((Level of (Triggering unit)))) Equal to 40.00) then do (Unit - Increase level of Etherlite for (Triggering unit)) else do (Do nothing)
    • If ((Real((Level of (Triggering unit)))) Equal to 50.00) then do (Unit - Increase level of Etherlite for (Triggering unit)) else do (Do nothing)

Triggers like this are very inefficient for a couple reasons,

1. Every time you see something in (brackets), that's a function call. The more you can minimize function calls the more efficient your trigger will be. So, for example, instead of getting ((Real((Level of (Triggering unit)))) every time, you can just set a temporary real variable so you only have to call those 4 functions once.

2. Running a bunch of separate single-function if/then/else like this in a row causes every one of them to run whenever the trigger is executed, whereas if you nested multi-function if/then/else blocks in eachother (e.g. if the unit isn't level 10, then check if it's level 20, otherwise level the ability up), the later would only run if the former came up false. Also, I'm pretty sure (Do nothing) is an absolutely meaningless function call which you can avoid using with the multi-function if/then/else. There's really no reason to ever use a single-function.

Though I don´t store the Position of the ShunpoStrike Caster into a variable I thought it´s not needed to remove it cause it´s refreshed every time you use it

And for the Level-Up System for Etherlite - I honestly don´t know how I can make it better

I´m a noob JASSer so if there are any solves via GUI I would be very happy

 

[Meticulous]

Any time you overwrite a location or unit group without destroying it, the previous one still exists in memory even if it'll never be referenced again. That's why it's called a memory leak. Hence, you store it just so you can destroy it afterwards.

The level up trigger can be optimized pretty easily.

trigger

• Set Integer = (Level of (Triggering unit))
• If (All Conditions are True) then do (Then Actions) else do (Else Actions)
  • If - Conditions
    • Or - Any (Conditions) are true
      • Conditions
        • Integer equal to 10
        • Integer equal to 20
        • Integer equal to 30
        • Integer equal to 40
        • Integer equal to 50
  • Then - Actions
    • Unit - Increase level of Etherlite for Unit
  • Else - Actions

The "If/Then/Else, Multiple Functions" trigger is functionally identical to a normal If/Then/Else except it's easier to work with and doesn't require you to fill in the "Else" section with a meaningless (Do Nothing) call.

 

[deathismyfriend]

Here are some things that leak.

• Hero - Instantly revive (Dying unit) at (Player 1 (Red) start location), Show revival graphics
• // Set the below unit group into a unit group or use the custom script i added.
• Custom script: set bj_wantDestroyGroup = true
• Unit Group - Pick every unit in (Units currently selected by Player 1 (Red)) and do (Actions)
  • Loop - Actions
• // A lot of your spells have location leaks like this.
• // The one below leaks two locations.
• Unit - Create 1 ExplosiveBarrageDummy for Player 1 (Red) at (Center of ExplosiveBarrageAoE) facing (Position of (Triggering unit))
• // This is also in the same spell and leaks a location.
• Set ExplosiveBarrageAoE = (Region centered at (Target point of ability being cast) with size (690.00, 690.00))
• // This leaks a unit group.
• If ((ExplosiveBarrageCaster has an item of type Magazine Clip) Equal to True) then do (Unit Group - Pick every unit in (Units in ExplosiveBarrageAoE matching (((Matching unit) belongs to an enemy of (Owner of ExplosiveBarrageCaster)) Equal to True)) and do (Unit - Cause ExplosiveBarrageCaster to damage (Picked unit), dealing ((Real((Level of E else do (Unit Group - Pick every unit in (Units in ExplosiveBarrageAoE matching (((Matching unit) belongs to an enemy of (Owner of ExplosiveBarrageCaster)) Equal to True)) and do (Unit - Cause ExplosiveBarrageCaster to damage (Picked unit), dealing ((Real((Level of E
• // This leaks three locations.
• Unit - Create 1 ExplosiveBarrageEffectDummy for (Owner of ExplosiveBarrageCaster) at ((Center of ExplosiveBarrageAoE) offset by 175.00 towards ExplosiveBarrageDummyPosition degrees) facing (Position of (Triggering unit))
• // You set these to a location but you never remove the locations that you have set so these also leak.
• Set BurstFireCasterPosition = (Position of BurstFireCaster)
• Set BurstFireEnemyPosition = (Position of BurstFireEnemy)
• // This leaks a location also.
• Unit - Create 1 LastArcBarrelReplicaEffectDummy for (Owner of LastArcCaster) at (LastArcCasterPosition offset by 10.00 towards LastArcAngle degrees) facing LastArcTarget
• // This has four location leaks.
• Unit - Move KyoukaSuigetsuCaster instantly to ((Position of KyoukaSuigetsuEnemy) offset by 150.00 towards (Angle from (Position of KyoukaSuigetsuCaster) to (Position of KyoukaSuigetsuEnemy)) degrees)

You should not use waits. ( especially in spells.) Look at my tutorial things a guier should know for more info on waits. ( link is in my sig.)
Use triggering unit instead of casting unit in all triggers with casting unit.
Don't use integer A / B. They are slower and less efficient than using your own custom variable. They can also cause bugs.
Your spells are also not MUI. That means that when more than one unit cast that spell at the same time or before the other spell is done being casted that it will bug. Look at the chapter how to index in my tutorial things a guier should know on how to make MUI spells.
Most of your triggers leak locations / unit groups like I showed above.