Rain of Cold Arrows v1.04

[Moderator]

Rain of Cold Arrows v1.04 | Reviewed by BPower | 15.06.2015 [COLOR="gray" [COLOR="gray" [COLOR="gray" [COLOR="gray" [COLOR="gray" [COLOR="gray" Concept[/COLOR]] The caster is jumping backwards in the air, while shooting a salve of cold arrows on a target area. Optionally slows all damaged targets. Optionally destroys nearby trees. Code[/COLOR]] The spell is MUI and working. Currently one unit leak exists. There are some things, which could have been done better: You leak a unit handle in CTlExpire ( d ) What happens if the caster dies? Fix this! private struct Fire --> private struct Fire extend array In order to enumerate trees a static rect is enough, currently you use an array. Same applies for unit enumeration --> Use just one static unit group. The angle of creation of each missile does not match the fly angle. This can look fishy. If you create a secondary death effect for each arrow, then use RemoveUnit() and not KillUnit() for the main arrow. PauseUnit() comes along with unwanted side effects. Can't you use a combination of UnitPropWindow and TurnSpeed? I'm not against PauseUnit() in all cases, but it should be avoided for public resources. PauseUnit() freezes the units animation, aslong as no new command is given to that unit What is the meaning of prefix "F" and "S" in the globals i.e F_SPEED? Please use complete variable names, so users understand directly their purpose. Ammout is written with 1 "m" Objects[/COLOR]] Object data is ok. There are 4 files in the import manager, which are not related to the spell. Effects[/COLOR]] Effects sometimes looks fishy, as the facing of each arrow does not match with it's fly angle. Rating[/COLOR]] CONCEPT CODE OBJECTS EFFECTS RATING STATUS 3/5 3/5 4/5 2/5 3/5 NEED FIX Links[/COLOR]] Missile allows to easily create projectile like spells. It's more a hint, than a real recommendation from my side. [COLOR="gray"

[/TD]

 

[edo494]

Provide links to required libraries(its not so easy to find them)

Jump.onCast is completly useless function and can(and should) be inlined into onRun, where you just define the arguments as local variables.

You call Fire.onFire inside Jump in the CTL module method, so you most likely generate trigger evaluations -> move struct Fire above struct Jump(Fire struct does not depend on Jump in any way, shape or form)

Im too tired for algorithm checking, but I assume it works fine

 

[Chaosy]

Shit. It's another one of those "explains every single line in the code" spells.

 

[BPower]

Shit. It's another one of those "explains every single line in the code" spells.

I agree that spacing and comments here and there are very nice and help read-ability in many cases!

First of all IsDestructableTree is not written by Bribe!

struct Fire should be private.

GroupUtils doesn't seem important to me for your spell. You could also use one static group handle instead.

Gonna check the demo map.

 

[BPower]

I gave you a detailed review for your spell. You might want to defend the usage
of PauseUnit() here in the comments as we normally don't allow it in public spells.

If you have any questions, fell free to ask.

Please do not update your submission until you changed, what I pointed out.
Not like in your last spell submission.

EDIT:
You can either create each arrow dummy unit in struct Fire instead of in struct Jump.
Just pass in desired x/y coordinates and use the angle you calculate in onFire for the dummy unit.

Another not so good solution would be to adjust the angle after creation, by using SetUnitFacing()

I know why you are using PauseUnit(). Let's see what IcemanBoo says to this.

 

[kawas11]

Ok so. Still don't know what about

PauseUnit()

"Effects sometimes looks fishy, as the facing of each arrow
does not match with it's fly angle."

This is real problem. I was trying to calculate angle in Fire struct but effect is the same arrows spawn like this:

*
*
*
*

static method onFire takes player p, integer lvl, real tx, real ty, real dHeight, real crx, real cry returns nothing
        
            local thistype this = create()
            
            local real AoE = GetAoE(lvl)
            local real RT
            
            set owner[this] = p
            
            /*Arrow destination location*/
            
            set targetx[this] = tx + AoE * Cos(GetRandomReal(0, 360))
            set targety[this] = ty + AoE * Sin(GetRandomReal(0, 360))
            
            set angle[this] = Atan2(targety[this] - cry, targetx[this] - crx)
            
            set dummy[this] = CreateUnit(owner[this], DUMMY_CODE, crx, cry, angle[this] * bj_RADTODEG)
            
            /*Arrows distance traveled*/
            
            set distance[this] = GetDist(crx, cry, targetx[this], targety[this])
            
            set speed[this] = FIRE_SPEED
            
            set damage[this] = GetDamage(lvl)
            set damageGroup = CreateGroup()
            
            /*Calcultaing time needed for arrows reach target location*/
            
            set RT = distance[this] / speed[this]
            set RT = RT * 0.031250
            
            /*Calculating height decrease based on arrows time needed to reach target location*/
            
            set height[this] = dHeight
            set heightDec[this] = height[this] / (RT / 0.031250)
            
            /*Tree kill rect*/
            
            static if DESTROY_TREE then
                set treeRect = Rect(0, 0, 0, 0)
            endif
            
            /*Slow apply dummy*/
            
            call UnitAddAbility(SLOW_DUMMY, SLOW_CODE)
            call SetUnitAbilityLevel(SLOW_DUMMY, SLOW_CODE, lvl)
        
        endmethod

 

[.OmG.]

Isn`t it better to launch arrows from the ground but parabolic?

Edit: however, i like the knocking archer back idea