[Snippet] UnitInventoryEvent

[Kazeon]

[Snippet] ItemSwapEvent

Description

A simple library which provides 3 trigger events which fires when a unit is ordered to move an item to a different or same inventory slot and when a unit is ordered to swap between items.​

Requirements

- JNGP​

Code

library ItemSwapEvent initializer ini /* v1.7

    Description
        This library provides 3 trigger events which fires when a unit is ordered to
        move an item to a different or same inventory slot and when a unit is ordered
        to swap between items.
    
    APIs
        1. Returns an item current slot index inside a units inventory
        
        function GetItemSlotIndex takes unit whichUnit, item whichItem returns integer
        
        2. To execute a function when inventory event fired
        
        function RegisterAnyUnitInventoryEvent takes boolexpr func, integer whichEvent returns triggercondition
        
        3. To prevent a function from being executed on inventory event
        
        function RemoveAnyUnitInventoryEvent takes triggercondition tc, integer whichEvent returns nothing
        
        
    Trigger Events
        1. Fires when a unit is ordered to move an item to an empty slot
        
            EVENT_UNIT_ITEM_MOVE
            
        2. Fires when a unit is ordered to swap between items
        
            EVENT_UNIT_ITEM_SWAP
            
        3. Fires when a unit is ordered to move an item to a same slot
        
            EVENT_UNIT_ITEM_CANCEL
        
        
    Event Responses
        1. Get swapping/moving unit
        
            function GetEventSwapUnit takes nothing returns unit
            
        2. Get targeted item at the event
        
            function GetEventSwapItemTarget takes nothing returns item
            
        3. Get source item at the event
        
            function GetEventSwapItemSource takes nothing returns item
*/

    // Edit them by your own risk

    globals
        private integer Total = -1
        private integer array Slot
        private item array Target
        private item EventTarget = null
        private item EventSource = null
        private unit EventUnit = null
        private unit array Unit
        private trigger array Trigger
        private timer Timer = CreateTimer()
        constant integer EVENT_UNIT_ITEM_SWAP = 0
        constant integer EVENT_UNIT_ITEM_MOVE = 1
        constant integer EVENT_UNIT_ITEM_CANCEL = 2
    endglobals
    
    function GetItemSlotIndex takes unit whichUnit, item whichItem returns integer
    
        local integer i = 0
        local integer ext = UnitInventorySize(whichUnit)
        
        loop
            exitwhen i == ext
            if UnitItemInSlot(whichUnit, i) == whichItem then
                return i
            endif
            set i = i + 1
        endloop
        
        return -1
    endfunction
    
    function GetEventSwapUnit takes nothing returns unit
        return EventUnit
    endfunction
    
    function GetEventSwapItemTarget takes nothing returns item
        return EventTarget
    endfunction
    
    function GetEventSwapItemSource takes nothing returns item
        return EventSource
    endfunction
    
    function RegisterAnyUnitInventoryEvent takes boolexpr func, integer whichEvent returns triggercondition
        return TriggerAddCondition(Trigger[whichEvent], func)
    endfunction
    
    function RemoveAnyUnitInventoryEvent takes triggercondition tc, integer whichEvent returns nothing
        call TriggerRemoveCondition(Trigger[whichEvent], tc)
    endfunction
    
    private function swapEvent takes nothing returns nothing
    
        local unit bu
        local item bt
        local item bs
        
        loop
            exitwhen Total < 0
            set bu = EventUnit
            set bt = EventTarget
            set bs = EventSource
            set EventUnit = Unit[Total]
            
            if Slot[Total] != GetItemSlotIndex(Unit[Total], Target[Total]) then
                set EventTarget = UnitItemInSlot(Unit[Total], Slot[Total])
                set EventSource = UnitItemInSlot(Unit[Total], GetItemSlotIndex(Unit[Total], Target[Total]))
                if GetItemTypeId(EventTarget) == 0 then
                    call TriggerEvaluate(Trigger[EVENT_UNIT_ITEM_MOVE])
                else
                    call TriggerEvaluate(Trigger[EVENT_UNIT_ITEM_SWAP])
                endif
            else
                set EventTarget = UnitItemInSlot(Unit[Total], GetItemSlotIndex(Unit[Total], Target[Total]))
                set EventSource = EventTarget
                call TriggerEvaluate(Trigger[EVENT_UNIT_ITEM_CANCEL])
            endif
            
            set EventUnit = bu
            set EventTarget = bt
            set EventSource = bs
            set Total = Total - 1
        endloop
        
        set bu = null
        set bt = null
        set bs = null
        
    endfunction
    
    private function onOrder takes nothing returns boolean
    
        if IsItemOwned(GetOrderTargetItem()) then
            set Total = Total + 1
            set Target[Total] = GetOrderTargetItem()
            set Unit[Total] = GetTriggerUnit()
            set Slot[Total] = GetItemSlotIndex(Unit[Total], Target[Total])
            call TimerStart(Timer, 0, false, function swapEvent)
        endif
        
        return false
    endfunction
    
    private function ini takes nothing returns nothing
    
        local trigger t = CreateTrigger()
        
        set Trigger[EVENT_UNIT_ITEM_MOVE] = CreateTrigger()
        set Trigger[EVENT_UNIT_ITEM_CANCEL] = CreateTrigger()
        set Trigger[EVENT_UNIT_ITEM_SWAP] = CreateTrigger()
        
        call TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_UNIT_ISSUED_TARGET_ORDER)
        call TriggerAddCondition(t, Condition(function onOrder))
        
    endfunction
    
