OrderQueue - "Shift with triggers"

PurplePoot · May 27, 2008

I did this system a while ago for someone, and I guess I may as well post it, as such a thing seems to be in demand from time to time.

I haven't found any bugs with it, but that does not guarantee that there are none (if any could be reported, it would be appreciated).

Comments at the top of the code document how to use this system.

Also, if you guys could give some reviews for me to base my work on, it would be appreciated, because I'm not sure that anyone else does much moderating here at the moment, and approving one's own systems by one's own standards is a definite no.

1.06

Fixed a bug which caused attempted destruction of null structs.
Fixed a bug with Push()

1.05

SetLooping method for cleaner code
OrderQueues now destroy orders by default when they are destroyed. This can be changed by a boolean flag on the struct.

1.04

Naming tweaks, reversed queue direction

1.03

Added static constant common OrderIds

1.02

Lots of renaming
SetUnitOrderQueue added, called automatically upon OrderQueue.create()
Removed destruct in favor of destroy
Pop() added

1.01

Orders are now independent from OrderQueues
Efficiency tweaks
Constants for usability
More powerful options
Support for permanent orders and looping order sequences

1.00

Initial release

JASS:

//*****************************************************************************
//*                          OrderQueue Library v1.06                         *
//*                               By: PurplePoot                              *
//*****************************************************************************

//=============================================================================
//The first step in using this system is to make sure that every unit which
//is to have an order queue has an instance of the OrderQueue struct attached to them.
//You'll have to edit the GetUnitOrderQueue function so that it can retrieve
//things with your specific storage system.
//=============================================================================

//To handle orders, there are several options in OrderQueue.
//The Concat method concatenates a new order - it is added to the end of the queue.
//The Push method pushes the new order under the stack - it is added to the front.
//The Remove method allows you to remove an indefinite order from an indefinite
//position in the queue.
//The Pop method removes the front order from the queue and returns it.
//The PopLast method removes the back order from the queue and returns it.

//=============================================================================
//A queue may be turned on and off without destroying it via SetActive
//=============================================================================

//For efficiency or precision reasons, you may want to change the speed on the enum.
//This is listed in a constant in the globals section. (ENUM_TIMER_INTERVAL)

//=============================================================================
//There are some useful OrderID constants in Order.
//They are named ID_<ordername>
//Included are: ATTACK, ATTACKGROUND, ATTACKONCE, CANCEL, STOP,
//PATROL, HOLDPOSITION, MOVE, SMART
//=============================================================================

//If <Order>.looping is set to true, the order will be sent to the end of
//a Queue instead of even being removed completely, allowing for repeating
//sequences of orders. Alternatively, <Order>.SetLooping(boolean) may be used
//so as to allow cleaner code, ex oq.Push(Order.create(..).SetLooping(true))
//=============================================================================

//******SYSTEM STARTS******
library OrderQueue
globals
    private timer tim = CreateTimer()
    private group queuedUnits = CreateGroup()
    private constant real ENUM_TIMER_INTERVAL = .25
endglobals

function GetUnitOrderQueue takes unit u returns OrderQueue
    return GetUnitUserData(u)//return whatever way you store your order queue
endfunction

function SetUnitOrderQueue takes unit u, OrderQueue oq returns nothing
    call SetUnitUserData(u,oq)//set whatever way you store your order queue
endfunction

private function CheckOrders_Child takes nothing returns nothing
    local unit u = GetEnumUnit()
    local OrderQueue dat = GetUnitOrderQueue(u)
    if GetUnitCurrentOrder(u) == 0 and dat.last != 0 then
        call dat.last.Issue(u)
        if not dat.last.looping then
            call dat.last.destroy()
        else
            call dat.Push(dat.Pop())
        endif
    endif
    set u = null
endfunction

private function CheckOrders takes nothing returns nothing
    call ForGroup(queuedUnits,function CheckOrders_Child)
endfunction

