• 🏆 Texturing Contest #33 is OPEN! Contestants must re-texture a SD unit model found in-game (Warcraft 3 Classic), recreating the unit into a peaceful NPC version. 🔗Click here to enter!
  • It's time for the first HD Modeling Contest of 2024. Join the theme discussion for Hive's HD Modeling Contest #6! Click here to post your idea!

BouncingGlaives v1.0.5

Well, after reading posts on THW for almost two years, here comes my first spell! It is a simple but fun spell. I hope you can enjoy it. :) Since this is my first spell, it sure needs improvement, so comments are welcomed.

Description: The caster hurls several glaives from his position. When the glaive hits an obstacle it will bounce to other direction. By default, each glaive can bounce five times if it keeps hitting obstacles. However, if an enemey is hit by the glaive, the glaive will damage the enemy before its vanishment.


Credits:
To Anitarf and Maker for makeing the IsTerrainWalkable library
To Malhorne for making the vJass spell tutorial
To PurgeandFire for answering all my questions
To Vexorian for making vJass
To many others how have helped me, such as Bannar, Chobibo, DIMF, muzzel and the list can go on and on.



1. Copy the "Library" and "Spell" folders to your map;
2. Copy the dummy ('h000') to your map, or you can create your own if you like.
3. Import the glaive model if you like it, or you can use any model you want.
Done!


