Z Slash v2.3

[Moderator]

Z Slash v2.3 | Reviewed by Maker | 28th Apr 2013

APPROVED

THe ability works, is MUI and leakless The indentation could be fixed Use static if in if TARGET_LIMIT == 0 then And here if INV then The onPeriod method could be shorter as you have repeated lines there There is no animation for the spell, it doesn't look good

[tr]

Old reviews

Z Slash v2.2 | Reviewed by Maker | 18th Apr 2013

NEEDS FIX

The indexing is wrong, the spell is not MUI The timer is never paused You can use return not break in IsLineWalkable You should enum TARGET_LIMIT + 1 in Z since it enums the caster also, but you remove it from the group You have the destroy block twice in the looping method and the code for the three phases is almost the same. You could shorten the code

[tr]

Z Slash v2.0 | Reviewed by Maker | 14th Apr 2013

NEEDS FIX

There is a debug message in the code, displayed during every loop The timer is never paused The ability can take you through pathing blockers When you set dx and dy the first time, you could use TRAVEL1 global You could abort the movement if the caster is dead

[tr]

Z Slash v1.1 | Reviewed by Maker | 10th Apr 2013

NEEDS FIX

The spell is MUI and leakless There is no pathing detection so the unit can get stuck in the middle of trees or structures. Additionally, you can go over cliffs and get stuck at a point you can get out of By default, the spell could filter out flying units Your SetUnitInvulnerable doesn't use the global boolean You don't need to call GroupClear in onCast because the group will be empty anyway You don't need to null tempUnit at the end of onCast as the unit group loop sets that to null You could use u instead of tempUnit in onCast in the unit group loop With 0.03 interval, it could be better to use a single timer for all instances You should precalculate travel as it is derived from constant globals Make it a global also You should precalculate travel*Cos(angle) and travel*Sin(angle) Panning camera could be optional ReleaseTimer pauses the timer, no need to call PauseTimer

[tr]

 

[Almia]

Vertex colors should be configurable,as well as animation.
You should also add functions for unit filters.
Timers for every instance is a bad idea.You should use linked list because you are also using custom allocation/deallocation.

 

[Dr.Killer]

I'll add the vertex configurables and the filter. And I have no idea how to use a linked list for timers.
Thanks for your feedback.

 

[Almia]

No not for timers.You can just only use one timer for the loop and iteration,not for every instance you have you will create a timer.

 

[Almia]

                if not IsLineWalkable(hero_X, hero_Y, angle[this], PATH_OFFSET, 150.) then
                    set break[this] = true
                endif

->
set break[this] = not IsLineWalkable(hero_X, hero_Y, angle[this], PATH_OFFSET, 150.)

            if totalTravel[this] < JUMP_DIST then
                
    
                set x = hero_X + dx[this]
                set y = hero_Y + dy[this]
                
                if not IsLineWalkable(hero_X, hero_Y, angle[this], PATH_OFFSET, 150.) then
                    set break[this] = true
                endif
                
                call SetUnitX(hero[this], x)
                call SetUnitY(hero[this], y)
                
                set totalTravel[this] = totalTravel[this] + (JUMP_DIST / (DUR/INTERVAL))
                
                if TARGET_LIMIT == 0 then
                    call GroupEnumUnitsInRange(tempGroup, x, y, RADIUS, null)
                else
                    call GroupEnumUnitsInRangeCounted(tempGroup, x, y, RADIUS, null, TARGET_LIMIT+1)
                endif
                call GroupRemoveUnit(tempGroup, hero[this])
                loop
                    set tempUnit = FirstOfGroup(tempGroup)
                    exitwhen tempUnit == null
                    
                    set target_id = GetUnitId(tempUnit)
                    
                    if TargetFilter(tempUnit, hero[this], injured[this][target_id]) then
                        call UnitDamageTarget(hero[this], tempUnit, DamageOnSlash(GetUnitAbilityLevel(hero[this], ABIL_CODE)), false, false, ATTACK_TYPE, DAMAGE_TYPE, WEAPON_TYPE)
                        call DestroyEffect(AddSpecialEffectTarget(BLOOD_EFFECT_PATH, tempUnit, ATTACH_POINT))
                        set injured[this][target_id] = 1
                    endif
                    call GroupRemoveUnit(tempGroup, tempUnit)
                endloop
    
            else
                set totalTravel[this] = 0.
                set travelStage[this] = travelStage[this] + 1
                set angle[this] = angle[this] + bj_PI + Z_ANGLE
                set dx[this] = TRAVEL_2*Cos(angle[this])
                set dy[this] = TRAVEL_2*Sin(angle[this])
                //call BJDebugMsg("first ende")
            endif