endlibrary

DEMO

// Display text when a unit is swapping items
function a takes nothing returns boolean
    call BJDebugMsg(GetUnitName(GetEventSwapUnit()) + " swapped " + GetItemName(GetEventSwapItemTarget()) + " with " + GetItemName(GetEventSwapItemSource()))
    return false
endfunction

// Display text when a unit is moving items
function b takes nothing returns boolean
    call BJDebugMsg(GetUnitName(GetEventSwapUnit()) + " moved " + GetItemName(GetEventSwapItemSource()) + " to slot-" + I2S(GetItemSlotIndex(GetEventSwapUnit(), GetEventSwapItemSource()) + 1))
    return false
endfunction

// Display text when a unit cancelled to move/swap items
function c takes nothing returns boolean
    call BJDebugMsg(GetUnitName(GetEventSwapUnit()) + " canceled to move " + GetItemName(GetEventSwapItemSource()))
    return false
endfunction

function InitTrig_Demo takes nothing returns nothing
    call RegisterAnyUnitInventoryEvent(Filter(function a), EVENT_UNIT_ITEM_SWAP)
    call RegisterAnyUnitInventoryEvent(Filter(function b), EVENT_UNIT_ITEM_MOVE)
    call RegisterAnyUnitInventoryEvent(Filter(function c), EVENT_UNIT_ITEM_CANCEL)
endfunction

 

[BPower]

How do you access the triggercondition for RemoveUnitInventoryEvent?
I don't remember natives which return triggercondition except triggeraddcondition.
The name is confusing, because it is not a single unit event but any unit event.

Is the timer required?

Why is the trigger public?

I would create two triggers instead of an array.

You could exit the loop when ext == i and avoid the -1 operation.

BPower@pina colada hashtag holidays.

 

[Bannar]

This is pretty easy with Bribe's OrderEvent, tho probably doesn't hurt to implement specific snippet..

Fix API function names, those are too common, too generic, instead make them more specific. Just look at other libraries, hows its done: UI -> event index? GetEventUnitIndex; and so on. Its rather important to add "Event" string into such function tags, GetSwappingUnit is more of a unit group kinda function (swapped group anyone?).

Why is timer here? Could you explain? Even if your answer is yes - its needed - please provide some demo code so we can test this.

RemoveUnitInventoryEvent is rather pointless - none stores triggercondition handles, thats why in my Trigger I've been storing boolexpr instead.

return 0 in GetItemSlotIndex as defaut one? If I'm not mistaken, slots are indexed from 0 to 5 (for a maximum number of items available), thus index 0 clearly is an appriopriate one while as default it should return NOT_FOUND which could be interpretted here as -1 or so. If I'm right, then returned value is also incorrect: you provide index+1 instead of index. In many languages tables are 0-indexed and when working with such using behaviour members like erase you need to pass a valid position, not the "human-friendly" iterator+1 but the iterator itself.

"UnitInventoryEvent_evt_trigger" -> SCORE_PRIVATE+ "evt_trigger" evt_trigger & array trig need to be private.

Usage of global integer "s" with timer make this unreliable. This is some poor approach.

Null trigger in "ini" function.

 

[PurgeandFire]

I agree with a lot of the points above. Mostly:

• Remove the function "RemoveUnitInventoryEvent" - there were a few discussions about "unregistering" events in the past. It turns out it is mostly useless to have as a feature (except in weirdly specific cases--but it still creates a lot of code bloat).
• The naming could use some work. e.g. RegisterInventorySlotEvent. That sounds pretty good to me.
  
  The globals should also be renamed to be more specific than "s" and "r". And I know this isn't a requirement, but you should try to be consistent with your coding style. i.e. Don't switch between camelCase and PascalCase for your functions. Either choose one or the other (I recommend PascalCase for functions, see JPAG). And generally we only use underscores for constants, but it depends on your coding style.
• Because of the timer, you have to add a stack to keep things MUI. I know it is a little bit of a hassle to implement, but if the orders are made via triggers, then the globals will be overwritten. To fix that, follow this model:
  

  globals
      unit array savedUnits
      item array savedItems
      integer size = 0
      timer t = CreateTimer()
  endglobals
  
  function Callback takes nothing returns nothing
     loop
         exitwhen size == 0
         // do your actions using savedUnits[size], savedItems[size], etc. 
         set size = size - 1
     endloop
  endfunction
  
  function Example takes nothing returns nothing
      set size = size + 1
      set savedUnits[size] = <Some Unit>
      set savedItems[size] = <Some Item>
      // etc. 
      call TimerStart(t, 0, false, function Callback)
  endfunction

