Amumu "The Sad Mummy" v1.1

[Moderator]

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

10:24, 13th Jun 2014
BPower:
There are still a lot of things need to be done.
http://www.hiveworkshop.com/forums/spells-569/amumu-sad-mummy-v1-1-a-251723/index2.html#post2540792

08:30, 7th May 2014
BPower:
Refer to my last post. For now set to Need Fix

 

[deathismyfriend]

Did you try to compile this ? As it is it will not compile. You can't create locals anywhere except the top of the function.

This does not check if unit is alive.
GetWidgetLife(e) > .405
You need to do this to check if a unit is alive.
GetWidgetLife( e) > .405 and not IsUnitType( e, UNIT_TYPE_DEAD) and UnitTypeId( u) != 0

You leak timers because you never destroy them.
You have to null groups after destroying them.

This is not needed and is bad to have.
call DestroyTrigger(gg_trg_Q)

This would be better if everything was in the struct. Check out my spell to see how to do that. http://www.hiveworkshop.com/forums/spells-569/doom-heal-v-1-0-5-a-235217/

Indentation is bad here.

    function WAct takes nothing returns boolean
        local W data
            if GetSpellAbilityId() == ID then
                set data = W.create()
                set data.caster = GetTriggerUnit()
                call TimerStart(NewTimerEx(data),REDIOD,true,function FirstLoop)
            endif
        return false
    endfunction

Should be this.

    function WAct takes nothing returns boolean
        local W data
        if GetSpellAbilityId() == ID then
            set data = W.create()
            set data.caster = GetTriggerUnit()
            call TimerStart(NewTimerEx(data),REDIOD,true,function FirstLoop)
        endif
        return false
    endfunction

The above function should also be private along with all functions that are not meant to be called outside the library / scope.

You should make a global group. Constantly creating groups is inefficient. Create the global group once and never destroy it.

 

[tRu.Style]

This does not check if unit is alive.
GetWidgetLife(e) > .405
You need to do this to check if a unit is alive.
GetWidgetLife( e) > .405 and not IsUnitType( e, UNIT_TYPE_DEAD) and UnitTypeId( u) != 0

I tested, it works fine and timer destroy, no leaks.

 

[BPower]

Struct Q and W should be private, otherwise you'll have to choose a longer and unique name for the struct.

Please write out all configurable variables in full, for instance BDMG should be BASE_DAMAGE. You also have some typos in your very short documentation.

AmumuQ:

        set bj_lastCreatedUnit = CreateUnit(Player(0),'h002',0,0,0)
        call UnitAddAbility(bj_lastCreatedUnit,STUNID)

What is this about? Preloading can be done by the mapper himself.
Also this unit will remain forever, furthermore 'h002' may not exist in the object editor.

function QAct should be private.
You don't have to call GetTriggerUnit() again, since you already stored it in data.caster
You could also store GetTriggerPlayer() into a struct member.

* 57.29 magic numbers should be part of the configuration.

Damage calculation should also be configurable. I recommend you write a constant function GetDamage(...) and add it to the user configuration part.

The timer interval should also be configurable.

Make sure you have all over a proper identation.

Create one global group for the FoG enumeration instead of multiple local ones.

Pausing units is always a very bad choice, if possible try to avoid it.

In all three functions you should cache GetUnitX/Y into a local variable. Variable lookups are much faster than function calls.

In second loop local unit e and group g is not required at all.

In order to check if a unit is alive or not you can either do:
IsUnitType(unit, UNIT_TYPE_DEAD) and GetUnitTypeId(unit) != 0 or you declare native UnitAlive takes unit id returns boolean and use it.

Function QBool should be found above the struct and have a more distinct name.

In general all struct members which could leak an handle should be nulled properly, once you call method destroy().

AmumuW:
Same issue like above concerning the user configuration.

In FirstLoop you could cache GetUnitX/Y, furthermore use one global group for the group enumeration.

The damage calculation should be part of the user config. Design a private function called GetDamage()

Struct members should be nulled properly at the end.

AmumuE and AmumuR:
Won't compile at all. Same issues like the two above.
Preloading should be within static ifs

 

[deathismyfriend]

