Surf and Condense v1.07

[Moderator]

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

00:40 24.01.2014
BPower:
Currently set to "Need Fix", because there are too many unsolved issues. You can find the new review here:
http://www.hiveworkshop.com/forums/spells-569/surf-condense-v1-07-a-227652/index2.html#post2477070

Old review

00:45, 22nd Jan 2013
Magtheridon96:

Surf

• You can get rid of 3 lines of code by changing the NewTimer()/SetTimerData(t,data)/set t = null chain into a single call to NewTimerEx(data).
• Some of the indentation is messed up in places :/ (Example: All the code inside the Filter/Handler functions and the struct members.)
• Spaces between function arguments bro, spaces :v
• You can optionally change the method of the group enumeration to a FirstOfGroup loop for readability. I think it makes the whole concept of picking units and iterating over them more clear for beginners when reading your code. This is totally optional though.
• The order id should probably be configurable in the globals block.
• Timer interval should be configurable. The 50 value you have in there would then be (1./INTERVAL)
• Although we can debate for hours on end about how nulling struct members is practically useless, or about how it's a fine practice, it's most definitely not going to hurt anyone. :/
• .6 -> 0.6 (Neatness. Also, it might as well be configurable along with all your other constants)
• The 170. range specified in the Handler should also probably be configurable. Currently, this value is practically model-specific, so I think it should be configurable from the globals block or possibly even from a function in case multiple unit types are going to be casting this spell.
• Could you insert, at the very least, a chunk of documentation at the top of the code that outlines the requirements and links to them?
• You could make the unit filtering condition inside the FilterActions function configurable from the top of the script. It would pretty much just be a function that takes a unit and some other arguments, then returns a boolean after evaluating that unit. This enhances the flexibility of the spell in the case where you have users who are actually going to listen to you and not touch the code under that magical comment barrier you placed
• You can merge OnSpell and Actions if you wish to.

Condense

• Indentation inside the structs is messed up.
• Nulling the cast and orb members of the Cdnse struct wouldn't hurt :c
• I see what you did there with the group g inside the Cdnse struct. I like it
• Indentation in general is all messed up. (After a function declaration, you should indent)
• WEAPON_TYPE_WHOKNOWS == null
• As I said earlier, the FirstOfGroup loop for enumeration is cool, but totally optional.
• Also as I said earlier, you could make the unit filters configurable from the top of the code.
• NewTimerEx(data) could also be used inside the FilterActions function and the Actions function.
• There's one thing I don't understand about most people and that is the usage of the period 0.0325 as opposed to 0.03125. Could you explain it to me? I never really got it, and I never asked before. Sometimes, I would assume it's a typo or a miscommunication :/
• Since you're modifying the angle a manually through addition and converting it to radians, I think working with radians directly shouldn't be too much of a hassle. I also think that the rotational speed should be configurable.
• The 400 should probably be configurable as well.
• You can merge OnSpell and Actions if you wish to.
• A chunk of documentation outlining the requirements would also be nice here.

 

[bowser499]

Add the spell code here, please.

 

[doomhammer99]

Looks Cool.
But You really need to post the codes

 

[Strikest]

Alright done.

 

[Almia]

The code indention is a bit messy.
In globals and functions , you have 5 spaces per block w/ should be 4
you dont have any indention in the struct and function
cache the GetFilterUnit() so it will be faster
local real d = SquareRoot((x2 - x1) * (x2 - x1) + (y2 - y1) * (y2 - y1))
Remove the square root,then square the required distance
and lot more needed changes

Also why AIDS?you can use Unit Indexer by Nestharus

 

[Strikest]

In globals and functions , you have 5 spaces per block w/ should be 4

... No comment.

cache the GetFilterUnit() so it will be faster

alright done.

Remove the square root,then square the required distance

Could you elaborate more please?
EDIT: Ah nvm I get it. Done.

Also why AIDS?you can use Unit Indexer by Nestharus

I like AIDS better. Because Damage is awesome.

 

