Electrified Wisps v1.11

Moderator · Oct 9, 2014

9th Jan.2016
IcemanBo:
Simple concept and works fine. Useful.
The lightning itself is not damaging, I would prefer to have them as optional.
Simple rotating dummy units that are damaging without lightnings would be cool, for me personal, too.
Though that is no downside, but just something I wanted to note.

Tank-Commander · Oct 9, 2014

puzzling, the ball(wisp) death trigger is clearly different from the others - why is that one so poorly coded by comparison?

Meatmuffin · Oct 9, 2014

Well I could merge it with the loop trigger, didn't think about it. Maybe the importer, who is more of a beginner, knows how to change the death effects/damage/AoE a little better. But does it really matter?

Tank-Commander · Oct 9, 2014

I was more referring to how it breaks standard conventions that the rest of the spell follows - location leaks, group leaks, incorrect event responce, special effect leaks Is all of this coded by you?

Meatmuffin · Oct 9, 2014

95%. DracoL1ch only helped to move some lightnings, other than that, everything's by me. If it's giving you sort of an eye-cancer, I can modify it, but I doubt that it's that much of a big deal.

BPower · Oct 9, 2014

I think you didn't understand what the Tank-Commander tried to point out.
The death event trigger violates basic rules for spell submissions and will automatically lead to a status: Need Fix
1. You have a location leak
2. You have a group leak
3. Aoe, damage and the effect path should be configuarable and not hardcoded.

4 (optional) dying unit could be stored into a global variable to avoid needless function calls ("dying unit" is a actually a function)
You should use TriggeringUnit instead, it refers to the same unit as dying unit does in this specific case.

Meatmuffin · Oct 9, 2014

v1.1 Modified death trigger, added memory cleaners and stuff. Thanks Tank-Commander and BPower!

Meatmuffin · Oct 9, 2014

v1.2 Actually imported the changes into hiveworkshop (silly me).

GywGod133 · Oct 10, 2014

Set Offset_Loc = (Position of (Casting unit))
change it into
Set Offset_Loc = (Position of (Triggering Unit))

» In condition, (Unit-type of (Triggering unit)) Equal to Ball Dummy, change into
(Unit-type of (Triggering unit)) Equal to Cnfg_Bounce_Model

» Set TempGroup = (Units within Cnfg_Bounce_DeathAoE of (Position of (Triggering unit)) matching (((Matching unit) belongs to an enemy of (Owner of (Triggering unit))) Equal to True))

The bold lettter is leak... you must store it in a variable

and btw, in TempGroup you must use ITE (If Then Else) instead and the Picked Unit in TempGroup need to store in a variable

» Special Effect - Create a special effect attached to the overhead of (Dying unit) using Abilities\Spells\NightElf\Starfall\StarfallTarget.mdl

WHITE, change it into Triggering Unit
GREEN, need to store in a variable instead....

Neksus · Oct 10, 2014

It's kinda good, me my self, a spell creater i can rate this 9.5/10, it's pretty well made by a beginner

Tal'Rarity · Oct 10, 2014

You should also move the wisp along with the caster, looks much better that way. Good job on this, just needs a bit more then it'll be perfect.

Meatmuffin · Oct 10, 2014

Updated.

You should also move the wisp along with the caster, looks much better that way. Good job on this, just needs a bit more then it'll be perfect.

Tried to do that, didn't know how

Tal'Rarity · Oct 10, 2014

Want my help? =)

Meatmuffin · Oct 10, 2014

Sure why not.

GywGod133 · Oct 10, 2014

TempGroup you must use ITE (If Then Else) instead

Like this..

Actions
- Set TempPoint = (Position of (Triggering unit))
- Set TempGroup = (Units within 300.00 of TempPoint)
- Unit Group - Pick every unit in TempGroup and do (Actions)
  - Loop - Actions
    - Set TempUnit = (Picked unit)
    - If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      - If - Conditions
        
        (TempUnit belongs to an enemy of (Owner of (Triggering unit))) Equal to True
      - Then - Actions
        
        Unit - Cause (Triggering unit) to damage TempUnit, dealing 100.00 damage of attack type Spells and damage type Normal
      - Else - Actions
- Custom script: call DestroyGroup( udg_TempGroup)
- Custom script: call RemoveLocation( udg_TempPoint)

