Rush v1.05

[Moderator]

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

[tr]

 

[sniper_zero]

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

 

[Doomlord]

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

   if condition then // Instead of == true
   
   // or
   
   if not condition then // Instead of == false

9. This code block can be improved

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

   and

       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.     if dist_check <= 100 then
            set facing = GetUnitFacing(caster)
        elseif dist_check > 100 then
            set facing = bj_RADTODEG*(Atan2(dy, dx))
        endif

    
    should be

        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

        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

 

[sniper_zero]

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

 

[Doomlord]

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.

 

[doomhammer99]

yay. He used my Description Template ))

 

[sniper_zero]

Doesn't everybody?

 

[deathismyfriend]

Doesn't everybody?

nope lol. although it is a nice one.

 

[Almia]

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

 

[Doomlord]

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

 

[sniper_zero]

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?

 

[Doomlord]

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:

   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:

       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:

        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:

        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:

        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

 

[sniper_zero]

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.

 

[Almia]

16. Optional.

 

[Doomlord]

3 - What do you mean impassable terrain?
4 - Can you show me a way to get current distance? All I can think of is saving 0 and adding offset to it every iteration.
5 - Didn't have enough time. Will work on this soon.
7 Thought about it. If I'm using it more than once wouldn't it be better to store it in a variable?
15 Haven't started yet. Would having a filter function really make it faster?

3/

        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.

 

[sniper_zero]

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

    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.

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

 

[Doomlord]

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

 

[sniper_zero]

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?

 

[Doomlord]

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.

 

[sniper_zero]

Updated again. I guess I misunderstood what you mean by end code.

 

[Doomlord]

Nice to hear that then

You may want to remove the debug messages though =))

 

[sniper_zero]

It's for the users to check for themselves if there's any errors in the map if they still have any doubts. If it really is that much of a bother though, I'll remove it.

 

[deathismyfriend]

no debug messages should be in the spell. It makes it look like the spell is bad / bugged.

This should be updated to a vJass spell if it requires vJass to use.

 

[sniper_zero]

Only the script terrain pathability check is in vJASS though. The rest is just in normal JASS. Got it, I'll remove the debug messages when I get home.

 

[deathismyfriend]

it stil requires vJass tho thats y it is a vJass spell not a jass spell.

 

[sniper_zero]

The map no longer contains the debug messages. You'll still see it in the description template but the actual map has them removed. Also changed the category to vJASS now.

 

[watermelon_1234]

The code in the test-map doesn't match the code you put in the description. I'm assuming the code in the description is the updated one.

Short Review

• GetUnitAbilityLevel(u, 'Avul') == 0is not really a good check for invulnerable units since it won't be true for all the ways a unit can become invulnerable in WC3. I would suggest just removing it.
• The I2R calls are useless since the calculations in your functions involves reals.
• You still have some SomeBoolean == false instead of not SomeBoolean
• I'd suggest a configurable string for setting the animation for the caster when it finishes the spell. Since this is a template, it should be as configurable as possible.
  In general, most literal or constants you put in the code should be configurable, except your hashtable accesses. Even having the 100 * 100 configurable isn't a bad idea in case the user wants more precision (though it seems better off as a template constant).

• call SaveGroupHandle(udg_Rush_Hash, id, 3, g)

  The local variable g is rather useless. You can just do this:
  

  call SaveGroupHandle(udg_Rush_Hash, id, 3, CreateGroup())

If you have the time, you could strive to make this an actual vJass template. For instance, making the template into a module that can be imported into a struct.
You can check the tutorials section for getting started with vJass.

 

[sniper_zero]

The code in the test map is the updated one. I'm just too lazy to update the one in the spell description.

I only see I2R in the top functions where ability level is involved. Since ability level is an integer, don't I have to convert it to a real?

SomeBoolean == false...are you talking about UnitOrSfx == false? Then what do you suggest? This is the only way I could think of to make it swtichable.

100*100 is better off as a constant.

You mean local group at the start?

 

[watermelon_1234]

The code in the test map is the updated one. I'm just too lazy to update the one in the spell description.

I've redownloaded your test map just to make sure and this is what the code looks like:

