[Trigger] -Devinity-'s GUI Spell Reviewing

[-Devinity-]

Welcome to my Spell Reviewing topic.
Please note that I ONLY review GUI (trigger) spells.


If you want me to make a review for you, feel free to send a VM or PM to me. I will also perform random reviews on randoms spells in the Spells section of the Hive.

Note: Did I make a review for your spell? If you have any questions or if your spell was "rejected" and want to "appeal" it, post a reply in this thread.



Reviews.

Total reviews: 1

Spells approved/rejected after the makes has read my reviews.

Total approved: 0
Total rejected: 0

GIANT_CRAB's Lightning spell (v1.00)

So, GIANT_CRAB. As far as I know this is one of your first spells uploaded right? I will start my review by making some tips/suggestions on how you can improve your spell(s) and make them work better.

The following are just comments, but I suggest you read it all and see what I have to say

Spell 1, Chain lightning

First of all, you will need to change the "chain lightning spell" by creating loops. This is how your trigger looks like now:

• chain lightning
  • Events
    • Unit - A unit Starts the effect of an ability
  • Conditions
  • Actions
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • (Ability being cast) Equal to |cffC0C0C0Chain Lightning|r
      • Then - Actions
        • Set TempDamage1 = (((Mana of (Triggering unit)) x ((Real((Level of |cffC0C0C0Chain Lightning|r for (Triggering unit)))) + 3.00)) x 1.00)
        • Set TempPoint1 = (Position of (Target unit of ability being cast))
        • Special Effect - Create a special effect at TempPoint1 using war3mapImported\Great Lightning.mdx
        • Special Effect - Destroy (Last created special effect)
        • Unit Group - Add (Target unit of ability being cast) to TempGroup1
        • Unit - Set mana of (Target unit of ability being cast) to ((Mana of (Target unit of ability being cast)) - (TempDamage1 x 0.10))
        • Unit - Cause (Triggering unit) to damage (Target unit of ability being cast), dealing TempDamage1 damage of attack type Spells and damage type Normal
        • Unit - Create 1 Purge Dummy for (Owner of (Triggering unit)) at TempPoint1 facing Default building facing degrees
        • Unit - Add a 1.00 second Generic expiration timer to (Last created unit)
        • Unit - Order (Last created unit) to Orc Shaman - Purge (Target unit of ability being cast)
        • Set TempGroup2 = (Units within 1000.00 of TempPoint1 matching ((((Matching unit) is alive) Equal to True) and ((((Matching unit) belongs to an enemy of (Owner of (Triggering unit))) Equal to True) and (((Matching unit) is in TempGroup1) Equal to False))))
        • Set TempUnit1 = (Random unit from TempGroup2)
        • Set TempDamage1 = (((Mana of (Triggering unit)) x ((Real((Level of |cffC0C0C0Chain Lightning|r for (Triggering unit)))) + 3.00)) x 0.80)
        • Unit Group - Add TempUnit1 to TempGroup1
        • Unit - Set mana of TempUnit1 to ((Mana of TempUnit1) - (TempDamage1 x 0.10))
        • Unit - Cause (Triggering unit) to damage TempUnit1, dealing TempDamage1 damage of attack type Spells and damage type Normal
        • Unit - Create 1 Purge Dummy for (Owner of (Triggering unit)) at TempPoint1 facing Default building facing degrees
        • Unit - Add a 1.00 second Generic expiration timer to (Last created unit)
        • Unit - Order (Last created unit) to Orc Shaman - Purge TempUnit1
        • Custom script: call RemoveLocation(udg_TempPoint1)
        • Custom script: call DestroyGroup(udg_TempGroup2)
        • Set TempPoint1 = (Position of TempUnit1)
        • Special Effect - Create a special effect at TempPoint1 using war3mapImported\Great Lightning.mdx
        • Special Effect - Destroy (Last created special effect)
        • Set TempGroup2 = (Units within 1000.00 of TempPoint1 matching ((((Matching unit) is alive) Equal to True) and ((((Matching unit) belongs to an enemy of (Owner of (Triggering unit))) Equal to True) and (((Matching unit) is in TempGroup1) Equal to False))))
        • Set TempUnit1 = (Random unit from TempGroup2)
        • Set TempDamage2 = (TempDamage1 x 0.80)
        • Unit Group - Add TempUnit1 to TempGroup1
        • Unit - Set mana of TempUnit1 to ((Mana of TempUnit1) - (TempDamage2 x 0.10))
        • Unit - Cause (Triggering unit) to damage TempUnit1, dealing TempDamage2 damage of attack type Spells and damage type Normal
        • Unit - Create 1 Purge Dummy for (Owner of (Triggering unit)) at TempPoint1 facing Default building facing degrees
        • Unit - Add a 1.00 second Generic expiration timer to (Last created unit)
        • Unit - Order (Last created unit) to Orc Shaman - Purge TempUnit1
        • Custom script: call RemoveLocation(udg_TempPoint1)
        • Custom script: call DestroyGroup(udg_TempGroup2)
        • Set TempPoint1 = (Position of TempUnit1)
        • Special Effect - Create a special effect at TempPoint1 using war3mapImported\Great Lightning.mdx
        • Special Effect - Destroy (Last created special effect)
        • Set TempGroup2 = (Units within 1000.00 of TempPoint1 matching ((((Matching unit) is alive) Equal to True) and ((((Matching unit) belongs to an enemy of (Owner of (Triggering unit))) Equal to True) and (((Matching unit) is in TempGroup1) Equal to False))))
        • Set TempUnit1 = (Random unit from TempGroup2)
        • Set TempDamage1 = (TempDamage2 x 0.80)
        • Unit Group - Add TempUnit1 to TempGroup1
        • Unit - Set mana of TempUnit1 to ((Mana of TempUnit1) - (TempDamage1 x 0.10))
        • Unit - Cause (Triggering unit) to damage TempUnit1, dealing TempDamage1 damage of attack type Spells and damage type Normal
        • Unit - Create 1 Purge Dummy for (Owner of (Triggering unit)) at TempPoint1 facing Default building facing degrees
        • Unit - Add a 1.00 second Generic expiration timer to (Last created unit)
        • Unit - Order (Last created unit) to Orc Shaman - Purge TempUnit1
        • Custom script: call RemoveLocation(udg_TempPoint1)
        • Custom script: call DestroyGroup(udg_TempGroup2)
        • Set TempPoint1 = (Position of TempUnit1)
        • Special Effect - Create a special effect at TempPoint1 using war3mapImported\Great Lightning.mdx
        • Special Effect - Destroy (Last created special effect)
        • Set TempGroup2 = (Units within 1000.00 of TempPoint1 matching ((((Matching unit) is alive) Equal to True) and ((((Matching unit) belongs to an enemy of (Owner of (Triggering unit))) Equal to True) and (((Matching unit) is in TempGroup1) Equal to False))))
        • Set TempUnit1 = (Random unit from TempGroup2)
        • Set TempDamage2 = (TempDamage1 x 0.80)
        • Unit Group - Add TempUnit1 to TempGroup1
        • Unit - Set mana of TempUnit1 to ((Mana of TempUnit1) - (TempDamage2 x 0.10))
        • Unit - Cause (Triggering unit) to damage TempUnit1, dealing TempDamage2 damage of attack type Spells and damage type Normal
        • Unit - Create 1 Purge Dummy for (Owner of (Triggering unit)) at TempPoint1 facing Default building facing degrees
        • Unit - Add a 1.00 second Generic expiration timer to (Last created unit)
        • Unit - Order (Last created unit) to Orc Shaman - Purge TempUnit1
        • Custom script: call RemoveLocation(udg_TempPoint1)
        • Custom script: call DestroyGroup(udg_TempGroup2)
        • Set TempPoint1 = (Position of TempUnit1)
        • Special Effect - Create a special effect at TempPoint1 using war3mapImported\Great Lightning.mdx
        • Special Effect - Destroy (Last created special effect)
        • Set TempGroup2 = (Units within 1000.00 of TempPoint1 matching ((((Matching unit) is alive) Equal to True) and ((((Matching unit) belongs to an enemy of (Owner of (Triggering unit))) Equal to True) and (((Matching unit) is in TempGroup1) Equal to False))))
        • Set TempUnit1 = (Random unit from TempGroup2)
        • Set TempDamage1 = (TempDamage2 x 0.80)
        • Unit Group - Add TempUnit1 to TempGroup1
        • Unit - Set mana of TempUnit1 to ((Mana of TempUnit1) - (TempDamage1 x 0.10))
        • Unit - Cause (Triggering unit) to damage TempUnit1, dealing TempDamage1 damage of attack type Spells and damage type Normal
        • Unit - Create 1 Purge Dummy for (Owner of (Triggering unit)) at TempPoint1 facing Default building facing degrees
        • Unit - Add a 1.00 second Generic expiration timer to (Last created unit)
        • Unit - Order (Last created unit) to Orc Shaman - Purge TempUnit1
        • Custom script: call RemoveLocation(udg_TempPoint1)
        • Custom script: call DestroyGroup(udg_TempGroup2)
        • Custom script: call DestroyGroup(udg_TempGroup1)
        • Unit - Set mana of (Triggering unit) to 0.00
      • Else - Actions

