[Spell] Is this spell done right?

[SapphireFrost]

Is my first time posting here, i'm not speak english so if i write something bad is for it.

I try to make a spell that summons an orb that orbits the caster and deals damage to units it hits, it's my first spell and I don't know if there's something wrong with it or if it can be improved in some way. I know it's not MUI but I don't want to get involved with that at the moment.

Spell

• Frost Orb
  • Acontecimientos
    • Unidad - A unit Inicia el efecto de una habilidad
  • Condiciones
    • (Ability being cast) Igual a Frost Orb
  • Acciones
    • Set FO_Caster = (Triggering unit)
    • Set FO_PosCaster = (Position of FO_Caster)
    • Set FO_OrbCant = ((Level of (Ability being cast) for FO_Caster) + 1)
    • For each (Integer A) from 1 to FO_OrbCant, do (Actions)
      • Bucle: Acciones
        • Set FO_Angle[(Integer A)] = ((Integer A) x (360 / FO_OrbCant))
        • Set FO_PosDummy[(Integer A)] = ((Position of FO_Caster) offset by 300.00 towards (Real(FO_Angle[(Integer A)])) degrees)
        • Unidad - Create 1 Dummy Esfera for (Owner of FO_Caster) at FO_PosDummy[(Integer A)] facing ((Real(FO_Angle[(Integer A)])) + 90.00) degrees
        • Set FO_Dummy[(Integer A)] = (Last created unit)
        • Unidad - Add a 5.00 second Genérico expiration timer to FO_Dummy[(Integer A)]
    • Custom script: call RemoveLocation(udg_FO_PosCaster)
    • For each (Integer A) from 1 to FO_OrbCant, do (Actions)
      • Bucle: Acciones
        • Custom script: call RemoveLocation(udg_FO_PosDummy[bj_forLoopAIndex])
    • Detonador - Turn on Frost Orb Loop <gen>

Spell Loop

• Frost Orb Loop
  • Acontecimientos
    • Tiempo - Every 0.05 seconds of game time
  • Condiciones
  • Acciones
    • For each (Integer A) from 1 to FO_OrbCant, do (Actions)
      • Bucle: Acciones
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • Si: Condiciones
            • FO_Angle[(Integer A)] Igual a 360
          • Entonces: Acciones
            • Set FO_Angle[(Integer A)] = 0
          • Otros: Acciones
    • Set FO_Time = (FO_Time + 0.05)
    • Set FO_PosCaster = (Position of FO_Caster)
    • For each (Integer A) from 1 to FO_OrbCant, do (Actions)
      • Bucle: Acciones
        • Set FO_Angle[(Integer A)] = (FO_Angle[(Integer A)] + 10)
        • Set FO_PosDummy[(Integer A)] = ((Position of FO_Caster) offset by 300.00 towards (Real(FO_Angle[(Integer A)])) degrees)
        • Unidad - Move FO_Dummy[(Integer A)] instantly to FO_PosDummy[(Integer A)], facing ((Real(FO_Angle[(Integer A)])) + 90.00) degrees
        • Set FO_Damaged = (Units within 50.00 of FO_PosDummy[(Integer A)] matching ((((Matching unit) is Una estructura) Igual a False) and ((((Matching unit) is alive) Igual a True) and (((Matching unit) belongs to an enemy of (Owner of FO_Caster)) Igual a True))))
        • Custom script: set bj_wantDestroyGroup = true
        • Grupo de unidad - Pick every unit in FO_Damaged and do (Actions)
          • Bucle: Acciones
            • Unidad - Cause FO_Caster to damage (Picked unit), dealing 10.00 damage of attack type Conjuros and damage type Normal
    • Custom script: call RemoveLocation(udg_FO_PosCaster)
    • For each (Integer A) from 1 to FO_OrbCant, do (Actions)
      • Bucle: Acciones
        • Custom script: call RemoveLocation(udg_FO_PosDummy[bj_forLoopAIndex])
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • Si: Condiciones
        • FO_Time Igual a 5.00
      • Entonces: Acciones
        • Set FO_Time = 0.00
        • Detonador - Turn off (This trigger)
      • Otros: Acciones

 

