[vJASS] How to make this cleaner and more efficient?

[xyzier_24]

Hi, I was wondering if anyone can help me make this cleaner and more efficient? The intended effect is to make an affected unit to keep moving towards a random point in the area.

scope MentalDisarray

    globals
        private constant integer ABILITY_ID = 'A8U7'
        private constant integer BUFF_ID = 'B8U7'
    endglobals

    private struct Data
        unit e
        real x
        real y
        real nx
        real ny
        timer cycle
        static integer index = -1
        static thistype array data

        method destroy takes nothing returns nothing
            call ReleaseTimer(this.cycle)
            set this.e = null
            set this.cycle = null
            call this.deallocate()
        endmethod

        private static method runTimer takes nothing returns nothing
            local Data this = GetTimerData(GetExpiredTimer())
            local real ex = GetUnitX(this.e)
            local real ey = GetUnitY(this.e)
            local real angle = bj_RADTODEG * Atan2(this.ny - ey, this.nx - ex) + GetRandomReal(-60, 60)
            local real distance = SquareRoot((this.nx - ex) * (this.nx - ex) + (this.ny - ey) * (this.ny - ey))
            if GetUnitMoveSpeed(this.e) > 0 then 
                if distance <= 32 or not udg_UnitMoving[GetUnitUserData(this.e)] then
                    set this.nx = this.x + 64 * Cos(angle * bj_DEGTORAD)
                    set this.ny = this.y + 64 * Sin(angle * bj_DEGTORAD)
                    call IssuePointOrder(this.e, "move", this.nx, this.ny)
                endif
            elseif GetUnitCurrentOrder(this.e) != 851972 then
                call IssueImmediateOrder(this.e, "stop")
            endif
            if GetUnitAbilityLevel(this.e, BUFF_ID) == 0 or GetWidgetLife(this.e) <= .405 then
                call ReleaseTimer(GetExpiredTimer())
                call this.destroy()
            endif
        endmethod

        static method resetTimer takes unit e returns nothing
            local integer i = 0
            local Data this

            loop
            exitwhen i > index
                set this = data[i]

                if e == this.e then
                    call this.destroy()
                    set data[i] = data[index]
                    set index = index - 1

                    return
                endif
                set i = i + 1
            endloop
        endmethod

        static method runEvent takes nothing returns nothing
            local thistype this
            call thistype.resetTimer(GetSpellTargetUnit())
            set this = Data.allocate()
            set this.e = GetSpellTargetUnit()
            set this.x = GetUnitX(this.e)
            set this.y = GetUnitY(this.e)
            set this.nx = this.x + 64 * Cos(GetRandomReal(0, 360) * bj_DEGTORAD)
            set this.ny = this.y + 64 * Sin(GetRandomReal(0, 360) * bj_DEGTORAD)
            set index = index + 1
            set data[index] = this
            set this.cycle = NewTimerEx(this)
            call TimerStart(this.cycle, .1, true, function thistype.runTimer)

            if GetUnitMoveSpeed(this.e) > 0 then 
                call IssuePointOrder(this.e, "move", this.nx, this.ny)
            else
                call IssueImmediateOrder(this.e, "stop")
            endif
        endmethod

        static method resumeEvent takes nothing returns nothing
            local unit u = GetTriggerUnit()
            local integer i = 0
            local Data this

            loop
            exitwhen i > index
                set this = data[i]

                if u == this.e then
                    if GetUnitMoveSpeed(this.e) > 0 then
                        if GetOrderPointX() != this.nx or GetOrderPointY() != this.ny then
                            call IssuePointOrder(this.e, "move", this.nx, this.ny)
                        endif
                    elseif GetIssuedOrderId() != 851972 then
                        call BlzUnitInterruptAttack(this.e)
                        call IssueImmediateOrder(this.e, "stop")
                    endif
                endif
                set i = i + 1
            endloop
            set u = null
        endmethod

        static method onInit takes nothing returns nothing
            call RegisterSpellEffectEvent(ABILITY_ID, function thistype.runEvent)
            call RegisterPlayerUnitEvent(EVENT_PLAYER_UNIT_ISSUED_ORDER, function thistype.resumeEvent)
            call RegisterPlayerUnitEvent(EVENT_PLAYER_UNIT_ISSUED_POINT_ORDER, function thistype.resumeEvent)
            call RegisterPlayerUnitEvent(EVENT_PLAYER_UNIT_ISSUED_TARGET_ORDER, function thistype.resumeEvent)
            call RegisterPlayerUnitEvent(EVENT_PLAYER_UNIT_ISSUED_UNIT_ORDER, function thistype.resumeEvent)
            set index = -1
        endmethod
    endstruct

