[JASS] First attempt, feedback

[nedio95]

Hello hivers

I took a request about making a spell and started it in GUI... somehow it turned into my first ever attempt at JASS...

If interested in the details

Spell Request
Fail #1
Fail #2

So I am done with teh spell but before I give it to the requester (is that a word ?) I would like to get some criticizm from JASS experienced people so... here is teh spell and atached you will find a test map.
Some parts of the spell are converted GUI so I do remember reading about some BJ which are better to be avoided and is better to use the native... and yea...

Code:

function ManaVamp takes integer MLevel returns nothing
   local real Percentage
   local real ReturnMana
   local real TargetMana


   set Percentage = ( udg_MVA_Base + ( udg_MVA_PerLevel * I2R(MLevel) ) )
   set ReturnMana = ( Percentage * udg_PDD_amount )
   set TargetMana = GetUnitStateSwap(UNIT_STATE_MANA, udg_PDD_target)
   
   if ( TargetMana < ReturnMana ) then
       set ReturnMana = TargetMana
   endif
   call SetUnitManaBJ( udg_PDD_source, ((GetUnitStateSwap(UNIT_STATE_MANA, udg_PDD_source)) + ReturnMana) )
   call SetUnitManaBJ( udg_PDD_target, ((GetUnitStateSwap(UNIT_STATE_MANA, udg_PDD_target)) - ReturnMana) )
   
endfunction

function CheckSource takes nothing returns boolean
    if ( GetUnitAbilityLevelSwapped(udg_MVA_Ability, GetFilterUnit()) > 0 ) then
        if (  IsUnitAlly(GetFilterUnit(), GetOwningPlayer(udg_PDD_source)) == true ) then
            return true
        endif
    else
        return false
    endif
endfunction

function CheckForAbility takes nothing returns boolean
    return ( GetUnitAbilityLevelSwapped(udg_MVA_Ability, GetFilterUnit()) > 0 )
endfunction

function GetMaxLevel takes nothing returns integer
    local integer array LevelMax
    local integer LoopInt
    local unit AuraSource
    local group SourceGroup
    local location TempPoint
    local integer NumUnits
    local integer MaxLevel

    set TempPoint = GetUnitLoc(udg_PDD_source)
    set SourceGroup = GetUnitsInRangeOfLocMatching(udg_MVA_Range, TempPoint, Condition(function CheckSource))
    call RemoveLocation(TempPoint)
    set NumUnits = CountUnitsInGroup(SourceGroup)
    call DisplayTextToForce( GetPlayersAll(), I2S(NumUnits) )
    if ((NumUnits > 0)==true)then
    set LoopInt = 0
        loop 
        exitwhen LoopInt > NumUnits
            set AuraSource = GroupPickRandomUnit(SourceGroup)
            set LevelMax[LoopInt] = GetUnitAbilityLevelSwapped(udg_MVA_Ability, AuraSource)
            call GroupRemoveUnitSimple( AuraSource, udg_TempUnitGroup )
            if (LoopInt == 0) then
                set MaxLevel = LevelMax[LoopInt]
            else
                set MaxLevel = IMaxBJ(MaxLevel,LevelMax[LoopInt])
            endif
            set LoopInt = LoopInt + 1
        endloop
    endif
    call DestroyGroup(SourceGroup)
    return MaxLevel
endfunction

function Trig_SVADetection_Func001C takes nothing returns boolean
    if ( udg_PDD_damageType != udg_PDD_PHYSICAL  ) then
        return false
    endif
    if ( UnitHasBuffBJ(udg_PDD_source, udg_MVA_Buff) != true  ) then
        return false
    endif
    return true
endfunction

function Trig_SVADetection_Actions takes nothing returns nothing
    if ( Trig_SVADetection_Func001C() ) then
        set udg_TempInt = GetMaxLevel()
        call ManaVamp(udg_TempInt)
    else
    endif
endfunction

//===========================================================================
function InitTrig_SVAActivation takes nothing returns nothing
    set gg_trg_SVAActivation = CreateTrigger(  )
    call TriggerRegisterVariableEvent( gg_trg_SVAActivation, "udg_PDD_damageEventTrigger", EQUAL, 1.00 )
    call TriggerAddAction( gg_trg_SVAActivation, function Trig_SVADetection_Actions )
endfunction

 

[Daffa]

