Rush v1.05 | Reviewed by Maker | 17th Aug 2013
APPROVED
The spell is leakless an MUI but not very original Your globals should be private constant Take the pathability library out of the header Get rid of Rush Dummy Dies trigger You could precalculate Offset * Cos(facing) and the Sin also You can't pick units with GetUnitTypeId(u) != 0 so no need to check it You could use SetUnitPropWindow to lock the facing Use the ability level at the time of casting Try making this all vjass

 

How can I edit the above post? Made a mistake.

 

Use the Update button.

Review:

1. Local group g is never destroyed.
2. You don't need to save the group handle with every iteration. The thread at that time will complete the loop first before being iterated again. Reconsider your logic there.
3. There isn't a code block for impassable terrain encounter.
4. You can use a real value to keep track of the distance moved and compare it to the max distance instead of having to do excessive calculations.
5. Put the damage and max distance into a global/constant function for easy configuration. It is ill-advised to leave a configurable value in the midst of your code.
6. Make dummy life span configurable.
7. The Offset global is unnecessary.
8. Remove

   == true

   and

   == false

   . They are unnecessary. You can inline them with
   Code (vJASS):

   if condition then // Instead of == true
   
   // or
   
   if not condition then // Instead of == false
9. This code block can be improved
   Code (vJASS):

           if UnitOrSfx == false then
               call DestroyEffect(AddSpecialEffect(Sfx, x, y))
           else
               // Code
           endif
   and
   Code (vJASS):

       if count < Duration then
           // Code
       else
           // Code
       endif
   
   
   Since a boolean can only return either true or false, you don't need to use elseif.
10. Code (vJASS):

        if dist_check <= 100 then
            set facing = GetUnitFacing(caster)
        elseif dist_check > 100 then
            set facing = bj_RADTODEG*(Atan2(dy, dx))
        endif
    
    
    should be
    Code (vJASS):

        if dist_check <= 100 then
            set facing = GetUnitFacing(caster)*bj_DEGTORAD
        elseif dist_check > 100 then
            set facing = (Atan2(dy, dx))
        endif
    
    
    This helps you avoid having to reconvert the values in the set x and y of the periodic function.
11. Duration is a wrong name. Use something like distance per iteration.
12. cx, cy, tx, ty are unnecessary. You don't have to set a local for every single thing. It is not efficient, it is the opposite of that. Either remove them or remove dx, dy and use them in dist_check.
13. cos, sin in the periodic func is also unnecessary. Just use the natives there directly.
14. It is called Nulling, not Leak removal.
15. Create a filter function like this
    Code (vJASS):

        function filterUnit takes unit caster, unit u returns boolean
            return /*
           
            */ IsUnitEnemy(u, GetOwningPlayer(caster)) and /* Target is an enemy of caster
            */ not IsUnitType(u, UNIT_TYPE_DEAD) and /* Target is alive
            */ GetUnitAbilityLevel(u, 'Avul') == 0 and /* Target is not invulnerable
            */ not IsUnitType(u, UNIT_TYPE_STRUCTURE) /* Target is not a structure
            */
        endfunction
    for configuration.
16. You may want to use Maker's Shadow Trail system to awesome-ize (I made this up) the spell.

That is all for now. Ask if you feel like I misread the code

 

Do I have to put the placeholder? Or is someone else going to work on that?

 

No it means I am going to post a review there when I am done. The sentence is to inform you of that so you won't miss it.

 

yay. He used my Description Template ))

 

Doesn't everybody?

 

nope lol. although it is a nice one.

 

And it seems our Crimson Lord of Darkness is now mini-moderating
#SWAG

 

Yeh but it seems like the OP is giving me zero damn =))

edit

Just kidding

 

1. Okay. So I just have to remove DestroyGroup(g) then?

2. So what do you suggest I do here?

3. The script at the map header doesn't check that?

4. Can you point out these excessive calculations?

5. I tried to make them as globals before but then they move at the same distance even if they have different levels of abilities.

6. You mean the expiration timer?

7. I'll remove it.

8. Okay.

9. So I should do it the way you showed me?

10. Will do.

11. Lol. Sure.

12. If I remove, dx and dy what do I use in the distance check? And when should I set something to a local?

13. What natives?

14. Okay.

15. Okay. But why check for invulnerability though? It's not like they'll get damaged.

EDIT: By the way, were you waiting for my reply? I went out for a bit.

EDIT: Didn't see 16. Can I not do that? I have nothiing against Maker's systems and spells. I'm just...not very fond of vJASS. If I can get it to work I'll try to make use of it.

