Critique my Trigger Please

[Gnuoy]

• soul reap research
  • Events
    • Unit - A unit Finishes research
  • Conditions
    • (Researched tech-type) Equal to Soul Reap
  • Actions
    • -------- sfx --- play animation on all demon corpses --------
    • Set VariableSet soul_reap_units = (Units in (Playable map area) matching ((Level of Legion Unit Class (Icon) for (Matching unit)) Equal to 1))
    • Unit Group - Pick every unit in soul_reap_units and do (Actions)
      • Loop - Actions
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • ((Picked unit) is dead) Equal to True
          • Then - Actions
            • Set VariableSet soul_reap_point = (Position of (Picked unit))
            • Special Effect - Create a special effect at soul_reap_point using Abilities\Spells\Undead\DeathPact\DeathPactCaster.mdl
            • -------- clear sfx leaks, point variable, and remove unit from unit group --------
            • Special Effect - Destroy (Last created special effect)
            • Custom script: call RemoveLocation(udg_soul_reap_point)
            • Unit Group - Remove (Picked unit) from soul_reap_units.
          • Else - Actions
            • Unit Group - Remove (Picked unit) from soul_reap_units.
    • -------- increase hero's mana based on how many units they killed --------
    • Set VariableSet soul_reap_player = (Owner of (Researching unit))
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • Or - Any (Conditions) are true
          • Conditions
            • soul_reap_player Equal to Player 1 (Red)
            • soul_reap_player Equal to Player 2 (Blue)
            • soul_reap_player Equal to Player 3 (Teal)
            • soul_reap_player Equal to Player 4 (Purple)
      • Then - Actions
        • Set VariableSet soul_reap_hero = hero_array[(Player number of soul_reap_player)]
      • Else - Actions
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • Or - Any (Conditions) are true
          • Conditions
            • soul_reap_player Equal to Player 13 (Maroon)
            • soul_reap_player Equal to Player 14 (Navy)
            • soul_reap_player Equal to Player 15 (Turquoise)
            • soul_reap_player Equal to Player 16 (Violet)
      • Then - Actions
        • Set VariableSet soul_reap_hero = runner_hero[(Player number of soul_reap_player)]
      • Else - Actions
    • Unit - Set Unit: soul_reap_hero's Real Field: Mana ('umpc') to Value: ((Max mana of soul_reap_hero) + (Real(demon_kills[(Player number of soul_reap_player)])))
    • -------- reset variables and clear unit group --------
    • Set VariableSet soul_reap_hero = No unit
    • Custom script: call DestroyGroup(udg_soul_reap_units)

 

[Pyrogasm]

• Why are you removing units manually from soul_reap_units? There's no purpose to that, since you never do anything else with the group before destroying it. And even if you were, you put the removal line in both then & else, so it should just be outside of the block but still in the group loop since it happens either way.
  
  
• soul_reap_player can be assigned to Triggering Player for any PlayerUnitEvents, where it will return the same value as Owner of (Triggering Unit). It's not really an important optimization bu interesting to know. In this instance, I see no reason why this has to be a player variable though; storing Player Number of (Triggering Player) is more useful information here because it allows you to simplify your if-conditions:
  
  • Set soul_reap_pn = (Player number of (Triggering Player))
  • If (All conditions)...
    • If - Conditions
      • soul_reap_pn less than or equal to 4
    • Then - Actions
      • Set soul_reap_hero = hero_array[soul_reap_pn]
    • Else - Actions
      • Set soul_reap_hero = runner_hero[soul_reap_pn]
  While on the subject, there's really no reason hero_array and runner_hero need to be separate unit arrays at all. I realize this would probably change a lot of triggers in the map so now it's baked in, but since those are indexed by player number anyway they'll never overlap. Keeping them (and other related array variables which I'm sure exist in your map) as single arrays instead of two different non-overlapping arrays would simplify some of the logic you are using in places like this by allowing you to use player number directly.
  
  