and btw... remove this

Custom script: set udg_BallDeathUnit = null

Tal'Rarity · Oct 10, 2014

Hopefully better

GywGod133 · Oct 10, 2014

Time - Cnfg_Timer expires

new method....

interesting..

EDIT:

Set TempUnit = No unit
You dont need this..

and some parts of Ball Death Trigger need to store in a variable

Meatmuffin · Oct 10, 2014

No, no, no, no, no. I thought you meant to leave the same trigger, just making the wisps move along with me, not a new method. Not implementing this, sorry.

Tal'Rarity · Oct 10, 2014

AKA.GywGod133 said:
new method....

interesting..

EDIT:

Set TempUnit = No unit
You dont need this..

and some parts of Ball Death Trigger need to store in a variable

Yeah it does need that, saving as much memory as possible is ideal. Agreed, but I figured to leave some stuff left for him to fix or think about.

LITHUFFIN said:
No, no, no, no, no. I thought you meant to leave the same trigger, just making the wisps move along with me, not a new method. Not implementing this, sorry.

I did leave the same trigger, I fixed the rest of your code for you as well made the unit group more useable as well made the wisps move along with the caster. Nothing is new, only better. That's your choice, I can just use what I made for you for something else I will make later. :grin:

Though I am curious, where exactly is this new method?

Meatmuffin · Oct 10, 2014

Yeah, but they are moving in a different accord, they are going TO me not FROM me.

Tal'Rarity · Oct 10, 2014

LITHUFFIN said:
Yeah, but they are moving in a different accord, they are going TO me not FROM me.

I will fix that sometime soon. There was no other way to achieve this without re-doing it. So in a way it Is a new method after all, I apologize. I was hoping you would figure out how to change it.

gorillabull · Oct 10, 2014

Tank-Commander said:
I was more referring to how it breaks standard conventions that the rest of the spell follows - location leaks, group leaks, incorrect event responce, special effect leaks Is all of this coded by you?

are you saying this is a stolen system with minor modifications? XD

Tank-Commander · Oct 10, 2014

The struggling to fix the simple problems, and the inconsistency would suggest it, but I'm keeping an open mind

Meatmuffin · Oct 10, 2014

Just to prevent you from any further hesitations, the spell is not a ripoff. While making the death trigger and modifying it, I was in a bit of a rush, sorry for that.

Kazeon · Oct 12, 2014

Review emerges!

Triggers

- Set Offset_Loc = (Position of (Casting unit))
You forgot to remove that leak. Anyway, (Casting unit) should be (Triggering unit).
- Actions
  - -------- Number of balls created --------
  - Set Cnfg_Bounce_BallCount = 5
  - -------- Number of lightnings created (this should be equal to the number of balls created) --------
  - Set Cnfg_Bounce_LightningCount = 5
Just merge those two variables, I mean you should remove one of them. There is no point in keeping two variables that should have exact same value.
- Actions
  - Set MaxIndex = (MaxIndex + 1)
  - Set Caster[MaxIndex] = (Triggering unit)
  - Set Duration[MaxIndex] = Cnfg_Bounce_Duration
  - Set Lightning_count[MaxIndex] = Cnfg_Bounce_LightningCount
  - Set Speed[MaxIndex] = Cnfg_Bounce_Speed
  - Set Player[MaxIndex] = (Owner of Caster[MaxIndex])
  - Set TempReal = (Cnfg_Bounce_Formation x (Real(TempIndex)))
  - Set BallDegree[MaxIndex] = TempReal
  - Set TempPoint = (Offset_Loc offset by Cnfg_Bounce_OffsetDistance towards TempReal degrees)
Your variable names are too generic. Please, give them unique prefixes. Example: EW_MaxIndex or EW__MaxIndex. Just think about something that won't collide with users' variable names easily.
- Set Speed[MaxIndex] = Cnfg_Bounce_Speed
You can directly use variable Cnfg_Bounce_Speed, just remove variable Speed[MaxIndex]. Since you are not modifying each wisp's speed in the process.
- Set Player[MaxIndex] = (Owner of Caster[MaxIndex])
=>
- Set Player[MaxIndex] = (Triggering player)
- Set Lightning_count[MaxIndex] = Cnfg_Bounce_LightningCount
It's one unused variable, remove it.
- Unit - Create 1 Ball Dummy for Player[MaxIndex] at TempPoint facing BallDegree[MaxIndex] degrees
Ball Dummy should be changed to variable Cnfg_Bounce_Model
- Actions
  - Do Multiple ActionsFor each (Integer TempIndex) from 1 to Cnfg_Bounce_BallCount, do (Actions)
    - Loop - Actions
      - Set TempPoint = (Offset_Loc offset by Cnfg_Bounce_OffsetDistance towards TempReal degrees)
      - ....
  - Custom script: call RemoveLocation(udg_TempPoint)