So, how do we fix this my friend?
Well first of all, go to "Actions" and select "For each Integer A, do Multiple Actions".
It should look like this:

• Learning how to use "Loop Actions"
  • Actions
    • For each (Integer A) from 1 to 10, do (Actions)
      • Loop - Actions

The integer number 1 means that it will start from one and continue to the second number. In this case, the actions in the "Loop - Actions" will run 10 times (1 to 10).

Your spell loops 5 times.
So this is how I suggest you improve this spell:

• chain lightning
  • Events
    • Unit - A unit Starts the effect of an ability
  • Conditions
  • Actions
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • (Ability being cast) Equal to |cffC0C0C0Chain Lightning|r
      • Then - Actions
        • Set TempDamage1 = (((Mana of (Triggering unit)) x ((Real((Level of |cffC0C0C0Chain Lightning|r for (Triggering unit)))) + 3.00)) x 1.00)
        • For each (Integer A) from 1 to 5, do (Actions)
          • Loop - Actions
            • Set TempPoint1 = (Position of (Target unit of ability being cast))
            • Special Effect - Create a special effect at TempPoint1 using war3mapImported\Great Lightning.mdx
            • Special Effect - Destroy (Last created special effect)
            • Unit Group - Add (Target unit of ability being cast) to TempGroup1
            • Unit - Set mana of (Target unit of ability being cast) to ((Mana of (Target unit of ability being cast)) - (TempDamage1 x 0.10))
            • Unit - Cause (Triggering unit) to damage (Target unit of ability being cast), dealing TempDamage1 damage of attack type Spells and damage type Normal
            • Unit - Create 1 Purge Dummy for (Owner of (Triggering unit)) at TempPoint1 facing Default building facing degrees
            • Unit - Add a 1.00 second Generic expiration timer to (Last created unit)
            • Unit - Order (Last created unit) to Orc Shaman - Purge (Target unit of ability being cast)
            • Set TempGroup2 = (Units within 1000.00 of TempPoint1 matching ((((Matching unit) is alive) Equal to True) and ((((Matching unit) belongs to an enemy of (Owner of (Triggering unit))) Equal to True) and (((Matching unit) is in TempGroup1) Equal to False))))
            • Set TempUnit1 = (Random unit from TempGroup2)
            • Set TempDamage1 = (((Mana of (Triggering unit)) x ((Real((Level of |cffC0C0C0Chain Lightning|r for (Triggering unit)))) + 3.00)) x 0.80)
            • Unit Group - Add TempUnit1 to TempGroup1
            • Unit - Set mana of TempUnit1 to ((Mana of TempUnit1) - (TempDamage1 x 0.10))
            • Unit - Cause (Triggering unit) to damage TempUnit1, dealing TempDamage1 damage of attack type Spells and damage type Normal
            • Unit - Create 1 Purge Dummy for (Owner of (Triggering unit)) at TempPoint1 facing Default building facing degrees
            • Unit - Add a 1.00 second Generic expiration timer to (Last created unit)
            • Unit - Order (Last created unit) to Orc Shaman - Purge TempUnit1
            • Custom script: call RemoveLocation(udg_TempPoint1)
            • Custom script: call DestroyGroup(udg_TempGroup2)
      • Else - Actions

