[JASS] JASS spell

[Meatmuffin]

//Linked list recycling function

function BE_Recycle takes integer Node returns nothing
    if (udg_BE_LastNode == Node) then
        set udg_BE_LastNode = udg_BE_PrevNode[Node]
    endif
    set udg_BE_RecycleNodes[udg_BE_RecyclableNodes] = Node
    set udg_BE_RecyclableNodes = udg_BE_RecyclableNodes + 1
    set udg_BE_NextNode[udg_BE_PrevNode[Node]] = udg_BE_NextNode[Node]
    set udg_BE_PrevNode[udg_BE_NextNode[Node]] = udg_BE_PrevNode[Node]
    set udg_BE_AbilityCounter = udg_BE_AbilityCounter - 1
    if (udg_BE_AbilityCounter == 0) then
        call PauseTimer(udg_BE_Timer)
    endif
endfunction

//Below is the main function of the spell (loop), it pulls one closest enemy towards the enchanted object
//If the unit goes out of range, selects a new target and exceeds the duration

function BE_Loop takes nothing returns nothing
    local real x
    local real y
    local real z
    local real x2
    local real y2
    local real z2
    local real dx
    local real dy
    local integer Node = 0
    local integer TempInt = 0
    local real angle
    local unit u
    local real d
    local real aoe = 300
    loop
        set Node = udg_BE_NextNode[Node]
        exitwhen Node == 0
        if udg_BE_Duration[Node] > 0.00 then
            set udg_BE_Duration[Node] = udg_BE_Duration[Node] - 0.031250
            set x = GetUnitX(udg_BE_EnchantedObj[Node])
            set y = GetUnitY(udg_BE_EnchantedObj[Node])
            // Calculating the nearest unit
            if udg_BE_Check[Node] == 0 then
                call GroupEnumUnitsInRange(udg_BE_TempGroup, x, y, aoe, null)
                loop
                    set u = FirstOfGroup(udg_BE_TempGroup)
                    exitwhen u == null
                    set x2 = GetUnitX(u)
                    set y2 = GetUnitY(u)
                    set dx = x2 - x
                    set dy = y2 - y
                    set d = SquareRoot(dx*dx + dy*dy)
                    if d < aoe and u != udg_BE_EnchantedObj[Node] and GetUnitAbilityLevel(u, 'ACEV') == 0 and IsUnitAlly(u, udg_BE_Player[Node]) == false and not(IsUnitType(u, UNIT_TYPE_DEAD)) then
                        set udg_BE_Target[Node] = FirstOfGroup(udg_BE_TempGroup)
                        set aoe = d
                    endif
                    call GroupRemoveUnit(udg_BE_TempGroup, u)
                endloop
            endif
            // Setting up the global integer to keep the spell from checking for a new target
            if udg_BE_Check[Node] == 0 and udg_BE_Target[Node] != null then
                set udg_BE_Check[Node] = 1
                set udg_BE_Duration[Node] = 5.00
            endif
            // Pulling the enemy
            if udg_BE_Target[Node] != null then
                set z = GetUnitFlyHeight(udg_BE_EnchantedObj[Node]) + 50.00
                set x2 = GetUnitX(udg_BE_Target[Node])
                set y2 = GetUnitY(udg_BE_Target[Node])
                set z2 = GetUnitFlyHeight(udg_BE_Target[Node]) + 50.00
                call MoveLightningEx(udg_BE_Lightning[Node], false, x, y, z, x2, y2, z2)
                set angle = Atan2(y - y2, x - x2)
                if (GetUnitAbilityLevel(udg_BE_EnchantedObj[Node], 'A0AX') == 1) then
                    call SetUnitX(udg_BE_Target[Node], x2 + 9.375 * Cos(angle)) // 300 MS
                    call SetUnitY(udg_BE_Target[Node], y2 + 9.375 * Sin(angle)) // 300 MS 
                else
                    call SetUnitX(udg_BE_Target[Node], x2 + 6.25 * Cos(angle)) // 200 MS
                    call SetUnitY(udg_BE_Target[Node], y2 + 6.25 * Sin(angle)) // 200 MS
                endif
                set dx = x2 - x
                set dy = y2 - y
                set d = SquareRoot(dx*dx + dy*dy)
            endif
            // Checking if the distance has been exceeded
            if d > 500 and udg_BE_Target[Node] != null and udg_BE_Check[Node] == 1 then
                set udg_BE_Duration[Node] = 0.00
            endif
        // If the duration has ended, clean up
        else
            call DestroyLightning(udg_BE_Lightning[Node])
            call DestroyEffect(udg_BE_SpecialEf[Node])
            call BE_Recycle(Node)
        endif
    endloop
