Needs Fixed

• There are a few too much hardcoded stuff that should be configurable. Here are some from what I can see: special effects, attachment points of the special effects, attack type, damage type, speed of the charge, animation speed, & dummy ability.
• You leak a lot of locations in the loop.
• Instead of using the single If/Then/Else statement at the top of the loop to turn off the trigger, just make the check when the spell has ended.

Suggestions

• LB is too generic for a prefix.
• It's not apparent what LB_EffectCount does until you look through the code. I'd add a note on what that is.
• Avoid repeating function calls.
• Avoid using Integer A as a loop integer. Create your own.
• @Daffa the Mage mentioned some good points you should consider.

Status

KILLCIDE said:

Instead of using the single If/Then/Else statement at the top of the loop to turn off the trigger, just make the check when the spell has ended.

Click to expand...

• Events
  • A unit ends casting an ability.
• Conditions
  • ....

• Life Break Loop
  • Events
    • Time - Every 0.02 seconds of game time
  • Conditions
  • Actions
    • Unit Group - Pick every unit in HuskarLB_Group and do (Actions)
      • Loop - Actions
        • Set HuskarLB_Unit = (Picked unit)
        • Set HuskarLB_Target = (Load 1 of (Key (Picked unit)) in HuskarLB_Table)
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • (HuskarLB_Unit is alive) Equal to True
            • (Distance between (Position of HuskarLB_Unit) and (Position of HuskarLB_Target)) Less than HuskarLB_Distance_Broke
          • Then - Actions
            • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
              • If - Conditions
                • (Distance between (Position of HuskarLB_Unit) and (Position of HuskarLB_Target)) Greater than or equal to HuskarLB_Distance_ToClose
              • If - Actions
                • Set HuskarLB_TempLoc = ((Position of HuskarLB_Unit) offset by ((HuskarLB_ChargeSpeed / 50.00) + (((Current movement speed of (HuskarLB_Unit)) x 0.01) x 0.10)) towards (Facing of HuskarLB_Unit) degrees)
                • Set HuskarLB_X = (X of HuskarLB_TempLoc)
                • Set HuskarLB_Y = (Y of HuskarLB_TempLoc)
                • Custom script: call RemoveLocation(udg_HuskarLB_TempLoc)
                • Custom script: call SetUnitX(udg_HuskarLB_Unit, udg_HuskarLB_X)
                • Custom script: call SetUnitY(udg_HuskarLB_Unit, udg_HuskarLB_Y)
                • Unit - Make HuskarLB_Unit face HuskarLB_Target over 0.00 seconds
                • -------- Apply the "Run" Animation --------
                • Custom script: call SetUnitAnimationByIndex( udg_HuskarLB_Unit, udg_HuskarLB_ChargeAnimIndex)
              • Otros: Acciones
                • Unit - Make (Picked unit) Vulnerable
                • Unit Group - Remove (Picked unit) from HuskarLB_Group
                • Unit - Cause HuskarLB_Unit to damage HuskarLB_Target, dealing ((Life of HuskarLB_Target) x (Load 2 of (Key (Picked unit)) from HuskarLB_Table)) damage of attack type HuskarLB_AttackType and damage type HuskarLB_DamageType
                • Special Effect - Destroy (Load 80 of (Key (Picked unit)) in HuskarLB_Table)
                • Special Effect - Destroy (Load 81 of (Key (Picked unit)) in HuskarLB_Table)
                • Special Effect - Create a special effect attached to the overhead of HuskarLB_Target using HuskarLB_SpecialEffect
                • Special Effect - Destroy (Last created special effect)
                • -------- Setting the Huskar to Normal --------
                • Animation - Change HuskarLB_Unit's animation speed to 100.00% of its original speed
                • Animation - Reset HuskarLB_Unit's animation
                • -------- Applying the Slow Effect --------
                • Set HuskarLB_TempLoc = (Position of HuskarLB_Unit)
                • Unit - Create 1 Dummy for (Owner of HuskarLB_Unit) at HuskarLB_TempLoc facing HuskarLB_TempLoc
                • Unit - Add HuskarLB_ABILITY_Dummy to (Last created unit)
                • Unit - Set level of HuskarLB_ABILITY_Dummy for (Last created unit) to (Level of HuskarLB_ABILITY for HuskarLB_Unit)
                • Unit - Order (Last created unit) to Sorceress : Slow HuskarLB_Target
                • Unit - Add a 0.01 second Generic expiration timer to (Last created unit)
                • -------- Removing the Location --------
                • Custom script: call RemoveLocation(udg_HuskarLB_TempLoc)
                • -------- - --------
                • Unit - Order HuskarLB_Unit to Attack HuskarLB_Target
                • Hashtable - Clear all child hashtables of child (Key (Picked unit)) in HuskarLB_Table
          • Otros: Acciones
            • Unit - Make (Picked unit) Vulnerable
            • Unit - Order HuskarLB_Unit to Stop
            • Unit Group - Remove (Picked unit) from HuskarLB_Group
            • Special Effect - Destroy (Load 80 of (Key (Picked unit)) in HuskarLB_Table)
            • Special Effect - Destroy (Load 81 of (Key (Picked unit)) in HuskarLB_Table)
            • Hashtable - Clear all child hashtables of child (Key (Picked unit)) in HuskarLB_Table
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • (Number of units in HuskarLB_Group) Equal to 0
      • Then - Actions
        • Trigger - Turn off (This trigger)
      • Else - Actions
        • Do nothing