=>
- Actions
  - Do Multiple ActionsFor each (Integer TempIndex) from 1 to Cnfg_Bounce_BallCount, do (Actions)
    - Loop - Actions
      - Set TempPoint = (Offset_Loc offset by Cnfg_Bounce_OffsetDistance towards TempReal degrees)
      - ....
      - Custom script: call RemoveLocation(udg_TempPoint)
Move leak removal inside the loop
- Actions
  - Lightning - Create a Cnfg_Bounce_LightningEffect lightning effect from source Offset_Loc to target TempPoint
    - Do Multiple ActionsFor each (Integer A) from 1 to Cnfg_Bounce_LightningCount, do (Actions)
      - Loop - Actions
        
        Set MultipleLightnings[Lightning_count[MaxIndex]] = Lightning[MaxIndex]
        
        Set Lightning[MaxIndex] = (Last created lightning effect)
        
        Lightning - Change color of Lightning[MaxIndex] to (1.00 0.00 0.00) with 1.00 alpha
MultipleLightnings[Lightning_count[MaxIndex]] is one unused variable, just remove it. And the loop itself is unnecessary, just remove it and move the actions outside the loop.

Though, the lightning's model, color, and alpha could be added into configuration trigger.
- Lightning - Change color of Lightning[MaxIndex] to (1.00 0.00 0.00) with 1.00 alpha
- Set Duration[TempIndex] = (Duration[TempIndex] - 0.03)
You should chose one between using duration variable or applying timed life to the wisps.
- Set BallDegree[TempIndex] = (BallDegree[TempIndex] - 3.00)
Value of -3.00 could be added into configuration trigger, called turn rate.
- Lightning - Move Lightning[TempIndex] to source (Position of Caster[TempIndex]) and target (Position of Ball[TempIndex])
Change (Position of Ball[TempIndex]) to variable Offset_Loc
(Position of Caster[TempIndex]) should be stored into separated variable for later leak removal. Currently it's leaked.
- Lightning - Destroy Lightning[MaxIndex]
=>
- Lightning - Destroy Lightning[TempIndex]
Then you need to deindex the Lightning variable too:
- Set Lightning[TempIndex] = Lightning[MaxIndex]
- Custom script: set udg_Lightning[udg_MaxIndex] = null
- Set Caster[TempIndex] = Caster[MaxIndex]
Nullify Caster[MaxIndex], it's an agent which may causes leaks
- Set Caster[MaxIndex] = No unit
- Set BallDeathTempPoint = (Position of (Triggering unit))
You can use only one temporary location variable (TempPoint), just remove variable BallDeathTempPoint
- Set BallDeathTempPoint = (Position of (Triggering unit))
- Set TempGroup = (Units within Cnfg_Bounce_DeathAoE of (Center of (Playable map area)))
(Center of (Playable map area)) should be changed to the temporary point/location. There is no wonder the wisps deal no damage on death. And it leaks.
- (BallDeathUnit belongs to an enemy of (Owner of (Triggering unit))) Equal to (==) True
(Triggering unit) and (Owner of (Triggering unit))) could be stored into a variable first.

Object datas & miscs:

Used models are not chosen wisely. Please, synergize them a bit. A wisp with holy missile and starfall death animation? Why not using a wisp explode effect ("Units\NightElf\Wisp\WispExplode.mdl")? Sounds more reasonable for me.
Change "Stats - Target allowed" to None
Wisps seem to be deformed when exceed the playable map area boundaries.

Rating:

I don't really like the visual effect of this spell, they are just not well-chosen. And I hope the lightning effect could do something more than becoming a simply effect-bloats. The code seems needing many fixes and optimizations. Object datas seem good enough for me. The plus point of this spell is easy to be imported. Also using Phoenix Fire ability is a neat idea to avoid extra bunch of codes.