[bowser499]

1. Use IssueImmediateOrderById instead of IssueImmediateOrder.
2. Null the local trigger trig inside the function OnInit.
3. Re-code the Filter(function FilterActions), make a FirstOfGroup loop instead. It is a lot more faster. The proof.
4. Don't use call ReleaseTimer(GetExpiredTimer()) when you have local timer t = GetExpiredTimer(). Use it, make that string look like that: call ReleaseTimer(t).
5. IsUnitEnemy(u,GetOwningPlayer(CASTER)) == true - no need to write == true.

4/5 for now.

 

[Strikest]

1. Use IssueImmediateOrderById instead of IssueImmediateOrder .

Done.

2. Null the local trigger trig inside the function OnInit .

I don't think that really does anything at all, but it doesn't hurt so, done.

3. Re-code the Filter(function FilterActions) , make a FirstOfGroup loop instead.

I like Filter(function FilterActions) better, mostly because I feel it's easier to understand for people. Plus I doubt the speed difference is enough to make it worth it to recode.

4. Don't use call ReleaseTimer(GetExpiredTimer()) when you have local timer t = GetExpiredTimer().

Done.

5. IsUnitEnemy(u,GetOwningPlayer(CASTER)) == true - no need to write == true .

Done.

 

[doomhammer99]

try to Cache the Triggering Player inside the private struct Surf
like this
player owner
and set it inside the
private function Actions takes nothing returns nothing
like this
set data.owner = GetTriggerPlayer()
and also fix the code's indentation using Script Language Aligner by The_Witcher

 

[bowser499]

5/5 now, okay =)
Changes made.

 

[Strikest]

I don't think there's any point in caching GetTriggerPlayer when it's only called once or twice at most...

Updated to v1.04, allowing the duration to be configured and making Condense not affect Magic Immune units.

 

[BPower]

Please take this into consideration JPAG.

For now I just went quickly through Surf.
- Use call TimerStart(NewTimerEx(data), .02, false function Handler) an advice mag already gave you 1 year ago. Also 0.02 should be set to a variable (TIMER_INTERVAL)
- I would recommend a FirstOfGroup loop. It has better read-ability and is faster the bigger the picked number of unit is.
If you want stick to your way:

...

Hu?? Put unit u = GetFilterUnit() to the top and use it.

        local real x2 = GetUnitX( GetFilterUnit( ) )
        local real y2 = GetUnitY( GetFilterUnit( ) )
        local real a = bj_RADTODEG * Atan2( y2 - y1, x2 - x1 )
        local unit u = GetFilterUnit( )

- Please null struct members in the end.
- private constant integer ORDER_ID = 852177 //Order ID of ability. Which ablilty? Make sure your comments provide a context.
- Use one periodic timer instead of re-starting it every 0.02 seconds.
- TIMER_INTERVAL should be set to 0.02 and not 50.
- use a player variable as member of the struct --> data.owner = GetTriggerPlayer() to get rid of tons of unnessesary GetOwningPlayer() calls.

Once major flaws are corrected we will also come to stuff like:
- if ALLOW_NO_COLLISION then --> static if ALLOW_NO_COLLISION then

There is more and we will get to that when I have more time (if you want).
Please also go through all of Mags suggestions again. Some of them are still not covered in the recent version.

Edit: I will give you a small push. (Can contain typos, because I never tried to save it.)

