Looking for Feedback on a Spell I made using XE

11-29-2008, 09:19 AM

I made my first spell and was wondering if anyone could give me some advice on things I did wrong/right here. I know this spell isn't MUI atm, and I'm assuming instantiating a new copy of targetLocs for each spell cast would achieve that so casts that occur while another is happening won't share targetLocs. A question I have is what's the difference between nulling a location and removing is using RemoveLocation() and such for other types as well, do they both stop leaks or no? Also for whatever reason I can't get the AncestralSpiritCaster model to appear, although as I type this I'm thinking I should've tried scaling up the model to see if it was just simply too small to be seen. Anyways I'm just looking for some general advice on how well this is coded, if I should've put something in a struct, or if I don't need this here, etc. Especially if I could've used XE better somewhere. I'll paste the spell below, thanks for looking. =)

JASS:

library ImpetuousHail initializer init requires xebasic, xepreload, xedamage, xecollider
    globals
        //The Triggering Spell ID
        private constant integer SPELLID = 'A004'  
        
        //The fx to display on the ground below players to warn them of the spell being cast
        private constant string SPELL_WARNING = "Abilities\\Spells\\Human\\FlameStrike\\FlameStrikeTarget.mdl" 
        //private constant string SPELL_WARNING = "Abilities\\Spells\\Orc\\AncestralSpirit\\AncestralSpiritCaster.mdl" //doesn't work lol?
        
        //The fx of the missiles falling down ontop of the enemies
        private constant string SPELL_MISSILE = "Units\\Demon\\Infernal\\InfernalBirth.mdl"
        
        //The fx to display when the missile impacts ontop of the unit
        private constant string SPELL_IMPACT = "Abilities\\Weapons\\SteamTank\\SteamTankImpact.mdl"
        
        //Set the time until impact - unfortunately this won't remove your
        //missile animation, it just controls when the impact animation plays
        private constant real MISSILE_LIFETIME = .85
    endglobals
    
    private function setMaxTargets takes integer level returns integer
        if (level == 1) then
            return 3
        else
            return (level + 2)
        endif
    endfunction
    
    private function getDamage takes integer level returns integer
        return 80 * level
    endfunction
    
    private function getDamageFactor takes integer level returns real
        if (level == 1) then
            return 2
        else
            return 2 + (level * 1.0)
        endif
    endfunction
    