JASS:
scope BouncingGlaives initializer init
/*************************************************************************************
*   BouncingGlaves v1.0.5
*   Discription: the caster hurls several glaives from his position. When the glaive hits an obstacle
*   it will bounce to other direction. By default, each glaive can bounce five times if it
*   keeps hitting obstacles. However, if an enemey is hit by the glaive, the glaive will 
*   damage the enemy before its vanishment.
*
*************************************************************************************
*
*   Requires IsTerrainWalkable
*
*************************************************************************************
*
*   Credits
*
*       To Anitarf and Maker for makeing the IsTerrainWalkable library
*       To Malhorne for making the vJass spell tutorial
*       To PurgeandFire for answering all my questions
*       To Vexorian for making vJass
*
************************************************************************************/
    
    globals
        //Configurations===============================================================
        //The dummy ID, please do match this ID to your dummy ID.
        private constant integer DUM_ID = 'h000' 
        //The ability ID, please match this ID to your ability ID.
        private constant integer ABID = 'A000'
        //The maximum number of glaives that can be hurled by the Hero. Do not set this more than 50.
        private constant integer MAX_NUM_OF_GLAIVE = 30
        //The number of glaive that will be hurled by the Hero at level one.
        private constant integer NUM_OF_GLAIVE = 3
        //How many more glaives when you level up (for each level).
        private constant integer LEVEL_UP = 3
        //The bouncing times of each glaive.
        private constant integer BOUNCE = 5
        //Whether the bouncing angle is random or not. If false, do remember to set the ANGLE bellow.
        private constant boolean RANDOM_ANGLE = true     
        //The bouncing angle. Only works when the above RANDOM_ANGLE is false.
        private constant real ANGLE = 180
        //How fast the glaive moves.
        private constant real SPEED = 25
        //How far away the glaives will appear from the Hero.
        private constant real DISTANCE = 300
        //The attack type of the damage done to the enemy.
        private constant attacktype AT = ATTACK_TYPE_CHAOS
        //The damage type of the damage done to the enemy.
        private constant damagetype DT = DAMAGE_TYPE_DEMOLITION
        //The detection AOE of the glaive. If the distance between the enemy and the glaive exceeds this range, the glaive will not damage it.
        private constant real RANGE = 128
        //The effect when the glaive hits an enemy.
        private constant string FX = "Abilities\\Spells\\Undead\\FrostArmor\\FrostArmorDamage.mdl"
        //The attach point of the effect 
        private constant string ATTACH = "origin"
    endglobals
    
    //The damage amount upon hiting an enemy.
    private constant function GetDamage takes integer level returns real
        return 35. + (10*level)
    endfunction
    
    //How many glaives for each level
    private constant function GetNumberofGlaives takes integer level returns integer 
        return NUM_OF_GLAIVE + (level -1 ) * LEVEL_UP
    endfunction
    
    //Filter out which units should be damaged.
    private function FilterUnits takes unit u, player p returns boolean
        return UnitAlive(u) and IsUnitEnemy(u, p)
    endfunction
    
    //Configuration Ends====================================================================
    
    globals
        private integer deindex = -1
        private timer t = CreateTimer()
        private group g = CreateGroup()
    endglobals
    
    native UnitAlive takes unit id returns boolean
    
    //Uses this struct to store data.
    private struct Data
        
        integer array bounce[50]   //bouncing times of a glaive
        unit array dum[50]         //glaive dmummy
        real array angle[50]       //bouncing angle
        real dmg                    //damage when hits an enemy
        real X                   //x coordinate of the spell casting unit
        real Y                   //y coordinate of the spell casting unit
        integer num                 //the number of glaives
        player p
        
        method destroy takes nothing returns nothing
            if deindex == -1 then
                call PauseTimer(t)
            endif             
            set this.p = null
            call this.deallocate()
        endmethod
    endstruct
    
    globals 
        private Data array data
    endglobals

    private function Loop takes nothing returns nothing
        local integer i = 0
        local integer ii = 0
        local integer iii = 0
        local Data this 
        local real x
        local real y
        local boolean bb = false 
        local unit u
        //Loop through all the instances
        loop
            exitwhen i > deindex
            set this = data[i]
            
            loop
                exitwhen ii > this.num
                
                //Calculate the new coordinates for the glaive
                set x = GetUnitX(this.dum[ii]) + SPEED * Cos(this.angle[ii] * bj_DEGTORAD)
                set y = GetUnitY(this.dum[ii]) + SPEED * Sin(this.angle[ii] * bj_DEGTORAD)
                
                //Moves the glaive if it hasn't reach it maximum bouncing limite and the terrain is walkable
                if this.bounce[ii] > 0 and IsTerrainWalkable(x,y) then 
                    call SetUnitX(this.dum[ii],x)
                    call SetUnitY(this.dum[ii],y)
                    
                    //Check if there are enemys near the glaive
                    call GroupEnumUnitsInRange(g,x,y,RANGE,null)
                    loop
                        set u = FirstOfGroup(g)
                        exitwhen u == null
                        call GroupRemoveUnit(g,u)
                        //If the glaive hits an enemy, does damage and remove the glaive
                        if FilterUnits(u,this.p) then 
                            call UnitDamageTarget(this.dum[ii],u,this.dmg,true,false,AT,DT,null)
                            call DestroyEffect(AddSpecialEffectTarget(FX,u,ATTACH))
                            set this.bounce[ii] = 0
                            set u = null
                            //Only needs the loop to operate once, so ends it here.
                            exitwhen true
                        endif
                    endloop
                    
                else
                    //If the glaive hits an obstacle, it bounces.
                    static if RANDOM_ANGLE then
                        set this.angle[ii] = this.angle[ii] - GetRandomReal(90,180)
                    else
                        set this.angle[ii] = this.angle[ii] - ANGLE
                    endif
                    
                    //Setting new locations for the bouncing glaive
                    set x = GetUnitX(this.dum[ii]) + SPEED * Cos(this.angle[ii] * bj_DEGTORAD)
                    set y = GetUnitY(this.dum[ii]) + SPEED * Sin(this.angle[ii] * bj_DEGTORAD)
                    call SetUnitX(this.dum[ii],x)
                    call SetUnitY(this.dum[ii],y)
                    
                    //Reduce its remaining bouncing times
                    set this.bounce[ii] = this.bounce[ii] - 1
                    
                    //If the glaive has reached its maximum bouncing times, it will be removed.
                    if this.bounce[ii] <= 0 then
                        call RemoveUnit(this.dum[ii])
                        set this.dum[ii] = null
                        
                        //Checks if all glaives are removed
                        loop
                            exitwhen iii > this.num
                            if UnitAlive(this.dum[iii]) then
                                set bb = true
                            endif
                            set iii = iii + 1
                        endloop
                        
                        set iii = 0
                        //If all glaives are removed then deallocates the instance
                        if not bb then
                            set data[i] = data[deindex]
                            set i = i - 1
                            set deindex = deindex - 1
                            //Ends the loop
                            set ii = this.num + 10
                            call this.destroy()
                            set bb = false
                        endif
                    endif
                endif
                set ii = ii + 1
            endloop
            set ii = 0
            set i = i + 1
        endloop
    endfunction
    
    
    private function onCast takes nothing returns boolean
        local unit u = GetTriggerUnit()
        local integer i = 0
        local real angle = 0
        local real newX 
        local real newY 
        local integer n
        local integer lvl = GetUnitAbilityLevel(u,ABID)
        local Data this = Data.create()
        //Stores data in the struct
        set this.p = GetTriggerPlayer()
        set this.X = GetUnitX(u)
        set this.Y = GetUnitY(u)
        set this.num = GetNumberofGlaives(lvl) - 1
        set this.dmg = GetDamage(lvl)
        set n = GetNumberofGlaives(lvl)
        if this.num >= MAX_NUM_OF_GLAIVE then
            set this.num = MAX_NUM_OF_GLAIVE - 1
            set n = MAX_NUM_OF_GLAIVE
        endif
        
        //Creates glaives=======================================
        loop
            exitwhen i > this.num
            set newX = this.X + DISTANCE * Cos(angle * bj_DEGTORAD)
            set newY = this.Y + DISTANCE * Sin(angle * bj_DEGTORAD)
            set this.dum[i] = CreateUnit(this.p,DUM_ID,newX,newY,angle)
            set this.bounce[i] = BOUNCE + 1
            set this.angle[i] = angle
            set angle = angle + 360/n
            set i = i + 1
        endloop
        //=======================================================
        set deindex = deindex + 1
        set data[deindex] = this
        if deindex == 0 then
            call TimerStart(t,0.0312500,true,function Loop)
        endif
        set u = null
        return false 
    endfunction
    
    private function run takes nothing returns boolean
        if GetSpellAbilityId() == ABID then
            call onCast()
        endif
        return false
    endfunction

    //===========================================================================
    private function init takes nothing returns nothing
        local trigger t = CreateTrigger()
        call TriggerRegisterAnyUnitEventBJ( t, EVENT_PLAYER_UNIT_SPELL_EFFECT )
        call TriggerAddCondition(t, Condition(function run))
        set t = null  //I don't think it needs to be nulled here, but nulling it won't hurt.
    endfunction