struct Order
    static constant integer NO_TARGET = 0
    static constant integer POINT_TARGET = 1
    static constant integer WIDGET_TARGET = 2

    static constant integer ID_MOVE = 851986
    static constant integer ID_STOP = 851972
    static constant integer ID_PATROL = 851990
    static constant integer ID_SMART = 851971
    static constant integer ID_HOLDPOSITION = 851993
    static constant integer ID_CANCEL = 851976
    static constant integer ID_ATTACK = 851983
    static constant integer ID_ATTACKONCE = 851985
    static constant integer ID_ATTACKGROUND = 851984

    private integer orderType = Order.NO_TARGET
    private integer orderId
    private real x
    private real y
    private widget w

    Order next = 0
    Order prev = 0
    OrderQueue queue = 0
    boolean looping = false

    static method createNoTarget takes integer orderId returns Order
        local Order o = Order.allocate()
        set o.orderId = orderId
        return o
    endmethod

    static method createWidgetTarget takes integer orderId, widget target returns Order
        local Order o = Order.allocate()
        set o.orderId = orderId
        set o.orderType = Order.WIDGET_TARGET
        set o.w = target
        debug if target == null then
            debug call BJDebugMsg("Target is null!")
        debug endif
        return o
    endmethod

    static method createPointTarget takes integer orderId, real x, real y returns Order
        local Order o = Order.allocate()
        set o.orderId = orderId
        set o.orderType = Order.POINT_TARGET
        set o.x = x
        set o.y = y
        return o
    endmethod

    method Issue takes unit subject returns nothing
        if this.orderType == Order.NO_TARGET then
            call IssueImmediateOrderById(subject,this.orderId)
        elseif this.orderType == Order.POINT_TARGET then
            call IssuePointOrderById(subject,this.orderId,this.x,this.y)
        elseif this.orderType == Order.WIDGET_TARGET then
            call IssueTargetOrderById(subject,this.orderId,this.w)
        debug else
            debug call BJDebugMsg("Stop editing the system code!")
        endif
    endmethod

    method SetLooping takes boolean looping returns Order
        set this.looping = looping
        return this
    endmethod

    method onDestroy takes nothing returns nothing
        call this.queue.Remove(this)
    endmethod
endstruct

struct OrderQueue
    private unit holder
    Order first = 0
    Order last = 0

    boolean active = false
    boolean clean = true

    method SetOwner takes unit owner, boolean activate returns OrderQueue
        set this.holder = owner
        call SetUnitOrderQueue(owner,this)
        call this.SetActive(activate)
        return this
    endmethod

    method Concat takes Order o returns nothing
        set o.queue = this
        if this.first == 0 then
            set this.first = o
        else
            set this.last.next = o
            set o.prev = this.last
        endif
        set this.last = o
    endmethod

    method Push takes Order o returns nothing
        set o.queue = this
        if this.first != 0 then
            set o.next = this.first
            set this.first.prev = o
        endif
        if this.last == 0 then
            set this.last = o
        endif
        set this.first = o
        set o.prev = 0
    endmethod

    method Remove takes Order o returns nothing
        set o.queue = 0
        if o.prev != 0 and o.next != 0 then
            set o.prev.next = o.next
            set o.next.prev = o.prev
        elseif o.prev != 0 and o.next == 0 then
            set o.prev.next = 0
        elseif o.prev == 0 and o.next != 0 then
            set o.next.prev = 0
        endif
        if o == this.first then
            set this.first = o.next
        endif
        if o == this.last then
            set this.last = o.prev
        endif
    endmethod

    method PopLast takes nothing returns Order
        local Order o = this.first
        set this.first = o.next
        set this.first.prev = 0
        return o
    endmethod

    method Pop takes nothing returns Order
        local Order o = this.last
        set this.last = o.prev
        set this.last.next = 0
        return o
    endmethod

    method SetActive takes boolean activate returns nothing
        set this.active = activate
        if activate then
            if FirstOfGroup(queuedUnits) == null then
                call TimerStart(tim,ENUM_TIMER_INTERVAL,true,function CheckOrders)
            endif
            call GroupAddUnit(queuedUnits,this.holder)
        else
            call GroupRemoveUnit(queuedUnits,this.holder)
            if FirstOfGroup(queuedUnits) == null then
                call PauseTimer(tim)
            endif
        endif
    endmethod

    method onDestroy takes nothing returns nothing
        if this.clean then
            loop
                exitwhen this.last == 0
                call this.last.destroy()
            endloop
        endif
    endmethod