2/5 for now and vote for Needs fix

kakuzu · Oct 13, 2014

Very good spell , nice work !

Meatmuffin · Oct 13, 2014

Updated, thanks to RD's review.

Sunreaver · Oct 14, 2014

Very Great I will check it out

Meatmuffin · Oct 14, 2014

Thanks for the positive feedback.

GywGod133 · Oct 26, 2014

Unit Group - Pick every unit in EW_TempGroup and do (Actions)
- Loop - Actions
  - Set EW_BallDeathUnit = (Picked unit)
  - If (All Conditions are True) then do (Then Actions) else do (Else Actions)
    - If - Conditions
      - (EW_BallDeathUnit belongs to an enemy of (Triggering Player)) Equal to True
  - Then - Actions
    - Unit - Cause (Triggering Unit) to damage EW_BallDeathUnit, dealing Cnfg_Bounce_DeathDamage damage of attack type Spells and damage type Normal
  - Else - Actions
    - Set EW_BallDeathUnit = No unit
    - Set EW_BallDeathUnit2 = No unit
Special Effect - Create a special effect attached to the Cnfg_Bounce_BDeathEffectLoc of (Triggering Unit) using Cnfg_Bounce_BallDeathSpecialEf
Special Effect - Destroy (Last created special effect)

->>>>

Unit Group - Pick every unit in EW_TempGroup and do (Actions)
- Loop - Actions
  - Set EW_BallDeathUnit = (Picked unit)
  - If (All Conditions are True) then do (Then Actions) else do (Else Actions)
    - If - Conditions
      - (EW_BallDeathUnit belongs to an enemy of (Owner of EW_BallDeathUnit2)) Equal to True
  - Then - Actions
    - Unit - Cause (EW_BallDeathUnit2) to damage EW_BallDeathUnit, dealing Cnfg_Bounce_DeathDamage damage of attack type Spells and damage type Normal
  - Else - Actions
    - Set EW_BallDeathUnit = No unit
    - Set EW_BallDeathUnit2 = No unit
Special Effect - Create a special effect attached to the Cnfg_Bounce_BDeathEffectLoc of (EW_BallDeathUnit2) using Cnfg_Bounce_BallDeathSpecialEf
Special Effect - Destroy (Last created special effect)

Cause the EW_BallDeathUnit2 = Triggering Unit

IcemanBo · Jan 7, 2015

Whisps should follow the caster. Fix this.
Make the spell support multiple levels. Also adopt the configuration trigger to multiple levels.
Trigger the AOE damaging instead of using phoenix fire. These are only few lines more, and user can config it to his needs and make it dependant from the ability level.
Store TriggeringUnit/TriggeringPlayer in temp variables in your cast trigger. It might be used multiple times because of the loop.
Effect should end if caster is dead or removed from game.
Make the final explosion onDeindex. And then you can just remove the dummy unit. No extra trigger is needed for it.
Damage- and attack type should be configurable.
Custom script: set udg_EW_Ball[udg_EW_MaxIndex] = bj_lastCreatedUnit
^No custom script needed. Make it GUI.
Null Ball[MaxIndex] onDeindex.
Nulling the lightning isn't necessarily needed. How ever, it doesn't hurt.

Needs Fix

Meatmuffin · Mar 30, 2015

Update consists of making the spell support multiple levels, made the wisps follow the caster by simply rotating around it and made a little more configurations.

IcemanBo · Apr 1, 2015

Hello, LITHUFFIN

nice you made multiple levels now and some other changes. Here is some feedback:

The config speed does not match the normal warcraft movement speed. It might be not clear what speed it actually is.
I don't like the damage is not adjustable for different levels at the moment.
You deindex if caster is dead. That's fine. But then the whisps also need to be removed, else all other doesn't make sense if they still damage nearby units.
-------- Created ball formation (circle is default) --------