endscope


v.1.0.5: Final version, minor bug fix.
v.1.0.4: now the number of glaives increases when the ability levels up, also fixs a minor bug.
v.1.0.3: fixes a possible unit leak.
v.1.0.2: fixes a bug and uses one global group variable for better efficiency.
v.1.0.1: makes some improvement.
v.1.0: initial release.



Keywords:
glaive, bounce, ninja, chi, hunter, bouncing
Contents

只是另外一张魔兽争霸III的地图 (Map)

Reviews
18th Apr 2016 Your resource has been reviewed by BPower. In case of any questions or for reconfirming the moderator's rating, please make use of the Quick Reply function of this thread. Review: Totally fun to test. This could be useful for...

Moderator

M

Moderator

18th Apr 2016

General Info

Your resource has been reviewed by BPower.
In case of any questions or for reconfirming the moderator's rating,
please make use of the Quick Reply function of this thread.
Review:

Totally fun to test. This could be useful for a mini-contest/map.

The configuration options could be a bit more level orientated.
I would say that vector calculation should be done in radians and not degrees.
The code is ok. Yet there is room for optimization.

Troubleshooting:

  • The timer timeout could be stored into a descriptive constant.
    ---
  • You don't update the dummy facing on bounce.
    ---
  • Dummy units could be paused to spare computation time.
    ---
  • static method onCast could return null instead of a boolean.
    ---
  • local integer n could be directly initialized to GetNumberofGlaives(lvl) by moving n under integer lvl.
    this.num would then be n - 1.
    ---
  • velocity in XY could be cached to get rid of extra Cos/Sin calculations.
    ---
  • JASS:
    //Ends the loop
    set ii = this.num + 10
    ^exitwhen true?

Review changelog:
  1. -
 
SPEED * Cos(this.angle[ii] * bj_DEGTORAD
SPEED * Sin(this.angle[ii] * bj_DEGTORAD should be used as a global variables.

You've leaked group g in Loop function, just create a global group and use it for entrie spell.
call GroupEnumUnitsInRange(g,x,y,128,null)

Area of effects should be in configurable part.

DISTANCE * Cos(angle * bj_DEGTORAD)
DISTANCE * Sin(angle * bj_DEGTORAD)

Should be outside the loop.

Since you've used struct to create spell (Yup!, it's vjass). Your spell can't become a jass because it's contain vjass func, so I suggest you to create spell using struct (vjass). If you want your spell become jass. Just use indexing instead of struct Data. Or if you really want it become a real vjass, put your code inside struct Data and recode it ;)..