EDIT: I'm curious, suppose every issue except 11 and 14 were gone. Would this still get a Rejected or Needs Fix status?

 

1. You never add it in the first place. Just put it in the timer destroy block.
2. Remove that line
3. No. You can debug a draft code execution and see what happens.
4. Set local damage and max distance, calculate duration. Everything will be simplified if you do it like this:
   Code (vJASS):

   if currentDist >= maxDist then
   // execute end code
   else
   // execute move code
   // save currentDist = currentDist + intervalDist
5. Global is just a suggestion. You can simply go with a function like this:
   Code (vJASS):

       function Rush_GetMaxDistance takes integer level returns real
           return 1000 + level*200
       endfunction
6. Indeed.
7. Nice
8. Nice
9. Yeh.
10. Nice
11. Nice
12. I recommend this:
    Code (vJASS):

       
        local real dx = GetSpellTargetX() - GetUnitX(caster)
        local real dy = GetSpellTargetY() - GetUnitY(caster)
    since you will be using dx, dy again. On a side note, SquareRoot is computationally expensive, you may want to do it like this:
    Code (vJASS):

        local real dist_check = (dx*dx) + (dy*dy)
    
        // Square the compare value here
        if dist_check <= 100*100 then
        else
        endif
13. Do it like this:
    Code (vJASS):

        local real x1 = x + Offset * Cos(facing)
        local real y1 = y + Offset * Sin(facing)
    The

    Sin()

    and

    Cos()

    natives are what I am talking about.
14. Nice
15. It is just an example. You can do it the way you want. Oh and filter out invulnerable units helps reduce computing effort for your spell
16. Be my guest. It is just a suggestion.

I am not an actual moderator so I cannot answer your last question. Sorry =/

edit

Whoops. Forgot to tell you that you can just set the dummy's death type to Can't raise, Does not decay and get rid of the Dummy removal trigger

 

I'll get to it when I'm in front of my desktop.

Where's the timer destroy block? Where I flushed the childtable? And you still have to destroy the group even if you save it in a hashtable and flush it?

For number 2. Will the spell still work fine even if I don't save it? Won't bug or anything?

I'll do something about the impassable terrain but what kind of impassable terrain? Cliffs? Trees?

Didn't think you were referring to the variables sin and cos. I'll never use them as variable names again.

And...never use squareroot and pow? Ever?

And the dummies play their death sound even with Can't Raise, Does not Decay. The Dummy removal trigger prevents that from happening. Should I still remove it?

EDIT: Map updated.

 

16. Optional.

 

3/

Code (vJASS):

        if IsTerrainWalkable(x1, y1) then
            call SetUnitX(caster, x1)
            call SetUnitY(caster, y1)
            else
            // This is the code to be executed when the hero encounters impassable terrain.
            // Add the end code here
        endif

4/ Indeed it is done the way you said. Add offset to a saved real with every iteration.

7/ The Offset variable is unnecessary because it is only a single calculation, it is not worthy of a global. You can put it into a local for use in periodic though.

15/ It is not about efficiency. It is about readability and configuration. The filter func allows easy configuration on the user's end.

 

The spell is updated now. Though since Offset is now used in these equations I thought it should stay. Is that alright?

Code (vJASS):

    local real x1 = x + Offset * Cos(facing)
    local real y1 = y + Offset * Sin(facing)
    set cur_dist = cur_dist + Offset

On another note, I couldn't do that thing you did with \* in the filter function. So my filter function ended up looking like this.

Code (vJASS):

function UnitCheck takes unit caster, unit u returns boolean
    return IsUnitEnemy(u, GetOwningPlayer(caster)) and not IsUnitType(u, UNIT_TYPE_DEAD) and IsUnitType(u, UNIT_TYPE_GROUND) and not IsUnitType(u, UNIT_TYPE_STRUCTURE) and  GetUnitAbilityLevel(u, 'Avul') == 0
endfunction

 

Yeh. It is a vJASS thing. Don't worry.

Anyway, nice job on improving this

edit

Wait. You are supposed to execute the spell end code if the caster encounters impassable terrain, not like that dude :/

Just copy the end code and replace the "stay in same position".

Another note, you should increase then save cur_dist after you moved the unit. It is the correct logic

 

I'm using JNGP so vJASS should be possible right? How do I do that while using vanilla JASS? Or does the trigger editor not let you do that unless you do the library, scope etc. thing only available to vJASS?

 

You have to use JNGP to use those things since they are unique to vJASS. Btw, I updated the previous post, you should check it.