Could look like

    private constant function FilterUnits takes unit u, player p returns boolean
        return IsUnitEnemy(u, p)//More conditions????
    endfunction

    private function Handler takes nothing returns nothing
        local Surf data = GetTimerData(GetExpiredTimer())
        local real x = GetUnitX(data.cast)
        local real y = GetUnitY(data.cast)
        local real angle
        local unit u
        //.......
        if data.dur <= 0 then
            call DestroyEffect(data.attach)
            
            static if LIBRARY_MSX then
                call MSX_RemoveAllSpeed(data.cast)
            endif
            
            static if ALLOW_NO_COLLISION then
                call SetUnitPathing(data.cast, true)
            endif
            
            call UnitRemoveAbility(data.cast, MOVE_ID)
            call data.destroy()
            /*
            *   Null every struct member, which can leak a handle.
            */
            set data.attach = null
            set data.owner  = null
            set data.cast   = null
            call ReleaseTimer(GetExpiredTimer())
        else
            set data.dur = data.dur - 1 
            /*
            *   IsUnitInRangeXY I guess the name is selfexplaining.
            *   data.x and data.y are the center x/y of the last created dummy.
            */
            if not IsUnitInRangeXY(data.cast, data.x, data.y, MINIMUM_DISTANCE) then
                set data.x = x
                set data.y = y
                //set u = Create new dummy
                // ....
            endif
            
            call GroupEnumUnitsInRange(GROUP, x, y, KB_AOE, null)
            loop
                set u = FirstOfGroup(GROUP)
                exitwhen u == null
                call GroupRemoveUnit(GROUP, u)
                /*
                *   FilterUnits is what you called FILTER_UNITS.
                */
                if UnitAlive(u) then
                    if FilterUnits(u, data.owner) then
                        set angle = bj_RADTODEG * Atan2(GetUnitY(u) - y, GetUnitX(u) - x)
                        call KBS_BeginEx(u, KB_DISTANCE, KB_DURATION, a, KB_SFX, TREE_AOE, ALLOW_MOVE, CHECK_PATHING)
                    endif
                endif
            endloop
        endif
    endfunction
    
    private function Actions takes nothing returns nothing
        local Surf data = Surf.create()
        local integer level
        set data.cast   = GetTriggerUnit()
        /*
        *   A player variable is very usefull for the
        *   FilterUnits function. You get rid of native GetOwningPlayer()
        */
        set data.owner  = GetTriggerPlayer()
        set data.x      = GetUnitX(data.cast)
        set data.y      = GetUnitY(data.cast)
        set data.attach = AddSpecialEffectTarget(EFFECT, data.cast, ATTACH)
        
        set level = GetUnitAbilityLevel(data.cast, ABIL_ID)
        set data.dur  = GetDuration(level)
        
        call UnitAddAbility(data.cast, MOVE_ID)
        
        static if LIBRARY_MSX then
            call MSX_SetPureSpeed(data.cast, BONUS_SPEED(level)))
        endif
        /*
        *   The JassHelper will ignore this block
        *   if the boolean ALLOW_NO_COLLISION is false.
        */
        static if ALLOW_NO_COLLISION then
            call SetUnitPathing(data.cast, false)
        endif
        /*
        *   NewTimerEx(instance)
        */
        call TimerStart(NewTimerEx(data), TIMER_INTERVAL, true, function Handler)
    endfunction

 

[Strikest]

Please take this into consideration JPAG.

meh. too inconsequential

For now I just went quickly through Surf.
- Use call TimerStart(NewTimerEx(data), .02, false function Handler) an advice mag already gave you 1 year ago. Also 0.02 should be set to a variable (TIMER_INTERVAL)

okay

- I would recommend a FirstOfGroup loop. It has better read-ability and is faster the bigger the picked number of unit is.
If you want stick to your way:

...

Hu?? Put unit u = GetFilterUnit() to the top and use it.

        local real x2 = GetUnitX( GetFilterUnit( ) )
        local real y2 = GetUnitY( GetFilterUnit( ) )
        local real a = bj_RADTODEG * Atan2( y2 - y1, x2 - x1 )
        local unit u = GetFilterUnit( )

okay.

- Please null struct members in the end.

where? in the struct code itself? or u mean like, set data.u = null

- private constant integer ORDER_ID = 852177 //Order ID of ability. Which ablilty? Make sure your comments provide a context.

ok

- Use one periodic timer instead of re-starting it every 0.02 seconds.

Periodic timers confuse me, sorry.