exitwhen ii > this.num - 1,this.num - 1 in Loop func calculating every .03125s, you have to fix it.

call DestroyEffect(AddSpecialEffectTarget(FX,u,"origin")), "origin" should be in configurable part.

Fun spell anyway :).
 
Last edited:

Bannar

Code Reviewer
Level 26
Joined
Mar 19, 2008
Messages
3,140
JASS:
        integer array bounce[50]      //bouncing times of a glaive
        unit array dum[50]              //glaive dummy
        real array angle[50]
Drop the size, it does nothing.
JASS:
    globals
        private Data array data
    endglobals
You aren't coding this in GUI, you would rather make use of linked list instead of iterating an array.

JASS:
local group g

loop
    set g = CreateGroup()
    // Enumerate (...)
endloop
should be just local group g = CreateGroup(). Each GroupEnum call clears the group, thus using just one is fine. You could even argue to use global group instead. The Create/Destroy adds unnecessary overhead.

set this.p = GetOwningPlayer(tri) -> set this.p = GetTriggerPlayer()
JASS:
if this.bounce[ii] > 0 and IsTerrainWalkable(x,y) then
    // stuff
else
    if this.bounce[ii] <= 0 or not IsTerrainWalkable(x,y) then
        // stuff
You already know after the first "if" that either bounces are <= 0 or terrain isn't walkable thus the second if statement is pointless, drop it.

JASS:
//Ends the loop.
set u = null
Nulling "u" is unnecessary, your FoG loop will do this for you: exitwhen u == null.

Improve the readability, it's bad right now. Insert breaks between locals/code and separate if statements as well as loops so we can finally see whats goin on instead of looking at code bloat.

You have forgotten to null the trigger in init() function.
 
@Bannar, no need to null the local trigger unless you don't destroy it.

Also players don't need to be nulled, but it's not really something bad to do.^^

@nhocklanhox6, where is the sense you moved call GroupRemoveUnit(g,u) to the end of loop in your example?

DISTANCE * Cos(angle * bj_DEGTORAD) DISTANCE * Sin(angle * bj_DEGTORAD)
Should be outside the loop.
Can't find this part. ^^
Since you've used struct to create spell (Yup!, it's vjass). Your spell can't become a jass because it's contain vjass func, so I suggest you to create spell using struct (vjass). If you want your spell become jass. Just use indexing instead of struct Data. Or if you really want it become a real vjass, put your code inside struct Data and recode it ;)..
"Your spell can't become jass, because it uses some vjass" What are you referring to? :eekani:

@Geshishouhu, an expiration timer would be cool, that defines the max life duration for a glaive.
 
Level 11
Joined
Oct 11, 2012
Messages
711
You've leaked group g in Loop function, just create a global group and use it for entrie spell.
call GroupEnumUnitsInRange(g,x,y,128,null)

First of all, thanks for the comment.
I think I have destroyed the group right after the end of the loop.
exitwhen ii > this.num - 1,this.num - 1 in Loop func calculating every .03125s, you have to fix it.

call DestroyEffect(AddSpecialEffectTarget(FX,u,"origin")), "origin" should be in configurable part.
I will fix these.

JASS:
        integer array bounce[50]      //bouncing times of a glaive
        unit array dum[50]              //glaive dummy
        real array angle[50]