[Rheiko]

Hi, you might want to post your trigger properly so it's more readable. You can follow this guideline to do so - How To Post Your Trigger

Since you're okay with it being not MUI, let's focus on how you can improve the current trigger instead.

-First Trigger-
You have assigned the position of caster into a variable here

• Set FO_PosCaster = (Position of FO_Caster)

but you did not use it inside the loop

• Set FO_PosDummy[(Integer A)] = ((Position of FO_Caster) offset by 300.00 towards (Real(FO_Angle[(Integer A)])) degrees)

you should replace (Position of FO_Caster) with FO_PosCaster instead.
=>

• Set FO_PosDummy[(Integer A)] = (FO_PosCaster offset by 300.00 towards (Real(FO_Angle[(Integer A)])) degrees)

• For each (Integer A) from 1 to FO_OrbCant, do (Actions)

You can replace Integer A with your own integer variable, it is much better that way.
=>

• For each FO_TempInteger from 1 to FO_OrbCant, do (Actions)

Take a look inside your loop, you have this which I have mentioned before

• Set FO_PosDummy[(Integer A)] = ((Position of FO_Caster) offset by 300.00 towards (Real(FO_Angle[(Integer A)])) degrees)

You are creating a new location handle everytime it loops, so you have to remove it to prevent memory leak. To do this, simply add this line at the very bottom inside your loop.

• Custom script: call RemoveLocation(udg_FO_PosDummy[udg_YourIntegerVariable])

I can also see that you're using arrays for the location variable and remove it with the second loop, but it's much less efficient that way. So you can turn your FO_PosDummy variable into a non-array variable and remove the second loop instead. The same applies to your FO_Angle variable.
=>

• Set FO_Angle = ((Integer A) x (360 / FO_OrbCant))
• Set FO_PosDummy = (FO_PosCaster offset by 300.00 towards (Real(FO_Angle)) degrees)
• -------- -------
• -------- Do your actions here -------
• -------- -------
• Custom script: call RemoveLocation(udg_FO_PosDummy)

-Second Trigger-
You should stop creating multiple loop functions whenever possible if you can do it in one loop.
The stuff I mentioned for your first trigger applies just the same for your second trigger.

 

[SapphireFrost]

Thank you so much for the reply, i'll do the changes in the trigger but i have a question.
The spell summons 2/3/4 dummys depending on the level and moves them to different points, for example, in level 1 summons 2 dummys, in angles 0 and 180, so ther position it's not the same and where they will move it's not the same point, that is why I use an array for the dolls and their position, if I do not use the array, how can I move each of them?

 

[Rheiko]

You can add them into a unit group, then enumerate all the units inside that unit group.
You can add them like this:

• For each (Integer TempInt) from 1 to FO_OrbCant, do (Actions)
  • Loop - Actions
    • Set FO_Angle = (TempInt x (360 / FO_OrbCant))
    • Set FO_PosDummy = (FO_PosCaster offset by 300.00 towards (Real(FO_Angle)) degrees)
    • Unit - Create 1 Dummy Esfera for (Owner of FO_Caster) at FO_PosDummy facing ((Real(FO_Angle)) + 90.00) degrees
    • Set FO_Dummy = (Last created unit)
    • Unit - Add a 5.00 second Generic expiration timer to FO_Dummy
    • Unit Group - Add FO_Dummy to FO_DummyGroup

To enumerate all the units inside the unit group, you do this:

• Set FO_PosCaster = (Position of FO_Caster)
• Unit Group - Pick every unit in FO_DummyGroup and do (Actions)
  • Loop - Actions
    • Set FO_Dummy = (Picked unit)
    • Set FO_PosDummy = (Position of FO_Dummy)
    • Set FO_Angle = (Angle from FO_PosCaster to FO_PosDummy)
    • Set FO_PosDummy2 = (FO_PosCaster offset by 300.00 towards ((Real(FO_Angle)) + 10.00) degrees)
    • Unit - Move FO_Dummy instantly to FO_PosDummy
    • Custom script: Call RemoveLocation(udg_FO_PosDummy)
    • Custom script: Call RemoveLocation(udg_FO_PosDummy2)