• As Bannar said, GetItemSlotIndex should probably return a number between 0-5 since you can use those directly in the JASS functions. The only reason that the BJ's increment it by 1 is to make it intuitive for GUI (because 1-6 is more natural than 0-5 [if you don't have a programming background]).
• For recursion, you have to use temporary variables for the event data. Again, it is a bit of a hassle, but it is to guarantee functionality. Here is a snippet from Track to show how it is done:
  
  Code

          private static method onInteract takes nothing returns boolean
              local thistype  temp = instance
              local trackable tr 
              local player    p
              
              set instance = TrackTable[GetHandleId(GetTriggeringTrigger())]
              
              if instance.flag then
                  /* store the current values into locals temporarily */
                  set tr = object 
                  set p  = tracker 
                  
                  /* update the event globals with values for THIS event */
                  set object  = GetTriggeringTrackable()
                  set tracker = Player(TrackTable[GetHandleId(object)])
                  
                  /* fire the event */
                  if GetTriggerEventId() == EVENT_GAME_TRACKABLE_TRACK then
                      call TriggerEvaluate(instance.onHover)
                      call TriggerEvaluate(anyHover)
                  else
                      call TriggerEvaluate(instance.onClick)
                      call TriggerEvaluate(anyClick)
                  endif
                  
                  /* set the global event data back to its old values */
                  set instance = temp
                  set object   = tr
                  set tracker  = p
                  
                  /* set the locals back to null */
                  set tr = null
                  set p  = null
              endif
              
              return false
          endmethod

• GetSwappingUnit could be GetItemSwapUnit, to be more specific. I don't know. I just think it could use a better name.
• Optionally, perhaps you could add a static if to support Bribe's OrderEvent? This definitely isn't necessary, but it is so simple to implement that it may be worth it.

Otherwise, I like the idea for the system. It just needs some fixing and then it'll be ready for approval.

 

[Kazeon]

You could exit the loop when ext == i and avoid the -1 operation.

okay, speedy freaky >.>

How do you access the triggercondition for RemoveUnitInventoryEvent?

maybe,

globals
    triggercondition c
endglobals

set c = Filter(function a)
call RegisterUnitInventoryEvent(c, EVENT_UNIT_SWAP_ITEM)
call RemoveUnitInventoryEvent(c, EVENT_UNIT_SWAP_ITEM)

wait, you are correct, I did it wrong..
maybe the function must return
return TriggerAddCondition(trig[whichEvent], func)
then you call it by
set c = RegisterUnitInventoryEvent(c, EVENT_UNIT_SWAP_ITEM)
which is not really friendly.. so how can I fix? or maybe better remove it?

BPower@pina colada hashtag holidays.

hey, off-topic is not allowed here

This is pretty easy with Bribe's OrderEvent, tho probably doesn't hurt to implement specific snippet..

maybe easy for you but I need so many tests just to make this work properly, I mean finding the working unit event, finding the condition if unit is swaping items, etc. wait, wut? I think I get your comment wrong I will consider it..

timer is truly needed, to check is the targeted item is moved or not, if not using timer, the condition if s != GetItemSlotIndex(u, t) then will never be found anyhow, except if you can tell me what function returns the dragged/clicked item then I can remove that timer (currently you can only detect the targeted item)

please provide some demo code so we can test this.

wut? I thought I have attached some demo codes above, or what kind of demo is that exactly did you mean?

return 0 in GetItemSlotIndex as defaut one? If I'm not mistaken, slots are indexed from 0 to 5 (for a maximum number of items available), thus index 0 clearly is an appriopriate one while as default it should return NOT_FOUND which could be interpretted here as -1 or so. If I'm right, then returned value is also incorrect: you provide index+1 instead of index. In many languages tables are 0-indexed and when working with such using behaviour members like erase you need to pass a valid position, not the "human-friendly" iterator+1 but the iterator itself.

I return 0 because at first I return i + 1, I will fix them all anyway

Usage of global integer "s" with timer make this unreliable. This is some poor approach.

if only I can find another way to detect is unit actually swapping items or not
btw, did you mean the naming is poor or what?

Because of the timer, you have to add a stack to keep things MUI. I know it is a little bit of a hassle to implement, but if the orders are made via triggers, then the globals will be overwritten. To fix that, follow this model:

that must be a very very rare condition <.<

For recursion, you have to use temporary variables for the event data. Again, it is a bit of a hassle, but it is to guarantee functionality. Here is a snippet from Track to show how it is done:

did you mean my variables like evt_unit etc? So what must I return on GetSwappingUnit()? I dont clearly understant your snippet reference
I'm so stupid this morning >.> so did you mean some kind of backup, ey? But BPow has asked about this once

local unit backup

set backup = u
set u = GetTriggerUnit()
set u = backup
// but when u is still null this is what happening
set backup = null
set u = GetTriggerUnit()
set u = null

can you explain it? but I know that backup can help in some rare situations.

you should try to be consistent with your coding style

okay, I will try to find mine

Otherwise, I like the idea for the system.

of course, I made this for a reason: I needed for FSI and find it useful in so many cases

I will reply more later.. thnks for many responses

 

[PurgeandFire]

Nice changes. I still recommend removing RemoveUnitInventoryEvent, but it is ultimately up to you.

About the recursion, yes you did it correctly. Good job. (although, you can move the nulling of the locals outside the loop)

For the reasoning behind the temporary variables, see:
http://www.hiveworkshop.com/forums/jass-ai-scripts-tutorials-280/recursion-safety-238740/
The key section to read is this:

Do note that upon the first recursion, the source, target and amount variables will be
changed, so all the functions that still haven't been executed by the damageEvent.fire()
call would be using different source/target/amount information. This also affects the
function that dealt the damage. By dealing damage, you are changing the source, the
target and the amount.

The event globals can change if you fire the event within one of the actions. Again, this is a somewhat rare case, but I've encountered it before and it is indeed a valid concern. It is always better to stay on the safe, functional side of things.

 

[Kazeon]

