Luck Aura v1.3

[Moderator]

13 Dec 2011
Bribe: Approved.

 

[baassee]

Nice to see you stepping up your coding knowledge. So here we go.

-Don't use actions, merge them with the conditions.

-Trigger t2 shouldn't have actions either, just add condition and make the actions there.

And remember to return false afterwards.

-Instead of creating your own hashtable, use this: Link to Bribe's New Table

-Please use TimerUtils, you never destroy timers in vJASS.

-These are unneeded. What are you trying to do? Shadow globals?

private integer level
private integer targetID

-No need to use this local or shadowed global in the filter, it's only used once anyway.
set targetID = GetHandleId(u)

-GetOwningPlayer(u) -> GetTriggerPlayer() in the cast trigger.

-In the loop function, the GetUnitX(u), GetUnitY(u) should be stored into locals, saving two additional calls.

-Should be a local integer
set level = LoadInteger(HASH, timerID, 5)// This is a global variable

That's what I could find for now. You should really learn to use the ability to use libraries in vJASS. Will save ya lots of work. Example that could make this even better is to use Axarion's advanced aura module.

 

[Magtheridon96]

private function LUCK_DEATHS takes nothing returns nothing
    local unit k = GetTriggerUnit()
    local unit u = GetKillingUnit()
    local real x = GetUnitX(k)
    local real y = GetUnitY(k)
    local integer targetID = GetHandleId(u) // This is a global variable
    local integer level = LoadInteger(HASH, targetID, 1) 
    if IsUnitEnemy(u, GetOwningPlayer(k)) and GetRandomInt(1,100) <= LUCK_CHANCE(level) and IsUnitType(k, UNIT_TYPE_STRUCTURE)==false then
        if GetUnitAbilityLevel(u, BUFF) > 0 then
            call CreateItem(ChooseRandomItemEx(ITEMID, level), x, y)
        endif
    endif
    set k = null
    set u = null
endfunction

Since you only used x and y once, it's better to just use the functions
You don't need locals if you're only going to use them once

private function LUCK_FILTER takes nothing returns boolean
    local unit u = GetFilterUnit()
    local integer targetID
    if IsUnitAlly(u, GetOwningPlayer(u)) and GetUnitAbilityLevel(u, BUFF) > 0 then
        set targetID = GetHandleId(u)
        call SaveInteger(HASH, targetID, 1, level) 
    endif
    return true
endfunction

Since it would make no difference, I think it's better to just set the targetID to the handle id
of the unit on local declaration

private function LUCK_LOOP takes nothing returns nothing
    local timer t = GetExpiredTimer()
    local integer timerID = GetHandleId(t)
    local unit u = LoadUnitHandle(HASH, timerID, 1)
    local unit dummy = LoadUnitHandle(HASH, timerID, 2)
    local real duration = LoadReal(HASH, timerID, 3)
    local real AoE = LoadReal(HASH, timerID, 4)
    local integer casterID = GetHandleId(u)
    set level = LoadInteger(HASH, timerID, 5)// This is a global variable
    if duration >0 and GetWidgetLife(u) >= 0.405 then
        call SetUnitX(dummy, GetUnitX(u))
        call SetUnitY(dummy, GetUnitY(u))
        call SaveReal(HASH, timerID, 3, duration - TIMERSPEED) 
        call GroupEnumUnitsInRange(bj_lastCreatedGroup, GetUnitX(u), GetUnitY(u), AoE, Filter(function LUCK_FILTER))
    else
        call UnitApplyTimedLife(dummy, 'BTLF', 0.01)
        call SetUnitExploded(dummy, true)
        call FlushChildHashtable(HASH, timerID)       
        call FlushChildHashtable(HASH, casterID)
        call PauseTimer(t)
        call DestroyTimer(t)
    endif
    set t = null
    set u = null
    set dummy = null 
endfunction

This would be a good place to declare 2 local real variables for the units
x and y since you're using the functions more than once.

Also, Filter(function LUCK_FILTER))
You don't really need to use the "Filter".
Without it, it would still function similarly :]
Using it in this case is a waste of efficiency :/