• Are you trying to overflow the unit's current mana above its current maximum? That's what you're doing here, not increasing its maximum mana.
• soul_reap_hero does not need to be cleared at the end of the trigger; it will be overwritten before it's used the next time.

 

[Gnuoy]

Appreciate your crituque thank you. Here's an updated version of the trigger.

udg_soul_reap_units can be used again even though it's Destroyed?

Spoiler

• soul reap research
  • Events
    • Unit - A unit Finishes research
  • Conditions
    • (Researched tech-type) Equal to Soul Reap
  • Actions
    • -------- set player --------
    • Set VariableSet soul_reap_pn = (Player number of (Triggering player))
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • soul_reap_pn Less than or equal to 4
      • Then - Actions
        • Set VariableSet soul_reap_hero = hero_array[soul_reap_pn]
      • Else - Actions
        • Set VariableSet soul_reap_hero = runner_hero[soul_reap_pn]
    • -------- sfx --- play animation on all demon corpses --------
    • Set VariableSet soul_reap_units = (Units in (Playable map area) matching ((Level of Legion Unit Class (Icon) for (Matching unit)) Equal to 1))
    • Unit Group - Pick every unit in soul_reap_units and do (Actions)
      • Loop - Actions
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • ((Picked unit) is dead) Equal to True
          • Then - Actions
            • Set VariableSet soul_reap_point = (Position of (Picked unit))
            • Special Effect - Create a special effect at soul_reap_point using Abilities\Spells\Undead\DeathPact\DeathPactCaster.mdl
            • -------- clear sfx leaks, point variable, and remove unit from unit group --------
            • Special Effect - Destroy (Last created special effect)
            • Custom script: call RemoveLocation(udg_soul_reap_point)
          • Else - Actions
    • -------- increase hero's mana based on how many units they killed --------
    • Unit - Set Max Mana of soul_reap_hero to ((Max Mana of soul_reap_hero) + demon_kills[soul_reap_pn])
    • -------- reset variables and clear unit group --------
    • Set VariableSet soul_reap_pn = 0
    • Set VariableSet soul_reap_hero = No unit
    • Custom script: call DestroyGroup(udg_soul_reap_units)

 

[Uncle]

You aren't destroying the variable, you're destroying the Unit Group that the variable is keeping track of. A variable is just a container of data - not the data itself. This is very important to understand and remember.

So when you set a Unit Group variable like this:

• Set VariableSet soul_reap_units = (Units in (Playable map area) matching ((Level of Legion Unit Class (Icon) for (Matching unit)) Equal to 1))

Here's what's essentially happening under the hood:

1) The game creates a brand new Unit Group object. At this stage the soul_reap_units variable is not involved at all.
2) The game proceeds to add units to this new Unit Group based on your given filter (units with the Legion Unit Class ability).
3) The game sets the soul_reap_units variable as the container for this new Unit Group. You'd have no way of using the Unit Group without it.

Hopefully that makes sense. So when you run the Custom Script to destroy soul_reap_units you're destroying what it's currently Set to, not the variable itself. In this case it will destroy the Unit Group object containing all of those Legion units. Afterwards, the soul_reap_units variable will continue to exist, it simply contains nothing and is ready to be used again in the future. If this trigger ever runs again then the whole process will start over and repeat steps 1 -> 3.

Note that Local variables are different since they are meant to be temporary. You need to set them to null when you're finished with them (the same memory leak rules apply with destroying their contained Unit Groups beforehand). This nulling marks them for "destruction" and the game will automatically clear them from memory at some point in the future. Note that this only applies to local variables that are NOT Integers, Reals, Strings, and Booleans. Those four are your basic data types and do not need to be cleaned up.

Also, you don't have to do this:

• Set VariableSet soul_reap_pn = 0
• Set VariableSet soul_reap_hero = No unit

This kind of "resetting" falls under the category of micro-optimization and is a time waster with microscopic results. In other words, you won't notice a difference in performance with or without it.

 

[Gnuoy]

...when you run the Custom Script to destroy soul_reap_units you're destroying what it's currently Set to, not the variable itself...

Yes thanks for explaining this again. I think it will stick this time.