endstruct
endlibrary
//******SYSTEM ENDS******

Troll-Brain · May 27, 2008

you didn't put any debug message

JASS:

    method Issue takes unit subject returns nothing
        if .orderType == 0 then
            call IssueImmediateOrderById(subject,.orderId)
        elseif .orderType == 1 then
            call IssuePointOrderById(subject,.orderId,.x,.y)
        else
            call IssueTargetOrderById(subject,.orderId,.w)
        endif
    endmethod

and if the subject is not equal to 0 and 1 but neither 2 ?

Just a question that i didn't test.
What happen if a target is removed of the game or in general being untargetable ?

Earth-Fury · May 28, 2008

JASS Helper Manual said:
and we cannot use index 0 which is null for structs

http://wc3campaigns.net/vexorian/jasshelpermanual.html#strucrest
0 is the value for a nil struct, not -1.

JASS:

    method QueueOrderStringNoTarget takes string orderString returns nothing
        call .QueueOrder(OrderId(orderString),0,0,null)
    endmethod
    method QueueOrderStringWidgetTarget takes string orderString, widget target returns nothing
        call .QueueOrder(OrderId(orderString),0,0,target)
    endmethod
    method QueueOrderStringPointTarget takes string orderString, real x, real y returns nothing
        call .QueueOrder(OrderId(orderString),x,y,null)
    endmethod

Just force people to call OrderId() themselves on order strings...

Use a constant for the timer's timeout.

oq_queuedUnits shouldn't be prefixed and should be private.. (or public if you so care.)

JASS:

    integer orderType = 0
    //0 = no target
    //1 = point target
    //2 = widget target

Use class constants for that for code cleanliness. (static constant type name = value)

The removal of "Order" objects from the list should be done in an onDestroy() method in Order, not in the CheckOrders_Child function.

The timer runs even when the group is empty. Fix that please.

Units should be added to the oq_queuedUnits group in the create method of UnitData. (And they should be removed in an onDestroy function.) A boolean should be used to determine if the queue for a unit is active, considering you'll only loose a few milliseconds per inactive unit...

"UnitData" is a crappy class name. "OrderQueue" would fit better. (Or at the least make it public.)

I would much rather implement this with DataSystem myself, simply to leave UnitUserData() untouched in released code. But this is just a library for coders anyway, so its no big deal.

You do know you can use empty lines in code, and its conventional to separate methods with a single or more space(s), right? Also, parameter lists should have a space after each comma...

And for the love of all that is holy, use this. instead of just . ... (I would call that a prerequisite for approval... grim001 put me off that with code like: 45.*.radius ...)

Thats all I can think of right now.

PurplePoot · May 28, 2008

and if the subject is not equal to 0 and 1 but neither 2 ?

Then you're touching parts of the code that you shouldn't be (In fact, I should make it private).

What happen if a target is removed of the game or in general being untargetable ?

Then it would just fail to issue the order and this do nothing, and thus move on to the next order (Or should, anyways).

you didn't put any debug message

Elaborate where I need them.

Just force people to call OrderId() themselves on order strings...

Wrapper functions suck in general, as I said I left those in for people like GUIers who want simplicity over efficiency.

0 is the value for a nil struct, not -1.

It doesn't really matter, since I manually set to -1, but interesting, I always thought 0 was used.

Use a constant for the timer's timeout.

Good point.

Use class constants for that for code cleanliness. (static constant type name = value)

Also a good point.

oq_queuedUnits shouldn't be prefixed and should be private.. (or public if you so care.)

Currently it should be open to the public since you manually add units to it... Although, I could always make it private and make a static method in Order.

Use class constants for that for code cleanliness. (static constant type name = value)

True, didn't think of that either.