Set Cnfg_Bounce_Formation = (360.00 / (Real(Cnfg_Bounce_BallCount)))
"Formation" is also kind of correct, but it's the angle difference between the dummies.
I would not clearly understand it by just reading this comment.
Some operations are fired unnecessarily more often onCast, because they are in loop. Move them out.
Also when doing so change
Set EW_TempPoint2 = (Position of (Triggering unit))
to
Set EW_TempPoint2 = (Position of (variable))
You leak a location onCast.
The tooltip might get some more love.
It looks a bit monoton and it's information is not level adapted
Please change names of some variables. Choose one prefix (you already did, "EW") + underscore + descriptive name.
Don't distinguish with config & other variables. Because "cnfg_" is not your system's prefix. Just let it be. So for example
Set Cnfg_Bounce_Ability = Electrified Wisps (Hotkey Q)
->
Set EW_Ability = Electrified Wisps (Hotkey Q)
Similar goes to your trigger names. "Configuration" is not identifical to be part of your system.
It might seem nitpicking, but we need some standards.

Meatmuffin · Jul 30, 2015

More or less a minor update, based on IcemanBo's points. I can't move functions out of the loop in the initialization, the whole spell falls apart.

Modux · Aug 2, 2015

Nice one. Keep it up

BPower · Aug 8, 2015

You have to be patient a bit, as I can't run world editor at the moment.
I will handle your resource as soon as possible.

Greetings BPower.

Meatmuffin · Dec 22, 2015

Quick update.

KILLCIDE · Dec 23, 2015

The effects on this look great!

Here are some pointers:

You don't have to keep recalculating EW_Speed on every cast since they will always be the same depending on the level. You can use a For Loop based on however many levels the user wants to have and just set the speed on init and then use a variable like Speed[_AbilityLevel[_MaxIndex]], kind of like what you do for Duration[]
Change EW_Player[] to Triggering Player from Owner of (unit); it is slightly faster
Preload the dummy and their abilities on init
EW_TempPoint2 leaks on cast
You should rearrange your loop to mimic what I said in this post
In your Unit Group function, you use both "EW_PickedUnits belongs to any ally of EW_Player[] Equal to False" AND "EW_PickedUnits Not equal to EW_Caster[]", but I can't seem to ever see a moment where both can ever be true? You can get rid of latter condition.
Since you're destroying EW_TempGroup right after its creation, you can use bj_wantDestroyGroup = true to save yourself a variable
EW_TempPoint leaks on deindex
You don't have to null EW_Ball[] or EW_Caster[]
Change Turn off (This trigger) to Turn off Electrified Wisps Loop <gen>. Again, slightly faster

Meatmuffin · Dec 23, 2015

You don't have to keep recalculating EW_Speed on every cast since they will always be the same depending on the level. You can use a For Loop based on however many levels the user wants to have and just set the speed on init and then use a variable like Speed[_AbilityLevel[_MaxIndex]], kind of like what you do for Duration[]]

True that, my mistake.

EW_TempPoint2 leaks on cast

EW_TempPoint leaks on deindex

I don't see how they leak to be honest.

Since you're destroying EW_TempGroup right after its creation, you can use bj_wantDestroyGroup = true to save yourself a variable

Alrighty, thanks.

In your Unit Group function, you use both "EW_PickedUnits belongs to any ally of EW_Player[] Equal to False" AND "EW_PickedUnits Not equal to EW_Caster[]", but I can't seem to ever see a moment where both can ever be true? You can get rid of latter condition.

Seems logical, although I just wanted to be extra sure, but I guess that's practically useless.

I'll fix the minor leaks as well.

--

Much appreciated for the feedback, review and the quick replies.

KILLCIDE · Dec 23, 2015

LITHUFFIN said:
I don't see how they leak to be honest.

They leak because you're creating a new object with them by using "Set variable", yet you never use call RemoveLocation() to get rid of the reference. It's the same exact reason you do this in your loop:

Set EW_TempPoint = (Position of EW_Caster[EW_TempIndex])
Set EW_BallDegree[EW_TempIndex] = (EW_BallDegree[EW_TempIndex] - EW_Speed[EW_TempIndex])
Set EW_TempPoint2 = (EW_TempPoint offset by EW_OffsetDistance[EW_AbilityLevel[EW_TempIndex]] towards EW_BallDegree[EW_TempIndex] degrees)
Lightning - Move EW_Lightning[EW_TempIndex] to source (Position of EW_Caster[EW_TempIndex]) and target EW_TempPoint2
Custom script: call RemoveLocation(udg_EW_TempPoint)
Unit - Move EW_Ball[EW_TempIndex] instantly to EW_TempPoint2, facing EW_BallDegree[EW_TempIndex] degrees
Custom script: call RemoveLocation(udg_EW_TempPoint2)