That looks much better, and much shorter/easier right?
Ok let's move on.

I think you have forgotten to make sure the targets are not magic immune. If you are unsure of how to do this, then of course I will show this as well.

Well first of all, when you set your variable for unit group, you must make sure the (Matching unit) is (Magic immune) equal to False.
Like this:

• MAKE SURE THE UNITS ARE NOT MAGIC IMMUNE!
  • Actions
    • Set TempGroup2 = (Units within 1000.00 of TempPoint1 matching ((((Matching unit) is alive) Equal to True) and ((((Matching unit) belongs to an enemy of (Owner of (Triggering unit))) Equal to True) and ((((Matching unit) is in TempGroup1) Equal to False) and (((Matching unit)

I would also recommend that you check so the targets are not structures etcetera.
Moving on...

As far as I can see, you have only 1 leak in that trigger.
I think you have forgotten this:

• Removing leaks
  • Actions
    • Custom script: call DestroyGroup(udg_TempGroup1)

But since you removed Group2, I assume this was just a random mistake (we can all make those!).

I think that's it for that trigger

Next up, is the "Lightning shield" spell.

Spell 2, Lightning Shield

GIANT_CRAB, first of all, you do not need a loop for the Condition.

• lightning shield
  • Events
    • Unit - A unit Starts the effect of an ability
  • Conditions
  • Actions
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • (Ability being cast) Equal to |cffC0C0C0Lightning Shield|r
      • Then - Actions
        • Set TempPoint1 = (Position of (Triggering unit))
        • Unit - Create 1 Lightning Shield Dummy for (Owner of (Triggering unit)) at TempPoint1 facing (Facing of (Triggering unit)) degrees
        • Unit - Add a (3.00 x (Real((Level of |cffC0C0C0Lightning Shield|r for (Triggering unit))))) second Generic expiration timer to (Last created unit)
        • Unit Group - Add (Last created unit) to LightningShieldMiddleGroup
        • For each (Integer A) from 1 to 36, do (Actions)
          • Loop - Actions
            • Set TempPoint2 = (TempPoint1 offset by 300.00 towards ((Real((Integer A))) x 10.00) degrees)
            • Unit - Create 1 Lightning Shield Dummy for (Owner of (Triggering unit)) at TempPoint2 facing ((Real((Integer A))) x 10.00) degrees
            • Unit - Add a (3.00 x (Real((Level of |cffC0C0C0Lightning Shield|r for (Triggering unit))))) second Generic expiration timer to (Last created unit)
            • Unit Group - Add (Last created unit) to LightningShieldGroup
            • Custom script: call RemoveLocation(udg_TempPoint2)
        • Custom script: call RemoveLocation(udg_TempPoint1)
        • Trigger - Turn on lightning shield damage <gen>
        • Trigger - Turn on lightning shield middle damage <gen>
      • Else - Actions

Set the (Ability being cast) Equal to Lightning shield in the Conditions instead of making a loop for it (since you do not have any "Else - Actions").

I can also not see where you destroy the "LightningShieldGroup" (Unit group). So this should be fixed.

Also, when you "Turn on lightning shield damage <gen>" and the "lightning shield middle damage <gen>", you do not check if the triggers are already enabled.

You can do this by making a simple Integer and change the value in this, for example like this:

• Making sure the triggers are not already ON
  • Actions
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • TriggerEnable[1] Equal to 0
      • Then - Actions
        • Trigger - Turn on lightning shield damage <gen>
        • Set TriggerEnable[1] = 1
      • Else - Actions
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • TriggerEnable[2] Equal to 0
      • Then - Actions
        • Trigger - Turn on lightning shield middle damage <gen>
        • Set TriggerEnable[2] = 1
      • Else - Action

In this trigger you finally used Loop integers, GOOD JOB!

Ok now for the next trigger.

• lightning shield damage
  • Events
    • Time - Every 0.10 seconds of game time
  • Conditions
  • Actions
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • (Number of units in LightningShieldGroup) Greater than 0
      • Then - Actions
        • Unit Group - Pick every unit in LightningShieldGroup and do (Actions)
          • Loop - Actions
            • Set TempUnit1 = (Picked unit)
            • Set TempPoint1 = (Position of (Picked unit))
            • Set TempGroup1 = (Units within 150.00 of TempPoint1 matching ((((Matching unit) is alive) Equal to True) and (((Matching unit) belongs to an enemy of (Owner of (Picked unit))) Equal to True)))
            • Unit Group - Pick every unit in TempGroup1 and do (Actions)
              • Loop - Actions
                • Set TempPoint2 = (Position of (Picked unit))
                • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
                  • If - Conditions
                    • (Mana of (Picked unit)) Greater than or equal to 1.00
                  • Then - Actions
                    • Special Effect - Create a special effect at TempPoint2 using Abilities\Spells\Human\Feedback\ArcaneTowerAttack.mdl
                    • Special Effect - Destroy (Last created special effect)
                    • Unit - Set mana of (Picked unit) to ((Mana of (Picked unit)) - 5.00)
                  • Else - Actions
                    • Special Effect - Create a special effect at TempPoint2 using war3mapImported\Great Lightning.mdx
                    • Special Effect - Destroy (Last created special effect)
                    • Unit - Cause TempUnit1 to damage (Picked unit), dealing 5.00 damage of attack type Spells and damage type Normal
                • Custom script: call RemoveLocation(udg_TempPoint2)
            • Custom script: call RemoveLocation(udg_TempPoint1)
            • Custom script: call DestroyGroup(udg_TempGroup1)
      • Else - Actions
        • Trigger - Turn off (This trigger)

This text is getting really long so I'll make it short.
1: You do not make sure the units are magic immune (again), but by now you should know how to fix it
2: You do not reset the unit you made in a variable, use this:

• Removing leaks
  • Actions
    • Set TempUnit1 = No unit

3: If you do as I said in the other trigger (making a variable to check that a trigger is not already enabled), you must add this variable to the end of your trigger:

• You should check the triggers so that they don't run already
  • Actions
    • Set TriggerEnable[1] = 0

(That will "Disable" the trigger so that it will work as it should)
4: Other than that, it looks good


The last and final trigger.

• lightning shield middle damage
  • Events
    • Time - Every 0.10 seconds of game time
  • Conditions
  • Actions
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • (Number of units in LightningShieldMiddleGroup) Greater than 0
      • Then - Actions
        • Unit Group - Pick every unit in LightningShieldMiddleGroup and do (Actions)
          • Loop - Actions
            • Set TempUnit1 = (Picked unit)
            • Set TempPoint1 = (Position of (Picked unit))
            • Set TempGroup1 = (Units within 200.00 of TempPoint1 matching ((((Matching unit) is alive) Equal to True) and (((Matching unit) belongs to an enemy of (Owner of (Picked unit))) Equal to True)))
            • Unit Group - Pick every unit in TempGroup1 and do (Actions)
              • Loop - Actions
                • Set TempPoint2 = (Position of (Picked unit))
                • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
                  • If - Conditions
                    • (Mana of (Picked unit)) Greater than or equal to 1.00
                  • Then - Actions
                    • Special Effect - Create a special effect at TempPoint2 using war3mapImported\Great Lightning.mdx
                    • Special Effect - Destroy (Last created special effect)
                    • Special Effect - Create a special effect at TempPoint2 using Abilities\Spells\Human\Feedback\ArcaneTowerAttack.mdl
                    • Special Effect - Destroy (Last created special effect)
                    • Unit - Cause TempUnit1 to damage (Picked unit), dealing 10.00 damage of attack type Spells and damage type Normal
                    • Unit - Set mana of (Picked unit) to ((Mana of (Picked unit)) - 10.00)
                  • Else - Actions
                    • Special Effect - Create a special effect at TempPoint2 using war3mapImported\Great Lightning.mdx
                    • Special Effect - Destroy (Last created special effect)
                    • Unit - Cause TempUnit1 to damage (Picked unit), dealing 20.00 damage of attack type Spells and damage type Normal
                • Custom script: call RemoveLocation(udg_TempPoint2)
            • Custom script: call RemoveLocation(udg_TempPoint1)
            • Custom script: call DestroyGroup(udg_TempGroup1)
      • Else - Actions
        • Trigger - Turn off (This trigger)

Once again, I'll make my comments sort.
1: Got nothing more to say here. Repeat my comments in the other trigger in this one as well

Let me rest my fingers for a while ^^

....

Okay, I hope you found that useful, now we will get to the rating of the spell and the full review

Ratings of GIANT_CRAB's Lightning spells

We will start of by rating the functionality. This is based on how well your spells perform. This rating can be increased by using loops and short triggers that performs well.

Second, is the "eye candy", how your spells look. The smoother and advanced they look, the better rating.

Third, is the MUI rating. This can only be rated between 0 and 1 (0 being non-mui and 1 being fully mui).

Fourth will be the difficulty of the spell. Does it make units fly and make really cool moves by really hard actions? If so, this rating will increase.

Fifth and final is the total score. This will combine all the other rating scores and I will judge if it is approve-able or if it will/might be rejected.

Top score is 15.
Functionality: 7 - The spell(s) does not use loops and some parts of the spells could be improved.
Eye candy: 5 - Due to the lack of loop usage etcetera, the spells can cause lagg and not perform very well. I also think the spells can have better special effects.
MUI: 0 - The trigger can cause malfunction in the MUI parts.
Difficulty: 5 - I did not find the spells to be that hard, but they cannot be made by newbies, therefor this score.
Final: 17/46 - Not approveable. It has some leaks and the triggers does not perform as good as they should.

I am not a moderator and my ratings and reviews have no matter in making spells approved or rejected. I only do this to help makers of spells so that they know what they can improve before a moderator makes a final review of it.

 

[Lambdadelta]

I don't get why you can't just post it in the spell's thread itself...

 

[GIANT_CRAB]

I don't get why you can't just post it in the spell's thread itself...

He want to make a collection??

 

[Brambleclaw]

I dont get why you have to check for immunity if its even is a unit is targeted by the effect of an abillity. Surely it could not be targeted if its magic immune therfore there is no need to check for it. Rest looks okay though nice review.

 

[sPy]

I think this thread suits better in Request section.. So anyways, since you're willing to review spells, please review mine
At my signature there's a "spell" word, please click it