Hunter's Instinct - v1.2 (FINAL VERISON)

[Moderator]

19:10, 11th Aug 2012
Magtheridon96:

Approved.

Tips:
- You don't need to pause a timer before you release it because ReleaseTimer(t) already does that.
- In the onAttack method, you can get rid of the locals and shorten the function by doing this:
call TimerStart(NewTimerEx(GetUnitId(GetAttacker())), ...)
You don't have to cache the attacker.
If you're going to repeat a handle-returning call 3 or more times, then caching it into a local is worth it.
If you're going to repeat a scalar-type-returning call (something that returns an integer, boolean, string, or anything that doesn't need to be nulled at the end) 2 or more times, then caching it into a local is also worth it.

That's a golden rule we came up with almost a year ago

Old Review

- onAttack leaks a timer and a unit.
attacker and t should be nulled regardless of whether the unit has the buff or not.
Also, NewTimer() should only be called after you make sure the unit has the buff, else, you're just creating a NewTimer() that does nothing

Also, you're leaking struct instances.
Instead of setting object to thistype.create() BEFORE you check if the right spell was casted, you should do it AFTER you check if the right spell was casted xD

Finally, the distances should be configurable.

By the way, you should look into the new NewTimerEx function

Old Review

Comments

• I would totally recommend looking into UnitIndexer by Nestharus. It has a GetUnitById function. A fast one too. Your method of enumerating over every unit on the map to get the unit given the id is very inefficient. His function is just an array lookup that inlines. I /can/ approve this while using AutoIndex, but the cost is just too much :/
• The GetDistance library's functions should just be in your spell resource. In fact, you don't need them because you can do them in the actual code directly and optimize them by removing the square root =o. You can remove the square root and just compare the retrieved distance value with your maxDist thing multiplied by itself.
• You can merge the actions and conditions into one function that has an if block to determine whether the spell should execute or not, and you'd just register that function as a condition and return false at the end since conditions run faster than actions. In fact, you can even use a system like SpellEffectEvent by Bribe for this.
• In the onInit function, you should null the triggers.
• In the attackCondition function, you should null the units. You don't even need to store them into variables, just use GetAttacker() instead of 'attacker', and don't declare victim because you aren't using it.
• I would totally recommend handling all the targets differently.
  Use a hashtable or a Table to store the targets and an integer to store the number of targets. The IsUnitGroupEmptyBJ functions enumerates over all units in the group, thus, it's slow. And you don't need to the set the movement of the unit to UNIT_MAX_SPEED inside the if block in this block of code since you're setting it at the end anyways:
  
  JASS Block

          if not IsUnitGroupEmptyBJ(object.targets) then 
              set originalSpeed     = GetUnitMoveSpeed(object.caster)
              set object.speedBonus = originalSpeed*SpeedBonus(object.lvl)
              set finalSpeed = object.speedBonus+originalSpeed
              if (finalSpeed > UNIT_MAX_SPEED) then 
                  set object.speedBonus = object.speedBonus - finalSpeed + UNIT_MAX_SPEED
                  set finalSpeed = UNIT_MAX_SPEED
                  call SetUnitMoveSpeed(object.caster, UNIT_MAX_SPEED)
              endif
              call SetUnitMoveSpeed(object.caster, finalSpeed)
          endif

• Modifying movement speed is not a very good thing to do. It could conflict with other systems. A good solution would be to use some standard movement speed system. I think there are a few in the JASS section on this site, but I never really use any of them, I just code my own.
• You don't need to initialize tempGroup inside the onInit function, you can do that in the declaration of the group.
• GetWidgetLife is faster than GetUnitState when wanting to retrieve unit HP values.
• Why are you repeating GetOwningPlayer(caster) if you just stored it into a struct variable? And it would be faster to set the owner variable to the triggering player since it points to the same thing but takes less arguments.
• When I mentioned how you should handle the group-emptiness checking differently, I forgot to say that you can simply set a boolean to true inside the loop after you add units to your target group. Then, instead of checking if the group isn't empty, just use that boolean in the if block.
• == true is the most redundant thing ever. You can remove it and get the same results.
• GetOwningPlayer(temp.caster) -> temp.owner

There are probably a ton of other things that could be done too, but I'll give you the rest as soon as you fix all the above so it would be easier for you to manage with all the changes :/

 

[Magtheridon96]

grim001 made AutoIndex.

 

[Felz_09]

That was awesome.. hero smells the wounded enemy..really creative!

 

[zv27]

post trigger

 

[Striker21]

Actually, it's a dota-like spell with the invisible stuff
Here: http://www.playdota.com/heroes/bloodseeker
Not i am saying your spell is bad, it still has its own original, but should there be some "balance" here?
Vote for 3/5 (since it lost the point in my mind at the idea, don't mad at me ok? )

 

[Dr.Killer]

Actually, it's a dota-like spell with the invisible stuff
Here: http://www.playdota.com/heroes/bloodseeker
Not i am saying your spell is bad, it still has its own original, but should there be some "balance" here?
Vote for 3/5 (since it lost the point in my mind at the idea, don't mad at me ok? )

Mad? Of course not. I always appreciate constructive criticism. But I don't get your point. Where does it lack balance? The Windwalk damage, movespeed buff or ...?

 

[Striker21]

Ok, since this is a spell, not a map, i don't really think the balance job here is very needed
Anyway, good job in mixing the existed idea with a new way to turns the idea up to new
I can see that's the most well-done of this spell in overall
Since i'm a GUI-er (sad), can't judge on your vJass stuff - Goodluck with the contest
Rating now 4/5 (I wasn't test the map when I 1st post the reply)

 

[maddeem]

Post the code please!

 

[Dr.Killer]

Sure, wait till I apply the fixes Mag mentioned.