The removal of "Order" objects from the list should be done in an onDestroy() method in Order, not in the CheckOrders_Child function.

Again, true.

The timer runs even when the group is empty. Fix that please.

And how would I get around that? There needs to be a time interval at which the group is checked.

"UnitData" is a crappy class name. "OrderQueue" would fit better. (Or at the least make it public.)

The reason I used the name 'UnitData' was because I wanted it to be usable for other things, since it is attached to UserData. However, I think I'll let people attach it manually to whatever they want and rename it - good point.

Units should be added to the oq_queuedUnits group in the create method of UnitData. (And they should be removed in an onDestroy function.) A boolean should be used to determine if the queue for a unit is active, considering you'll only loose a few milliseconds per inactive unit...

Hmm, that would be an interesting feature (The second part). And with the changed class I guess it does make more sense to have it a method in the former rather than the latter.

You do know you can use empty lines in code, and its conventional to separate methods with a single or more space(s), right? Also, parameter lists should have a space after each comma...

About param lists, they do, unless you mean the internal stuff, which is just coding style. About empty lines, again, coding style, but I guess I may as well add them for those that are reading it.

And for the love of all that is holy, use this. instead of just . ... (I would call that a prerequisite for approval... grim001 put me off that with code like: 45.*.radius ...)

Haha, I'll make that a later priority, but I guess...

Updating, uncompiled as of yet.

EDIT: Got to go, halfway through editing, will finish it later.

Earth-Fury · May 28, 2008

PurplePoot said:
And how would I get around that? There needs to be a time interval at which the group is checked.

If you include addition and removal from the group within the system, you just have to check FirstOfGroup() on removal. If null, stop timer set flag. On addition, if flag, start timer. (or just store a count.)

You may want to consider going for something like:

call orderqueue.Queue(Order.createPointTarget(x, y))

handling addition and removal from the list in the Queue and an additional DeQueue method. The type of order is determined by the variant of create used, and a simple if-elseif-etc structure when executing an order would differentiate them. (Store a pointer to prev for constant-time removal from the list given an arbitrary order. Otherwise deal with the overhead of an o(n) search. Remember: memory is cheap.) And you can add wrappers in a separate library if needed... functions which take the object and deal with addition of different order types directly. Chain libraries to separate noobcode from my pretty vJASS.

oh, and, .25? What is it with you people and the hatin' on 0?

Troll-Brain · May 28, 2008

you should put debug when it is a target order and the target is null, for the static method create at least.

PurplePoot · May 28, 2008

V1.01 is up, untested and uncompiled since I'm at school.

Earth-Fury · May 28, 2008

the "ORDER_TYPE" in the type constants is a bit useless, considering they are class constants... (the only class constants, at that.)

JASS:

    //Should be called instead of .destroy()!
    method destruct takes OrderQueue owner returns nothing
        if this.last != -1 and this.next != -1 then
            set this.last.next = this.next
            set this.next.last = this.last
        elseif this.last != -1 and this.next == -1 then
            set this.last.next = -1
        elseif this.last == -1 and this.next != -1 then
            set this.next.last = -1
        endif
        call this.destroy()
    endmethod

Do you have something against onDestroy? http://wc3campaigns.net/vexorian/jasshelpermanual.html#strucondes

Still using -1 makes my brain hurt. 0 is null for structs, so use it as such please.

Few syntax errors, but you'll catch em and fix em simply enough when you get home.

PurplePoot · May 28, 2008

the "ORDER_TYPE" in the type constants is a bit useless, considering they are class constants... (the only class constants, at that.)

Fair enough.

Do you have something against onDestroy? http://wc3campaigns.net/vexorian/jas...tml#strucondes

Actually, it's because onDestroy can't take parameters - I know very well about it. However, it looks like I accidentally omitted a small part of the destruct function as I was editing the code there (the part which actually uses owner).

Still using -1 makes my brain hurt. 0 is null for structs, so use it as such please.

Fine >>

Anyways, will compile as soon as I've done the edits.

Earth-Fury · May 31, 2008