//This part is where you can configure the spell.
globals
    //Configurables
    integer Rush = 'A000' //This is the ability id the spell uses. Change it after copying the trigger.
    real Speed = 1150 //This is the speed of the caster when he uses the spell.
    real Interval = .03 //How often you want the timer to run.
    real Offset = Speed * Interval //How far the caster moves per interval.
    real Range = 175 //How close units should be to be damaged by the spell.
    boolean UnitOrSfx = false //This decides if you want to use a dummy unit or sfx as the well...sfx of the spell.
    integer Transparency = 80 //How transparent you want the dummy to be.
    integer RushDummy = 'e001' // This is the id of the dummy unit created.
    string Sfx = "Abilities\\Spells\\Orc\\MirrorImage\\MirrorImageCaster.mdl" //The sfx that appears when the caster dashes.
    string Animation = "stand ready" //What animation plays when the caster dashes.
    attacktype AttackType = ATTACK_TYPE_CHAOS //The attack type.
    damagetype DamageType = DAMAGE_TYPE_UNIVERSAL //The damage type. They're both currently set to these configurations to allow easy testing.
    
    //Non-configurables
    group RushGroup = CreateGroup() //Don't touch this. Don't null or destroy this.
endglobals

function Rush_Periodic takes nothing returns nothing
    //Local Variable Setup
    local timer t = GetExpiredTimer()
    local integer id = GetHandleId(t)
    local unit caster = LoadUnitHandle(udg_Rush_Hash, id, 0)
    local real facing = LoadReal(udg_Rush_Hash, id, 1)
    local integer count = LoadInteger(udg_Rush_Hash, id, 2)
    local group g = LoadGroupHandle(udg_Rush_Hash, id, 3)
    local real x = GetUnitX(caster)
    local real y = GetUnitY(caster)
    local real cos = Cos(Deg2Rad(facing))
    local real sin = Sin(Deg2Rad(facing))
    local real x1 = x + Offset * cos
    local real y1 = y + Offset * sin
    local player owner = GetOwningPlayer(caster)
    local unit dummy
    local unit u
    
    //Configurable Locals
    local real damage = (GetUnitAbilityLevel(caster, Rush) * 75)//You can change this to whatever you want.
    local real MaxDistance = (GetUnitAbilityLevel(caster, Rush) * 200) + 200 //This makes the distance you dash scale with the level.
    //End of Configurable Locals
    
    local real Duration = MaxDistance * Interval //This determines how long you dash. Don't touch this unless you know what to do.
    //End Setup
    
    set count = count + 1//This counts how many times you've moved.
    call SaveInteger(udg_Rush_Hash, id, 2, count)
    
    if count < Duration then
        call SetUnitAnimation(caster, Animation)
        if UnitOrSfx == false then
            call DestroyEffect(AddSpecialEffect(Sfx, x, y))
        elseif UnitOrSfx == true then
            set dummy = CreateUnit(owner, RushDummy, x, y, facing)
            call SetUnitAnimation(dummy, Animation)
            call SetUnitVertexColor(dummy, 255, 255, 255, Transparency)
            call UnitApplyTimedLife(dummy, 'BTLF', .5)
            set dummy = null
        endif
        call GroupEnumUnitsInRange(RushGroup, x, y, Range, null)
        loop //What we do here is check if the target is in g. If not, then we damage it and add it to g to prevent it from being damaged again.
            set u = FirstOfGroup(RushGroup)
            exitwhen u == null
            if IsUnitInGroup(u, g) == false and IsUnitEnemy(u, owner) and GetUnitState(u, UNIT_STATE_LIFE) > .405 and IsUnitType(u, UNIT_TYPE_GROUND) and IsUnitType(u, UNIT_TYPE_STRUCTURE) == false then
                call UnitDamageTarget(caster, u, damage, false, false, AttackType, DamageType, null)
                call GroupAddUnit(g, u)
            endif
            call GroupRemoveUnit(RushGroup, u)
            call SaveGroupHandle(udg_Rush_Hash, id, 3, g)
        endloop
        if IsTerrainWalkable(x1, y1) then
            call SetUnitX(caster, x1)
            call SetUnitY(caster, y1)
        endif
        
    elseif count > Duration then
        call GroupClear(g)
        call PauseTimer(t)
        call DestroyTimer(t)
        call SetUnitAnimation(caster, "stand") //Resets the animation.
        call FlushChildHashtable(udg_Rush_Hash, id)
    endif
    
    //Leak Removal
    set t = null
    set caster = null
    set g = null
endfunction