May I ask why you think it DOESN'T leak?

I also actually just realized an error here:

Lightning - Move EW_Lightning[EW_TempIndex] to source (Position of EW_Caster[EW_TempIndex]) and target EW_TempPoint

Change (Position of EW_Caster[]) to EW_TempPoint

Meatmuffin · Dec 23, 2015

Oh I see, I was looking for a different kind of leakage, my bad.

Meatmuffin · Dec 23, 2015

Quick update, a big thanks to KILLCIDE

KILLCIDE · Dec 24, 2015

Just a couple of things I missed on the first review

You're currently relying on the expiration timer of the dummies to disappear the same time when the duration is over. However, the caster can die during the duration. This means that if this were to happen, the dummies will stay there until their expiration timer ends. Instead of giving them an expiration timer, just use Unit - Remove EW_ Ball[] when it is time to deindex
I don't see the need to check if EW_Ball[] is alive before moving them. The only way they can "die" is when the spell is over. With that situation in mind, you wouldn't need to move them anymore

Meatmuffin · Dec 24, 2015

KILLCIDE said:
Just a couple of things I missed on the first review

You're currently relying on the expiration timer of the dummies to disappear the same time when the duration is over. However, the caster can die during the duration. This means that if this were to happen, the dummies will stay there until their expiration timer ends. Instead of giving them an expiration timer, just use Unit - Remove EW_ Ball[] when it is time to deindex

I don't see the need to check if EW_Ball[] is alive before moving them. The only way they can "die" is when the spell is over. With that situation in mind, you wouldn't need to move them anymore

Thanks, updated.

KILLCIDE · Dec 24, 2015

Looks good to me

I give this a 3.5/5, so I'll round it up to 4. I really liked the idea of giving the dummies the phoenix ability.

Meatmuffin · Dec 24, 2015

4 is more than I expected, thanks a lot

IcemanBo · Jan 7, 2016

onCast trigger there is no need to store some stuff multiple times. Like (Triggering Unit), (LevelOfAbility), etc.
I don't understood this:

I can't move functions out of the loop in the initialization, the whole spell falls apart.

Click to expand...
The damage while looping should be triggered and configurable.
It's literaly like only PickingUnitsInRange and deal x damage for enemies.
And less object editor data is mostly better for spells.

Meatmuffin · Jan 7, 2016

The damage while looping should be triggered and configurable.
It's literaly like only PickingUnitsInRange and deal x damage for enemies.
And less object editor data is mostly better for spells.

It is actually configurable in object editor.
A lot of people here who have tested and used the spell, including me, have loved the phoenix fire damage as a way to avoid a block of extra functions. It also allows the person to configure the missile speed of the damaging projectile AND if a user wants to add DOT, they can just simply increase the duration of PF.

onCast trigger there is no need to store some stuff multiple times. Like (Triggering Unit), (LevelOfAbility), etc.

If I move out, for i.e., Set <variable> = (Triggering Unit), I also have to move out the MaxIndex integer increment because the triggering unit variable is arrayed and requires the variable MaxIndex to make it MUI. If I move out MaxIndex as well, it is only calculated once because it's outside the loop and the loop affects one of the five wisps, the other 4 are clustered in 1 place and are messed up in a lot of ways. I don't know if there is a way to fix it.

IcemanBo · Jan 7, 2016

Btw, the score points and sight radius should be removed for the dummy.

About the dummy ability:

+ Caster deals damage. That means that if a unit dies, then the "dummy unit" won't be the Killing Unit.
+ Directly dealing damage instead of firing a very fast missle. (unit would not count as "dead" even it will maybe be)
+ Config is better because it is in your system and not in object editor.
+ Less object editor data, and so easier to import.

- some more lines to code

I see that it might be a cool thing easily to allow DOT, but honestly, this is not even mentioned or implemented somewhere atm.
But then again the original caster is not damaging the unit and so using triggers is better here again. :/

About the other issue. TempVariables are the way to go. However, the more dummies are created the more efficient it is.

Electrified Wisps v1.11

Electrified Wisps v1.11 (Map)

Moderator

Moderator

Attachments

Similar threads