Nice changes. I still recommend removing RemoveUnitInventoryEvent, but it is ultimately up to you.

since the trigger is private and users are unable to access it, so I guess it's still useful if user want to remove the action (removing action instead of disabling the trigger). what do you think?

I've encountered it before

meh, me too (in my UCS). thanks for the link btw..

I would create two triggers instead of an array.

how about this? I haven't fix it bcs I'm not sure it's a good fix or not. since using array makes everything looks neater and perhaps, inline? (it's still hard for me to understand which thing will be inlined, TH has given a link to me but it's just me, too lazy to read it :>)
thank for the help guys..

EDIT:

Usage of global integer "s" with timer make this unreliable. This is some poor approach.

Okay, my brain is somewhat clear now so I will try to explain why I must use a timer.. well, since:
1. I didn't find a native to access the clicked item (item that we want to move/swap or for a better idea, item that we firstly right-click)
2. Only can access the targeted item by GetOrderTargetItem()

so then I need the timer to check item's index inside the inventory both in the current frame (checking the current item's position index at order event) and next frame (checking the item's position index right after the order event [idk what's suitable word for this case, I think frame is understandable]).

so if the both item's position index is different (if slot[total] != GetItemSlotIndex(u[total], t[total]) then), it indicates that the item is truly moved, well.. you know what happens after that..

EDIT 2:
If only I can access the clicked item, everything will be a lot neater.. I will just check if item1 != item2 then and remove that timer and some global variables..

EDIT 3:
okay guys, I'm struggling in finding my own coding style
what do you think about this one? :3

forget it, it looks stupid

EDIT 4:
there is the update
- function names looks cool now
- some globals are also renamed
- imho the coding style looks neat and at the same time doesn't ridiculously wasting spaces

 

[PurgeandFire]

About the coding style, it looks odd. IMO, it is better to just follow the standard conventions. (EDIT: I didn't see your 4th edit.)

But when I said "your own style", I mostly meant about naming and spacing. Some people prefer compact, others might prefer more spacing.
Do whatever looks pleasing to your eye, but don't overdo any particular style (and don't try anything wonky).

I usually put a decent amount of time into just naming variables and organizing the code. I don't know if it is really that great of a habit
since that stuff can often be trivial, but it is pretty important for public code. In general, consider multiple options and choose the best.
For example, here is that snippet written several different ways:

Code

Current Way

    function GetItemSlotIndex takes unit whichUnit, item whichItem returns integer
    
        local integer i = 0
        local integer ext = UnitInventorySize(whichUnit)
        
        loop
            exitwhen i == ext
            if UnitItemInSlot(whichUnit, i) == whichItem then
                return i
            endif
            set i = i + 1
        endloop
        
        return -1
    endfunction

Compact

    function GetItemSlotIndex takes unit whichUnit, item whichItem returns integer
        local integer i = 0
        local integer ext = UnitInventorySize(whichUnit)
        loop
            exitwhen i == ext
            if UnitItemInSlot(whichUnit, i) == whichItem then
                return i
            endif
            set i = i + 1
        endloop
        return -1
    endfunction

Spaced

    function GetItemSlotIndex takes unit whichUnit, item whichItem returns integer
    
        local integer i = 0

        local integer ext = UnitInventorySize(whichUnit)
        
        loop

            exitwhen i == ext

            if UnitItemInSlot(whichUnit, i) == whichItem then

                return i

            endif

            set i = i + 1

        endloop
        
        return -1

    endfunction

Some spacing

    function GetItemSlotIndex takes unit whichUnit, item whichItem returns integer
        local integer i = 0
        local integer ext = UnitInventorySize(whichUnit)
        
        loop
            exitwhen i == ext

            if UnitItemInSlot(whichUnit, i) == whichItem then
                return i
            endif

            set i = i + 1
        endloop
        
        return -1
    endfunction

A mixture (how I would do it. It is very similar to your current one, except no space after the function declaration):

    function GetItemSlotIndex takes unit whichUnit, item whichItem returns integer
        local integer i = 0
        local integer ext = UnitInventorySize(whichUnit)
        
        loop
            exitwhen i == ext
            if UnitItemInSlot(whichUnit, i) == whichItem then
                return i
            endif
            set i = i + 1
        endloop
        
        return -1
    endfunction