All of the create methods should actually allocate and deal with an object on their own... And your createPointOrder function really sucks, as people may actually want to order things to move to 0,0 and having it spit debug messages for perfectly valid input is annoying. (switcing to implementing each create function on its own solves the problem..) Also, you should likely just return if null is passed to a widget target order... That would be the expected behaviour IMHO. One final thing... is the "Order" at the end of the create names nessisary? Order.createTargetOrder... its kinda redundant.

I still don't like the use of a "destruct" instead of destroy method. You could solve the issue by holding a pointer to the Queue in the order object, setting it in QueuePush and QueueConcat. (Also, is the Queue needed in the name? Simply Push and Concat would be sufficient, given the class is named "OrderQueue"... as well for Remove)

Following blizzards native convention, Activate and Deactivate should be a single function taking a flag parameter. (Hey, blizzard may be a bunch of fuckups, but a language convention is a language convention...)

your use of the name "last" instead of "prev" to hold the previous object confuses the fuck out of me when reading the code... Especially considering the queue object has a "last" member as well with a different meaning.

You should just call owner.QueueRemove(this) from Destruct... avoid dual implementation.

It may be useful to provide a Pop() method in OrderQueue. If nothing else, because you easily can.

I'm too lazy to actually test code now, but I would say this is almost ready to be approved if its in full working order.

Edit: You should include a SetUnitOrderQueue() function. Just so that anything that depends on OrderQueue has a lower-implementation neutral API to work with.

PurplePoot · Jun 1, 2008

Following blizzards native convention, Activate and Deactivate should be a single function taking a flag parameter. (Hey, blizzard may be a bunch of fuckups, but a language convention is a language convention...)

Done.

your use of the name "last" instead of "prev" to hold the previous object confuses the fuck out of me when reading the code... Especially considering the queue object has a "last" member as well with a different meaning.

Changed.

You should just call owner.QueueRemove(this) from Destruct... avoid dual implementation.

Done.

Edit: You should include a SetUnitOrderQueue() function. Just so that anything that depends on OrderQueue has a lower-implementation neutral API to work with.

Done.

It may be useful to provide a Pop() method in OrderQueue. If nothing else, because you easily can.

Added.

I still don't like the use of a "destruct" instead of destroy method. You could solve the issue by holding a pointer to the Queue in the order object, setting it in QueuePush and QueueConcat.

I originally had this, but I removed it because the option to have orders reusable allows the same order to be in multiple queues at once. Now thinking about it, though, since you can't have the order in more than one queue anyways (due to prev and next), fixed.

(Also, is the Queue needed in the name? Simply Push and Concat would be sufficient, given the class is named "OrderQueue"... as well for Remove)

Changed.

All of the create methods should actually allocate and deal with an object on their own... And your createPointOrder function really sucks, as people may actually want to order things to move to 0,0 and having it spit debug messages for perfectly valid input is annoying. (switcing to implementing each create function on its own solves the problem..) Also, you should likely just return if null is passed to a widget target order... That would be the expected behaviour IMHO. One final thing... is the "Order" at the end of the create names nessisary? Order.createTargetOrder... its kinda redundant.

Changed.

Earth-Fury · Jun 2, 2008

Your queue is reversed lol... (How did i not catch that earlier?) Push() to a queue pushes to the end, whereas Pop() takes from the front. You could implement as a deque, where Push() and Pop() go to and from the last, with an alternate Unshift/Shift or PushBack/PopBack for the back, but I would just use PushFront() and PopBack() to add some flexibility to the queue. You should be Pop()ing an item in CheckOrders_Child(), and Push()ing it back on if looping is set. (Push() is the linear addition to a queue, whereas Pop() is the linear removal from a queue.)

Looping should be a property of a queue, not an order. (With automated destruction either an implied property of a non-looping queue, or an independent property of a queue. Looping would of course override destruction, as a looping queue that destroys as it goes makes no sense...)

As well, it may be beneficial to add a "SetOwner()" method for the queue, providing a create method that takes nothing. (The current one renamed something like createOn()) Of course a queue created with no unit would be inactive by default. (Should provide a boolean member of queue to indicate activated status.)