JasssHelper throws me errors if I do not specify an array size. :(
JASS:
local group g

loop
    set g = CreateGroup()
    // Enumerate (...)
endloop
should be just local group g = CreateGroup()
set this.p = GetOwningPlayer(tri) -> set this.p = GetTriggerPlayer()
JASS:
if this.bounce[ii] > 0 and IsTerrainWalkable(x,y) then
    // stuff
else
    if this.bounce[ii] <= 0 or not IsTerrainWalkable(x,y) then
        // stuff
You already know after the first "if" that either bounces are <= 0 or terrain isn't walkable thus the second if statement is pointless, drop it.
Thanks for pointing these out.
JASS:
//Ends the loop.
set u = null
Nulling "u" is unnecessary, your FoG loop will do this for you: exitwhen u == null.
I set u to null because I want to stop the loop immediately.

Improve the readability, it's bad right now. Insert breaks between locals/code and separate if statements as well as loops so we can finally see whats goin on instead of looking at code bloat.

You have forgotten to null the trigger in init() function.
I will fix this for more readibility.

@Geshishouhu, an expiration timer would be cool, that defines the max life duration for a glaive.
I didn't do that because I want the glaive to keep bouncing until it reaches the maximum bouncing limit. But maybe I will make your opinion as a configurable option for players. Thanks.

native UnitAlive takes unit u returns boolean --> native UnitAlive takes unit id returns boolean it maybe works with "u" aswell, but I'm not sure about this.
I think it should be fine, but I will change it. Thanks.
 
@IceManBo: Here ^^!
JASS:
 loop
            exitwhen i > this.num - 1
            set newX = this.triX + DISTANCE * Cos(angle * bj_DEGTORAD)
            set newY = this.triY + DISTANCE * Sin(angle * bj_DEGTORAD)
            set this.dum[i] = CreateUnit(this.p,DUM_ID,newX,newY,angle)
            set this.bounce[i] = BOUNCE + 1
            set this.angle[i] = angle
            set angle = angle + 360/this.num
            set i = i + 1
        endloop

and about GroupRemoveUnit, I am wrong, my bad >.<..., This is none sense ==!..Just my habit >.<...

My wrong imagination: u=FirstOfGroup(g)
GroupRemoveUnit(g,u) <=> u=null


IceManBo said:
"Your spell can't become jass, because it uses some vjass" What are you referring to?

I don't know 0.0..

Ges said:
First of all, thanks for the comment.
I think I have destroyed the group right after the end of the loop.

Yes. But..

The Helper said:
Destroying a group that has had an enumeration called for it leaks RAM.

For more information: http://world-editor-tutorials.thehelper.net/cat_usersubmit.php?view=138374

Native GroupEnumUnitsInRange doesn't requires DestroyGroup like ForGroup or ForGroupBJbecause it used GroupRemoveUnit, which remove unit from the group, after removed all the unit, that group will be empty!. And yes, that group still have CreateGroup() until DestroyGroup is called. But what the point ?, because that group is empty, just simply to re-use it instead of local group g and CreateGroup() and DestroyGroup and g=null every .0312500s.
 
Last edited:
Level 11
Joined
Oct 11, 2012
Messages
711
Native GroupEnumUnitsInRange doesn't requires DestroyGroup like ForGroup or ForGroupBJbecause it used GroupRemoveUnit, which remove unit from the group, after removed all the unit, that group will be empty!. And yes, that group still have CreateGroup() until DestroyGroup is called. But what the point ?, because that group is empty, just simply to re-use it instead of local group g and CreateGoup() and DestroyGroup and g=null every .0312500s.

So GroupEnumUnitsInRange leaks anyway unless I use a global group variable? :(
 

Bannar

Code Reviewer
Level 26
Joined
Mar 19, 2008
Messages
3,140
I set u to null because I want to stop the loop immediately.
JASS:
                    loop
                        set u = FirstOfGroup(g)
                        exitwhen u == null
                        call GroupRemoveUnit(g,u)

                        set u = null
                    endloop
What do you want to stop? When you set unnecessarily u to null, loop starts new iteration anyways and now it sets u to returned value from FirstOfGroup() function. When group if empty (and it will be sooner or later) the FirstOfGroup() will automatically return null, thus your last operation wasn't needed at all.

GroupEnum doesn't leak if handled properly, but it clears the group prior to actual enumeration.

Btw, if you just drop the size, then yes it will throw you some errors. Code has to be reorganized (approach a bit modified) to compile properly.
 
Level 11
Joined
Oct 11, 2012
Messages
711
JASS:
                    loop
                        set u = FirstOfGroup(g)
                        exitwhen u == null
                        call GroupRemoveUnit(g,u)

                        set u = null
                    endloop
What do you want to stop? When you set unnecessarily u to null, loop starts new iteration anyways and now it sets u to returned value from FirstOfGroup() function. When group if empty (and it will be sooner or later) the FirstOfGroup() will automatically return null, thus your last operation wasn't needed at all.

GroupEnum doesn't leak if handled properly, but it clears the group prior to actual enumeration.

Btw, if you just drop the size, then yes it will throw you some errors. Code has to be reorganized (approach a bit modified) to compile properly.

Oh, I got your point. I set u to null because I only want the loop to operate once. But you are right, setting u to null causes the FoG function to return a value to u again. I will fix that.

So do I correctly handle the GroupEnum issue? I have read the tutorial and it recommends using a global group variable, maybe I should use GroupUtils. However, I want my spell less dependent on other resources, so if someone want to use it, it will be easier for them to import.

I am not sure what do you mean by reorganizing the code to avoid the JassHelper error. Sorry.. :(

Yes and no, if that native is used right, it will be fine, but for more safer, you have to use global group variable!.

For global group variable:
When using group in period func to operate some stuff every <s>, try to avoid DestroyGroup, just use GroupRemoveUnit instead!

Read more:
Thanks and I have read the tutorial. It recommends a global group...
 
Level 11
Joined
Oct 11, 2012
Messages
711
If you want to use list instead of iteration method (through an array) and get rid of sized arrays then yes, your code has to be reorganized.
However, you will feel better with your code once you do this, the spell will be more efficient.

You don't need to do anything tho :)
Oh, got it. I will use a linked list if that is required for approval. :)
+Rep to you and nhocklanhox6
 

Bannar

Code Reviewer
Level 26
Joined
Mar 19, 2008
Messages
3,140
It won't be. You will never need to code your stuff like someelse does.

Personaly, before I finally reached the point where I have "static framework" for my spells I've been using plenty of approaches, at the end I'm convenient that I've went through most of available methods regardless if it's GUI or jass extension. After such process, you got enough knowledge of what to do and when to do it :)
 
Level 11
Joined
Oct 11, 2012
Messages
711
It won't be. You will never need to code your stuff like someelse does.

Personaly, before I finally reached the point where I have "static framework" for my spells I've been using plenty of approaches, at the end I'm convenient that I've went through most of available methods regardless if it's GUI or jass extension. After such process, you got enough knowledge of what to do and when to do it :)