endfunction

// Linked list node creation

function BE_CreateNode takes nothing returns integer
    local integer Node = 0
    if (udg_BE_RecyclableNodes == 0) then
        set udg_BE_NodeNumber = udg_BE_NodeNumber + 1
        set Node = udg_BE_NodeNumber
    else
        set udg_BE_RecyclableNodes = udg_BE_RecyclableNodes - 1
        set Node = udg_BE_RecycleNodes[udg_BE_RecyclableNodes]
    endif
    set udg_BE_NextNode[Node] = 0
    set udg_BE_NextNode[udg_BE_LastNode] = Node
    set udg_BE_PrevNode[Node] = udg_BE_LastNode
    set udg_BE_LastNode = Node
    set udg_BE_AbilityCounter = udg_BE_AbilityCounter + 1
    if (udg_BE_AbilityCounter == 1) then
        call TimerStart(udg_BE_Timer, 0.031250, true, function BE_Loop)
    endif
    return Node
endfunction

// Function onCast, sets up the base for the ability

function BE_OnCast takes nothing returns boolean
    local integer Node
    local unit u
    local real x
    local real y
    local real z
    local integer i
    local integer i2
    local integer count = 0
    local integer random
    // Checking if the casted ability is right
    if (GetSpellAbilityId() == 'A0AV') then
        set Node = BE_CreateNode()
        set udg_BE_Player[Node] = GetTriggerPlayer()
        set udg_BE_Target[Node] = null
        set udg_BE_Check[Node] = 0
        set udg_BE_Duration[Node] = 6.00
        set x = GetSpellTargetX()
        set y = GetSpellTargetY()
        // Getting the random unit from the targeted AoE
        call GroupEnumUnitsInRange(udg_BE_TempGroup, x, y, 300, null)
        loop
            set u = FirstOfGroup(udg_BE_TempGroup)
            exitwhen u == null
            set i = GetUnitAbilityLevel(u, 'A0AX')
            set i2 = GetUnitAbilityLevel(u, 'ACEV')
            if (not IsUnitType(u, UNIT_TYPE_DEAD)) and (not IsUnitType(u, UNIT_TYPE_STRUCTURE)) and (i2 == 0 or i == 1) then
                set count = count + 1
                call GroupAddUnit(udg_BE_Swap, u)
            endif
            call GroupRemoveUnit(udg_BE_TempGroup, u)
        endloop
        set random = GetRandomInt(0, count)
        set count = 0
        loop
            set u = FirstOfGroup(udg_BE_Swap)
            exitwhen u == null
            set count = count + 1
            if count == random then
                set udg_BE_Unit = u
            endif
            call GroupRemoveUnit(udg_BE_Swap, u)
        endloop
        // Adding the lightning and the special "brilliance aura" effect to the target
        set udg_BE_EnchantedObj[Node] = udg_BE_Unit
        set udg_BE_SpecialEf[Node] = AddSpecialEffectTarget("Abilities\\Spells\\Human\\Brilliance\\Brilliance.mdl", udg_BE_EnchantedObj[Node], "origin")
        set z = GetUnitFlyHeight(udg_BE_EnchantedObj[Node]) + 50.00
        set udg_BE_Lightning[Node] = AddLightningEx("CHIM", false, x, y, z, x, y, z)
    endif
    return false