Also, ignoring blizzard convention because of gramatical sense, Activate should be named something like SetActive()

PurplePoot · Jun 2, 2008

Also, ignoring blizzard convention because of gramatical sense, Activate should be named something like SetActive()

Changed.

As well, it may be beneficial to add a "SetOwner()" method for the queue, providing a create method that takes nothing. (The current one renamed something like createOn()) Of course a queue created with no unit would be inactive by default. (Should provide a boolean member of queue to indicate activated status.)

Both changed.

Looping should be a property of a queue, not an order. (With automated destruction either an implied property of a non-looping queue, or an independent property of a queue. Looping would of course override destruction, as a looping queue that destroys as it goes makes no sense...)

Why would looping be a property of a queue? What if you want to have some non-looping orders inserted?

Also, though, that's a good point that looping orders should be implicitly active.

Your queue is reversed lol... (How did i not catch that earlier?) Push() to a queue pushes to the end, whereas Pop() takes from the front. You could implement as a deque, where Push() and Pop() go to and from the last, with an alternate Unshift/Shift or PushBack/PopBack for the back, but I would just use PushFront() and PopBack() to add some flexibility to the queue. You should be Pop()ing an item in CheckOrders_Child(), and Push()ing it back on if looping is set. (Push() is the linear addition to a queue, whereas Pop() is the linear removal from a queue.)

My queue runs in the direction such that the dat.first element is the next to execute. So yes, technically, it is backwards. However, since it takes a whole one line to fix, changed.

As for Pushing, that just works as 'Concat', due to the direction of the queue.

For Pop(), I switched it around and renamed the current one to PopLast().

I was just thinking, is there any reason to need oneUse, excepting for looping (which is now independent)? Should I leave it in for safety, or remove it (I can't think of a use)?

Earth-Fury · Jun 2, 2008

Your point on looping is true. Add a method: o.SetLooping(true) to Order which sets looping to true, and returns "this". So I can do:

oq.Push(Order.createPointTarget(...).SetLooping(true))

Or just add "looping" as a bool parameter to the create functions. (This is likely the better course of action.)

Also, a bool on OrderQueue to destroy all orders in the queue when the queue is destroyed would be nice. (Less manual cleanup = good)

Once you've looked over these comments and done any changing you see fit, I think this is ready for me to actually test and approve with a seal of quality.

PurplePoot · Jun 2, 2008

Your point on looping is true. Add a method: o.SetLooping(true) to Order which sets looping to true, and returns "this". So I can do:

Done.

Or just add "looping" as a bool parameter to the create functions. (This is likely the better course of action.)

Since most of the time you won't be using it, it's far more convenient with it as a separate method than having to stick in a useless parameter 99% of the time.

Also, a bool on OrderQueue to destroy all orders in the queue when the queue is destroyed would be nice. (Less manual cleanup = good)

Good point, done.

Ready for testing, then. 1.05 isn't compiled.

Earth-Fury · Jun 4, 2008

Tested and works as far as I can see. Approved.

Silvenon · Jun 20, 2009

What does this do exactly? (I suppose you love questions like that)

Anachron · Jul 14, 2009

Ahh, finally I am able to understand scripts like this. This is very good, but I can't think of any use right now.

Silvenon · Jul 14, 2009

Can you explain it to me then? With this you can basically have orders following after other orders, right?

Anachron · Jul 14, 2009

Yeah. This is like the Shift Trick in wc3. You can order anything in an order, and that ingame in JASS... This could for example allow multiple uses for dummy units (not that fitting, but it could) ... But also increase the AI strengh and something like that.

Troll-Brain · Aug 4, 2009

But it can fail, not like shift orders.
for example if you "shift order" to collect wood on a tree, it can fail with this if the tree is dead when the unit get the order, not with shift orders.

Bribe · Feb 7, 2011

There's this: http://www.wc3c.net/showthread.php?t=104175 which was released more recently and really dates this library.

OrderQueue - "Shift with triggers"

Similar threads