Opinion about simple Dota spell !

07-23-2009, 05:37 PM

Hey guys, so I am making another vJass tutorial. In this new tutorial I will talk about structures, timers, and give a quick introduction to what I think modularity should be and how it should be used.
As a motivation I decided to make a spell from a map we all hate by its infamous popularity - Dota - I will make one of the arrow abilities from the Ranger hero.

Basically, I would like opinions about the code. Do you think it is efficient, modular and neat? Please, remember I do NOT want to use xecollider in purpose, using it would take away all the fun xD
I am open to suggestions.

Also, is there another way to make the unit move ? I am not sure if the way I am moving the arrow is the best.

JASS:

//===========================================================================
//Sends an arrow that will travel and damage enemy units in a line.
//
//@Requires:
// - TimedLoop
// - GroupUtils
//
//@author Flame_Phoenix 
//
//@Credits:
// - Vexorian, for TimedLoop
// - Rising_Dusk, for GroupUtils
//
//@version 1.2.0
//===========================================================================
scope ForestArrowThree initializer Init
//===========================================================================
//=============================SETUP START===================================
//===========================================================================
    globals
        private constant integer AID = 'A001'  //id of the hero ability
        private constant integer ARROW_ID = 'h000' //id of the arrow unit
        private constant real ARROW_COLLISION = 40. //collision size of the arrow
        private constant real ARROW_SPEED = 30. //the speed of the arrow
        private constant attacktype ATT_TYPE = ATTACK_TYPE_MAGIC    //attack type of arrow
        private constant damagetype DAM_TYPE = DAMAGE_TYPE_NORMAL   //damage type of arrow
    endglobals
    
    private constant function DamageReduction takes integer level returns real
    //the damage reduction the arrow will suffer after hiting a target
        return 30. //+ level * 0
    endfunction
    
    private constant function Damage takes integer level returns real
    //the damage the arrow will make to each target
        return 60. * level
    endfunction
    
    private constant function Distance takes integer level returns real
    //the distance the arrow will run before dying
        return 750. * level
    endfunction
    
    private function Targets takes unit caster, unit target returns boolean
    //the targets the arrow will affect
        return IsUnitEnemy(target, GetOwningPlayer(caster)) and (GetWidgetLife(target) > .405) and (IsUnitType(target, UNIT_TYPE_MAGIC_IMMUNE) == false) and (IsUnitType(target, UNIT_TYPE_STRUCTURE) == false) and (IsUnitType(target, UNIT_TYPE_MECHANICAL) == false)
    endfunction
//===========================================================================
//=============================SETUP END=====================================
//===========================================================================
    globals
        private boolexpr chooseVictims
        private group victims
        private unit tmpCaster
    endglobals
    
    private function PickVictims takes nothing returns boolean
        return Targets(tmpCaster, GetFilterUnit())
    endfunction
    
    private struct anArrow
        unit caster
        unit arrow
        integer level
        group oldVictims
        real angle
        real distanceMade
        integer unitsHit
        
        //method required by TimedLoop 
        private method onTimedLoop takes nothing returns boolean
            //we get the structure from the timer
            local real newX = GetUnitX(.arrow) + ARROW_SPEED * Cos( .angle )
            local real newY = GetUnitY(.arrow) + ARROW_SPEED * Sin( .angle )
            local unit f
            
            //if the arrow is outside the map, we simple kill it !
            if (newX > GetRectMaxX(bj_mapInitialPlayableArea)) or (newY > GetRectMaxY(bj_mapInitialPlayableArea)) or (newX < GetRectMinX(bj_mapInitialPlayableArea)) or (newY < GetRectMinY(bj_mapInitialPlayableArea)) then
                return TimedLoop_STOP   
            endif
            
            //moveArrow
            call SetUnitPosition(.arrow, newX, newY)
            
            //increment the distance
            set .distanceMade = .distanceMade + ARROW_SPEED
            
            //cause the damage
            set tmpCaster = .caster
            call GroupEnumUnitsInRange(victims, newX, newY, ARROW_COLLISION, chooseVictims)
            loop
                set f = FirstOfGroup(victims)
                exitwhen(f == null)
                call GroupRemoveUnit(victims, f)
                
                //we only damage the target if it is the first time we meet him
                if (IsUnitInGroup(f, .oldVictims)==false) then
                    //damage the bad guy! xD
                    call UnitDamageTarget(.caster, f, Damage(.level) - .unitsHit * DamageReduction(.level), true, false, ATT_TYPE, DAM_TYPE, null)
                    
                    set .unitsHit = .unitsHit + 1 
                    
                    //add the bad guy to the .oldVictims group so we don't damage him again 0.03 seconds after
                    call GroupAddUnit(.oldVictims, f)
                endif
            
            endloop
            
            
            //if the distance made by the arrow is greater then the distance the arrow needs to run, we kill it!
            if .distanceMade >= Distance(.level) then
                return TimedLoop_STOP
            endif
            
            return TimedLoop_CONTINUE
        endmethod
     
        implement TimedLoop //This does the module magic
        
        static method create takes unit aCaster, location spellLoc returns anArrow
            local anArrow this = anArrow.allocate()
            
            //set some members
            set .caster = aCaster
            set .level = GetUnitAbilityLevel(.caster, AID)
            set .angle = Atan2(GetLocationY(spellLoc) - GetUnitY(.caster), GetLocationX(spellLoc) - GetUnitX(.caster))
            set .distanceMade = 0
            set .arrow = CreateUnit(GetOwningPlayer(.caster), ARROW_ID, GetUnitX(.caster), GetUnitY(.caster), bj_RADTODEG * .angle)
            set .unitsHit = 0
            
            //now we create the group to save teh old victims
            set .oldVictims = NewGroup()
            
            //The TimedLoop script works by
            // creating a startTimedLoop method that will
            // do all the dirty work and end up calling
            // .onTimedLoop...
            call .startTimedLoop()
            
            return this
        endmethod
        
        method onDestroy takes nothing returns nothing
            //Kill the arrow 
            call KillUnit(.arrow)
        
            //we destroy the group and stop the Timer!
            call ReleaseGroup(.oldVictims)
        endmethod
    endstruct
