Zephyr Strike v1.2b

[Moderator]

12th Dec 2015
IcemanBo: Too long as NeedsFix. Rejected.

Zephyr Strike v1.2b | Reviewed by Maker | 6th Jun 2013

NEEDS FIX

You leak ZS_CasterLoc because you set it in ZS Cast trigger and overwrite it in the looping trigger There is an unneeded Custom script: call RemoveLocation(udg_ZS_TempPoint) in the looping trigger, twice There is an unneeded Custom script: call RemoveLocation(udg_ZS_TempPoint2) in the looping trigger You don't need TempPoint2 variable, use TempPoint instead ZS_CasterLoc, ZS_PickedUnit, ZS_PickedUnitLoc do not need to be arrays There is no need to set TempUnit = Picked unit when you damage units, you only call Picked unit function once It is useless to play the caster's animation multiple times in the loops, only play it once

[tr]

Old review Zephyr Strike v1.1a | Reviewed by Maker | 2nd Jun 2013 NEEDS FIX Load the ability level from a variable in the looping trigger You are leaking ZS_TempPoint in the looping trigger several times You are overwriting ZS_CasterLoc data in the looping trigger and therefore it leaks This is not needed Unit Group - Remove ZS_TempUnit from ZS_TempGroup1 [*]I don't think you should use both the jump system and the knocback system [tr] Zephyr Strike v1.0 | Reviewed by Maker | 28th May 2013 NEEDS FIX Abilities must support more that one level You are overwriting ZS_CasterLoc data in the looping trigger and therefore it leaks The jump system adds/removes Strom Crow ability, what if the unit should have that ability The jump system doesn't allow units to jump straight up The jump system should alter flying height at rate of 0 The jump system uses a rudimentary tree desroying method The learn tooltip is missing the hotkey You could use a variable for the ability The dummies do not need o give 800/600 vision The action to play caster's attack 1 animation doesn't do anything Channel should generally not be configured to disable other abilities SetUnitPositionLoc gives the unit stop order. Basically the unit is not casting the ability anymore. Maybe you could use a pathing check and use SetUnitX/Y instead You use array variables for temporary locatons and groups in the looping trigger for no apparent reason If you are doing actions regardless of he trigger going to THEN or ELSE, then take the actions out of the if/then/else [tr]

 

[MasterTrainer]

~Reserved

 

[deathismyfriend]

this leaks.

• Set ZS_CasterLoc[ZS_MaxIndex] = (Position of ZS_Caster[ZS_MaxIndex])

because u reset it here and dont remove it b4 u set it to a new point.

• For each (Integer ZS_CurrentIndex) from 1 to ZS_MaxIndex, do (Actions)
• Loop - Actions
• Animation - Change ZS_Caster[ZS_CurrentIndex]'s animation speed to 200.00% of its original speed
• Animation - Play ZS_Caster[ZS_CurrentIndex]'s attack 1 animation
• Set ZS_CasterLoc[ZS_CurrentIndex] = (Position of ZS_Caster[ZS_CurrentIndex])

 

[MasterTrainer]

this leaks.

• Set ZS_CasterLoc[ZS_MaxIndex] = (Position of ZS_Caster[ZS_MaxIndex])

because u reset it here and dont remove it b4 u set it to a new point.

• For each (Integer ZS_CurrentIndex) from 1 to ZS_MaxIndex, do (Actions)
• Loop - Actions
• Animation - Change ZS_Caster[ZS_CurrentIndex]'s animation speed to 200.00% of its original speed
• Animation - Play ZS_Caster[ZS_CurrentIndex]'s attack 1 animation
• Set ZS_CasterLoc[ZS_CurrentIndex] = (Position of ZS_Caster[ZS_CurrentIndex])

Thanks! I will update the spell shortly.

 

[Doomlord]

You should go with Maker's Jump lib. It is more versatile imo and besides I heard that Paladon's sys had some issues.

 