I think you are right, it is good to know the pros and cons for each approach. You sound like a Zen master BTW. :D
 

Bannar

Code Reviewer
Level 26
Joined
Mar 19, 2008
Messages
3,140
Now I know what you meant in regard to looping "just once". However you shouldnt do that like you just did:
JASS:
                    //Check if there are enemys near the glaive
                    call GroupEnumUnitsInRange(g,x,y,128,null)
                    loop
                        set u = FirstOfGroup(g)
                        exitwhen u == null
                        call GroupRemoveUnit(g,u)
                        //If the glaive hits an enemy, does damage and remove the glaive
                        if FilterUnits(u,this.p) then
                            call UnitDamageTarget(this.dum[ii],u,this.dmg,true,false,AT,DT,null)
                            call DestroyEffect(AddSpecialEffectTarget(FX,u,ATTACH))
                            set this.bounce[ii] = 0
                        endif
                        //Only needs the loop to operate once, so ends it here.
                        call GroupClear(g)
                    endloop
I think it's rather bad design i.e you should loop through units within group and check if one of those is viable target, if so, perform the effect and break the loop:
JASS:
                    //Check if there are enemys near the glaive
                    call GroupEnumUnitsInRange(g,x,y,128,null)
                    loop
                        set u = FirstOfGroup(g)
                        exitwhen u == null

                        call GroupRemoveUnit(g,u)
                        //If the glaive hits an enemy, does damage and remove the glaive
                        if FilterUnits(u,this.p) then
                            call UnitDamageTarget(this.dum[ii],u,this.dmg,true,false,AT,DT,null)
                            call DestroyEffect(AddSpecialEffectTarget(FX,u,ATTACH))
                            set this.bounce[ii] = 0
                            exitwhen true
                        endif

                    endloop
 
Level 19
Joined
Mar 18, 2012
Messages
1,716
set t = null //I don't think it needs to be nulled here, but nulling it won't hurt.
A potential leak can only occur, if the corresponding trigger would be destroyed at some point of the game. Nulling "t" is optional.

local unit tri = GetTriggerUnit() a more convenient name for a local unit is the character "u".

