[Trigger] Help (Reviewing) an Olde Spell

[MasterTrainer]

Earth's Rampage v1.1a

Here is the link. It was an old spell, and the first one i tired to make using static indexing.

i thought I had gone over most if not all of purge's marks. But then again, I will have definitely missed something. I don't know if it's allowed for me to ask for this sort of thing, this kind of peer review, but if anyone could help me go through the code, and spot any errors or shortcomings in the triggers, that would be of most help.

As of this moment, I am almost sure that there are no leaks with the code overall, and i think that was the last issue i had with this.

 

[nedio95]

I'm looking at the posted triggers only, not downloaded the spell and tested.

Cast trigger

• Unit - Make ER_Caster[ER_MaxIndex] Invulnerable
• Set ER_Caster[ER_MaxIndex] = (Triggering unit)

Those look like they are in the wrong order. Or is that intended?

Loop trigger:
I've almost missed this(as the nested loop + long line are a bit confusing for readers), it leaks a point here:

• Unit - Create 1 ER_Dummy for ER_ControlPlayer[ER_CurrentIndex] at (ER_SomePoint offset by (ER_SpiralDistance[ER_Level] + (ER_SpireDistance[ER_Level] x (Real(ER_TempInt2[1])))) towards (((360.00 / (Real(ER_SpireNum[ER_Level]))) x (Real(ER_TempInt2[2]))) + (ER_SpireCurve[ER_Level] x (Real(ER_TempInt2[1])))) degrees) facing Default building facing degrees

It would be like:

• Set ER_SomePoint2 = ER_SomePoint offset by (ER_SpiralDistance[ER_Level] + (ER_SpireDistance[ER_Level] x (Real(ER_TempInt2[1])))) towards (((360.00 / (Real(ER_SpireNum[ER_Level]))) x (Real(ER_TempInt2[2]))) + (ER_SpireCurve[ER_Level] x (Real(ER_TempInt2[1])))) degrees
• Unit - Create 1 ER_Dummy for ER_ControlPlayer[ER_CurrentIndex] at ER_SomePoint2 facing Default building facing degrees
• Custom script: call RemoveLocation(udg_ER_SomePoint2)

You don't need this at all

• Set ER_DummyPoint = (Position of ER_SomeUnit)

as you have already created a dummy at ER_SomePoint2 Edir// Note my previous point, you'll move the CallRemove further down and not immediately after creating the unit so you could use it //EndEdit

Some comments do a lot for both you and any readers/reviewers. Suggested every 3-6 lines, depending on grouping/purpose, just like in the Set-up trigger.
It's been way too often when I've written a comment explaining how something works and while writing it I realize that it actually does not, or there is better way, etc.

I'll take a better look later, but for now this is my input here.

Hope this helps!
-Ned

 

[MasterTrainer]

I'm looking at the posted triggers only, not downloaded the spell and tested.

Cast trigger

• Unit - Make ER_Caster[ER_MaxIndex] Invulnerable
• Set ER_Caster[ER_MaxIndex] = (Triggering unit)

Those look like they are in the wrong order. Or is that intended?

Loop trigger:
I've almost missed this(as the nested loop + long line are a bit confusing for readers), it leaks a point here:

• Unit - Create 1 ER_Dummy for ER_ControlPlayer[ER_CurrentIndex] at (ER_SomePoint offset by (ER_SpiralDistance[ER_Level] + (ER_SpireDistance[ER_Level] x (Real(ER_TempInt2[1])))) towards (((360.00 / (Real(ER_SpireNum[ER_Level]))) x (Real(ER_TempInt2[2]))) + (ER_SpireCurve[ER_Level] x (Real(ER_TempInt2[1])))) degrees) facing Default building facing degrees

It would be like:

• Set ER_SomePoint2 = ER_SomePoint offset by (ER_SpiralDistance[ER_Level] + (ER_SpireDistance[ER_Level] x (Real(ER_TempInt2[1])))) towards (((360.00 / (Real(ER_SpireNum[ER_Level]))) x (Real(ER_TempInt2[2]))) + (ER_SpireCurve[ER_Level] x (Real(ER_TempInt2[1])))) degrees
• Unit - Create 1 ER_Dummy for ER_ControlPlayer[ER_CurrentIndex] at ER_SomePoint2 facing Default building facing degrees
• Custom script: call RemoveLocation(udg_ER_SomePoint2)

You don't need this at all

• Set ER_DummyPoint = (Position of ER_SomeUnit)

as you have already created a dummy at ER_SomePoint2 Edir// Note my previous point, you'll move the CallRemove further down and not immediately after creating the unit so you could use it //EndEdit

Some comments do a lot for both you and any readers/reviewers. Suggested every 3-6 lines, depending on grouping/purpose, just like in the Set-up trigger.
It's been way too often when I've written a comment explaining how something works and while writing it I realize that it actually does not, or there is better way, etc.

I'll take a better look later, but for now this is my input here.

Hope this helps!
-Ned

Everything helps my friend!

But i skimmed through the comments once more and it said that perhaps, the dummy unit isn't even needed, as i could stick to special effects instead. I will try to make a version of this spell that is a bit cleaner to look at.

That comments habit, sure might be of some help this time. I will try and do it!

But I think Ned, you'll be in for a surprise. Because the last time I tested this spell, it felt a bit off. I didn't know with what exactly, but it doesn't feel smooth to me. But perhaps you will see something in there. I truly appreciate the help man!

 

[PrinceYaser]

Damage actions kill the caster himself too (and ally units), you must filter them to avoid this. They must be like this:

• Custom script: set bj_wantDestroyGroup = true
• Unit Group - Pick every unit in ER_DummyGroup and do (Actions)
  • Loop - Actions
    • Set ER_TempUnit = (Picked unit)
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • (ER_TempUnit is dead) Equal to False
        • (ER_TempUnit is A structure) Equal to False
        • (ER_TempUnit belongs to an enemy of ER_ControlPlayer[ER_CurrentIndex]) Equal to True
      • Then - Actions
        • Unit - Cause ER_Caster[ER_CurrentIndex] to damage ER_TempUnit, dealing ER_StompDamage[ER_Level[ER_CurrentIndex]] damage of attack type Chaos and damage type Divine
      • Else - Actions

Tho, I dunno if you also want to damage the buildings. If so, you know you should remove this filter:

• (ER_TempUnit is A structure) Equal to False

I also suppose that there should be an Order Comparison for recycling, but that's not intended as I see.

 

[nedio95]

Dummy unit
They are entirely correct, Special Effect would be better if you are playing only a single animation, special effects are the way to go.
If you are doing animation transitions, e.g. states, attacks, spell casts. etc., dummy units are easier to manage and control.
Units also leak,
I believe each unit takes something like 0.40KB (or 40? Exact number is lost in my memory (pun intended) I am sure it is a 4 followed by a 0 and some dot somewhere around...) of memory (0.44 for Heroes).
If you spam Dummy units (like this spell) you re going to run out of RAM soon, which I believe war3 is limited to 1.5GB.
Now on a second look, it does not seem like you are doing anything with the dummies, as such, special effects would be the way to go. I am not 100% sure if they'd allow you the level of control you have with a unit though...

I'll check the spell when I get home tonight.
You understand that "it felt a bit off" is a personal experience, e.g. I watched Terminator, it was ok, nothing special, there are people who would murder me for this statement and then again there are people who would shun me for saying it is "ok" instead of "the worst movie ever created in human history"
Ofc, you are the creator, you are completely free to tweak until you, yourself, are satisfied with the spell.

regards
-Ned