endfunction

// Initializing the trigger

function InitTrig_Binding_Enchant takes nothing returns nothing
    local trigger BE = CreateTrigger()
    call TriggerRegisterAnyUnitEventBJ(BE, EVENT_PLAYER_UNIT_SPELL_EFFECT)
    call TriggerAddCondition(BE, Condition(function BE_OnCast))
endfunction

The spell is called Binding Enchant and here is its tooltip:

Tooltip

The Shaman enchants one random unit in the targeted AoE. Should another unit come close to the enchanted object, it will get pulled and snagged and pulled towards it. Duration is reset once the target has been found. The leash can be broken if distance becomes greater than 500.

However its indexing/deindexing with the linked list is somehow incorrect. I deleted all the actions at one point, leaving only the linked list + timer structure and the loop only fired once.

I would appreciate it if someone would review this spell and tell me what am I doing wrong.

Already a huge thanks to KILLCIDE for improving the coding in this spell, however, we did not completely debunk what's wrong.

 

[KILLCIDE]

• exitwhen Node > udg_BE_AbilityCounter -> exitwhen Node == 0
  
  

• local integer i = 0
  
  loop
  
      if i == 0 and udg_BE_Target[Node] != null then
          set i = 1
          set udg_BE_Duration[Node] = 5.00
      endif
  
  endloop

  ^ integer i will always be 0 at the start of a new periodic loop, meaning that BE_Duration[Node] will always reset to 5 seconds. A fix for this would be using a global variable instead of relying on a local
  
  

• if (GetSpellAbilityId() == 'A0AV') then
          // stuff
      set Node = BE_CreateNode()
      if (udg_BE_AbilityCounter == 1) then
          call TimerStart()
      endif
      endif // <-- move this over to match its corresponding if statement

• Try to keep some consistency with your code. You pause the timer inside the deindex function, but you start the timer outside of the index function?
• You don't really need to use BE_LastNode, but I guess it's just preference

 

[Meatmuffin]

EDIT: Nvm, updating.
If I try to start the timer inside the indexing, it returns an error "Undefined function BE_Loop"