Needs Fixed

• You still leak a lot of locations. I'll point out the ones I can see here:
  • (Distance between (Position of HuskarLB_Unit) and (Position of HuskarLB_Target)) Less than HuskarLB_Distance_Broke
  • (Distance between (Position of HuskarLB_Unit) and (Position of HuskarLB_Target)) Greater than or equal to HuskarLB_Distance_ToClose
  • ((Position of HuskarLB_Unit) offset by ((HuskarLB_ChargeSpeed / 50.00) + (((Current movement speed of (Picked unit)) x 0.01) x 0.10)) towards (Facing of HuskarLB_Unit) degrees)

Suggestions

• You still have A LOT of repeating function calls in your code. If you're going to call the functions (Triggering unit), (Picked unit), or (Last created unit) more than once, it's best to just store it into a variable.
• Move that If/Then/Else that checks if it is time to turn off the trigger inside the actions block that ends the current spell instance
• It's not necessary to have a Do nothing action. It literally calls a function that does nothing.

Status

Needs Fixed

• FirstPos & EndPos leaks. Be sure to clear them after the giant If/Then/Else block like this:
  
  • Set HuskarLB_FirstPos = (Position of HuskarLB_Unit)
  • Set HuskarLB_EndPos = (Position of HuskarLB_Target)
  • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
  • ...
  • Custom script: call RemoveLocation(udg_HuskarLB_FirstPos)
  • Custom script: call RemoveLocation(udg_HuskarLB_EndPos)

Suggestions

• Instead of using Identifier to store the (Picked unit), it would be better to store the Key of (Picked unit). This would go for both onCast and loop triggers.
• The SFX you attach to the weapon left and weapon right attachment points should be configurable (attachement point should be configurable too)
• (Level of HuskarLB_ABILITY could be stored into a variable onCast for slightly better efficiency
• You should only turn on the loop if it hasn't been already turned on. A workaround to this is having an integer keep track of the number of units you add to the group, and then checking if the integer is 1 to turn on. You could also use this check to turn off the trigger (if integer is 0, turn off). You would have to increment the integer for every onCast, and decrement the integer everytime you end a spell instance. This is honestly much better than using (Number of units in Unit Group)
• It seems useless to have "Order HuskarLB_Unit to Stop" because this action will only run if the unit is not alive.

Status

Needs Fixed

• The name of some of the variables are lacking prefixes so are way too generic (TempUnit, TempUnit2, TempInteger, etc)

Suggestions

• Make the attachment point of the special effects configurable
• Avoid repeating function calls (ex: GetTriggerUnit() is called 10 times in the cast trigger)
• Make attack type and damage type configurable

Status

Needs Fixed

• TempPoint and TempPoint2 leak every loop iteration. As of right now, you only clear the leak with TempPoint if the spell has ended.

Suggestions

• Only turn on the loop when you have to. Simarily to what you do when you turn it off, you can do this:
  
  • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
    • If - Conditions
      • HuskarLB_CastersAlive Equal to 1
    • Then - Actions
      • Trigger - Turn on Life Break Loop <gen>
    • Else - Actions
• Single If/Then/Else are ugly I'd change it to Multiple Actions If/Then/Else
• Store (Distance between TempPoint and TempPoint 2) into a variable so you don't have to do the calculation twice in the condition check.
• I'd add an extra check to make sure the target is also alive. The caster shouldn't be charging at the target if they already died imo.
• Dummy should be stored into a configurable variable

Status