And you can also add comments and stuff (but definitely don't overdo comments).
While there is no true "proper" style, it is pretty easy to tell which ones look weird. Full spacing is
rather odd. Some people will also play with indenting, and IMO that makes it difficult to read.
In the end, it is your decision. Just be practical about your decision.

since the trigger is private and users are unable to access it, so I guess it's still useful if user want to remove the action (removing action instead of disabling the trigger). what do you think?

Generally you should keep it private. Either private or readonly. If a user is given full control over something, they have the potential to ruin your system. That's why encapsulation is so important. It allows the coder to control what the user can do with some object, data, etc.

how about this? I haven't fix it bcs I'm not sure it's a good fix or not. since using array makes everything looks neater and perhaps, inline? (it's still hard for me to understand which thing will be inlined, TH has given a link to me but it's just me, too lazy to read it :>)
thank for the help guys..

It is mostly a matter of style. With two triggers, you can have more specific names:

globals
    private trigger swapItemTrigger = CreateTrigger()
    private trigger moveItemTrigger = CreateTrigger()
endglobals

(and you save two lines in your init function).

There are some minor differences memory-wise and speed-wise, but those are negligible. It is your choice. Generally, I use arrays for 3 or 4+ data (or wherever it is clearly logical to use one), as long as all the objects are similar (e.g. an array of effects for one spell, an array of units in a group, etc.). In this case, I would probably use two triggers, but that is just my opinion.

 

[Kazeon]

A mixture (how I would do it. It is very similar to your current one, except no space after the function declaration):

mine is better

I would probably use two triggers, but that is just my opinion.

I will just use array :>

okay, is there anything I need to fix or anything I forgot to fix?

 

[edo494]

I dont see how this gets fired tho, swapEvent is missing even fire

Also you could potentionally extend this for ITEM_SELF_USE or something with similar name(when player is "moving item to itself", basically right click item in slot 4 and place it back to 4)

 

[Kazeon]

when player is "moving item to itself", basically right click item in slot 4 and place it back to 4

It's an easy addition but, if only you can mention in what case that event is useful of course I will add it I will add it (so I have more reason to use trigger array

EDIT:
and of course it's useful btw :>)

EDIT2:
there is an update. Edo's suggestion has been added, but I think the name is still very vague, I can't give a proper name to the new event. so any idea?

at first, I name it with "CANCEL" but it's also not proper since the event doesn't fire when you press 'esc' after right-click..

 

[Krogoth]

This is good, I might use it (for orbs priority for example)
Could you clean up unneeded functions and make optional those TriggerRegisterVariableEvent, which is pretty useless when you can just call function, and not devour our beloved memory with those triggers, bla bla?
I mean, you can simply do:

private constant GENERAL_UNIT_INVENTORY_EVENT = true

...

static if GENERAL_UNIT_INVENTORY_EVENT then
//! runtextmacro callGeneralUnitInventoryEventFunction()
endif

...

static if not GENERAL_UNIT_INVENTORY_EVENT then
    TriggerRegisterVariableEvent bla bla
endif

...
/* somewhere in my trigger */

//! textmacro callGeneralUnitInventoryEventFunction
    call BestGalaxy348.ISnitsaNamNeRokotKosmodroma()
//! endtextmacro

 

[Krogoth]

Why anyone needs those burden triggers if he can directly call function?

Added:

uhm, that looks not pretty.

You are too subjective. I think, it's awesome.

 

[Kazeon]

Why anyone needs those burden triggers if he can directly call function?

so that their function can be executed when the event fired? idk what do you mean by "if he can directly call function"? If they don't know the event then how?

EDIT:
wait, I think I understand. so do you mean I don't need this
call TriggerRegisterVariableEvent(Trigger[EVENT_UNIT_ITEM_MOVE], SCOPE_PRIVATE + "EventTrigger", EQUAL, EVENT_UNIT_ITEM_MOVE)? it's just wrappers so it will be easier for user to understand and use.

 

[Krogoth]

Look, you do triggers to attach functions to them, to keep interface look okay in your understanding. But for this you:
1) create at least 3 triggers,
2) create X conditions,
3) do register function.
This is not okay object-oriented language.
In demo you showed register 3 times, but what noob would do that? Excessive triggers/conditions for almost nothing, when you can just:
1) call global event function right when event fires,
2) from global event function call each local.
0 triggers, 0 conditions
And looks pretty nice once you got used to

Now, how can I use your snippet if it does so many useless things for me?

Added:
The purpose is to add interface possibility, not rework what you've done
By simple macroses

 

[Kazeon]

I'm not creating any condition, look dude, I think you just dont understand how it works, this library does only provide the events, and the wrappers mean to ease users registering their function to be executed when the event fired, I do create an array trigger but it's better than if user must create multiple triggers if they use the event in many triggers.

In demo you showed register 3 times, but what noob would do that? Excessive triggers/conditions for almost nothing,

the demo only want to prove that the system works correctly, dont give a fuck about efficiency of the code there.

and ftw
//! runtextmacro callGeneralUnitInventoryEventFunction() //! textmacro callGeneralUnitInventoryEventFunction call BestGalaxy348.ISnitsaNamNeRokotKosmodroma() //! endtextmacro
HELL NO.

EDIT:
oh, forgot to mention, if registering trigger condition is really that excessive, so I guess wc3 is a fail game

 

[PurgeandFire]

Why anyone needs those burden triggers if he can directly call function?

The issue is order of placement.

If you have a library A that uses library B, then library B will be placed before library A. Therefore, B cannot call functions from A without abusing vJASS syntax (will turn out being trigger evaluations anyway).

As such, implementing that would require the library to be split up into two parts: one part that has the necessary functions, and a separate library (below any libraries that use the first part) which would handle firing the event. But how do you fire that function if it is below? With a trigger evaluation.

So in the end, you'll still end up with a trigger evaluation. It is the only option to keep resources systematic. It becomes a matter of systematic approach vs. map-specific approach. In a map-specific approach, you know where you'll use the functions, so you can tailor the system to your needs. In a systematic approach, you must leave behind those assumptions and ensure that it will work even if a user registers it from multiple places.

 

[Krogoth]

I'm not creating any condition, look dude, I think you just dont understand how it works, this library does only provide the events, and the wrappers mean to ease users registering their function to be executed when the event fired, I do create an array trigger but it's better than if user must create multiple triggers if they use the event in many triggers.

the demo only want to prove that the system works correctly, dont give a fuck about efficiency of the code there.