endscope

 

[Pyrogasm]

• Don't use SquareRoot to compute distance when you can, and instead compare >= 32^2 (1024). Additionally don't compute angle and distance until inside the if distance <= 32 block since they're not needed otherwise.
  
  
• Things like this can just be reduced: GetRandomReal(0, 360) * bj_DEGTORAD becomes GetRandomReal(0, 2*bj_PI)
  Or this one local real angle = bj_RADTODEG * Atan2(this.ny - ey, this.nx - ex) + GetRandomReal(-60, 60) that ultimately causes you to have to reconvert back with Cos(angle * bj_DEGTORAD). Just work in radians directly. 60 is bj_PI/3.
  
  
• Instead of looping over the whole list on every order given in the map (which ultimately isn't that costly), you would benefit from a unit -> struct association. Since you're already using a unit indexer this would be quite easy. By making it a static struct you can then just use the unit's ID as the struct reference everywhere you need to get it. Properly null instance members or temporarily put all affected units into a unit group to prevent false positives when reading structs on issued orders. Would clean up the ResetTimer stuff too. If you were using a single timer for all instances this would be a little annoying and require a linked list or something, but you're using one timer per instance already.
  
  
• Make the order period configurable.
  
  
• It's technically possible to crash the game by casting this within 64 units of a map edge and hitting the lottery on the order location. That's mostly user error for letting units get that close, but I guess it's something worth considering here. Normally you would only project by a minimum distance to avoid such an issue but doing that here means computing unit move speed and that's probably more hassle than it's worth.

 

[xyzier_24]

• Don't use SquareRoot to compute distance when you can, and instead compare >= 32^2 (1024). Additionally don't compute angle and distance until inside the if distance <= 32 block since they're not needed otherwise.
  
  
• Things like this can just be reduced: GetRandomReal(0, 360) * bj_DEGTORAD becomes GetRandomReal(0, 2*bj_PI)
  Or this one local real angle = bj_RADTODEG * Atan2(this.ny - ey, this.nx - ex) + GetRandomReal(-60, 60) that ultimately causes you to have to reconvert back with Cos(angle * bj_DEGTORAD). Just work in radians directly. 60 is bj_PI/3.
  
  
• Instead of looping over the whole list on every order given in the map (which ultimately isn't that costly), you would benefit from a unit -> struct association. Since you're already using a unit indexer this would be quite easy. By making it a static struct you can then just use the unit's ID as the struct reference everywhere you need to get it. Properly null instance members or temporarily put all affected units into a unit group to prevent false positives when reading structs on issued orders. Would clean up the ResetTimer stuff too. If you were using a single timer for all instances this would be a little annoying and require a linked list or something, but you're using one timer per instance already.
  
  
• Make the order period configurable.
  
  
• It's technically possible to crash the game by casting this within 64 units of a map edge and hitting the lottery on the order location. That's mostly user error for letting units get that close, but I guess it's something worth considering here. Normally you would only project by a minimum distance to avoid such an issue but doing that here means computing unit move speed and that's probably more hassle than it's worth.

Thanks a lot sir Pyro! This is what I've done.

scope MentalDisarray

    globals
        private constant integer ABILITY_ID = 'A8U7'
        private constant integer BUFF_ID = 'B8U7'
        private constant real TIMER_INTERVAL = .1
    endglobals

    private struct Data
        unit e
        real x
        real y
        real nx
        real ny
        timer cycle
        static thistype array instances

        method destroy takes nothing returns nothing
            set instances[GetUnitUserData(this.e)] = 0
            call ReleaseTimer(this.cycle)
            set this.e = null
            set this.cycle = null
            call this.deallocate()
        endmethod

        private static method runTimer takes nothing returns nothing
            local Data this = GetTimerData(GetExpiredTimer())
            local real ex = GetUnitX(this.e)
            local real ey = GetUnitY(this.e)
            local real distanceSquared = (this.nx - ex) * (this.nx - ex) + (this.ny - ey) * (this.ny - ey)
            local real angle

            if GetUnitMoveSpeed(this.e) > 0 then
                if distanceSquared <= 1024 or not udg_UnitMoving[GetUnitUserData(this.e)] then
                    set angle = Atan2(this.ny - ey, this.nx - ex) + GetRandomReal(-bj_PI / 3, bj_PI / 3)
                    set this.nx = this.x + 64 * Cos(angle)
                    set this.ny = this.y + 64 * Sin(angle)
                    call IssuePointOrder(this.e, "move", this.nx, this.ny)
                endif
            elseif GetUnitCurrentOrder(this.e) != 851972 then
                call IssueImmediateOrder(this.e, "stop")
            endif

            if GetUnitAbilityLevel(this.e, BUFF_ID) == 0 or GetWidgetLife(this.e) <= 0.405 then
                call ReleaseTimer(GetExpiredTimer())
                call this.destroy()
            endif
        endmethod

        private static method resetTimer takes unit e returns nothing
            local integer id = GetUnitUserData(e)
            if instances[id] != 0 then
                call instances[id].destroy()
            endif
        endmethod

        private static method runEvent takes nothing returns nothing
            local Data this
            local unit target = GetSpellTargetUnit()
            local integer id = GetUnitUserData(target)
            call thistype.resetTimer(target)
            set this = thistype.allocate()
            set this.e = target
            set this.x = GetUnitX(this.e)
            set this.y = GetUnitY(this.e)
            set this.nx = this.x + 64 * Cos(GetRandomReal(0, 2 * bj_PI))
            set this.ny = this.y + 64 * Sin(GetRandomReal(0, 2 * bj_PI))
            set this.cycle = NewTimerEx(this)
            set instances[id] = this
            call TimerStart(this.cycle, TIMER_INTERVAL, true, function thistype.runTimer)

            if GetUnitMoveSpeed(this.e) > 0 then
                call IssuePointOrder(this.e, "move", this.nx, this.ny)
            else
                call IssueImmediateOrder(this.e, "stop")
            endif
            set target = null
        endmethod

        private static method resumeEvent takes nothing returns nothing
            local unit u = GetTriggerUnit()
            local integer id = GetUnitUserData(u)
            local Data this = instances[id]

            if this != 0 then
                if GetUnitMoveSpeed(this.e) > 0 then
                    if GetOrderPointX() != this.nx or GetOrderPointY() != this.ny then
                        call IssuePointOrder(this.e, "move", this.nx, this.ny)
                    endif
                elseif GetIssuedOrderId() != 851972 then
                    call BlzUnitInterruptAttack(this.e)
                    call IssueImmediateOrder(this.e, "stop")
                endif
            endif
        endmethod

        static method onInit takes nothing returns nothing
            call RegisterSpellEffectEvent(ABILITY_ID, function thistype.runEvent)
            call RegisterPlayerUnitEvent(EVENT_PLAYER_UNIT_ISSUED_ORDER, function thistype.resumeEvent)
            call RegisterPlayerUnitEvent(EVENT_PLAYER_UNIT_ISSUED_POINT_ORDER, function thistype.resumeEvent)
            call RegisterPlayerUnitEvent(EVENT_PLAYER_UNIT_ISSUED_TARGET_ORDER, function thistype.resumeEvent)
            call RegisterPlayerUnitEvent(EVENT_PLAYER_UNIT_ISSUED_UNIT_ORDER, function thistype.resumeEvent)
        endmethod
    endstruct

endscope

 

[Pyrogasm]

I didn’t look at your updated code longer than to realize I said the wrong thing above and need to correct it before I sleep. I said “static struct” but meant making the struct “extends array”. Obviously a static struct array variable doesn’t really achieve anything helpful, that’s my bad.