• ------- Don't forget to clear unit group when you're done -------
• If (All Conditions are True) then do (Then Actions) else do (Else Actions)
  • If - Conditions
    • FO_Time is equal to 5.00
  • Then - Actions
    • Unit Group - Remove all units from FO_DummyGroup
    • Set FO_Time = 0.00
    • Trigger - Turn off (This trigger)
  • Else - Actions
• Custom script: Call RemoveLocation(udg_FO_PosCaster)

It should be something like that.

I have to praise you for coming up with the solution of using array. It's great but for this particular case, we have:

• Unit Group - Pick every unit in YourUnitGroup and do (Actions)
  • Loop - Actions

which is a loop designed specifically for units inside a unit group. If you were using special effects instead of dummy units (which possible in 1.31.1 patch of the game and onwards), your method would have been nearly perfect.

 

[SapphireFrost]

Thanks again for your help.
One last thing, it's not necessary destroy the unit group? It not cause leaks?

Edit:
Oh, about the trigger posting, i have the world editor in spanish, i need to wrote in English the words that are in spanish for it posted correctly?

 

[Rheiko]

One last thing, it's not necessary destroy the unit group? It not cause leaks?

If you create a new unit group, it will leak. Only adding and removing units from a unit group however do not leak because they don't create a new unit group and this is what I did in my example. So keep in mind that it is necessary to destroy a unit group when you create a new unit group and you will overwrite it with something new later on.

You don't necessarily need to destroy a unit group right away, depending on how you use the unit group. If you know you're going to use that specific unit group later and you don't lose reference to it, then you don't need to destroy it. Because memory leak occurs only when you lose reference to it.

Here's some example of creating a unit group:

• Unit Group - Pick every unit in (Units in (Playable map area)) and do (Actions)
  • Loop - Actions

• Unit Group - Pick every unit in (Units within 512.00 of (Center of (Playable map area))) and do (Actions)
  • Loop - Actions

• Set FO_DummyGroup = (Units within 512.00 of (Center of (Playable map area)) matching (((Matching unit) is A structure) Equal to False))

You need to destroy these groups because they always create a unit group. To do that you can do something like this:

• Custom script: set bj_wantDestroyGroup = true
• Unit Group - Pick every unit in (Units in (Playable map area)) and do (Actions)
  • Loop - Actions

• Custom script: set bj_wantDestroyGroup = true
• Unit Group - Pick every unit in (Units within 512.00 of (Center of (Playable map area))) and do (Actions)
  • Loop - Actions

• Set FO_DummyGroup = (Units within 512.00 of (Center of (Playable map area)) matching (((Matching unit) is A structure) Equal to False))
• ------- Do your actions -------
• Unit Group - Pick every unit in FO_Damaged and do (Actions)
  • Loop - Actions
    • ------- Your action -------
• ------- and when you're finally done -------
• Custom script: call DestroyGroup(FO_DummyGroup)

Speaking of unit groups, I just realized I missed some stuff from the trigger you provided earlier.
So you have this in your trigger:

• Set FO_Damaged = (Units within 50.00 of FO_PosDummy[(Integer A)] matching ((((Matching unit) is Una estructura) Igual a False) and ((((Matching unit) is alive) Igual a True) and (((Matching unit) belongs to an enemy of (Owner of FO_Caster)) Igual a True))))

Well, this is actually alright BUT it's kind of painful to read. An alternative I would recommend is to do something like this
=>

• Custom script: set bj_wantDestroyGroup = true
• Unit Group - Pick every unit in (Units within 50.00 of FO_PosDummy) and do (Actions)
  • Loop - Actions
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • ((Picked unit) is A structure) Equal to False
        • ((Picked unit) belongs to an enemy of (Owner of FO_Caster)) Equal to True
        • ((Picked unit) is alive) Equal to False
      • Then - Actions
        • -------- Do your actions here --------
      • Else - Actions

