Time Lapse v1.4b

[Moderator]

BPower 19th Feb 2014
Approved.
You can find the full review here

old

14:46, 11th Feb 2014
BPower:
Use a hashtable, a queue, anything ... but not substrings.

 

[BPower]

A proper way to check wether a unit is alive/removed or not would be

    function IsUnitAlive takes unit u returns boolean
        return not IsUnitType(u, UNIT_TYPE_DEAD) and (GetUnitTypeId(u) != 0)
    endfunction

or just for checking life

    function IsUnitAlive takes unit u returns boolean
        return GetWidgetLife(u) > 0.405 // and (GetUnitTypeId(u) != 0) 
    endfunction
    // MAGTHERIDON96 SAYS NOOOOOO

Values like mana, life, facing can easily be stored into real arrays, why do you use strings?
For life use SetWidgetLife(..) and GetWidgetLife(..) instead of GetUnitState(...)
Don't use locations, use coordinates only instead. --> DestroyEffect(AddSpecialEffect(effect, x, y))
You use a loop to determine the correct index. A hashtable coupled with HandleId(unit) or simply an array coupled with any UnitIndexer udg_TL_Caster[UnitUserData(unit)] would be better.

 

[Kazeon]

Thnks, may i ask you something? Once i saw a spell without any variable, how can i do that? Or i was just misremembered?

Values like mana, life, facing can easily be stored into real arrays, why do you use strings?

it's because each hero need to save 1 value per TL_Accuracy, I guess using hashtable the spell will be so much simpler, and I don't like simple

 

[FUJI]

set loc = Location( GetUnitX(caster), GetUnitY(caster) )
^ Why don't just use coordinates, it is better to use coordinates than locations because it is much faster and it doesn't need to remove.

local real x = GetUnitX(caster)
local real y = GetUnitY(caster)
call AddSpecialEffect(TL_CastSfx(), x, y )

 

[BPower]

it's because each hero need to save 1 value per TL_Accuracy, I guess using hashtable the spell will be so much simpler, and I don't like simple

I forgot about that and yes it is possible with a hashtable.

May be over your head at the moment:
In my mind the most beautiful way to solve this problem, is to use a special form of data structure called Queue.
A Queue has the following API -> create(), enqueue(), pop(), clear() and destroy().
You can envision it like the mobile game "snake". You have a queue of nodes (One node per accuracy) with the total number of Time/accuracy (6/0.1 = 60) nodes.
Each clock timeout the first node will be removed and a new last will be added. Each node points to a struct instance with a life, mana, x, y member. On spell cast you read the first node of the Queue.

 

[Malhorne]

constant function TL_Time takes integer level returns real
       
        local real array time
       
        set time[1] = 6.0 // rewind time for level 1
        set time[2] = 6.0 // rewind time for level 2
        set time[3] = 6.0 // rewind time for level 3
       
        return time[level]
    endfunction

->

constant function TL_Time takes integer level returns real
    return 6.
endfunction

That can be easily changed like this :

constant function TL_Time takes integer level returns real
    return 6.+level
endfunction

Or

constant function TL_Time takes integer level returns real
    if level == 1 then
        return 6
    elseif level == 2 then
        return 8
    else
        return 14
    endif
endfunction

function IsUnitAlive takes unit u returns boolean
        if ( GetUnitState(u,UNIT_STATE_LIFE) <= 0.24 ) then
            return false
        endif
        return true
    endfunction

->

constant function TL_IsUnitAlive takes unit u returns boolean
    return not IsUnitType(u, UNIT_TYPE_ALIVE) and GetUnitTypeId(u)!=0
endfunction

call TriggerAddAction( t1, function learn )
call TriggerAddAction( t2, function cast )

->

call TriggerAddCondition( t1, Condition(function learn) )
call TriggerAddCondition( t2, Condition(function cast) )

And makes those callback funcs return boolean and return false at the end of these funcs.

Don't use location but real instead.

if ( casted == TL_Ability() ) then
->
if ( GetSpellAbilityId() == TL_Ability() ) then

Rhaaa I stopped when I saw that much strings >_<
Why using strings and not real/integer :/ ?
I'll review it again after changing strings to real/integer ^^

 

[Kazeon]