Location might be replaced with XY
What's with this line? call DisplayTextToForce( GetPlayersAll(), I2S(NumUnits) )
BJ.... Mind if replace them with the native ones? BJ kinda do unnecessary actions, exceptions exist though (but I don't remember which)
Use TriggerAddCondition instead of TriggerAddAction
Rename the Func001 please
Make sure to set all locals to null after they're used, local leaks if not Nulled.

That's what I can remember
I'm not pretty used to writing Jass but at least I can spare my knowledge to help you

Hope these helps!

 

[nedio95]

What's with this line? call DisplayTextToForce( GetPlayersAll(), I2S(NumUnits) )

Yeeeea,this is a debug message which I forgot to delete...

BJ.... Mind if replace them with the native ones? BJ kinda do unnecessary actions, exceptions exist though (but I don't remember which)

I do know of that, one of the safe ones is IMaxBJ. Any API around ? I did search for one but...

Use TriggerAddCondition instead of TriggerAddAction

Uggh... why ? What ?

Edit// Aaah, I see where that come from, originaly I started by converting a GUI conditionless trigger, and all teh conditions were in a If-the-else action. No ?

 

[Daffa]

I think by default there are several Blizzard natives that are similar with the BJ (and do the exact same function at less operation cost), do you use JNGP?
TriggerAddCondition is faster than the action one (or so I have been told).

 

[nedio95]

Nah, standard editor. I do remember that in JNGP you can "browse" the different functions and see them in a list but I do not have it here and I actually do not want to install it...

 

[Daffa]

I doubt if I'm correct, but these might be the replacements :
SetUnitManaBJ -> SetUnitMana
GetUnitAbilityLevelSwapped -> GetUnitAbilityLevel
UnitHasBuffBJ -> UnitHasBuff

 

[Gismo359]

UnitHasBuffBJ -> GetUnitAbilityLevel(whichUnit, buffcode) > 0

SetUnitManaBJ -> SetUnitState(whichUnit, UNIT_STATE_MANA, RMaxBJ(0,newValue))

GetUnitStateSwap(UNIT_STATE_MANA, udg_PDD_target) -> GetUnitState(udg_PDD_target, UNIT_STATE_MANA)

GetUnitAbilityLevelSwapped(udg_MVA_Ability, GetFilterUnit()) -> GetUnitAbilityLevel(GetFilterUnit(), udg_MVA_Ability)

Thats some, you can browse the Blizzard.j to see what functions convert to. Keep in mind that some of those may have more efficient alternatives while some just arent worth redoing (like ReplaceUnitBJ or the default map initialization BJs - they aint worth the hassle).

You can use DisplayTextToPlayer(GetLocalPlayer(), 0, 0, YourStringVariableHere) to display text to all players (i did read it was only debug).
For debuging BJDebugMsg(text) is more convenient.

 

[Wietlol]

When you are writing JASS after you converted GUI functions to custom text, try ctrl+click on the red functions (BJs) and you see what they do.
You will see something like this:

(Don't mind my own color-styles.)

You see that SetUnitManaBJ() actually redirects the function to SetUnitState().
In every coding language (that I know), calling functions consumes runtime.
You can imagine that this is total bullsit:

function f8 takes nothing returns boolean
    return false
endfunction
function f7 takes nothing returns boolean
    return f8()
endfunction
function f6 takes nothing returns boolean
    return f7()
endfunction
function f5 takes nothing returns boolean
    return f6()
endfunction
function f4 takes nothing returns boolean
    return f5()
endfunction
function f3 takes nothing returns boolean
    return f4()
endfunction
function f2 takes nothing returns boolean
    return f3()
endfunction
function f1 takes nothing returns boolean
    return f2()
endfunction

So you should really try to not use function redirects like BJs.
So instead of using SetUnitManaBJ() you copy and paste the actions in that function list to your trigger.
Then you see that there is a second BJ called RMaxBJ(). When you open that one you will see that it is actually stupid in some cases. In this case you cant set a units mana to a negative value... I never tested it but for me it can be removed.

When you see functions like:

function TriggerRegisterPlayerKeyEventBJ takes trigger trig, player whichPlayer, integer keType, integer keKey returns event
    if (keType == bj_KEYEVENTTYPE_DEPRESS) then
        // Depress event - find out what key
        if (keKey == bj_KEYEVENTKEY_LEFT) then
            return TriggerRegisterPlayerEvent(trig, whichPlayer, EVENT_PLAYER_ARROW_LEFT_DOWN)
        elseif (keKey == bj_KEYEVENTKEY_RIGHT) then
            return TriggerRegisterPlayerEvent(trig, whichPlayer, EVENT_PLAYER_ARROW_RIGHT_DOWN)
        elseif (keKey == bj_KEYEVENTKEY_DOWN) then
            return TriggerRegisterPlayerEvent(trig, whichPlayer, EVENT_PLAYER_ARROW_DOWN_DOWN)
        elseif (keKey == bj_KEYEVENTKEY_UP) then
            return TriggerRegisterPlayerEvent(trig, whichPlayer, EVENT_PLAYER_ARROW_UP_DOWN)
        else
            // Unrecognized key - ignore the request and return failure.
            return null
        endif
    elseif (keType == bj_KEYEVENTTYPE_RELEASE) then
        // Release event - find out what key
        if (keKey == bj_KEYEVENTKEY_LEFT) then
            return TriggerRegisterPlayerEvent(trig, whichPlayer, EVENT_PLAYER_ARROW_LEFT_UP)
        elseif (keKey == bj_KEYEVENTKEY_RIGHT) then
            return TriggerRegisterPlayerEvent(trig, whichPlayer, EVENT_PLAYER_ARROW_RIGHT_UP)
        elseif (keKey == bj_KEYEVENTKEY_DOWN) then
            return TriggerRegisterPlayerEvent(trig, whichPlayer, EVENT_PLAYER_ARROW_DOWN_UP)
        elseif (keKey == bj_KEYEVENTKEY_UP) then
            return TriggerRegisterPlayerEvent(trig, whichPlayer, EVENT_PLAYER_ARROW_UP_UP)
        else
            // Unrecognized key - ignore the request and return failure.
            return null
        endif
    else
        // Unrecognized type - ignore the request and return failure.
        return null
    endif
endfunction

You might want to ask yourself if you want to change it.
(In this case you just have to find your key but other BJ functions might not be so simple and still that long.)
If you only use it once then you can leave it that way.
If you use it for every ... time of gametime then you should really remove BJ functions to improve performance.

 

[Daffa]

@Wietlol, you're reminding me of recursion
@Gismo, thanks for the correction

 

[Wietlol]

@Wietlol, you're reminding me of recursion

????

 

[edo494]

while it is true that in most languages nonoptimized function call adds runtime overhead, in Jass it is especially deadly, because the jump table does not consist of static value, but the Interpreter has to do some kind of search for the function and only then jump to that position, and keep interpreting. So longer function names == longer search time, which is laughable, but we cant really change this.

SetWidgetLife(unit, val) > SetUnitState(unit, UNIT_STATE_LIFE, val)

 

[Daffa]

@Wietlol, http://en.wikipedia.org/wiki/Recursion
Though I know the trigger you made is not recursive

 

[nedio95]

Thank you all for the feedback, today I had some time to implement the suggested improvements and unfortunately stumbled upon a few problems...

Fails:

When I try to do:

if ( UnitHasBuffBJ(udg_PDD_source, udg_MVA_Buff) != true  ) then
return false

>>> >>> >>> >>>

if ( GetUnitAbilityLevel(udg_PDD_source, udg_MVA_Buff) == 0  ) then
return false

or

if ( GetUnitAbilityLevel(udg_PDD_source, udg_MVA_Buff) > 0  ) then
return false

or

if ( GetUnitAbilityLevel(udg_PDD_source, udg_MVA_Buff) < 1  ) then
return false

The editor crashes... no error, directly crashes.
Same when I try:

TriggerAddAction

>>>

TriggerAddCondition

I suspect the second one is not to be done...

Current code

//UDGs: MVA_Range,MVA_Buff, MVA_Ability, MVA_Base, MVA_PerLevel

function ManaVamp takes integer MLevel returns nothing
   local real Percentage
   local real ReturnMana
   local real TargetMana


   set Percentage = ( udg_MVA_Base + ( udg_MVA_PerLevel * I2R(MLevel) ) )
   set ReturnMana = ( Percentage * udg_PDD_amount )
   set TargetMana = GetUnitState( udg_PDD_target,UNIT_STATE_MANA)
   
   if ( TargetMana < ReturnMana ) then
       set ReturnMana = TargetMana
   endif
   call SetUnitState(udg_PDD_source, UNIT_STATE_MANA, ((GetUnitState(udg_PDD_source, UNIT_STATE_MANA)) + ReturnMana))
   call SetUnitState(udg_PDD_target, UNIT_STATE_MANA, ((GetUnitState(udg_PDD_target, UNIT_STATE_MANA)) - ReturnMana))
endfunction

function CheckSource takes nothing returns boolean
    if ( GetUnitAbilityLevel(GetFilterUnit(),udg_MVA_Ability) > 0 ) then
        if (  IsUnitAlly(GetFilterUnit(), GetOwningPlayer(udg_PDD_source)) == true ) then
            return true
        endif
    else
        return false
    endif
endfunction

function CheckForAbility takes nothing returns boolean
    return ( GetUnitAbilityLevel(GetFilterUnit(), udg_MVA_Ability) > 0 )
endfunction

function GetMaxLevel takes nothing returns integer
    local integer array LevelMax
    local integer LoopInt
    local unit AuraSource
    local group SourceGroup
    local location TempPoint
    local integer NumUnits
    local integer MaxLevel

    set TempPoint = GetUnitLoc(udg_PDD_source)
    set SourceGroup = GetUnitsInRangeOfLocMatching(udg_MVA_Range, TempPoint, Condition(function CheckSource))
    call RemoveLocation(TempPoint)
    set NumUnits = CountUnitsInGroup(SourceGroup)
    call DisplayTextToForce( GetPlayersAll(), I2S(NumUnits) )
    if ((NumUnits > 0)==true)then
    set LoopInt = 0
        loop 
        exitwhen LoopInt > NumUnits
            set AuraSource = GroupPickRandomUnit(SourceGroup)
            set LevelMax[LoopInt] = GetUnitAbilityLevel(AuraSource, udg_MVA_Ability)
            call GroupRemoveUnitSimple( AuraSource, udg_TempUnitGroup )
            if (LoopInt == 0) then
                set MaxLevel = LevelMax[LoopInt]
            else
                set MaxLevel = IMaxBJ(MaxLevel,LevelMax[LoopInt])
            endif
            set LoopInt = LoopInt + 1
        endloop
    endif
    call DestroyGroup(SourceGroup)
    set SourceGroup = null
    set TempPoint = null
    set AuraSource = null
    return MaxLevel
endfunction

function Trig_SVADetection_Func001C takes nothing returns boolean
    if ( udg_PDD_damageType != udg_PDD_PHYSICAL  ) then
        return false
    endif
    if ( UnitHasBuffBJ(udg_PDD_source, udg_MVA_Buff) != true  ) then
        return false
    endif
    return true
endfunction

function Trig_SVADetection_Actions takes nothing returns nothing
    if ( Trig_SVADetection_Func001C() ) then
        set udg_TempInt = GetMaxLevel()
        call ManaVamp(udg_TempInt)
    else
    endif
endfunction

//===========================================================================
function InitTrig_MVAActivation takes nothing returns nothing
    set gg_trg_MVAActivation = CreateTrigger(  )
    call TriggerRegisterVariableEvent( gg_trg_MVAActivation, "udg_PDD_damageEventTrigger", EQUAL, 1.00 )
    call TriggerAddAction( gg_trg_MVAActivation, function Trig_SVADetection_Actions )
endfunction

Another thing: reals/integers can not be nulled ?

 

[edo494]

function CheckSource takes nothing returns boolean
    if ( GetUnitAbilityLevel(GetFilterUnit(),udg_MVA_Ability) > 0 ) then
        if (  IsUnitAlly(GetFilterUnit(), GetOwningPlayer(udg_PDD_source)) == true ) then
            return true
        endif
    else
        return false
    endif
endfunction

->

function CheckSource takes nothing returns boolean
    return GetUnitAbilityLevel(GetFilterUnit(),udg_MVA_Ability) > 0 and IsUnitAlly(GetFilterUnit(), GetOwningPlayer(udg_PDD_source))
endfunction

 

[Gismo359]

call TriggerAddAction( gg_trg_MVAActivation, function Trig_SVADetection_Actions )
->
call TriggerAddCondition(gg_trg_MVAActivation, /*Condition(*/function Trig_SVADetection_Actions/*)*/)
The code I comented out isnt required if you use JNGP.

And then:

function Trig_SVADetection_Actions takes nothing returns nothing
    // Etc.
endfunction

->

function Trig_SVADetection_Actions takes nothing returns boolean
    // Etc.
    return false
endfunction

function Trig_SVADetection_Func001C takes nothing returns boolean
    if ( udg_PDD_damageType != udg_PDD_PHYSICAL  ) then
        return false
    endif
    if ( UnitHasBuffBJ(udg_PDD_source, udg_MVA_Buff) != true  ) then
        return false
    endif
    return true
endfunction

->

function Trig_SVADetection_Func001C takes nothing returns boolean
    return udg_PDD_damageType == udg_PDD_PHYSICAL and GetUnitAbilityLevel(udg_PDD_source, udg_MVA_Buff) <= 0
endfunction

You should also try to use as few function calls as possible. For example, you can just put the GetMaxLevel() and ManaVamp() inside the condition function.

E: No, you cant null integers and reals. Setting them to 0 doesnt make a difference either.