It will greatly increase readability and it doesn't make much difference as far as I know.
But if you prefer to have it your way then you need to fix your method a little. Let's take a look at your code again

• Set FO_Damaged = (Units within 50.00 of FO_PosDummy[(Integer A)] matching ((((Matching unit) is Una estructura) Igual a False) and ((((Matching unit) is alive) Igual a True) and (((Matching unit) belongs to an enemy of (Owner of FO_Caster)) Igual a True))))
• Custom script: set bj_wantDestroyGroup = true
• Unit Group - Pick every unit in FO_Damaged and do (Actions)
  • Loop - Actions
    • ------- Your action -------

This will still leak. bj_wantDestroyGroup doesn't help in this case. The correct method would be to do this
=>

• Set FO_Damaged = (Units within 50.00 of FO_PosDummy[(Integer A)] matching ((((Matching unit) is Una estructura) Igual a False) and ((((Matching unit) is alive) Igual a True) and (((Matching unit) belongs to an enemy of (Owner of FO_Caster)) Igual a True))))
• Unit Group - Pick every unit in FO_Damaged and do (Actions)
  • Loop - Actions
    • ------- Your action -------
• Custom script: call DestroyGroup(udg_FO_Damaged)

Hope that clears things up.

Oh, about the trigger posting, i have the world editor in spanish, i need to wrote in English the words that are in spanish for it posted correctly?

Ah, yes, you might have to write it in english. You can also provide a screenshot if that's too much of a trouble, though.

 

[SapphireFrost]

I have a problem, i modify the loop trigger but now not work, it enters in the unit group only the first time and then in the nexts loops not.

this is what i done

• Frost Orb Loop
  • Events
    • Time - Every 0.50 seconds of game time
  • Conditions
  • Actions
    • Set FO_Time = (FO_Time + 0.50)
    • Set FO_PosCaster = (Position of FO_Caster)
    • Unit Group - Pick every unit in FO_GrupoDummy and do (Actions)
      • Loop - Actions
        • Set FO_Dummy = (Picked unit)
        • Set FO_PosDummy = (Position of FO_Dummy)
        • Set FO_Angle = (Integer((Angle from FO_PosCaster to FO_PosDummy)))
        • Set FO_PosDummy2 = (FO_PosCaster offset by 300.00 towards ((Real(FO_Angle)) + 10.00) degrees)
        • Unit - Move FO_Dummy instantly to FO_PosDummy2
        • Custom script: call RemoveLocation(udg_FO_PosDummy)
        • Custom script: call RemoveLocation(udg_FO_PosDummy2)
    • Custom script: call RemoveLocation(udg_FO_PosCaster)
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • FO_Time Igual a 5.00
      • Then - Actions
        • Unit Group - Remove all units from FO_GrupoDummy
        • Set FO_Time = 0.00
        • Trigger - Turn off (This trigger)
      • Else - Actions

 

[Uncle]

That trigger doesn't Add units to the Unit Group. We'd also need to see where you're actually adding the units to be certain.

But you're making a very big mistake with Fo_Time.

You should never check if a Real is Equal to an exact value after using Arithmetic on it:

• Set FO_Time = (FO_Time + 0.50)
• If (All Conditions are True) then do (Then Actions) else do (Else Actions)
  • If - Conditions
    • FO_Time Equal to 5.00

This is because Reals can lose precision after using Arithmetic, so FO_Time could end up being something like 4.999999 instead of 5.00.

Either change that variable to an Integer:

• Set FO_Time = (FO_Time + 1)
• If (All Conditions are True) then do (Then Actions) else do (Else Actions)
  • If - Conditions
    • FO_Time Equal to 10

Or reduce FO_Time by a small amount like 0.01:

• Set FO_Time = (FO_Time + 0.50)
• If (All Conditions are True) then do (Then Actions) else do (Else Actions)
  • If - Conditions
    • FO_Time Greater than or equal to 4.99

You could also start FO_Time at 0.01. Then you need to check if your variable is GREATER than OR equal to the target value, since you'll likely go above it and miss the exact value you were aiming for.