and ftw
//! runtextmacro callGeneralUnitInventoryEventFunction() //! textmacro callGeneralUnitInventoryEventFunction call BestGalaxy348.ISnitsaNamNeRokotKosmodroma() //! endtextmacro
HELL NO.

EDIT:
oh, forgot to mention, if registering trigger condition is really that excessive, so I guess wc3 is a fail game

I'm not creating any condition, look dude, I think you just dont understand how it works, this library does only provide the events, and the wrappers mean to ease users registering their function to be executed when the event fired, I do create an array trigger but it's better than if user must create multiple triggers if they use the event in many triggers.

You do create condition by Condition(func) and then add it to trigger. You can't deny that, lol.
No, this is not that excessive, but not cool definetely. When you can do without triggers, I see no reason in use of it. Triggers are used to be slow, that's why you use conditions instead of actions, you know. (This does not make them super-fast)
And you think your snippet will be used many times, really?

The issue is order of placement.

If you have a library A that uses library B, then library B will be placed before library A. Therefore, B cannot call functions from A without abusing vJASS syntax (will turn out being trigger evaluations anyway).

As such, implementing that would require the library to be split up into two parts: one part that has the necessary functions, and a separate library (below any libraries that use the first part) which would handle firing the event. But how do you fire that function if it is below? With a trigger evaluation.

So in the end, you'll still end up with a trigger evaluation. It is the only option to keep resources systematic. It becomes a matter of systematic approach vs. map-specific approach. In a map-specific approach, you know where you'll use the functions, so you can tailor the system to your needs. In a systematic approach, you must leave behind those assumptions and ensure that it will work even if a user registers it from multiple places.

To be honest, I am not sure how this works exactly
I use 1 coordination library for all events happen anywhere
Next words are my pure guess
When library A is supposed to use B, on real B also uses A, to fire its functions. It means that function where A is called, is placed below all code. (Which is not part of B interface) Meanwhile A can use B interface, because it is placed above, and A itself is used with trigger evaluation by functions way above.
Compilator never asks me about that, and it works for a long time. ^_^

 

[PurgeandFire]

When library A is supposed to use B, on real B also uses A, to fire its functions. It means that function where A is called, is placed below all code. (Which is not part of B interface) Meanwhile A can use B interface, because it is placed above, and A itself is used with trigger evaluation by functions way above.
Compilator never asks me about that, and it works for a long time. ^_^

vJASS doesn't actually have smart ordering. By default, the compiler is set to automatically handle cases where functions are in the wrong order, but it does so in a somewhat inefficient way:

struct A
    static method test takes nothing returns nothing
        call A.below()
    endmethod
    static method below takes nothing returns nothing
    endmethod
endstruct

This might not give an error, but it will actually involve creating a copy of "below" (the entire function) above "test", and it will likely involve using triggers to evaluate the code and globals to pass arguments (if the function takes in arguments). Even if it doesn't give an error, it still fires it through triggers.

Vexorian added .evaluate() and .execute(), so I don't see any reason for people to use them implicitly. I'm not really sure why he made it have that behavior without throwing an error. It only leads to false assumptions. ;-; That is why it is always important to check the underbelly of the code. Add an error such as:

globals
    a b
endglobals

Or something else. I forget what. There was a particular snippet of code that showed the regular script properly (or you can just open the war3mapoutput.j in your JNGP/jasshelper folder). Anyway, when you see functions such as "prototype", or if you scroll all the way down and see a bunch of "prototype" triggers being generated, then that means that jasshelper likely needed to compensate for bad ordering.

 

[Kazeon]

you think your snippet will be used many times, really?

I don't like how you said it. it is just possibility. we must consider any possibility.

When you can do without triggers, I see no reason in use of it.

so then, please say it to all other code resources around here lol. Even to Nesth if you dare. anyway this is your answer no matter what:

So in the end, you'll still end up with a trigger evaluation. It is the only option to keep resources systematic. It becomes a matter of systematic approach vs. map-specific approach. In a map-specific approach, you know where you'll use the functions, so you can tailor the system to your needs. In a systematic approach, you must leave behind those assumptions and ensure that it will work even if a user registers it from multiple places.

we MAY NOT upload a map-specific resource here remember that

 

[Krogoth]

PurgeandFire,
I see, I see ...
Just took a look at my code
Indeed it does TriggerEvaluate. So stupid. Why not first check functions used and just put below them?
*SHOCKED*
I would better to live without this knowledge ...

This moment is like Snowpiercer: "OMG, we ate this!!"

 

[Krogoth]

private function onOrder takes nothing returns boolean
   
        if not IsItemVisible(GetOrderTargetItem()) then
            set Total = Total + 1
            set Target[Total] = GetOrderTargetItem()
            set Unit[Total] = GetTriggerUnit()
            set Slot[Total] = GetItemSlotIndex(Unit[Total], Target[Total])
            call TimerStart(Timer, .0, false, function swapEvent)
        endif
       
        return false
    endfunction

You don't check if action is about item or not (GetOrderTargetItem() != null)
What does IsItemVisible for no item?

 

[edo494]

most likely false

 

[Kazeon]

private function onOrder takes nothing returns boolean
   
        if not IsItemVisible(GetOrderTargetItem()) then
            set Total = Total + 1
            set Target[Total] = GetOrderTargetItem()
            set Unit[Total] = GetTriggerUnit()
            set Slot[Total] = GetItemSlotIndex(Unit[Total], Target[Total])
            call TimerStart(Timer, .0, false, function swapEvent)
        endif
       
        return false
    endfunction

