Mana Wave v1.4

[Moderator]

08:17, 10th Feb 2014
BPower:

The coding looks now far better and the objects are fair enough.
What's still left undone is the way you determine the z offset of the lightning.
Using GetUnitFlyHeight only will look bad if one adds cliffs or valleys to the map.
The correct way would be:

call MoveLocation(loc, GetUnitX(unit), GetUnitY(unit))
set z  = GetLocationZ(loc) + GetUnitFlyHeight(unit)

old

04.02.2014:
BPower:


You have unit allies as struct member, but it isn't used at all.

Per spell cast you start 46 needless timers call TimerStart(TIMER,0.,false,function ManaWave.onPeriodic),
because unlike the ManaWavePluggin struct the corresponding ManaWave struct instance is already deallocated.

19:04, 30th Jan 2014:
Make those two changes I mentioned:
http://www.hiveworkshop.com/forums/spells-569/mana-wave-v1-2-a-246277/?prev=status%3Dp%26of%3Ddateline%26o%3DASC%26imporder%3D1#post2479763

 

[Chaosy]

looks neat.
However restore mana is a better name I think

 

[BPower]

Use GroupPickRandomUnit
Or can you point out one valid argument, why you made an excat copy of an existing function.

Well I have one against it. It prolongs your code needlessly.

Edit: Same issue for call TriggerRegisterAnyUnitEventBJ

What do you think about

            if IsUnitEnemy(f,caster_owner) then
                set destroy_mana = true
            else
                set destroy_mana = false
            endif

--> set destroy_mana = IsUnitEnemy(f, caster_owner)

use one static group for the enumeration instead of creating a local Group G per cast instance.

                    if destroy_mana then
                        if IsUnitEnemy(f,caster_owner) then
                            call GroupAddUnit(g,f)
                        endif
                        
                        set total_unit = total_unit + 1
                    else
                        if IsUnitAlly(f,caster_owner) then
                            call GroupAddUnit(g,f)
                        endif
                        
                        set total_unit = total_unit + 1
                    endif

move total_unit outside the condition and delete one of these.

set caster_owner = null is not nulled in all cases.

I guess it is not mandatory, but still JPAG.

You never destroy nor null group this.g
You have a unit handle leak --> local unit r

 

[deathismyfriend]

It is better to not use that BJ as it does not get inlined which means it is slightly slower than making it yourself. Also BJs are just ugly looking in code IMO.

 

[BPower]

I don't think you are right here. CnP from the JassHelper manual:

• The function must be a one-liner.
• Every argument must be evaluated once and only once by the function, in the same order as they appear in the arguments list.

Also not every BJ is trash.

Edit:

Also BJs are just ugly looking in code IMO.

Because of the color and the cursive text? ... Then change the settings of the highlighting in the editor or in the styles.ini

 

[deathismyfriend]

I did not say every BJ is trash lol.
That function is not a one liner either.
Calling a function vs not calling a function means that the one that does not call a function is faster.

 

[BPower]

If you target yourself while no unit is near the whole skript will fail. Afterwards you can't cast the spell anymore.

Also I think you could optimize the code by alot. For instance:

            if IsUnitEnemy(f,caster_owner) then
                set destroy_mana = true
            else
                set destroy_mana = false
            endif
           
            call GroupEnumUnitsInRange(G,GetUnitX(f),GetUnitY(f),AOE,null)
           
            loop
                set f = FirstOfGroup(G)
                exitwhen f == null
               
                if f != caster and GetUnitState(f,UNIT_STATE_MAX_MANA) > 0. and GetWidgetLife(f) > .0425 then
                    if destroy_mana then
                        if IsUnitEnemy(f,caster_owner) then
                            call GroupAddUnit(g,f)
                        endif
                       
                        set total_unit = total_unit + 1
                    else
                        if IsUnitAlly(f,caster_owner) then
                            call GroupAddUnit(g,f)
                        endif
                       
                        set total_unit = total_unit + 1
                    endif
                endif
               
                call GroupRemoveUnit(G,f)
            endloop
           
            set integer_loop = total_unit - count

