Psionic Destruction v1.4

[Moderator]

8th Jan 2016
IcemanBo: With the effect on caster it looks better now. Can be useful.

Psionic Destruction v1.1 | Reviewed by BPower | 01.08.2015 [COLOR="gray" [COLOR="gray" [COLOR="gray" [COLOR="gray" [COLOR="gray" Concept[/COLOR]] "Death by electrocution" The concept itself is very simple, but I can see this spell beeing useful for some users. Good Job. Code[/COLOR]] The spell is MUI, leakless and working. Very simple code. Place comments with reason. Not every line requires explanation. Damagetype and Attacktype don't have to be arrays in my opinion. Instead of using two groups, you could also extend your filter function and go only with one group. ( understood? ) Objects[/COLOR]] Object data seems to be ok. Effects[/COLOR]] A simple lightning effect. Nothing special, but fits to the descriptions. Rating[/COLOR]] CONCEPT CODE OBJECTS EFFECTS RATING STATUS 2/5 3/5 4/5 2/5 2.5/5 APPROVED

 

[BPower]

Easy concept of coding, but looks properly done. Good Job.

In future I would recommend to leave out comments which are obvious to get by the commented operation.
Only comment blocks which are harder to get at a glance or really important for you/user.

i.e. "This sets all picked units" is not a useful comment. ( the function unit == PickUnit is obvious )

Before approving I have to check the demo map. Overall approximatly a rating of ~3.

Cheers.

 

[GywGod133]

Not Bad

> Melee Map-Friendly

 

[IcemanBo]

You leak unit group. The "set bj_wantDestroyGroup" does not help you here.
You can read it up here how to remove unit group properly. Feel free to ask in case.

Why do we need this:

Unit Group - Pick every unit in PD_Group2 and do (Actions) Loop - Actions -------- This picks a random unit from this group. -------- Set PD_RandomUnit = (Random unit from Group)

1. Why you randomize more than once?
2. You don't need the second unit group. You can remove all units from first unit group that don't pass the filter, and then take a random unit just from group1.

Fix the leak.

 

[KILLCIDE]

If you're setting PD_Group to a variable, you should just use this to remove the leak.

• Custom script: call DestroyGroup (udg_PD_Group)

I would have also suggested to remove your second unit group, but IcemanBo has already covered that. I also think doing it this way will prevent the damage from stacking on one unit which is a good thing!

 

[Meatmuffin]

Quick update, consists of making the code more eye-friendly, fixing some leaked things that the reviewers pointed out (thanks) and simplified some of the variables' names.

 

[KILLCIDE]

A rather simple spell, but it can be useful! As BPower said, try not to have so many comments in the triggers. Most people reviewing the spell will already know what you are trying to do. For example, you don't need to have a comment explaining what your For Loop does, or why you have call RemoveLocation scripts.

• Change PD_Player[] to (Triggering Player) from Owner of (unit). Triggering Player is slightly faster (;
• Make your loop look like this so that spell deindexes when it has to and not at the next iteration. A wise man told me it's not nice to have actions run before the spell needs to deindex. It should only deindex when it is time to deindex.
  

  Psionic Destruction Loop Events Time - Every 0.20 seconds of game time Conditions Actions For each (Integer PD_TempIndex) from 1 to PD_MaxIndex, do (Actions) Loop - Actions Set PD_Duration[PD_TempIndex] = (PD_Duration[PD_TempIndex] - 0.20) If (All Conditions are True) then do (Then Actions) else do (Else Actions) If - Conditions (PD_Caster[PD_TempIndex] is alive) Equal to True PD_Duration[PD_TempIndex] Greater than 0.00 Then - Actions -------- loop actions -------- Else - Actions -------- deindex --------

• You don't need to null PD_RandomUnit
• Why do you create the special effect on a location instead of having it attached to the unit? It looks really awkward if they move too fast and the effect appears behind them
• I think you should add a config option to set how often the spell searches and damages a random enemy unit instead of having it every 0.2 seconds. This will also allow you to have a loop of 0.03 seconds and give users the flexibility of how they want the spell to act
• Change Turn off (This trigger) to Turn off Psionid Descrtion Loop <gen>. Again, slightly faster (;

 

[Meatmuffin]

1. Hmmmm, I always used owner of <unit> and nobody batted an eye on previous spells, will change, thanks.
2. I do deindex only when it's time, it's just as you said, didn't understand your point.
3. Yes, in fact, I do, since if I don't and while the spell is in action and I go out of range, the last randomed unit will get damaged for the duration.
4. You meant point, yeah, but I want to keep it this way.
5. Okay, thanks.
6. Same dilemma as in the first point of the review, but will do, thanks.

 

[LordDz]

Couldn't the whole trigger just be replaced with immolation?

 

[KILLCIDE]

Hmmmm, I always used owner of <unit> and nobody batted an eye on previous spells, will change, thanks.

There is nothing wrong with Owner of (unit), I'm just saying it's slightly faster

I do deindex only when it's time, it's just as you said, didn't understand your point.

Let me visualize it for you: Imagine a spell duration of 3 seconds with a loop timer of 1 second

• With your loop:
  
  1 second has passed -> check if spell duration is over -> no, add 1 second (total: 1 second) to spell duration and continue to run loop -> 2 seconds has passed -> check if spell duration is over -> no, add 2 seconds (total: 3 seconds) and continue to run loop -> 4 seconds has passed -> check if spell duration is over -> yes, deindex​
• With my proposed loop:
  
  1 second has passed -> add 1 second to spell duration (total: 1 second) and check if spell duration is over -> no, continue to run loop -> 2 seconds has passed -> add 2 seconds to spell duration (total: 3 seconds) and check if spell duration is over -> yes, deindex.​

Do you see how you go through 4 seconds of a spell duration meant for 3 seconds with your setup? My proposal makes the spell end at 3 seconds, not at the next loop iteration. It's not really a big deal, but there is a possibility of getting unwanted results with loops that act like this. I'm also just trying to improve your triggering.

Yes, in fact, I do, since if I don't and while the spell is in action and I go out of range, the last randomed unit will get damaged for the duration.

Ahh my mistake. I guess you can just use Unit Group - Remove unit from PD_Group instead of nulling, but I'm not sure which is better.

You meant point, yeah, but I want to keep it this way.

Location and point are the same thing, hence why in the variable editor they are called points, yet we use "call RemoveLocation" and not "call RemovePoint". Anyway, it was just a suggestion, so no worries.

 

[Meatmuffin]

Couldn't the whole trigger just be replaced with immolation?

No, since it attacks random selected units in AoE, not all units in AoE.

Let me visualize it for you: Imagine a spell duration of 3 seconds with a loop timer of 1 second
With your loop:
1 second has passed -> check if spell duration is over -> no, add 1 second (total: 1 second) to spell duration and continue to run loop -> 2 seconds has passed -> check if spell duration is over -> no, add 2 seconds (total: 3 seconds) and continue to run loop -> 4 seconds has passed -> check if spell duration is over -> yes, deindex
With my proposed loop:
1 second has passed -> add 1 second to spell duration (total: 1 second) and check if spell duration is over -> no, continue to run loop -> 2 seconds has passed -> add 2 seconds to spell duration (total: 3 seconds) and check if spell duration is over -> yes, deindex.

Do you see how you go through 4 seconds of a spell duration meant for 3 seconds with your setup? My proposal makes the spell end at 3 seconds, not at the next loop iteration. It's not really a big deal, but there is a possibility of getting unwanted results with loops that act like this. I'm also just trying to improve your triggering.

So basically I would just change the duration before the ITE block in the loop? I guess it makes sense.

Thanks for the review, by the way, appreciated.

 

[Meatmuffin]

Quick update, changes can be seen in changelog, thanks KILLCIDE

 

[KILLCIDE]

I think you misunderstood me when I suggested the new configurable option xD

Right now, your spell is hardcoded to find and damage a random enemy unit in range every 0.2 seconds. Personally, I think a loop timer of 0.2 seconds is too long. On top of that, a map maker might want to change it and have it damage a random enemy unit every 1 second, or even every 0.05 seconds. They wouldn't be able to do that unless they understood triggers and change what you hardcoded. To avoid this, you can just add 2 new variables to your spell:

• DamageFrequency (real) - You can have a multi-level or just single level variable that lets a user decide when it is time to deal damage. For your case, you want 0.2 seconds
• CounterDamage[] (real) - CounterDamage[] works like PD_Duration[]; it will keep track of the time passed. You will need to index + deindex it when it is time

Psionic Destruction Loop Events Time - Every 0.03 seconds of game time Conditions Actions For each (Integer PD_TempIndex) from 1 to PD_MaxIndex, do (Actions) Loop - Actions Set PD_Duration[PD_TempIndex] = (PD_Duration[PD_TempIndex] - 0.03) If (All Conditions are True) then do (Then Actions) else do (Else Actions) If - Conditions (PD_Caster[PD_TempIndex] is alive) Equal to True PD_Duration[PD_TempIndex] Greater than 0.00 Then - Actions Set CounterDamage[] = CounterDamage[] + 0.03 If (All Conditions are True) then do (Then Actions) else do (Else Actions) If - Conditions CounterDamage[] Greater than or equal to DamageFrequency Then - Actions Set CounterDamage[] = 0.00 -------- find random enemy unit and damage target -------- Else - Actions -------- it is not time to deal damage; do nothing -------- Else - Actions -------- deindex --------

You can also go as far as letting the user decide how many random units get damage.

 

[Meatmuffin]

Right now, your spell is hardcoded to find and damage a random enemy unit in range every 0.2 seconds. Personally, I think a loop timer of 0.2 seconds is too long. On top of that, a map maker might want to change it and have it damage a random enemy unit every 1 second, or even every 0.05 seconds. They wouldn't be able to do that unless they understood triggers and change what you hardcoded. To avoid this, you can just add 2 new variables to your spell:

Doesn't PD_Period allow you to change the frequency?

 

[KILLCIDE]

Doesn't PD_Period allow you to change the frequency?

Well, sort of... but it is currently the same value as your loop timer, and like I said, a loop timer of 0.2 seconds can be ugly. You should always try to use really small loop timers like 0.03 seconds to get "smooth" movements and effects. PD_Period should be changed to 0.03, and you will need to give a variable like DamageFrequency to 0.2 so that your original spell integrity is kept. The biggest pro to this is that you now give users extreme flexibility with your spell.

 

[Meatmuffin]

Well, the point of this spell was supposed to be a recreation of a cool ability in a certain ORPG, it had the loop of 0.2 seconds, so I left it that way. The spell itself is not too complicated, so the loop time doesn't matter that much, it doesn't require any smoothness at all. If a map maker decides to implement this in their map, they can just change the frequency with PD_Period variable in the configuration trigger and no additional code/variables is needed. Like I said, the point of this spell was to keep it simple.

--

I do think that your methods are efficient, but I don't think they're this ability's material. Thanks for the suggestion tho =P

 

[IcemanBo]

Would you like to add some visual effect or anything markable for caster to see if the effect is currently activated?

 

[Meatmuffin]

Actually haven't thought about that since the spell in Twilight's ORPG (This spell was recreated from that spell) didn't have one, but having an effect for that seems pretty cool and logical. Will do.

EDIT: Updated.