You don't check if action is about item or not (GetOrderTargetItem() != null)
What does IsItemVisible for no item?

GetOrderTargetItem()
it's automatically check if the target is item or not, there is no way that the target is null

What does IsItemVisible for no item?

I have changed that condition in the newest version

 

[Krogoth]

GetOrderTargetItem()
it's automatically check if the target is item or not, there is no way that the target is null

No, it must be null when order is not about item
Any target order with no item involved will cause IsItemOwned(null). It works for you because it's false (probably that's why you changed condition :]), but quantity of non-item target orders (in generic custom map) is much more than the item. Most time you do 1 unnecessary call action, which is less efficient than 1 comparison | 1 comparison + 1 call.

 

[Kazeon]

No, it must be null when order is not about item

I missed it

Any target order with no item involved will cause IsItemOwned(null)

that's right, and what's wrong with that, it will always become false and the action wont be executed.

Most time you do 1 unnecessary call action, which is less efficient than 1 comparison | 1 comparison + 1 call.

you still call one action & comparasion even if you add if (GetOrderTargetItem() != null) then it will just become useless extra condition checking

EDIT:

probably that's why you changed condition :]

I changed it because the prev condition bugged when you do such actions like this

function InitTrig_Demo takes nothing returns nothing
    local item i = CreateItem('ssil', 0., 0.)
    
    call SetItemVisible(i, false)
    call IssueTargetItemOrder(CreateUnit(Player(0), 'Hpal', 800., 800., 270.), "smart", i)
endfunction

 

[Krogoth]

Haha no ))
Look:
1. Your code

    private function onOrder takes nothing returns boolean
   
        if IsItemOwned(GetOrderTargetItem()) then
            set Total = Total + 1
            set Target[Total] = GetOrderTargetItem()
            set Unit[Total] = GetTriggerUnit()
            set Slot[Total] = GetItemSlotIndex(Unit[Total], Target[Total])
            call TimerStart(Timer, .0, false, function swapEvent)
        endif
       
        return false
    endfunction

does
- GetOrderTargetItem() every time
- IsItemOwned() every time
- GetOrderTargetItem() once more if item is owned

My edit

    private function onOrder takes nothing returns boolean
        local triggerItem = GetOrderTargetItem()
        if triggerItem != null and IsItemOwned(triggerItem) then
            set Total = Total + 1
            set Target[Total] = triggerItem
            set Unit[Total] = GetTriggerUnit()
            set Slot[Total] = GetItemSlotIndex(Unit[Total], Target[Total])
            call TimerStart(Timer, .0, false, function swapEvent)
        endif
        set triggerItem = null
        return false
    endfunction

does
- local variable
- GetOrderTargetItem() every time
- IsItemOwned() if item is targeted

 

[Kazeon]

- GetOrderTargetItem() once more if item is owned

you should not bother it because I just call it twice.

- IsItemOwned() if item is targeted

it's not any better, triggerItem != null itself is an extra condition checking which doesn't makes thing faster. just let's benchmark it, 1 condition checking vs 2 condition checkings, which is faster? I'm enough with this crap.

 

[Krogoth]

No no, I will bother to reply you, my ignorant friend. Maybe there is a chance for you. :]
This is not right way to do it just comparing quantity of conditions. Second condition is touched only if first gives false, but considering that most orders are connected with units, locations or even destructables, the overwhelming majority of time it is 1 condition (IsItemOwned(item)) vs 1 condition (GetOrderTargetItem() != null), and the first requires memory and time to send argument, and has inner (GetOrderTargetItem() == null) condition (otherwise it would not work for you).
By activating least thinking ability you can get my point.

 

[edo494]

I really think this is not problem, because the function call will yield false when the function returns null(hence, the order is not on item), so it would be unnecessary function call.

It wouldnt work, but it does

However, caching the item into variable would be beneficial, because you call GetOrderTargetItem twice, and as Deathismyfriend says, when you use it more than once, you better put it to variable!

 

[edo494]

4 bytes never killed anyone but yes this is rather microoptimization

 

[TriggerHappy]

The wrapper could be useful for people that won't remember the order id's, but doing this manually would only take a few lines (event + orderid check).

It's a nice different approach but I feel it may be slower.

 

[Kazeon]

(event + orderid check).

Is that another approach for this library? I think the order id is always 0 (null) when the unit is ordered to move items. So that approach can't work.

I would not write this library if that approach is working lol

 

[Bannar]

Yes, inventory events are classified as immidiate-type and the getter for id returns the actual event id attached to given action.

Event ids related to item manipulation within inventory:

constant integer ORDER_moveslot1=852002
constant integer ORDER_moveslot2=852003
constant integer ORDER_moveslot3=852004
constant integer ORDER_moveslot4=852005
constant integer ORDER_moveslot5=852006
constant integer ORDER_moveslot6=852007
constant integer ORDER_useslot1=852008
constant integer ORDER_useslot2=852009
constant integer ORDER_useslot3=852010
constant integer ORDER_useslot4=852011
constant integer ORDER_useslot5=852012
constant integer ORDER_useslot6=852013

 

[Kazeon]

I dont see how this gets fired tho, swapEvent is missing even fire