local group g = CreateGroup() --> Usually I recommend to use only one global group for group enumerations (if possible). That way you avoid the more or less heavy create- destroygroup operation.

The detection aoe of glaives should be configurable and not hardcoded.

if RANDOM_ANGLE == true then --> RANDOM_ANGLE is a constant boolean, hence it should a static if instead. Also cut out the "== true" static if RANDOM_ANGLE then

It's optional, but a possible way for a linked list would be:
JASS:
private integer array next
private integer array prev
//...
set next[this] = 0
set prev[this] = prev[0]
set next[prev[0]] = this
set prev[0] = this

if (0 == prev[this]) then
    call TimerStart(...)
//....
set next[prev[this]] = next[this]
set prev[next[this]] = prev[this]
           
if (0 == next[0]) then
    call PauseTimer(..)
 
Level 11
Joined
Oct 11, 2012
Messages
711
Yeah, but Flying Shadows look cool. ;_;
Well, you can add the shadows in the object editor if you want them. :)

Now I know what you meant in regard to looping "just once". However you shouldnt do that like you just did:
JASS:
                    //Check if there are enemys near the glaive
                    call GroupEnumUnitsInRange(g,x,y,128,null)
                    loop
                        set u = FirstOfGroup(g)
                        exitwhen u == null
                        call GroupRemoveUnit(g,u)
                        //If the glaive hits an enemy, does damage and remove the glaive
                        if FilterUnits(u,this.p) then
                            call UnitDamageTarget(this.dum[ii],u,this.dmg,true,false,AT,DT,null)
                            call DestroyEffect(AddSpecialEffectTarget(FX,u,ATTACH))
                            set this.bounce[ii] = 0
                        endif
                        //Only needs the loop to operate once, so ends it here.
                        call GroupClear(g)
                    endloop
I think it's rather bad design i.e you should loop through units within group and check if one of those is viable target, if so, perform the effect and break the loop:
JASS:
                    //Check if there are enemys near the glaive
                    call GroupEnumUnitsInRange(g,x,y,128,null)
                    loop
                        set u = FirstOfGroup(g)
                        exitwhen u == null

                        call GroupRemoveUnit(g,u)
                        //If the glaive hits an enemy, does damage and remove the glaive
                        if FilterUnits(u,this.p) then
                            call UnitDamageTarget(this.dum[ii],u,this.dmg,true,false,AT,DT,null)
                            call DestroyEffect(AddSpecialEffectTarget(FX,u,ATTACH))
                            set this.bounce[ii] = 0
                            exitwhen true
                        endif

                    endloop
Thanks for pointing that out. Fixed. :)
A potential leak can only occur, if the corresponding trigger would be destroyed at some point of the game. Nulling "t" is optional.

local unit tri = GetTriggerUnit() a more convenient name for a local unit is the character "u".

local group g = CreateGroup() --> Usually I recommend to use only one global group for group enumerations (if possible). That way you avoid the more or less heavy create- destroygroup operation.

The detection aoe of glaives should be configurable and not hardcoded.

if RANDOM_ANGLE == true then --> RANDOM_ANGLE is a constant boolean, hence it should a static if instead. Also cut out the "== true" static if RANDOM_ANGLE then

It's optional, but a possible way for a linked list would be:
JASS:
private integer array next
private integer array prev
//...
set next[this] = 0
set prev[this] = prev[0]
set next[prev[0]] = this
set prev[0] = this

if (0 == prev[this]) then
    call TimerStart(...)
//....
set next[prev[this]] = next[this]
set prev[next[this]] = prev[this]
           
if (0 == next[0]) then
    call PauseTimer(..)

Fixed everything except the last one(linked list), I may add a linked list version later.
Edit:v.1.0.2 also fixes a bug which is related to the quantity of glaives.
 
Last edited:
Level 19
Joined
Mar 18, 2012
Messages
1,716
Object editor
Remove Teechtree - StructureBuild and Techtree - Upgrades used in the object editor for the glaive dummy unit.

Code
How can this.num exceed MAX_NUM_OF_GLAIVE, since this.num is calculated via set this.num = NUM_OF_GLAIVE - 1?
I think it would be nice if the number of created glaives is dependent on the ability level.
You could add a function GetNumberOfGlaives takes integer level returns integer.

