JASS Script causing a crash

[CryptLord1234]

Alright, I'm gonna preface this with a "I'm really rusty at JASS at the moment". Anyway, what I'm trying to do with this trigger is check if the target of the spell ("target") is an enemy; if it is, cast a Storm Bolt-like ability at it. If it isn't, cast a Bloodlust-like ability on the target. It always gets up to the point of my unit casting, then wc3 crashes to desktop. WE and JASSCraft say I don't have any kinda syntax error. Help, please?

function PiousStrike takes nothing returns nothing
local unit caster
local unit target
local unit temp
local player CastOwner
local player TargOwner
set caster = GetSpellAbilityUnit()
set target = GetSpellTargetUnit()
set CastOwner = GetOwningPlayer(caster)
set TargOwner = GetOwningPlayer(target)
if (IsPlayerEnemy(CastOwner, TargOwner)) then
    set temp = CreateUnitAtLoc (CastOwner, 'h000', GetUnitLoc(target), 0.00)
    call SetUnitAbilityLevelSwapped ('A005', temp, GetUnitAbilityLevelSwapped('A004', caster))
    call IssueTargetOrderBJ(temp, "thunderbolt", target)
    call RemoveUnit(temp)
else
    set temp = CreateUnitAtLoc (CastOwner, 'h000', GetUnitLoc(target), 0.00)
    call SetUnitAbilityLevelSwapped ('Ablo', temp, GetUnitAbilityLevelSwapped('A004', caster))
    call IssueTargetOrderBJ(temp, "bloodlust", target)
    call RemoveUnit(temp)
endif
set caster = null
set target = null
set temp = null
set CastOwner = null
set TargOwner = null
endfunction

Thanks in advance. Also, if there's an easier way, either GUI or JASS, If I could see that, it'd be awesome.

 

[Marcelo Hossomi]

A few advices:

You can change GetSpellAbilityUnit to GetTriggerUnit. That's faster (I read that somewhere)

You could use IsUnitEnemy( unit, player ) to check if target is an enemy of CastOwner. That should be faster.

You leak a location at the lines:
set temp = CreateUnitAtLoc (CastOwner, 'h000', GetUnitLoc(target), 0.00)

Create a location variable to store GetUnitLoc(target), so you can remove it later.

If you're going to create the dummy anyway, put it outside the if/then/else and save one line. Same for removing it.

Change GetUnitAbilityLevelSwapped( abil, unit ) to GetUnitAbilityLevel( unit, abil ). Same for setting the level (the level is after the unit parameter).

Change IssueTargetOrderBJ to IssueTargetOrder. The parameters are exactly the same.

Lastly, removing the unit right after ordering it to cast the spell will remove it BEFORE it does so. Use UnitApplyTimedLife( unit, 'BTLF', time ) to remove it after time seconds ('BTLF' is the buff "Timed Life". But as no one will see it, nvm ).

But I still don't see reasons for crashing. Maybe one of the players are getting to null (because there's no GetSpellAbilityUnit or GetSpellTargetUnit). What event are you using?

 

[Freiddie]

Here's a cleaned-up version that does exactly the same thing. I doubt this would solve your problem, however. Here's an idea for debugging: try commenting out some of the lines to see if they can prevent crashes. This way you'll know which one is the culprit of those crashes. I can't test your code because I don't have those abilities. I suggest you temporarily comment out the SetUnitAbilityLevel() line (I know this ruins the spell, but for testing purposes, disable it to see if crashes can be avoided) for debugging purposes.

Summary:
- Changed non-native functions to natives.
- Moved some statements outside the if-condition.
- Moved some set statements into the local statements.
- Used a timed life instead of removing the unit immediately (since I don't think the removing the unit immediately after creating it gives it sufficient time to cast the spell).
- Also fixed a location leak by units coordinates instead of location.

function PiousStrike takes nothing returns nothing
    local unit caster = GetSpellAbilityUnit()
    local unit target = GetSpellTargetUnit()
    local unit temp
    local player CastOwner = GetOwningPlayer(caster)
    local player TargOwner = GetOwningPlayer(target)
    
    set temp = CreateUnit(CastOwner, 'h000', GetUnitX(target), GetUnitY(target), 0.)
    if IsPlayerEnemy(CastOwner, TargOwner) then
        call SetUnitAbilityLevel(temp, 'A005', GetUnitAbilityLevel(caster, 'A004'))
        call IssueTargetOrder(temp, "thunderbolt", target)
        
    else
        call SetUnitAbilityLevel(temp, 'Ablo', GetUnitAbilityLevel(caster, 'A004'))
        call IssueTargetOrder(temp, "bloodlust", target)
    endif
    
    call UnitApplyTimedLife(temp, 0, 3.)
    
    set caster = null
    set target = null
    set temp = null
    set CastOwner = null
    set TargOwner = null
endfunction

 

[Troll-Brain]

Warcraft is simply close without any error message ?
If it is, then you call a function which call itself.

Like a trigger with an unit damage event and in you trigger action you damage the unit.

 

[CryptLord1234]

First off, thanks for cleaning up my code, Freddie. It's been awhile. I tried commenting out the SetUnitAbilityLevel line, and that wasn't what was messing up my code. I actually did figure it out, although the problem wasn't in the script I posted here (Although I did't think about the memory leak).

Turns out, I was adding a condition to the trigger. Yeah, that's good, since I wanted it to only go off when the right spell is cast. What I wanted for the condition is IF the right spell is cast, not (which I accidentally entered), the script I just gave you. >_<

Anyway. Thanks for everyone who replied.

 

[Troll-Brain]

The problem seems to not be in your function but in your trigger where you use it.
Post your trigger and then we will see why it's recursive.

 

[CryptLord1234]

I figured out why it was entering an infite loop; I was using the trigger as a condition for the trigger. I wrote the AddTriggerCondition on the initial part to add the trigger as a condition, not the condition function where I checked to see if it was the right ability.

Hopefully I said that right.

 

[Troll-Brain]

Btw it's not very safe to remove the dummy unit immediately after order it to cast a spell, add an expiration life will be better.