Ability Problem - Caster keeps playing spell animation after stopped

02-25-2011, 11:08 PM

Hello fellow vJass experts, I am in need of your aid.
I've triggered an ability called "Dark Ritual" (creative, eh?), basically what it does is you cast it on a unit, and if this unit is a Skeleton, it should kill the skeleton and dispel surrounding units. However, if the targeted unit is not a skeleton, it should stop the caster and display a SimError.

I got help from PurplePoot with this ability, and it was almost entirely finished when he went AFK, so there was one small problem that was never solved. If the cast fails, the unit stops casting (so no spell is fired) and the SimError is properly displayed, but the caster repeatedly keeps playing his spell animation (like on a loop) until given a new order. I suppose you understand this looks rather dumb, so I'd like to get this fixed.

Oh and while you're at it, could you give me any hints on how to improve and optimize the code?

Thanks in advance.

DarkRitualValidation:

scope DarkRitualValidation initializer Init
    private function Conditions takes nothing returns boolean
        return GetSpellAbilityId() == 'A005'
    endfunction

    private function Actions takes nothing returns nothing
        local unit caster       = GetSpellAbilityUnit()
        local unit target       = GetSpellTargetUnit()
        local string effectpath = "Abilities\\Spells\\Human\\DispelMagic\\DispelMagicTarget.mdx"
        if(GetUnitTypeId(target) != 'u001') then
            call SimError(GetOwningPlayer(caster),"Must target a skeleton.")
            call IssueImmediateOrder(caster,"stop")
        endif
    endfunction

    private function Init takes nothing returns nothing
        local trigger t = CreateTrigger()
        call TriggerRegisterAnyUnitEventBJ(t,EVENT_PLAYER_UNIT_SPELL_CAST)
        call TriggerAddCondition(t,Condition(function Conditions))
        call TriggerAddAction(t,function Actions)
    endfunction
endscope

DarkRitualDispel:

scope DarkRitualDispel initializer Init
    private function Conditions takes nothing returns boolean
        return GetSpellAbilityId() == 'A005'
    endfunction
    
    private function Actions takes nothing returns nothing
        local unit caster       = GetSpellAbilityUnit()
        local unit target       = GetSpellTargetUnit()
        local unit u
        local group g           = CreateGroup()
        local string effectpath = "Abilities\\Spells\\Human\\DispelMagic\\DispelMagicTarget.mdx"
        call GroupEnumUnitsInRange(g,GetUnitX(target),GetUnitY(target),225,null)
        call KillUnit(target)
        loop
            set u = FirstOfGroup(g)
            exitwhen u == null
            if (IsPlayerAlly(GetOwningPlayer(u),GetOwningPlayer(caster))) then
                call UnitRemoveBuffsEx(u,false,true,true,true,false,false,true)
                call DestroyEffectTimed(AddSpecialEffectLoc(effectpath, GetUnitLoc(u)), 0.)
            else
                call UnitRemoveBuffsEx(u,true,false,true,true,false,false,true)
                call DestroyEffectTimed(AddSpecialEffectLoc(effectpath, GetUnitLoc(u)), 0.)
            endif
            call GroupRemoveUnit(g,u)
        endloop
    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 Conditions))
        call TriggerAddAction(t,function Actions)
    endfunction
endscope

02-26-2011, 01:24 AM	#2
Anitarf	I would either use an unused classification (such as ancient or suicidal) to make the ability castable only on skeletons or, in case you have more such abilities than there are unit classifications available, I would use Dusk's LastOrder library in combination with an order event rather than a spell cast event.

02-26-2011, 02:05 AM	#3
Nuclear Arbitor	couldn't you also theoretically do something like order him to move forward an imperceptible amount before/after it displays the error?

02-26-2011, 03:21 AM

PurgeandFire111

@First problem:

JASS:

call PauseUnit(u,true)
call IssueImmediateOrder(u,"stop")
call PauseUnit(u,false)

Quote:

couldn't you also theoretically do something like order him to move forward an imperceptible amount before/after it displays the error?

You can, but it has to be enough so that it actually moves (or the angle difference has to be enough). Either way, it still has a slightly noticeable effect. :(

------------

In terms of optimization, there are a few options. For the spell, "DarkRitualDispel":

1) First of all:
call DestroyEffectTimed(AddSpecialEffectLoc(effectpath, GetUnitLoc(u)), 0.) GetUnitLoc(u) will create a location, and since there is no removal, it ultimately leads to a leak. Also, since I assume (correct me if I'm wrong) the duration is 0, then you can use the normal destroy effect native.

JASS:

call DestroyEffect(AddSpecialEffect(effectpath,GetUnitX(u),GetUnitY(u)))

2) You can use filter enumeration instead of a FirstOfGroup() loop, as it is more efficient.
3) You create a group per spell cast, which is another leak. Instead, use one global group. :)
4) The effect path can be a global constant string.
5) The destroy effect can be moved outside the block since it is cast regardless of the conditions.
6) Don't forget to null your locals. ;P
7) You can also merge your actions + conditions to make it even more compact, and to save a handle. :p

This is how it would probably look optimized:

DarkRitualDispel:

scope DarkRitualDispel initializer Init
    globals
        private constant string effectpath = "Abilities\\Spells\\Human\\DispelMagic\\DispelMagicTarget.mdx"
        private group g = CreateGroup()
        private player temp
    endglobals
    
    private function GroupFilter takes nothing returns boolean
        local unit u = GetFilterUnit()
        if IsUnitAlly(u,temp) then
            call UnitRemoveBuffsEx(u,false,true,true,true,false,false,true)
        else
            call UnitRemoveBuffsEx(u,true,false,true,true,false,false,true)
        endif
        call DestroyEffect(AddSpecialEffect(effectpath,GetUnitX(u),GetUnitY(u)))
        set u = null
        return false
    endfunction
    
    private function Conditions takes nothing returns boolean
        local unit target
        if GetSpellAbilityId() == 'A005' then
            set temp   = GetTriggerPlayer()
            set target = GetSpellTargetUnit()
            call GroupEnumUnitsInRange(g,GetUnitX(target),GetUnitY(target),225,Condition(function GroupFilter))
            call KillUnit(target)
            set target = null
        endif
    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 Conditions))
    endfunction
endscope

That is that trigger optimized. (not sure what else can be optimized of it) Now for the next trigger.

-------------------

For the first trigger "Dark Ritual Validation", there is not much to improve. The only thing I suggest is to merge as I did above and just inline some of the variables since they are used once:

DarkRitualValidation:

scope DarkRitualValidation initializer Init
    private function Conditions takes nothing returns boolean
        local unit u
        if GetSpellAbilityId() == 'A005' and (GetUnitTypeId(GetSpellTargetUnit()) != 'u001') then
            set u = GetTriggerUnit()
            call SimError(GetTriggerPlayer(),"Must target a skeleton.")
            call PauseUnit(u,true)
            call IssueImmediateOrder(u,"stop")
            call PauseUnit(u,false)
            set u = null
        endif
        return false
    endfunction

    private function Init takes nothing returns nothing
        local trigger t = CreateTrigger()
        call TriggerRegisterAnyUnitEventBJ(t,EVENT_PLAYER_UNIT_SPELL_CAST)
        call TriggerAddCondition(t,Condition(function Conditions))
    endfunction
endscope

02-26-2011, 11:07 AM

Linaze

Thanks a lot, pausing and unpausing works just fine.

As for optimizing, I did as you say, I have a few questions, however:

*You only inline things when it's used once?

*Do you need to null every local variable at a function's end?

*When merging actions and conditions, wouldn't it be sweeter to make it an Action function instead of a Condition one so you don't have to return something?

*As for global variables, you use them when you need them in two or more functions, and locals if you only need them in one, or what?

Thanks.

02-26-2011, 02:26 PM

Michael Peppers

Quote:

Originally Posted by Linaze

As for optimizing, I did as you say, I have a few questions, however:

*You only inline things when it's used once?

*Do you need to null every local variable at a function's end?

*When merging actions and conditions, wouldn't it be sweeter to make it an Action function instead of a Condition one so you don't have to return something?

*As for global variables, you use them when you need them in two or more functions, and locals if you only need them in one, or what?

It depends. Most of the cases, yes, since the most frequently inlined functions (like the DestroyEffect(AddSpecialEffect()) above) don't return any value, so variables can't be stored.
Always try to inline as much as possible, as the code will gain in speed.
Every one of them except for integer, real and... maybe also string? I don't remember.
Also, local group variables and a bunch of others (effect, lightning, sound and some i don't remember) should be destroyed before nulling them.
Yes, it would, but Conditions are faster and use less memory IIRC.
Generally, code into Conditions unless you need to use a Wait, since Conditions can't handle those.
More or less. You may want to use Structs + TimerUtils (or Hashtables and some weird *Voodoo*) instead of Globals in case you're dealing with Timers, though.
Ah, also making every Global and Function in your code private is generally a good idea.

People, correct me if I'm wrong and add more detailed information, as my vJass is rusty.

02-27-2011, 12:45 PM	#7
Linaze	Thanks, peppers. While we're on the subject of optimization, from what I understood, you'll always want to merge conditions and actions (when possible), is this correct?

02-27-2011, 02:29 PM

Michael Peppers

Quote:

Originally Posted by Linaze

Thanks, peppers.
While we're on the subject of optimization, from what I understood, you'll always want to merge conditions and actions (when possible), is this correct?

Correct it is.

For overkill optimization, when merging Conditions and Actions make the resulting Condition always return false.

More info in this thread.

Also, on what I said about Globals and Locals (point 4), the fact is that Globals can be overwritten if the trigger is meant to be used more than once (eg. spell triggers). That is nothing to worry about if said trigger is istantaneous, but if it has Waits (most probably a badly coded spell) or Timers (the right way to do it), then it becomes a problem and if not solved the spell will surely break every now and then. (or from the 2nd cast onwards, depends on how long the trigger lasts)

That's why the best way of dealing with spells that use Timers is storing values needed later into Structs or Hashtables... or even into Global arrays in some rare cases.

Locals don't suffer from this problem, as they're specific to every function call and won't be overwritten by another call of the same function.