function Rush_Actions takes nothing returns nothing
    //Local Variable Setup
    local timer t = CreateTimer()
    local integer id = GetHandleId(t)
    local unit caster = GetTriggerUnit()
    local real cx = GetUnitX(caster)
    local real cy = GetUnitY(caster)
    local real tx = GetSpellTargetX()
    local real ty = GetSpellTargetY()
    local real dx = tx - cx
    local real dy = ty - cy
    local real dist_check = SquareRoot((dx*dx) + (dy*dy))
    local real facing
    local group g = CreateGroup()
    
    //A little distance check to avoid bugs when you cast the spell in your current position.
    if dist_check <= 100 then
        set facing = GetUnitFacing(caster)
    elseif dist_check > 100 then
        set facing = bj_RADTODEG*(Atan2(dy, dx))
    endif
    
    //Hashtable Setup
    call SaveUnitHandle(udg_Rush_Hash, id, 0, caster)
    call SaveReal(udg_Rush_Hash, id, 1, facing)
    call SaveInteger(udg_Rush_Hash, id, 2, 0)
    call SaveGroupHandle(udg_Rush_Hash, id, 3, g)
    //End Hashtable Setup
    
    call TimerStart(t, Interval, true, function Rush_Periodic)
    
    //Leak Removal
    set t = null
    set caster = null
    set g = null
endfunction

function Rush_Conditions takes nothing returns boolean
    if GetSpellAbilityId() == Rush then
        call Rush_Actions()
    endif
    return false
endfunction

//===========================================================================
function InitTrig_Rush takes nothing returns nothing
    local trigger t = CreateTrigger(  )
    call TriggerRegisterAnyUnitEventBJ( t, EVENT_PLAYER_UNIT_SPELL_EFFECT )
    call TriggerAddCondition( t, Condition( function Rush_Conditions) )
    set udg_Rush_Hash = InitHashtable()
    set t = null
endfunction

Since ability level is an integer, don't I have to convert it to a real?

The integers would be implicitly converted to reals as long as you have reals in the same calculation. Just try removing both I2R calls; you'll see that your map still compiles fine.

Then what do you suggest?

I already suggested an alternative: You still have some SomeBoolean == false instead of not SomeBoolean

You mean local group at the start?

I should have been more clear. I'm talking about the local group g in Rush_Actions.

 

[sniper_zero]

Yep, looks like the updated one.

So I should do it like this? UnitOrSfx not false Anyway, it's late where I am and I'll be busy tomorrow, so I'll probably update the day after tomorrow if I can't find time.

 

[watermelon_1234]

So I should do it like this? UnitOrSfx not false

No. If I'm not being clear enough, in your code, it would look just like this:

if not UnitOrSfx then

not is a logical operator that negates the condition after it.

 

[sniper_zero]

Updated! And I have to apologize, the map that was uploaded before was actually the wrong one. NOW, I've made sure the correct one is uploaded and I've corrected the issues watermelon1234 pointed out.

Lastly, if you're going to tell me to do if not IsUnitType(u, UNIT_TYPE_DEAD) and GetUnitTypeId(u) != 0 then you're going to have to tell me how to bypass the error because every time I try to place it in the filter function, cannot convert integer to boolean and cannot compare variables of primitive types blah blah error pops up. I tried doing this to no avail.

if blah blah then
   return true
endif
   return false

EDIT: Also updated the script in the spell description.
EDIT: I've decided to keep 100*100 and 'stand' animation after the spell as constants.

 

[Almia]

@sniper_zero
put "(" and ")" in the condition.

 

[sniper_zero]

Oohhh, it suddenly works now! Thanks!

 

[deathismyfriend]

change this 100 * 100 to this 10000

your global variable names are 2 generic. They will most likely be used by something else and should be changed so they dont create problems.

 

[sniper_zero]

Okay, but I'm busy at the moment.Will update after I beat Pewter City Gym.

 

[sniper_zero]

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 could use SetUnitPropWindow to lock the facing

Isn't private constant a vjass thing?
Do I use SetUnitPropWindow to precalculate Offset * Cos(facing)? Otherwise, what would happen if I use SetUnitPropWindow? I haven't tried it out yet.

 

[Maker]

Setting prop window to 0 makes the unit unable to move, but it can turn, attack and cast spells etc.

 

[TheFireMage]

Why do I get error with globals? And I'm not very good at vJass so please be simple and tender
This did not happen when I first tested the map. And I'm using patch 1.29
Edit: The globals worked fine now, now I got error "Identifier already in used: Damage"
Edit 2: For the above Edit, I'm still trying to figure to fix that, not sure what triggers interfere with it. Anyway, when I set boolean UnitorSfx to true, the dummy unit faced 0 degree? Hope you can fix that.