constant function TL_Time takes integer level returns real
       
        local real array time
       
        set time[1] = 6.0 // rewind time for level 1
        set time[2] = 6.0 // rewind time for level 2
        set time[3] = 6.0 // rewind time for level 3
       
        return time[level]
    endfunction

->

constant function TL_Time takes integer level returns real
    return 6.
endfunction

That can be easily changed like this :

constant function TL_Time takes integer level returns real
    return 6.+level
endfunction

Or

constant function TL_Time takes integer level returns real
    if level == 1 then
        return 6
    elseif level == 2 then
        return 8
    else
        return 14
    endif
endfunction

Mine allows user to define it per level

function IsUnitAlive takes unit u returns boolean
        if ( GetUnitState(u,UNIT_STATE_LIFE) <= 0.24 ) then
            return false
        endif
        return true
    endfunction

->

constant function TL_IsUnitAlive takes unit u returns boolean
    return not IsUnitType(u, UNIT_TYPE_ALIVE) and GetUnitTypeId(u)!=0
endfunction

I have fixed it on the newest version

call TriggerAddAction( t1, function learn )
call TriggerAddAction( t2, function cast )

->

call TriggerAddCondition( t1, Condition(function learn) )
call TriggerAddCondition( t2, Condition(function cast) )

And makes those callback funcs return boolean and return false at the end of these funcs.

okay

Don't use location but real instead.
fixed

if ( casted == TL_Ability() ) then
->
if ( GetSpellAbilityId() == TL_Ability() ) then

local casted = GetSpellAbilityId()

Rhaaa I stopped when I saw that much strings >_<
Why using strings and not real/integer :/ ?
I'll review it again after changing strings to real/integer ^^

Values like mana, life, facing can easily be stored into real arrays, why do you use strings?

it's because each hero need to save 1 value per TL_Accuracy, I guess using hashtable the spell will be so much simpler, and I don't like simple

I have updated it, please check the newest one thnks alot anyway

 

[Malhorne]

For the function TL_Time I'll say again what I told because it is more efficient my way because most of the time there is a link between levels ...

For the GetSpellAbilityId() == Tl_Ability() I'll say again because the local is useless for one call....

Did you upload the code :/ ?

 

[Kazeon]

updated again, quick but big update
almost all of your suggestion has been applied, except that TL_Timer. Why I should not use my way? what's wrong

 

[Malhorne]

local real array is bad \o/ !
Anyway you can keep but that's not the best way

 

[Kazeon]

well, why this looks bad in my eye

constant function TL_Time takes integer level returns real
    if level == 1 then
        return 6
    elseif level == 2 then
        return 8
    else
        return 14
    endif
endfunction

which is the best of all you have mentioned above? still, optimization is the priority

 

[Malhorne]

My way is better because the func is constant so inline.

 

[Kazeon]

you know what? I think I will keep on my way
I know yours is better but, I hope user will understand easier..

 

[TriggerHappy]

My way is better because the func is constant so inline.

Constant functions don't mean inlining

 

[BPower]

Just to bring light into the darkness, those are the requirements for inlined functions. You can also find this info inside the JassHelper manual.

• The function is a one-liner
• If the function is called by call the function's contents must begin with set or call or be a return of a single function.
• If the inlined function is an assigment (set) it should not assign one of its arguments.
• Every argument must be evaluated once and only once by the function, in the same order as they appear in the arguments list.
• If the function contains function calls, they should all be evaluated after the arguments UNLESS the function is marked as non-state changing, at the moment some very few native functions and also return bug exploiters are considered as non-state changing.

 

[Malhorne]

Did you update the code >< ?

 

[Kazeon]

yes. looks a same?
I just changed Set/GetUnitState to Set/GetWidgetLife.. just that >

 

[Malhorne]

You still use strings...

 

[Kazeon]

is there a problem in using string?

 

[Malhorne]

Yes because it is a useles typecast ...
Why are you not using integers/reals instead of strings everywhere and this useless typecasting >_<


EDIT : Holy >_< My 1k9th post to do criticism

 

[Kazeon]

hey, one hero needs to save more than 1 reals remember?

and
what's wrong with that?

 

[Malhorne]