EDIT2: It works now, thanks.
However I'm still left with the two problems I had at the very start:
1. Sometimes the spell just doesn't work -- doesn't do anything (don't really know why)
2. The BE_SpecialEf[Node] variable doesn't get destroyed

 

[IcemanBo]

The whole stuff connecting with local integer i seems strange and not clean.
I strongly think something is wrong or redundant with it.
I'm not sure why it starts directly with 0, so the first condition is always met,
and it probably should be assigned new for each iteration of the loop.

local real dis initialized in an other scope where it's checked.
It might be even un-initialized when it comes to the condition check which will lead to thread crash.

KILLCIDE also mentioned something very important.

We need to see the changes in code.

Also, the loop function must be above, so it can be called from indexing function.

 

[Meatmuffin]

Updated code.
EDIT: Apparently there was something wrong with the Brilliance Aura effect, it doesn't get destroyed. Changed it. But the fact that it sometimes doesn't do anything stays.

--

Solved.

 

[Anitarf]

I looked through the code quickly to see if I could find an obvious error but no such luck, since I don't have more time I'll just recommend vJass and this module, although if it's not the (de)allocator that's causing the problem then it won't help.

What do you mean it sometimes doesn't do anything? You have several steps in your spell code, try at least figuring out at which step it fails. Does the loop even start?


Some unrelated things I noticed:

Not sure why you need udg_BE_LastNode, you could just use the "prev" value of node 0 like you use the "next" value of node 0 to link to the first node. Similarly, you don't really need separate variables for the recycle list, you could just reuse the regular list's "next" (or rather, you'd need to reuse "prev", since you recycle instances mid-loop and so you still need the "next" value tor remain intact). Otherwise, the allocator seems correct, although it doesn't provide features such as double free protection like vJass struct allocators do.

You could use the variable u in your secondary target search loop instead of calling FirstOfGroup twice.

 

[Meatmuffin]

1. I've just recently started coding in JASS and I haven't yet pushed myself as far as to learn vJASS structures, I don't know if I ever will, but thanks for the recommendation.

2. I didn't try debugging because ever since I changed the special effect from Brilliance Aura at "origin" to Far Seer missile at "chest" it seemed to always work. I'll debug the functions step by step if that happens again.

2. KILLCIDE already explained to me that LastNode is redundant, but to me it makes it easier to read and edit. I still don't fully understand Linked List structure, but I'm getting the hang of it. Once again, I don't know vJASS yet.

3. Seems logical, will do.

 

[Anitarf]

I agree, it helps with readability to have a separate LastNode variable, however, it would equally help to have a separate FirstNode variable. I'm not complaining that you have one, I just find it odd that you have one but not the other.

 

[KILLCIDE]

I've just recently started coding in JASS and I haven't yet pushed myself as far as to learn vJASS structures...

I don't know vJASS yet.

Linked listing isn't a "vJASS structure". Matter of fact, I use it all the time in my GUI spells xD

 

[Rheiko]

Linked List is just another version of Dynamic Indexing except the order is better. I'm learning it recently.

 

[Aniki]

Spells don't really need node recycling nor "linked list indexing":

globals
    integer BE_node = 0
    integer array BE_instances
    integer BE_instances_count = 0
endglobals

function BE_remove_node_by_index takes integer node_index returns nothing
    set BE_instances[node_index] = BE_instances[BE_instances_count]
    set BE_instances_count = BE_instances_count - 1
    if BE_instances_count <= 0 then
        call PauseTimer(BE_Timer)
    endif
endfunction

function BE_loop takes nothing returns nothing
    local integer i
    local integer node

    set i = 1
    loop
        exitwhen i > BE_instances_count
        set node = BE_instances[i]

        if ... then
            call BE_remove_node_by_index(i)

            // could even inline BE_remove_node_by_index:
            // set BE_instances[i] = BE_instances[BE_instances_count]
            // set BE_instances_count = BE_instances_count - 1
            // if BE_instances_count <= 0 then
            //     call PauseTimer(BE_Timer)
            // endif

            // we replaced BE_instances[i] with the last be-instance (BE_instances[BE_instances_count])
            // if we don't decrement i here, we'll skip the last be-instance
            set i = i - 1
        else
            // do something with node
        endif

        set i = i + 1
    endloop
endfunction

function BE_new_node takes nothing returns integer
    set BE_node = BE_node + 1
    if BE_node >= 8191 then
        // if we access any array with index 8191 we won't be able to load a save-game for the map
        set BE_node = 1
    endif

    set BE_instances_count = BE_instances_count + 1
    set BE_instances[BE_instances_count] = BE_node

    if BE_instances_count == 1 then
        call TimerStart(BE_Timer, 0.031250, true, function BE_loop)
    endif

    return BE_node
endfunction

node recycling gives you "double-free protection" (which you are not using), but "double-freeing" it's not a likely mistake to be made in a spell (because BE_remove_node_by_index / or equivalent is called only at a few locations in the script, in your spell just once)

"linked list indexing" gives you dynamism (you can add as many nodes to the list as you want (<= 8190)), and it preserves the order in which the nodes were added when some nodes get removed from the list

The "order preservation" property of linked-lists is not needed for spells because it doesn't matter in which order we loop/update the spell instances as long as we don't skip any.


With that said, I would highly recommend that you learn about vjass and it's struct feature, which will improve the readability of your scripts a lot.

PS: the "udg_" prefixes are so annoying =)

 

[Meatmuffin]

@KILLCIDE

You understood me wrong, I was replying to Anitarf, who suggested me using some vJASS features, I wasn't associating linked listing with vJASS.