Warlike Spirit v1.1

Moderator · Apr 8, 2015

Submission: Warlike Spirit v1.1

Date: 18.05.2015

Status: Approved

Rating: 4/5

Link Post

Moderator: IcemanBo

Almia · Apr 8, 2015

OMG.

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

I can't review your Zinc code like that.

bowser499 · Apr 8, 2015

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

Code (vJASS):

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 · Apr 8, 2015

Fixed XD.

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

Oops!, forgot .

IcemanBo · Apr 23, 2015

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.
Click to expand...

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 · May 1, 2015

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

, 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 · May 19, 2015

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 · Jun 4, 2015

IcemanBo said:
= 6.283188 / SPIRIT_COUNT ; //6.283188 = bj_PI*2
^I don't understand why you still prefer to use the number over the constant. :/
Click to expand...
Because that's the way on how it works faster and has more oplimit space remaining.

Log in or Sign up

Warlike Spirit v1.1

Just another Warcraft III map (Map)

Moderator

Almia

bowser499

nhocklanhox6

IcemanBo

nhocklanhox6

IcemanBo

bowser499

Warlike Spirit v1.1

Just another Warcraft III map (Map)

Useful Searches