//*************************************************************************************
    globals 
        private unit spellCaster
        
        private integer targetCount = 0
        private integer maxTargetCount = 0
        
        private location array targetLocs[4]
        
        private boolexpr getTargets      
    
        private xedamage damageOptions      
    endglobals    
        
    function onSpellEnd takes nothing returns nothing       
        local integer i = 0

        loop
            exitwhen i >= targetCount
            call RemoveLocation(targetLocs[i])
            set i = i + 1
        endloop
        
        set targetCount = 0        
        set spellCaster = null           
    endfunction       
    
    private function setDamageOptions takes xedamage xd returns nothing
        //damage and attack types + valid targets
        set xd.dtype = DAMAGE_TYPE_FIRE
        set xd.atype = ATTACK_TYPE_NORMAL

        set xd.damageEnemies = true
        set xd.damageAllies = true
        set xd.damageNeutral = false
        set xd.damageSelf = true
    endfunction        

    private struct Missile extends xecollider
        location target
        integer level
        
        method onDestroy takes nothing returns nothing        
            //noway to destroy fx mid-animation to only show them for partial animation? =/
            call SetUnitAnimation(spellCaster, "stand 2")        
            call DestroyEffect(AddSpecialEffectLoc(SPELL_IMPACT, this.target))
            call damageOptions.damageAOE(spellCaster, GetLocationX(this.target), GetLocationY(this.target), 100, getDamage(.level))
            
            set targetCount = targetCount - 1
            if (targetCount <= 0) then
                call onSpellEnd()
            endif
            
            //I'm assuming the xe system will call .terminate, since lifetime is up..
            //call this.terminate()
        endmethod
    endstruct    
        
    private function isTarget takes nothing returns boolean
        //if ((GetRandomReal(0, 1) >= .75) and (targetCount < maxTargetCount)) then
            //call BJDebugMsg("unit targeted")
            if ((damageOptions.allowedTarget(spellCaster, GetFilterUnit())) and (targetCount < maxTargetCount)) then
                set targetLocs[targetCount] = GetUnitLoc(GetFilterUnit())
                set targetCount = targetCount + 1
            //else
                //call BJDebugMsg("unit skipped - invalid target")
            endif
        //else
            //call BJDebugMsg("unit skipped - no dice")
        //endif        
        return false
    endfunction
    
    private function onSpellEffect takes nothing returns nothing
        local Missile m
        local integer i = 0
        local integer level
        local group possibleTargets = CreateGroup()
        local location spellTarget = GetSpellTargetLoc()        
        set spellCaster = GetTriggerUnit()
        set level = GetUnitAbilityLevel(spellCaster, SPELLID)
        
        set maxTargetCount = setMaxTargets(level)
        call damageOptions.factor(UNIT_TYPE_STRUCTURE, getDamageFactor(level))
        
        //Find the Unit's we're going to "target" and then store their locations and attempt
        //to make it appear as if the infernal is throwing something up in the air lol.
        call GroupEnumUnitsInRange(possibleTargets, GetLocationX(spellTarget), GetLocationY(spellTarget), 400, getTargets)
        call SetUnitAnimation(spellCaster, "attack 2")   
        call GroupClear(possibleTargets)
        
        //Show a Warning to the Targets that a cast has happened      
        call DestroyEffect(AddSpecialEffectLoc(SPELL_WARNING, spellTarget))
        
        //Create our missiles that fall from the sky
        loop
            exitwhen i >= targetCount
            
            set m = Missile.create(GetLocationX(targetLocs[i]), GetLocationY(targetLocs[i]), 0)
            set m.level = level            
            set m.fxpath = SPELL_MISSILE
            set m.expirationTime = MISSILE_LIFETIME
            set m.target = targetLocs[i]
            
            set i = i + 1
        endloop
        
        set spellTarget = null
    endfunction
    
    private function spellIDMatch takes nothing returns boolean
        return (GetSpellAbilityId() == SPELLID)
    endfunction    
    
    private function init takes nothing returns nothing
        local trigger t = CreateTrigger()
    
        set getTargets = Condition(function isTarget)

        set damageOptions = xedamage.create()
        call setDamageOptions(damageOptions)
        call XE_PreloadAbility(SPELLID)
        call TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_UNIT_SPELL_EFFECT)
        call TriggerAddCondition(t, Condition(function spellIDMatch))
        call TriggerAddAction(t, function onSpellEffect)
        
        set t = null
    endfunction    
endlibrary

12-01-2008, 10:07 PM

Anitarf

Using xecollider for making simple timed effects seems like a bit of an overkill.
Using coordinates is preferred to using locations.
You could create structs in the group callback function directly, no need to store targets in an array first.
Use a static group for your enums. The way you currently do it leaks.
SetDamageOptions should be in the calibration section.
Can we get a description of what the spell is supposed to do? It helps in reading the code.

12-01-2008, 10:36 PM

hitokirixeno

Quote:

Originally Posted by Anitarf

Using xecollider for making simple timed effects seems like a bit of an overkill.
Using coordinates is preferred to using locations.
You could create structs in the group callback function directly, no need to store targets in an array first.
Use a static group for your enums. The way you currently do it leaks.
SetDamageOptions should be in the calibration section.
Can we get a description of what the spell is supposed to do? It helps in reading the code.

Ah, I thought I had put a description in here. Basically it's an AoE target spell that picks x units in the target area and plays an effect, then deals damage in a radius of 100 around each unit.

I know that using xecollider is pretty much overkill but I don't believe that xefx has a lifetime property so I extended xecollider to to use the lifespan and then used onDestroy to deal the second animation and deal damage.

12-02-2008, 10:25 AM

Anitarf

Quote:

Originally Posted by hitokirixeno

I know that using xecollider is pretty much overkill but I don't believe that xefx has a lifetime property so I extended xecollider to to use the lifespan and then used onDestroy to deal the second animation and deal damage.

Why would you even use xefx, you're not moving the effects around or anything. You could just create plain old effects, store them to a struct and run a timer; you'd need TimerUtils then, but on the other hand you wouldn't need xefx or xecollider.