Dude this is a pretty bad way to do so....
Mods will tell the same way
You can store a pretty huge number of variables in wc3 so why are you doing this ?

Just do with integers/reals and everything like this...
If you want those kind of optimizations there is better way xD

 

[Kazeon]

well, I'm pretty sad that you said like that instead EDIT:*than* "Hey, it's a very unique way. I like it! 5/5 rep+" :'(
anyway, I'll change it soon.. after some cries :'(

 

[Malhorne]

Ahah don't worry we all try those kind of things...
But if you benchmarked this against the classic way this is very slower

After I can't really tell notes since I usually can't download xD !
So I just review codes most of the time
I think I'll be off for today no more battery sadly :/

 

[BPower]

There is also a lot of feedback from Malhorne in the thread, who is also an experienced wc3 coder.
I might be the one moderating your resource, but other opinions are also very valuable.

You have to present a coding concept "we" you, me and other members can work with. Come up with another solution, then I can give you specific tips how to improve it.

I'm not going to code the whole spell for you. If you need help to get started you just have to ask for it.
I think working with Table (Bribes/Vexorians) or an Hashtable would be a solid solution for a new approach.

Not recommended, mainly because it is more difficult, by me is the queue thing, but if you really want to go for it I will write a queue module for you.
The jass section is in need of one anyway.
It would take some time, because I'm currently short in time :/)

 

[Kazeon]

malbro has only one suggestion that is just like yours, "dont use strings" I will change it anyway..

EDIT:
if I change to hashtable, you will say, "the spell is too simple" because, really, using hashtable the spell will be looks just like a sandbox, I mean very heavenly craby patty simple..

EDIT 2:
I think I will work with the table or queue that you have mentioned above, thanks for the suggestion, BPow!

 

[BPower]

Keep your stuff always as simple as possible without losing important features.
Noone ever said needlessly complicated == cool.

 

[Malhorne]

There is also a lot of feedback from Malhorne in the thread, who is also an experienced wc3 coder.
I might be the one moderating your resource, but other opinions are also very valuable.

Loved it <3

Queue isn't the best way : linked list > queue

Then I suggested some code improvements but I didn't finish to review the code because substrings messed the code
I'll redo it again after those changements

 

[Wrda]

EDIT:
if I change to hashtable, you will say, "the spell is too simple" because, really, using hashtable the spell will be looks just like a sandbox, I mean very heavenly craby patty simple..

Then you better code in chinese, if it is so simple
It's better something simple and with effectiveness than something complex and totally CRAP and confusing.
So do by the easiest way

Keep your stuff always as simple as possible without losing important features.
Noone ever said needlessly complicated == cool.

Follow these sentences, best advice

 

[Kazeon]

finally.. UPDATED!!!! argh..

 

[Malhorne]

local unit caster = GetTriggerUnit()
        local real accuracy = TL_Accuracy()

Init this values only if the condition is true so you can put the set caster = null in the if block too.
The reason of this optimization is that every time you run a spell this will create vars and init their values and null it whereas if you do as I said it will do it only if the spell is the good one.

local integer learned = GetLearnedSkill() local unit u = GetTriggerUnit()
Same there
And so you can remove the learned variable
Because if you do with my way there is only one reference (don't count the cond)

I suggest to inline your ForGroup with a FoG loop

if CountUnitsInGroup(udg_TL_Group) == 0 then
                call DestroyTimer(udg_TL_Timer)
            endif

Just PauseTimer don't destroy it

 

[Kazeon]

you are right, will fix it not soon..

 

[Malhorne]

No problem

 

[Kazeon]

final update, the code is perfect, flawless, looks great 5/5

 

[Malhorne]

Maybe this can improve with an inline FoG loop instead of the ForGroup but anyway that looks better for the code

 

[Kazeon]

what is FoG? :/ I won't take my word, it's FINAL

 

[Malhorne]

Ok no problem.
Ah btw FoG loop destroys the group so maybe it is not that good in your case so it is okay.

 

[Kazeon]

you have been very helpful here.. rep+ for that.. please, help me with another resource

 

[BPower]

The spell is leakless and it works.
Instuctions and objects are also ok.

TriggerRegisterPlayerUnitEvent and a loop from 0 to bj_MAX_PLAYER_SLOTS is exactly the same as TriggerRegisterAnyUnitEventBJ unless you add a specific player filter (no computers, etc...)
The ForGroup operation loses efficiency the more units are affected, however as long as the number is kept low it's no big deal.
Talking about efficiency the way you save/move unit values might critically harm the ingame performance if:

• The accuracy is high
• The number of considered units is too large
• TL_Time is set to a high value, while the accuracy is also high

You could add a notice for users that (TL_Time/TL_Accuracy*considered units) should not surpass a certain number. I would suggest ~350.
A different strategy would remove this problem.

I think this one --> call IssueImmediateOrder(caster, "stop") is not required, because the channel ability itself interupts the current order.
Unlikely to happen, but if so the ability would go into cooldown while nothing happens.

 

[Kazeon]

I think this one --> call IssueImmediateOrder(caster, "stop") is not required, because the channel ability itself interupts the current order.
Unlikely to happen, but if so the ability would go into cooldown while nothing happens.

that's why I order the caster to stop so that if the hashtable is still empty the spell won't be cooldowned.. and finally... thnks for the review and approval!!

 

[BPower]

Your trigger fires on event SPELL_EFFECT, therefore stop will not reset the cooldown.

 

[Kazeon]

i think it's not reseting the cooldown.. the hero stopped before the ability cooldowned..

as far as I know, he is right, stopping a unit on start the effect of ability will stop the unit right before the ability cooled down..

 

[BunnyAng]

I got a stupid question. Please don't get mad. What if the hero just respawned from death and uses his Time Lapse immediately. Will he die?

 

[Kazeon]

it's not stupid, but good instead
look at the 'else' part

            if l > 0.405 then
                set dex = GetHandleId(u)
                set node = R2I(TL_Time(LoadInteger(udg_TL_Hashtable, dex, -2)) / TL_Accuracy())
                set i = node
                loop
                    call SaveReal(udg_TL_Hashtable, dex, i, LoadReal(udg_TL_Hashtable, dex, i - 1))
                    call SaveReal(udg_TL_Hashtable, dex, i + node , LoadReal(udg_TL_Hashtable, dex, i + node - 1))
                    call SaveReal(udg_TL_Hashtable, dex, i + node * 2 , LoadReal(udg_TL_Hashtable, dex, i + node * 2 - 1))
                    call SaveReal(udg_TL_Hashtable, dex, i + node * 3 , LoadReal(udg_TL_Hashtable, dex, i + node * 3 - 1))
                    call SaveReal(udg_TL_Hashtable, dex, i + node * 4 , LoadReal(udg_TL_Hashtable, dex, i + node * 4 - 1))
                    set i = i - 1
                    exitwhen i == 1
                endloop
                call SaveReal(udg_TL_Hashtable, dex, 1, l)
                call SaveReal(udg_TL_Hashtable, dex, 1 + node, GetUnitState(u, UNIT_STATE_MANA))
                call SaveReal(udg_TL_Hashtable, dex, 1 + 2 * node, GetUnitFacing(u))
                call SaveReal(udg_TL_Hashtable, dex, 1 + 3 * node, GetUnitX(u))
                call SaveReal(udg_TL_Hashtable, dex, 1 + 4 * node, GetUnitY(u))
                call SaveInteger(udg_TL_Hashtable, dex, -1, LoadInteger(udg_TL_Hashtable, dex, -1) + 1)
            else
                call FlushChildHashtable(udg_TL_Hashtable,dex)
                call SaveInteger(udg_TL_Hashtable, dex, -1, 0)

when the Hero is dead, the hashtable get flushed then the counter is reseted to 0, where if the counter is still 0, the Hero will automatically ordered to stop.. so it's safe..

                if total > 0 then
                else
                    call IssueImmediateOrder(caster, "stop")

 

[Malhorne]

I suggest using this instead of the stop order :
call IssueImmediateOrderById(caster, 851973)

It will prevent any action of the unit.

 

[Kazeon]

okay, thnks!

 

[Malhorne]

You're welcome as usual

 

[BunnyAng]

Thanks Dalvengyr. But you know i can't read vJASS lol. Still trying to learn.

 

[BPower]

What changed?