This makes everything longer,cache this into a method.

Indention is messed up.

 

[Dr.Killer]

What you said here doesn;t affect the performance I think, and about the indention I'm really too lazy to tab all those lines forward. Forgive me about this

 

[Almia]

We want the code as simple as possible.

 

[mckill2009]

- Remove the TimerUtils coz it's useless
- You could use T32 or TimerTools instead of making your own periodic timer
- You could make the travel stage another static function above onPeriod so that it's more readable...
- You may use the index of the unit instead of Table e.g. injured or slashed, just make a static integer array and set it to 1...

 

[Almia]

^ Read that.
Because awesome mckill2009 is awesome.

 

[Maker]

If you want to drop some requirements, you could use GetHandleId instead of GetUnitId.

 

[Dr.Killer]

Thanks for your feedback guys, I'll change the code when I get rid of my exams!

 

[MasterTrainer]

This is indeed wonderful... I'm going to use it in my map. Thanks for making it!

 

[Cokemonkey11]

Timers for every instance is a bad idea.You should use linked list because you are also using custom allocation/deallocation.

Not really. Timers are quite efficient actually. Linked list is better but a stack is the correct method. None of that matters though because individual timers for a spell that lasts 1.5 seconds is virtually unnoticeable. I suggest you take both versions of the spell and compare how many simultaneous casts are required to lag

Also, the correct reason is not because he's using allocation/deallocation, but because he's using a timer period small enough such that the spell is intended to be smooth (Notice a linked list with one timer can look terrible for effects with timer periods on the order of 1 second)

There's another reason but I'll get to that soon.

What you said here doesn;t affect the performance I think, and about the indention I'm really too lazy to tab all those lines forward. Forgive me about this

You're correct about the performance, but you really should endeavor to tab lines. You want your spell scripts to teach others good habits.

If you're using JNGP you can select multiple lines and tab them simultaneously (as well as shift-tab them)

We want the code as simple as possible.

The code is simpler with one timer per instanciation actually. What you should have said is "We want code that is indicative of good practices for others to learn from."

- Remove the TimerUtils coz it's useless

That's quite an outrageous claim. If he's not instanciating his own hashtable, everything is fine.

- You may use the index of the unit instead of Table e.g. injured or slashed, just make a static integer array and set it to 1...

Indexing units is terrible practice, I can't believe you guys still suggest it. The correct solution is to use a group.


----


Back to the actual spell:

The Good:

• Your coding style and attitude is great by comparison to others with similar hive experience.
• Your spell works as intended and doesn't lag; doesn't have any obvious glitches/bugs.
• Your script is properly customizable.

The Bad:

• You don't prevent the caster from moving/attacking while casting. With the default duration of 1.5, you can see many cases of deflection in the slide trajectory and it looks very poor. PauseUnit on its own is unsafe - you have some other options though.
• You use IsTerrainWalkable but don't list it in your description or requirements (by the way, some others will bash you for using this, and then shut up when you tell them nothing better exists)
• Honestly I have no idea why you need UnitIndexer, but it's so common I won't complain.

Overall: ***. Will update to ***** if you fix the issue with caster attacking (because there are cases when it completely ruins the spell - take DUR=5. and JUMP_DIST=100. and you will see)

 

[mckill2009]

That's quite an outrageous claim. If he's not instanciating his own hashtable, everything is fine.

Pls dont get me wrong, TU is very useful indeed but not by this spell coz he's NOT using it so it's useless...

 

[Cokemonkey11]

Pls dont get me wrong, TU is very useful indeed but not by this spell coz he's NOT using it so it's useless...

That makes more sense. Never requires a system you're not using