Tidal Payback v1.2

[Moderator]

Tidal Payback v1.2 | Reviewed by Maker | 6th Oct 2013

APPROVED

The spell is now so much better than what it first was You could make Actions and Init part of the struct Method callback is redundant, remove it CTL by Nestharus is an awesome library

[tr]

Old reviews Tidal Payback v1.1b | Reviewed by Maker | 4th Oct 2013 NEEDS FIX Null handle type struct members after you destroy/remove them to avoid leaks Units, timers, effects... You need pathing detection because you can get stuck everywhere, for example in the middle of buildings timer is not a good variable name as it gets highlighted You could make Actions and Init part of the struct CTL by Nestharus is an awesome library You could add support for limiting units being damaged only once per cast You set facing at 0 rate, so why not use SetUnitFacing instead of SetUnitFacingTimed You could try not hiding the caster, set vertex color to 0 instead. Move it along with the dummy. This way the caster won't disappear from the minimap and you don't need to lose selection Selection status should be saved before hiding the unit [tr] Tidal Payback v1.0 | Reviewed by Maker | 28th Sep 2013 NEEDS FIX Well chosen sound effects and icon. The Thunderclap and Naga Death effects are a bit overused in spells generally but are ok for this spell The spell is leakless an MUI. Well, creating units leaks but only very little When casting the ability to go out of walkable map bounds, the geysir can appear quite far away from where the caster exited the map. The caster itself also teleports here and there which looks unprofessional Under no circumstance do you use PauseUnit to disable user commands. Do not use it with spells, ever Use the ability level at the time of casting The text in the tooltip is ok, but it could be improved. It is not in a standard ability format. I prefer sticking to the standard Warcraft style, because if you import 20 spells into your map and have to change every single one, it can be a pain in the rear end.You should list the next level to be learned like "Ability Name - [Level x]" and it should list the per level stats as every standard non ultimate spell does. The text is not completely in Warcraft style. You have some excessive "the" articles in there, for example "Does the charge", "all the units nearby". The text could use some polishing and could be slightly compressed. The text should not be very long. Always place two or more caster in a test map so it is easy to test if the spell is MUI If the spell alters flying hight, it is a very good idea to not have completely flat terrain The dummy units do not need Invulnerability ability. They have Locust which makes them invulnerable You could consider using Spell Effect Event You could consider using Table You could consider using TimerUtils You could have used struct and TimerUtils to attach the struct to the timer You could consider merging actions with conditions, you woul only add a condition to your trigger, not actions 0.04 seconds is a bit slow period for constant movement as it makes things look choppy. It updates the position only 25 times per second 1/32 rate is the most common one used, it offers good performance and smoothness When using low rates like around 0.04, a single timer can and should be used to handle all instances. Now that I think of it, CTL library by Netharus would have been perfect for this Precalculate RP_DISTANCE_PER_PERIOD * Cos(coordAngle) and the Sin one also In Timer_Walkyou calculate nextX, nextY, abilityLevel even if you do not need them. Same thing in Timer_Jump, you calculate a lot of stuff you might not need Usually spells based on Channel should not have "Disable other abilities" enabled [tr]

 

[Rheiko]

wow, cool
it's awesome
i like it, damn it..
good job bowser
5/5

 

[FUJI]

You can store the Cosine and Sine to the real variable seems they're constant but nvm !

4.5/5 ( Good Spell )

 

[bowser499]

Storing Cos and Sin is still more function cost than using them directly. That is why I did so.

 

[iAyanami]

You could improve the spell's configuration. Currently, your incremental values are limited to static increases. Let's say what if I wanted distance values to be like 1000/1100/1300/1600. You could use a function to improve customization.

CalcAngleBetweenCords is unnecessary. Just use Atan2 directly when needed.

Weapon type can be left as null in UnitDamageTarget.

You do not need to create a group for TempGroup. Simply use bj_lastCreatedGroup

You don't need the Action. Just merge your actions into your Condition.

private function Actions takes nothing returns boolean
    if GetSpellAbilityId() == ABILITY_ID then
        // your actions
    endif

    return false
endfunction

private function Init takes nothing returns nothing
    local trigger AbilityCast = CreateTrigger()
    call TriggerRegisterAnyUnitEventBJ(AbilityCast,EVENT_PLAYER_UNIT_SPELL_EFFECT)
    call TriggerAddCondition(AbilityCast,Condition(function Actions))
    set AbilityCast = null
endfunction

You could be using a struct for this. Any reason why you're not using it? Or do you just prefer hashtables? You could use a struct-based timer for your movement part if you were to use a struct, which would improve efficiency.

 

[Luorax]

Storing Cos and Sin is still more function cost than using them directly. That is why I did so.

At the moment you do:

• 1 Hashtable read
• 1 Sin and 1 Cos calls respectively
• 2 (real) multiplications and 2 additions

With them being precalculated you'd do:

• 2 Hashtable reads (yup, you don't need to read the angle for other things)
• 2 additions.

So how is this slower than calling Sin and Cos every single time?

I'd have also considered using arrays and storing only an integer (kinda like how structs are implemented in vJASS), the instance in the hashtable, OR using a unit indexer and attaching it to the unit's index (via an int array). With the latter one you'd only be working with arrays, giving the code a huge speed boost. You could have also used a struct with one of the mentioned storage methods, but I'm pretty sure you have your reasons why you avoided them.

 

[bowser499]

Updated!

 

[bowser499]

Let's say what if I wanted distance values to be like 1000/1100/1300/1600.

50 * level * level - 50 * level + 1000

Go for it.

 

[iAyanami]

50 * level * level - 50 * level + 1000

Go for it.

Yep, precisely. That's why I would have preferred a constant function for the configuration rather than constant variables

 

[bowser499]

Updated.

 

[Ofel]

Awesome!

Edit
Ahh, can't give you more rep... ^^

 

[NightStalker]

wow i didnt download it but in the video i can see the awesomennesss after the sliding the jump and rolling made me go insane!!!

 

[Uyarrr]

Wow really awesome i like panda jumping when spell done.Good job 5/5