[MasterTrainer]

You should go with Maker's Jump lib. It is more versatile imo and besides I heard that Paladon's sys had some issues.

Thanks

That is now my plan, but before I go implementing it I have to re-edit most of the variables and re-add those variables which I removed the arrays from.

Also, making it into a multi level ability will take me a while, because I spent so much time making this a level 1 ability, till I found out the Spell section's standards.

 

[deathismyfriend]

u could make this multi lvl ability by doing it the way i suggested. Then u will just have to add one more variable for each field.

 

[Doomlord]

A few things that could be improved:

1/ If an action is repeated in the Then and Else, takes it outside of the If.
2/ On a side note, a dummy isn't needed. You can go with special effects instead. Elaborate if the dummy serves another purpose.

3/ Stomp Circle part of the code lacks some configurability.

This is my first review so feel free to object if needed.

 

[MasterTrainer]

1/ I'm still trying to figure that out, but I will do so after I've settled down every variable from 1 level to multi-level variables

2/ I shall go with special effects instead, since I now know that dummies in this case are a waste of object

3/ I will make that part more configurable too.

@deathismyfriend I will do it here also.

Thanks you two!

And thank you too Maker

EDIT: I take back my #2 answer and change it to: 'I use the dummies because they are needed to cast the dummy impale'

 

[Doomlord]

Tip of the day: Do you know that you only need 1 dummy with 0 cast point, animation/cast backswing and movement speed to cast a 0s cd, non-channeling spell continuously as if there are multiple dummies? Furthermore, you can use only 1 dummy for all spell instances (I am yet to test this but it should be possible in theory).

 

[MasterTrainer]

That may be so, but still 1 dummy - 1 model file. But then again, I could make it have a none.mdl and just attach a special effect to it's origin.

Okay, that's what I will do. Thanks Doomlord!

 

[Doomlord]

You can't attach a model to none.mdl. I am referring to invisible caster dummies only, not effect dummies.

 

[MasterTrainer]

Oh then I shall use the dummy.mdx models I've been seeing lately on projectile systems.
And technically in this spell the effect dummies are the caster dummies.

 

[Doomlord]

How 'bout stick with special effect and use invisible caster dummy for casting? Wouldn't it be neat?

 

[MasterTrainer]

Yeah; your right... that way I only use one dummy. Okay I will do that instead.

Thanks again

UPDATE:I have un-arrayed the Temporary points and groups in the loop, I will now start making it a multi-level ability.

 

[Doomlord]

No problemo. Glad I helped

 

[MasterTrainer]

@deathismyfriend sorry looks like I'll just need to stick to three levels for this one, because adding more will take more time than expected. I'll just do your suggestion for my other spell, AHL.

 

[deathismyfriend]

it may take more time to do it my way but it is a lot better. plus i wouldnt want to make this spell have 10 lvls lol that means a very long config trigger.

 

[MasterTrainer]

Thanks deathismyfriend, but I'll just stick to 3 levels for now.

UPDATE: I have updated the spell with Maker's Jump System, though there is still a problem with the code because the units that are lifted off the ground arrive at one single point in the map.

I also removed the dummies' 600/800 vision, though pretty soon I will remove the dummies themselves, but I just need to fix the jump. Also a new problem has arisen, now since the jump was imported the special effect dummies are no longer shown, so I'm fixing that too.

UPDATE 2: I have fixed all known bugs (i.e. bugs posted above) and I am now working on making the spell's configurables. Also, I am now making the spell switch from effect dummies to plain sfx. Also I have successfuly imported Maker's Jump System, thanks to him because he helped me also on that particular problem.

UPDATE 3: Version 1.1a is up and running, included therein are Maker's jump system, a multi-level spell, and most corrections after Maker's review.

 

[pOke]

If you go to the spell breakers and spam it and the spell breakers can fly go off the map making it it can crash , got it to happen twice.

Cool spell though