Where is this function. call ReleaseTimer(t)

Also all the problems I said above will stop this from working. You do have leaks.
You leak local variables. You leak timers unless that function above destroys timers in which case you should simply call DestroyTimer rather than making a function that does the same thing.

And as said above by me and Bpower. Your GetWidgetLife(e) > .405 does not check if unit is actually dead.

 

[Cokemonkey11]

I tested, it works fine and timer destroy, no leaks.

Another lol champion, cool!

This doesn't work because a dead units life can be set to an arbitrary value.

Use:

native UnitAlive takes unit u returns boolean

...

if UnitAlive(u) then ...

 

[BPower]

You leak timers unless that function above destroys timers in which case you should simply call DestroyTimer rather than making a function that does the same thing.

ReleaseTimer is part of the TimerUtils API, these timers get recylced and (never) destroyed.

 

[deathismyfriend]

ReleaseTimer is part of the TimerUtils API, these timers get recylced and (never) destroyed.

ooo ok. I missed the thing that says uses timerUtils.

 

[tRu.Style]

Struct Q and W should be private, otherwise you'll have to choose a longer and unique name for the struct.

Please write out all configurable variables in full, for instance BDMG should be BASE_DAMAGE. You also have some typos in your very short documentation.

AmumuQ:

        set bj_lastCreatedUnit = CreateUnit(Player(0),'h002',0,0,0)
        call UnitAddAbility(bj_lastCreatedUnit,STUNID)

What is this about? Preloading can be done by the mapper himself.
Also this unit will remain forever, furthermore 'h002' may not exist in the object editor.

function QAct should be private.
You don't have to call GetTriggerUnit() again, since you already stored it in data.caster
You could also store GetTriggerPlayer() into a struct member.

* 57.29 magic numbers should be part of the configuration.

Damage calculation should also be configurable. I recommend you write a constant function GetDamage(...) and add it to the user configuration part.

The timer interval should also be configurable.

Make sure you have all over a proper identation.

Create one global group for the FoG enumeration instead of multiple local ones.

Pausing units is always a very bad choice, if possible try to avoid it.

In all three functions you should cache GetUnitX/Y into a local variable. Variable lookups are much faster than function calls.

In second loop local unit e and group g is not required at all.

In order to check if a unit is alive or not you can either do:
IsUnitType(unit, UNIT_TYPE_DEAD) and GetUnitTypeId(unit) != 0 or you declare native UnitAlive takes unit id returns boolean and use it.

Function QBool should be found above the struct and have a more distinct name.

In general all struct members which could leak an handle should be nulled properly, once you call method destroy().

AmumuW:
Same issue like above concerning the user configuration.

In FirstLoop you could cache GetUnitX/Y, furthermore use one global group for the group enumeration.

The damage calculation should be part of the user config. Design a private function called GetDamage()

Struct members should be nulled properly at the end.

AmumuE and AmumuR:
Won't compile at all. Same issues like the two above.
Preloading should be within static ifs

I corrected, then that could translate.
Check

 

[deathismyfriend]

In your unit alive check you need to check if GetWidgetLife( unit) > 0.405

You really should use a local variable instead of this. bj_lastCreatedUnit

Never use PauseUnit() in spells or systems.
Instead get unit prop window. then set the units prop window to 0. At end of spell set unit prop window back to the original.

Also this is vJass only so it should not have jass category tag in spell description.

 

[Cokemonkey11]

In your unit alive check you need to check if GetWidgetLife( unit) > 0.405

No.

Never use PauseUnit() in spells or systems.
Instead get unit prop window. then set the units prop window to 0. At end of spell set unit prop window back to the original. Or, use a system that handles CC.

Fixed.

 

[deathismyfriend]

Fixed.

Explain further please. Saying a one word answer does nothing.

 

[Malhorne]

I didn't read the previous comment I'm here on a fast look :

On the first spell :
I highly suggest to use native UnitAlive takes unit id return bolean
Make the function private for the coordinates or just inline it everywhere (I prefer inline).
I HIGHLY recommend not to use PauseUnit to stun a unit but to use the stun system here in the JASS section.
if STUN == true then->if STUN then

 