Also you could potentionally extend this for ITEM_SELF_USE or something with similar name(when player is "moving item to itself", basically right click item in slot 4 and place it back to 4)

Then I assume he doesn't aware of this event either. Is this still belong here or GY? Not sure how can this is better than the default one.

 

[BPower]

You never set EventUnit before firing the trigger.

I have to work myself into this first ...

 

[Kazeon]

You never set EventUnit before firing the trigger.

I have to work myself into this first ...

Maybe you need to read the code again.

Now I think this can be useful (I did not think so) since it provides 3 different events.

 

[BPower]

Maybe you need to read the code again.

I was blind.

 

[BPower]

According to the JPAG, you could change your variable naming in the global block.
Total --> total, Slot --> slot, EventTarget --> eventTarget ...

trigger Trigger --> trigger eventTrigger or trig. Don't use trigger trigger.

private function @swapEvent@ --> private function SwapEvent
( used your fancy highlighting )

Within the loop you could use eventUnit over unit[total], which would be a speed benefit.

Once done with it, you could null unit[total] and target[total] for a more accurate cleanup.

RegisterPlayerUnitEvent would be a great requirement for the system. It's
a very frequently used library anyways.

initializer ini .... "without t" will always disturb me.

Off topic:
Could you please comment on your thread in the staff contact. I would
like to understand why you locked your latest spell submission.

 

[Kazeon]

private function @swapEvent@ --> private function SwapEvent
( used your fancy highlighting )

It's private function @swapEvent@ actually.

Mostly, it was my old coding style which didn't really follow the JPAG. I will fix those.

RegisterPlayerUnitEvent would be a great requirement for the system. It's
a very frequently used library anyways.

Systems/spells with less requirements are always much easier to import. I will prefer to have less requirements as possible.

Could you please comment on your thread in the staff contact. I would
like to understand why you locked your latest spell submission.

No one will give feedback anyway and moderators can still moderate it so why not. I don't feel any real disadvantage.

Btw, can you open the map of my last spell submission? I used black tiles there, the last time I did it, it caused fatal error.

 

[edo494]

this is a bit ridiculous, let people program in way that fits them, and only require public API to follow standards.

Not like swapEvent is a lot harder to read compared to SwapRead

 

[BPower]

this is a bit ridiculous, let people program in way that fits them, and only require public API to follow standards.

Not like swapEvent is a lot harder to read compared to SwapRead

Beeing written down in the Jass submission rules in 2012. Hence we adopt it for public released snippets.

Seen individually Swap_Event, swap_Event, swap__event, SwapEvent is read-able per se.
We are lucky Blizzard didn't do such mumbo-jumbo, when writing down the blizzard.j
CreateUnit, createDestructable, Create_Trigger.

-----------------------

@Quilnez: Are you still working on this?

 

[Kazeon]

What should I work on? The JPAG thingy?

 

[edo494]

yes, 2012, time for update imo. But ok I dont really care all that much, I just think that JPAG should be enforced only on public API, and only be recommended/pointed out on private stuff(not things like real r unit u integer i inside struct as members, in functions they are fine)

 

[BPower]

You can consider what I posted here.
Especially the cleanup part.

 

[Bribe]

I swear there is already a resource which is approved somewhere which tracks the instant items are moved around in inventory. I thought it was by Maker but could not find it in the Spells section. If someone can help me look, I would appreciate it. Basically, the goal is to find out which one is superior.

 

[BPower]

Quoting myself:

According to the JPAG, you could change your variable naming in the global block.
Total --> total, Slot --> slot, EventTarget --> eventTarget ...

trigger Trigger --> trigger eventTrigger or trig. Don't use trigger trigger.

private function @swapEvent@ --> private function SwapEvent
( used your fancy highlighting )

Within the loop you could use eventUnit over unit[total], which would be a speed benefit.

Once done with it, you could null unit[total] and target[total] for a more accurate cleanup.

RegisterPlayerUnitEvent would be a great requirement for the system. It's
a very frequently used library anyways.

initializer ini .... "without t" will always disturb me.

Do the proper cleanup!

I swear there is already a resource which is approved somewhere which tracks the instant items are moved around in inventory. I thought it was by Maker but could not find it in the Spells section. If someone can help me look, I would appreciate it. Basically, the goal is to find out which one is superior.

I will search a bit.

 

[Bribe]

Here we are: www.wc3c.net/showthread.php?t=108330

It's probably not the system I'm thinking of, but grim001's system is way more robust.

 

[edo494]

Here we are: www.wc3c.net/showthread.php?t=108330

It's probably not the system I'm thinking of, but grim001's system is way more robust.

I agree, but the linked system is also meant to do something a bit different, plus it is order of magnitude bigger in code size, it is full Item system after all, this is a mere inventory system.

What about adding enable/disable functionality? Maybe there are operations I want to perform on inventory that I would rather not make fire the swap/drop events etc.

 

[Almia]

    constant integer ORDER_moveslot1=852002
    constant integer ORDER_moveslot2=852003
    constant integer ORDER_moveslot3=852004
    constant integer ORDER_moveslot4=852005
    constant integer ORDER_moveslot5=852006
    constant integer ORDER_moveslot6=852007
    constant integer ORDER_useslot1=852008
    constant integer ORDER_useslot2=852009
    constant integer ORDER_useslot3=852010
    constant integer ORDER_useslot4=852011
    constant integer ORDER_useslot5=852012
    constant integer ORDER_useslot6=852013