- TIMER_INTERVAL should be set to 0.02 and not 50.

? 50 is easier for users.(my code does 1/TIMER_INTERVAL, giving you .02).

- use a player variable as member of the struct --> data.owner = GetTriggerPlayer() to get rid of tons of unnessesary GetOwningPlayer() calls.

I thought it was better to have smaller structs? So only store stuff you need to store?

 

[Malhorne]

private constant real TIMER_INTERVAL = 50. //Timer interval.
I suggest 32 rather than 50.

local real x1 = data.x
        local real y1 = data.y

Just why XD ?

I suggest you to inline the func :

private function OnSpell takes nothing returns boolean
        if GetSpellAbilityId( ) == ABIL_ID then
            call Actions( )
        endif
        return false
    endfunction

local integer i = GetUnitAbilityLevel( cast, GetSpellAbilityId( ) )
No use since you'll store it in the struct datas.

unit cast = null
        unit orb = null

Why ?
There is exactly the same thing after.

 

[Strikest]

private constant real TIMER_INTERVAL = 50. //Timer interval.
I suggest 32 rather than 50.

why?

local real x1 = data.x
        local real y1 = data.y

Just why XD ?

What?

I suggest you to inline the func :

private function OnSpell takes nothing returns boolean
        if GetSpellAbilityId( ) == ABIL_ID then
            call Actions( )
        endif
        return false
    endfunction

but then i gotta make nulls structs

local integer i = GetUnitAbilityLevel( cast, GetSpellAbilityId( ) )
No use since you'll store it in the struct datas.

ok

unit cast = null
        unit orb = null

Why ?
There is exactly the same thing after.

yeah i know i thought thats what the above commenters meant by nulling struct members lol.

 

[Malhorne]

- Because 32 is smoother and less laggy.

- Why using local when you already stored this ?

-

but then i gotta make nulls structs

Didn't understand oO

 

[BPower]

SurfS

• Please null all struct members which potentially could leak a handle once you deallocate the struct instance --> caster, attach
• function names should follow the JPAG convention, I will make this clear once and for all. I don't approve your resource aslong as the function names stay as they are now.
• The timeout should be calculated in the global block or directly set to the desired value. Also the timer should be periodic
  call TimerStart(NewTimerEx(data), 1/TIMER_INTERVAL, false, function Handler )
• if ALLOW_NO_COLLISION then -->static if ALLOW_NO_COLLISION then
• 50 * DURATION the 50 has to be represented in the globals in logical context to the timer interval.
• There is advantage in setting those to locals.
  

          local real x1 = data.x
          local real y1 = data.y

• casterX/Y set to x2 y2 use them instead call GroupEnumUnitsInRange( GROUP, GetUnitX( data.cast ), GetUnitY( data.cast ), KB_AOE, Filter( function FilterActions ) )
• Redundant if you use a periodic timer.
  

              call SetTimerData( t, data )
              call TimerStart( t, 1./TIMER_INTERVAL, false, function Handler

• Apart from what I meantioned the coding is on an average level. I recomment to use a FoG loop and add a player member to the struct. OnSpell could be merged with Actions

Condense

• Move the native UnitAlive above all functions, not every JassHelper allows double defined native, hence users have to find them quickly.
• JPAG
• Null struct members.
• local real tarx/y can be set directly in function Actions
• Use NewTimerEx(data) instead of SetTimerData
• The orb unit could have the crow ability added in the object editor
• Just No! method onDestroy
• In function Handler x and y should be initialized directly.
• The rotation radius should be configurable
• Again Fog and a periodic timer are highly recommended.
• Overall avarage coding, I saw more tiny things which could be done better. We can discuss them later.
• static method create takes nothing returns Cdnse, if you write the create method yourself the struct could also exentds array.

 

[Malhorne]

The struct don't have to extend array because we use the create method !!
That's a bad point of view !
You can use it to index a static members for example.

EDIT :

local real x1 = data.x
local real y1 = data.y

This is pretty useless >_<