[Cokemonkey11]

Explain further please. Saying a one word answer does nothing.

I fixed your comment. Using PauseUnit in systems is sometimes acceptable. I also added a sentence "Or use a system that handles CC".

 

[deathismyfriend]

I fixed your comment. Using PauseUnit in systems is sometimes acceptable. I also added a sentence "Or use a system that handles CC".

What do you mean a system that handles CC ?

 

[Cokemonkey11]

Crowd Control. http://www.hiveworkshop.com/forums/jass-resources-412/system-stun-196749/ for example.

 

[deathismyfriend]

Crowd Control. http://www.hiveworkshop.com/forums/jass-resources-412/system-stun-196749/ for example.

ooo ok that makes sense thanks.

 

[tRu.Style]

Guys, stop flame. I remove PauseUnit

 

[deathismyfriend]

Guys, stop flame. I remove PauseUnit

How is the above us flaming ?
Also read post above for other fixes needed.

 

[BPower]

        static if PRELOAD then
            set bj_lastCreatedUnit = CreateUnit(Player(0),DUMMY_CASTER_ID,0,0,0)
            call UnitAddAbility(bj_lastCreatedUnit,STUN_ID)
        endif

You should remove this unit, otherwise you have it on the map for the whole game season.

        local real x = GetUnitX(data.dummy)
        local real y = GetUnitY(data.dummy)
        local real X = x + SPEED * Cos(Atan2(data.y - GetUnitY(data.caster),data.x - GetUnitX(data.caster)))
        local real Y = y + SPEED * Sin(Atan2(data.y - GetUnitY(data.caster),data.x - GetUnitX(data.caster)))

Use the information you stored into the local reals.

 

[Karr123]

I think E range should be much smaller and it should reduce cd each time attacked

 

[Gekigengar]

I think E range should be much smaller and it should reduce cd each time attacked

CD Reduction possible in wc3? :O

 

[Malhorne]

It is not really possible ^^'
You can simulate the cd reduction but you can't really do the graphical effect on the icon of the spell.

Btw you can disable the spell and re-enable it but that's ugly as fuck

 

[BPower]

Q Spell:

Variables within the configuration part should be written out in full. This makes the code more comprehensible on the first look.
private function Q_Boolean takes unit u,unit e returns boolean --> private function QBoolean takes unit caster,unit target returns boolean
private function GetDamage takes unit u returns real --> private function GetDamage takes unit caster returns real

You also have to remove the unit used for preloading. Otherwise it would remain for the rest of the game season.

        static if PRELOAD then
            set bj_lastCreatedUnit = CreateUnit(Player(0),DUMMY_CASTER_ID,0,0,0)
            call UnitAddAbility(bj_lastCreatedUnit,STUN_ID)
        endif

In QAct you could store GetUnitX/Y into a local real variable, because they are used 4 times. However the speed gain is not noticeable, but looks better and is a good practise for yourself.

In "FirstLoop" relabel local real X/Y into x1/y1 instead of capitalizing local variable names.

Proper lightning movement also has to take the z-axis into account and use MoveLightningEx(...). In order to retrieve the correct z value you have to use a location and the native function GetLocationZ(loc).

                        if STUN then
                            set bj_lastCreatedUnit = CreateUnit(GetOwningPlayer(data.caster),DUMMY_CASTER_ID,x,y,GetUnitFacing(data.target))
                            call UnitAddAbility(bj_lastCreatedUnit,STUN_ID)
                            call IssueTargetOrder(bj_lastCreatedUnit,"thunderbolt",e)
                        endif

This dummy caster is never removed. In general I recommend to use only one dummy caster for the whole spell. As an alternative you could also use a dummy caster resource from the JASS section as requirement.

call GroupClear(GROUP) Because of the FoG loop this group is always empty by default.

WSpell:

W_Boolean --> WBoolean, u and e --> caster and target

        call GroupClear(GROUP)
        set e = null

These two lines are not needed, as e is null after the loop and the group will be empty

ESpell: pretty much the same as above.

RSpell: pretty much the same as QSpell.

The despair turn off tooltip is a joke.

I never played the game this spellpack is from, so I can't really judge if it is fitting or not.