---->

            set destroy_mana = IsUnitEnemy(f,caster_owner)
            /*
            *   static group enu = CreateGroup()
            */
            call GroupEnumUnitsInRange(enu ,GetUnitX(f),GetUnitY(f),AOE,null)
            
            loop
                set f = FirstOfGroup(enu)
                exitwhen f == null
                /*
                *     UnitAlive is faster than GetWidgetLife
                */
                if f != caster and GetUnitState(f,UNIT_STATE_MAX_MANA) > 0. and UnitAlive(f) then
                    /*
                    *     ^^
                    */
                    if IsUnitEnemy(f,caster_owner) == destroy_mana then
                        call GroupAddUnit(g, f)
                        set total_unit = total_unit + 1
                    endif
                    
                endif
                call GroupRemoveUnit(enu,f)
            endloop
            /*
            *     re-use count
            */
            set count = total_unit - count

 

[Malhorne]

I truly suggest to change the name -> Healing Mana is a bit strange. Healing refers to health whereas mana refers to mana ^^
So why not Mana Replenish or something like this.

I'll review the code later

EDIT :

static method onInit takes nothing returns nothing
            local integer i = 0
            local trigger t = CreateTrigger()
            //
            loop
                exitwhen i > bj_MAX_PLAYERS
                call TriggerRegisterPlayerUnitEvent(t,Player(i),EVENT_PLAYER_UNIT_SPELL_EFFECT,null)
                set i = i + 1
            endloop
            //
            call TriggerAddCondition(t,function thistype.onCast)
            set t = null
        endmethod

If you truly want to do so -> bj_MAX_PLAYERS = 12
So just replace the constant by 12.

if IsUnitEnemy(f,caster_owner) then
                set destroy_mana = true
            else
                set destroy_mana = false
            endif

->
set destroy_mana = IsUnitEnemy(f, caster_owner)

GetWidgetLife(f) > .0425 is not enough to detect living units. Either use native UnitAlive takes unit id returns boolean or IIRC IsUnitType(u, UNIT_TYPE_DEAD) or GetUnitTypeId(u)==0

I suggest to start MUI and PLUGIN_MUI at -1 to use the 0 index of arrays

You can private the second struct, you'll be able to access its function even if it is private because you're inside the scope !

It is a fast one if you think I said something wrong feel free to say ^^


EDIT FOR BPower : One group per cast isn't that slow to have a static one.

 

[BPower]

Do you plan to update this spell?

 

[BPower]

You leak two local units if no targets are around.
You can't pick removed units, the unittypeid check onCast is not needed.
It looks now much better than before and works as intended.

 

[Malhorne]

You have to do the check only if a unit can be removed during the moment you picked and the moment you do your loop with the group.

 

[BPower]

set ct = null
set f = null           
call DestroyEffect(AddSpecialEffect(MODEL_EMPTY,GetUnitX(ct),GetUnitY(ct)))

Well that one doesn't work out so good ^^ --> null ct after you tried to get its coordinates.
Create group g in onCast, because it is not required if there are no targets around.
Also, but not nessesary to get it approved:

• You could use TriggerRegisterAnyUnitEventBJ instead of writing it by yourself.
• caster_owner could be owner, but that just a personal preference of mine to avoid overlong local variable names
• TIMER shouldn't be in the configurable part.

 

[nhocklanhox6]

@BPower: Sorry my friend ^^~, i've busy in the last time, so i can't fix it.

Now: Fixed and thanks, of course!

Create group g in onCast, because it is not required if there are no targets around.

Sorry, but i don't want to create group g everytime spell is activating.., i think it's fine to do that, hope you understand !

 

[BPower]

You have unit allies as struct member, but it isn't used at all.