private function LUCK_CAST takes nothing returns nothing
    local timer t = CreateTimer()
    local unit u = GetTriggerUnit()
    local unit dummy
    local integer timerID = GetHandleId(t)
    local integer casterID = GetHandleId(u)
    local integer level = GetUnitAbilityLevel(u, ABILITY_ID) 
    local real duration = LUCK_DURATION(level)
    local real AoE = LUCK_AOE(level)
    local texttag tag = CreateTextTag()
    if HaveSavedReal(HASH, casterID, 1)==false then
        set dummy = CreateUnit(GetOwningPlayer(u), DUMMY_ID, GetUnitX(u), GetUnitY(u), 0)    
        call UnitAddAbility(dummy, DUMMYABILITY_ID)
        call SetUnitAbilityLevel(dummy, DUMMYABILITY_ID, level) 
        
        //SAVE FOR THE CASTER ID
        call SaveReal(HASH, casterID, 1, duration)
        
        //SAVE FOR THE TIMER ID
        call SaveUnitHandle(HASH, timerID, 1, u)
        call SaveUnitHandle(HASH, timerID, 2, dummy)
        call SaveReal(HASH, timerID, 3, duration)
        call SaveReal(HASH, timerID, 4, AoE)
        call SaveInteger(HASH, timerID, 5, level)
        call TimerStart(t, TIMERSPEED, true, function LUCK_LOOP) 
    else
        call IssueImmediateOrder(u, "stop")
        call SetTextTagPosUnit(tag, u, 0.) 
        call SetTextTagText(tag, "You can't use this yet!", 0.025)
        call SetTextTagPermanent(tag, false)
        call SetTextTagVelocity(tag, 0.03, 0.03)
        call SetTextTagLifespan(tag, 3)
        call SetTextTagFadepoint(tag, 0.01)        
    endif
    set t = null
    set u = null
    set dummy = null
    set tag = null
endfunction

This function looks good


I think that almost ANY spell that happens to be efficient and original
deserves a 5/5 (In 90% of all cases that is ;P)

 

[baassee]

Also, Filter(function LUCK_FILTER))
You don't really need to use the "Filter".
Without it, it would still function similarly :]
Using it in this case is a waste of efficiency :/

Does it? As it seems to inline a filter without the "Filter" infront of it.

 

[Magtheridon96]

Does it? As it seems to inline a filter without the "Filter" infront of it.

You're most probably right

 

[Marcos DAB]

Its too imba for an balanced Map.

 

[mckill2009]

thanks for all your suggestion guys, however, I just started to make vJASS spells since my JNGP fucked up before...So I may need time to learn libraries and structs...

I really need to use filter since I dont want to pick up every unit, but I think I will change it to "GetUnitAbilityLevel(u, BUFF) > 0" only...

As for merging actions, Ima excercise that next update...

@Marcos DAB
The configurations are there for a reason ...

 

[baassee]

What he meant was that without typing keyword Filter it will inline and add a filter by default and do not give a syntax error as long as the function returns a boolean.

 

[Bribe]

Currently your system will cause errors - what if RemoveUnit is called? The death event won't fire and you're stuck with unused space in your hashtable that will probably never be clear thus filling up unnecessary memory.

If you use a unit-indexing system, you will not have to worry about this as the indexer takes care of the deindexing portion and the arrays' data will be replaced with other data at some point.

 

[mckill2009]

RemoveUnit meaning globally or the FlushChildHandle?...from what I see, the Flushing of targetID is not there so maybe it will cause error or leak...I assume that unit-indexing system is a library but idk it yet, so I better stick to hashtables first...

@baassee
my bad...

 

[Bribe]

Nestharus' UnitIndexer is pretty simple (edit: after you install that LUA stuff). I advise looking into his system. http://www.hiveworkshop.com/forums/jass-functions-413/system-unit-indexer-172090/

 

[SpectreD]

i love the idea of this spell. thank you for the share

 

[mckill2009]

Bribe: "FilterThem" needs to null the local unit. However, you'd be better using a FirstOfGroup loop instead of a filter, anyway.

Please fix the first bug mentioned, also please note that abilities shouldn't usually support more than 4 levels because then the object data won't get converted to SLK and the map loading time gets enormous.

Changed most of it however I really dont know why I'd be using FirstOfGroup coz
TimerLoop is meant to Filter 'all' those who gets near the hero, not the FirstOfGroup...

 

[Bribe]

A FirstOfGroup loop is the same thing as doing a group filter if you only use the group for that one moment. I don't know what you're talking about because it seems like you think that FirstOfGroup only picks one unit, but if you do the FirstOfGroup loop it picks all the units, the same as the filter.

 

[mckill2009]

you mean something like this?...
I'm surprised, its faster ...

function Actions takes nothing returns nothing
    local unit u
    call GroupEnumUnitsInRange(bj_lastCreatedGroup,0,0,5000,null)  
    loop
        set u = FirstOfGroup(bj_lastCreatedGroup)
        exitwhen u==null
        if not IsUnitType(u, UNIT_TYPE_DEAD) then
            call KillUnit(u)
            set I = I+1
            call BJDebugMsg(I2S(I))
        endif
        call GroupRemoveUnit(bj_lastCreatedGroup, u)
    endloop
endfunction

 

[Bribe]

People used to do all their loops via FirstOfGroup, but years ago the null filter leaked so everyone just did everything via the filter, because it didn't leak that way. But now that null filter can be used safely the FirstOfGroup loop is the best of the best.

 

[baassee]

Don't use UnitAlive, it's the same as IsUnitType(unit, UNIT_TYPE_DEAD).

 

[mckill2009]

I know that but the native is easier for me to write ...