//===========================================================================
    private function Conditions takes nothing returns boolean
        
        if GetSpellAbilityId() == AID then
            //we create our arrow and we are done !
            call anArrow.create(GetTriggerUnit(), GetSpellTargetLoc())
        endif
        
        return false
    endfunction
//===========================================================================
    private function Init takes nothing returns nothing
        local trigger ForestArrowThreeTrg = CreateTrigger(  )
        call TriggerRegisterAnyUnitEventBJ(ForestArrowThreeTrg, EVENT_PLAYER_UNIT_SPELL_EFFECT )
        call TriggerAddCondition(ForestArrowThreeTrg, Condition(function Conditions))
        
        //setting our globals
        set victims = CreateGroup()
        set chooseVictims = Condition(function PickVictims)
    endfunction
endscope

07-23-2009, 06:15 PM

chobibo

standard:

    
     local real newX = GetUnitX(.arrow) + ARROW_SPEED * Cos( .angle )
     local real newY = GetUnitY(.arrow) + ARROW_SPEED * Sin( .angle )

chobibo's way:

struct Arrow
    unit arrow
    real x_constant
    real y_constant
    
    method move takes nothing returns nothing
        /*  To move the unit, just add a coordinate constant to
            the present coordinate of a unit.                  */
            
        call SetUnitPosition( .arrow, GetUnitX(.arrow)+.x_constant, GetUnitY(.arrow)+.y_constant )
        
        /*  No Trigonometric functions, only addition.          */
    endmethod
    
    static method create takes unit whichArrow, real arrow_speed, real angle returns Arrow
        local Arrow this=Arrow.allocate()
        local real x=GetUnitX(whichArrow)
        local real y=GetUnitY(whichArrow)
        
        set angle=angle*bj_DEGTORAD
        
        set .arrow=whichArrow
        
        /*  Instead of saving the angle and the targets position
            use the difference from A to B, since the movement's
            velocity is constant, so is it's increments per 
            interval. Only works for linear movement though     */
            
        set .x_constant=( x+arrow_speed*Cos(angle) )-x
        set .y_constant=( y+arrow_speed*Sin(angle) )-y
        
        return this
    endmethod
    
endstruct

You're using modular libraries, but your spell isn't modular. Collision detection, Map Bounds checking and Damage application are all embedded on the arrow's movement function. The object's properties aren't configurable, you can't edit the missile speed, missile damage etc., those configurations must be present on the object, and it must be configurable.

That's my analysis, don't get offended ok.

07-23-2009, 11:12 PM

Anitarf

Quote:

Originally Posted by Flame_Phoenix

Quote:

Please, remember I do NOT want to use xecollider in purpose, using it would take away all the fun xD

Wait, is this supposed to be fun, or is it supposed to be a good tutorial on modularity? Seriously, do it right or don't bother submitting it.

07-23-2009, 11:40 PM	#4
Blubb-Tec	i think there is a use for a _tutorial_ to not consist to 98% of already made systems. stuff like xe is nice, but having done something similar before will give you a very, very good feeling for how to use that stuff. and, remember, its a vJass tutorial, not a XE tutorial. and i really don't remember xe being a part of vJass :P

07-24-2009, 12:06 AM	#5
Kwah	I need to write a tutorial telling people that using systems is actually a good thing.

07-24-2009, 12:17 AM	#6
busterkomo	If you're truly persistent on not using xe, then at least do this: 1. Store the GetRectMaxX/equivalents in globals. 2. Don't leak a location. 3. Use GroupUtils. 4. Store the Cos and Sin as struct members (already mentioned).

07-24-2009, 02:45 AM

Anitarf

Quote:

Originally Posted by Blubb-Tec

i think there is a use for a _tutorial_ to not consist to 98% of already made systems.

Possibly, but not when the topic of the tutorial is modularity. That kind of tutorial is precisely the kind of thing that should be all about using existing systems.

Quote:

and, remember, its a vJass tutorial, not a XE tutorial. and i really don't remember xe being a part of vJass :P

It is a dummy unit standard, though, and any resource that deals with dummy casters or projectiles should utilize it.

07-24-2009, 07:00 AM

Litany

Quote:

Originally Posted by Anitarf

It is a dummy unit standard, though, and any resource that deals with dummy casters or projectiles should utilize it.

xecollider? I don't think so. Last I checked it can't even reproduce Blizzard's normal projectiles, and the code and feature set are kind of a mess. xefx is OK for dummies in general though.

07-24-2009, 09:04 AM

Flame_Phoenix

Quote:

Possibly, but not when the topic of the tutorial is modularity.

The tutorial is not about modularity neither about using xe. It makes a small mention to modularity. And don't worry, I decided to submit this to people who care, not to wc3c. Looks like mods have trouble reading huge tutorials around here. Anyway I am not up for discussing this matter so I will move on.

Quote:

Personally I think it makes sense to check map bounds in the movement method, we kinda want to avoid the arrow moving out of the map. However, do you know any library for map bounds? I know BoundSentinel but it is useless in this specific case. As for damage, you think I should call another method? Like a damage method? As for the object, I am well aware. You can edit the speed at which it moves by changing ARROW_SPEED, you can also change the damage it does ass well as the damage reduction per target. You can even change the collision of the missile, it is all on the SETUP section.

Quote:

I need to write a tutorial telling people that using systems is actually a good thing.

Using systems is good if you are a user and want to be nothing else. If you want to be a creator, you will need a second approach. My tutorial will try providing both approaches.

Quote:

If you're truly persistent on not using xe, then at least do this:
1. Store the GetRectMaxX/equivalents in globals.
2. Don't leak a location.
3. Use GroupUtils.
4. Store the Cos and Sin as struct members (already mentioned).

It is not about not using xe. In fact, I will use it. You see, in the end of the tutorial I will show an extreme case of modularity in which I will use xecollider and I will demonstrate how this spell can be done in 10 lines or less using the power of extreme modularity and importing systems.
As for your comments:
- Mmm that idea occurred to me ... maybe I should make a lib for that?
- I was afraid of this. Thx for the confirmation!
- I am using GroupUtils lol...
- I get the idea about saving cos and sin, this way I don't need to calculate the all the time. Thx.

07-24-2009, 09:23 AM

#10

chobibo

Quote:

- I get the idea about saving cos and sin, this way I don't need to calculate the all the time. Thx.

Hey why didn't you read this...

more efficient way:

struct Arrow
    unit arrow
    real x_constant
    real y_constant
    
    method move takes nothing returns nothing
        /*  To move the unit, just add a coordinate constant to
            the present coordinate of a unit.                  */
            
        call SetUnitPosition( .arrow, GetUnitX(.arrow)+.x_constant, GetUnitY(.arrow)+.y_constant )
        
        /*  No Trigonometric functions, only addition.          */
    endmethod
    
    static method create takes unit whichArrow, real arrow_speed, real angle returns Arrow
        local Arrow this=Arrow.allocate()
        local real x=GetUnitX(whichArrow)
        local real y=GetUnitY(whichArrow)
        
        set angle=angle*bj_DEGTORAD
        
        set .arrow=whichArrow
        
        /*  Instead of saving the angle and the targets position
            use the difference from A to B, since the movement's
            velocity is constant, so is it's increments per 
            interval. Only works for linear movement though     */
            
        set .x_constant=( x+arrow_speed*Cos(angle) )-x
        set .y_constant=( y+arrow_speed*Sin(angle) )-y
        
        return this
    endmethod
    
endstruct

The truth is you don't need to store the cos and sin, it requires multiplication, while my method works with just addition...

Quote:

What I'm saying is that to make the spell modularized, you need to segregate the functions or methods into small chunks of code that works for a specific case, i.e. A special section of code for collision detection, arrow movement, damaga application. I think writing it instead of explaining it will do better to make my statement clearer. I'll do it later.

07-24-2009, 09:40 AM

#11

Rising_Dusk

Quote:

Originally Posted by Flame_Phoenix

Using systems is good if you are a user and want to be nothing else. If you want to be a creator, you will need a second approach. My tutorial will try providing both approaches.

What the hell are you talking about? I don't care what you call yourself, if you use things and if you create things, abstracting code away to systems and scripts is positively the correct and most proper way to do it. Period.

Quote:

Originally Posted by Flame_Phoenix

And don't worry, I decided to submit this to people who care, not to wc3c.

You say this and then expect us to help you optimize your code? Are you for real? Go ask your people "who care" to optimize your code for you. Locked -- and if another person unlocks it, I'm going to delete it. I'm sick of this kid acting like he can abuse our help despite his utter lack of respect for the staff, their jobs, and the site.

07-25-2009, 01:07 AM

#12

Anitarf

I'll just abuse my admin powers for one small clarification:

Quote:

Originally Posted by Litany

xecollider? I don't think so.

I was refering to xe in general and, if anything, xebasic in particular.