Warlike Spirit v1.1

[Moderator]

Submission: Warlike Spirit v1.1 Date: 18.05.2015 Status: Approved Rating: 4/5 Link Post Moderator: IcemanBo

 

[Almia]

OMG.

Why would you code functions like that? (Those spacings).

I can't review your Zinc code like that.

 

[bowser499]

Okay, let's analyze this spell properly.

Visual:
Volchachka told me his opinion about visuals. I'm not very good at rating these since I love every flashy eyecandy. ^^

Effects are 10/10. Swift execution, nice idea, good harmony, no effect overloading and stable FPS rate on them.

Coding:

• In Configuration section of your code there are following things which can be improved:
  • PERIODIC = .0312500 is better to be typed without unnecessary zeros, just like: .03125.
  • constant attacktype ATTACK_TYPE, constant damagetype DAMAGE_TYPE and constant weapontype WEAPON_TYPE should definetly be private. Else your spell may be incompatible with something that somebody implemented in the map. For example, in his own damage system he wants something names ATTACK_TYPE to be public.
  • private constant group G is not a good name for global variable because it's not too understandable for end user (and no matter that it's non-configurable).
  • private constant real inBetween = 360. / SPIRIT_COUNT * .0174533;, if it's constant, must be simplified as much as its possible. For the compiler, a/b*c is same with a*c/b (to make it as a/(b*c) for compiler you'd placed brackets. So you can calculate the value of 360*0.0174533 = 360*PI/180 = 2*PI which is equal to 6.283185. With the way I have shown, in your case, you can get much more precise result than just reapproximating the approximated. The direct calculation'd given you 6.283188.
  • You do not need the GetUnitTypeId(filterUnit) != 0 comparison because only units which do not exist in the map have the unit type 0. I did a test on a blank 32x32 map:
    
    Tested Code

    function CheckUnitWithZeroType takes nothing returns nothing
        local unit  testUnit  = CreateUnit(Player(0),0,0,0,0)
        local group testGroup = CreateGroup()
        local unit  groupUnit
        
        if IsUnitType(testUnit,UNIT_TYPE_DEAD) then
            call BJDebugMsg("The unit with type 0 is dead.")
        else
            call BJDebugMsg("The unit with type 0 is not dead.")
        endif
        
        call GroupEnumUnitsInRange(testGroup,0.,0.,200.,null)
        
        loop
            set groupUnit = FirstOfGroup(testGroup)
            exitwhen groupUnit == null
            call BJDebugMsg("Unit detected! Type: "+I2S(GetUnitTypeId(groupUnit)))
            call GroupRemoveUnit(testGroup,groupUnit)
        endloop
        
        call DestroyGroup(testGroup)
        
        set testGroup = null
        set testUnit  = null
    endfunction

    This resulted the following debug: truly, the unit with a type 0 considered as not dead, but such a unit wasn't included in group (no debug from group loop at all => 0 units). So you gotta remove this condition as unnecessary one. Units with a type 0 will be correctly interpreted by Blizzard's functions, this should not be a point of worry to filter it out. It turns out as unnecessary code line.
• In Spell section of your code there are following things which can be improved:
  • As endless times before this one, I'm pointing out the same mistake: please use GetWidgetX and GetWidgetY instead of GetUnitX and GetUnitY because first ones are slightly faster than second ones because of less type inherits.
  • Why struct WarlikeSpiritPlugin is public? This is used only by your spell, so how about making it private as all other things which are inside? This increases the compatibility of your spell with other spells in some map.
  • Regarding dmg=(GetUnitState(target,UNIT_STATE_MAX_LIFE)*dmg)/100.;... Actually *.01 is much faster than /100.
  • Why you need to declare player p = GetTriggerPlayer(); when you already have CasterOwner = GetTriggerPlayer();? Besides of that, that player wasn't nulled properly by you => it produced a memory leak.
  • I fear that it's pretty much non-MUI because of globals like Caster and CasterOwner which will overwrite if a different unit will cast the spell. Please reorganize that part so the spell becomes full MUI.
  
  Cannot give it more than 4/5 mostly because of that non-MUI'ness. Fix it up and I will update my review.
  
  
  4/5: Recommended
  
  How that helped me to become better coder:
  • Very smart usage of IsUnitInRangeXY function. After I did a couple of benchmark, comparing the speed of SquareRoot method and IsUnitInRangeXY, second one executed 81 times more than the SquareRoot-based method. That's pretty impressive finding!

 

[nhocklanhox6]

Fixed XD.

Why you need to declare player p = GetTriggerPlayer();

Oops!, forgot .

 

[IcemanBo]

I had the honour to read 2 zinc spells today in spell section. \o/

• Please deindex properly if caster was removed from game. That's necessarily needed.
• Your speed setting is actually influenced by interval. Better would be to adapt it in config to normal WC3 movement speed/second.
  Or at least you should mention that it's /interval.
• 6.283188 -> bj_pi*2.
  You can stay with it actually, but with "pi" it's more intuitive and easier to understand/read.
• bj_RADTODEG*Atan2(GetUnitY(target)-tempY,GetUnitX(target)-tempX ) * .0174533;
  why multiply with bj_RADTODEG, then with .0174533? Also .0174533 is harder to understand, it's not good to work with such values directly in code.
• Names like "M" and "MD" are not descritive.

I fear that it's pretty much non-MUI because of globals like Caster and CasterOwner which will overwrite if a different unit will cast the spell. Please reorganize that part so the spell becomes full MUI.

nhocklanhox6 has not commented it, but as I could see he uses them as temporary container.

Good and solid spell. But daheck, for me personaly you use too much free space sometimes in variable assignment or just between some code. ... ^_°

 

[nhocklanhox6]

for me personaly you use too much free space sometimes in variable assignment or just between some code.

, I just split types of variable assignment like: real with real, integer with integer, I think it will increase readable things. And the code, if the code length is not fit like: AAAAAAA with AAAAAAAAAAAAAAAA, I will split them by those spacings ^_<..

Btw, thanks for the review ..

 

[IcemanBo]

I just notice it now, because you're using the constants now,
but if you use DEGTORAD and then RADTODEG again, it's redundant.

MUI and MUI2 is also not descriptive. "MUI" actually describes a compatibility.

Is it intended that you still see the spirits for a few more seconds after death/removal?

= 6.283188 / SPIRIT_COUNT ; //6.283188 = bj_PI*2
^I don't understand why you still prefer to use the number over the constant. :/

Edit:

But spell is coded well and works fine all in all.

Approved

 

[bowser499]

= 6.283188 / SPIRIT_COUNT ; //6.283188 = bj_PI*2
^I don't understand why you still prefer to use the number over the constant. :/

Because that's the way on how it works faster and has more oplimit space remaining.