if bb == false then --> if not bb then
 
Level 11
Joined
Oct 11, 2012
Messages
711
Remove Teechtree - StructureBuild and Techtree - Upgrades used in the object editor for the glaive dummy unit.
Will fix that.

How can this.num exceed MAX_NUM_OF_GLAIVE, since this.num is calculated via set this.num = NUM_OF_GLAIVE - 1?
Because MAX_NUM_OF_GLAIVE is not NUM_OF_GLAIVE? :D I'm not sure if I got your point, sorry.

Edit: I think i get it, will fix.

I think it would be nice if the number of created glaives is dependent on the ability level.
You could add a function GetNumberOfGlaives takes integer level returns integer.
Yea, it is doable.

if bb == false then --> if not bb then
Huh... will fix.
 

Bannar

Code Reviewer
Level 26
Joined
Mar 19, 2008
Messages
3,140
There is a typo:
* Rquires IsTerrainWalkable

@Sunreaver - you will need JNPG or newer JNPG 2.0.X to work with vJass without problems. The only requirement of this spell is IsTerrainWalkable. Import that one into your map, together with this spell to use the ability.
Don't forget about dummy unit/setting the ability.

Read the OP notes and see the test map for additional info.
 
Level 11
Joined
Oct 11, 2012
Messages
711
I don't know how to use so i only need to CnP ?
Also i can't save map after using Jassz i don't know what problem maybe I'm a noob.

As Bannar said, you need JNPG for this spell to work. vJass spell is actually easy to implement, for this one, you only need to copy the "library" and "spell" folders along with the dummy unit to your map, and change the ability and dummy IDs if necessary. If you like the model you can also import it to your map. There is no difference between vJass spell and GUI spell in terms of implementation procedure.

@Bannar, typo fixed. :D
 
Level 14
Joined
Nov 18, 2007
Messages
816
JASS:
        //The maximum number of glaives that can be hurled by the Hero. Do not set this more than 50.
        private constant integer MAX_NUM_OF_GLAIVE = 30
Why not more than 50? Why such an arbitrary limit?

How about doing this instead:
JASS:
        integer array bounce[MAX_NUM_OF_GLAIVE]   //bouncing times of a glaive
        unit array dum[MAX_NUM_OF_GLAIVE]         //glaive dmummy
        real array angle[MAX_NUM_OF_GLAIVE]       //bouncing angle
This removes the artificial limit of 50 and potentially increases the maximum amount of instances that can be run in parallel.



JASS:
        //How fast the glaive moves.
        private constant real SPEED = 25
Is this the amount moved per second?

JASS:
                //Calculate the new coordinates for the glaive
                set x = GetUnitX(this.dum[ii]) + SPEED * Cos(this.angle[ii] * bj_DEGTORAD)
                set y = GetUnitY(this.dum[ii]) + SPEED * Sin(this.angle[ii] * bj_DEGTORAD)
I guess not. So whats the actual speed?

JASS:
        if deindex == 0 then
            call TimerStart(t,0.0312500,true,function Loop)
        endif
Oh. So the timer triggers 32 times per second, which means the glaives move at a speed of 25*32 = 800 units per second.

Point being: Always use normalized values for configuration and allow others to adjust how often the timer will trigger.



Okay, so imagine MAX_NUM_OF_GLAIVE was 50, and GetNumberofGlaives returned 51 for a particular unit using this spell.

this.num would be set to 50, passing the following check against MAX_NUM_OF_GLAIVE.

Now, within the loop you only exitwhen i > this.num, which causes it to write to this.dum[50]. However, index 50 is actually the 51st index because arrays start at 0, so your loop is writing past the end of its array, potentially overwriting data of another instance of this spell.



JASS:
                    //If the glaive hits an obstacle, it bounces.
                    static if RANDOM_ANGLE then
                        set this.angle[ii] = this.angle[ii] - GetRandomReal(90,180)
                    else
                        set this.angle[ii] = this.angle[ii] - ANGLE
                    endif
I dont like this. Ill grant you that collision detection is hard, but this just causes the glaives to bounce around in unexpected ways. Not sure i have a quick fix for this though, so i wouldnt worry about it.




Maybe switch to radians entirely. It would simplify the code enough that the readability alone might be a good reason to change this.



If you have further questions, just ask.
 
Top