HomeUser Control Panel (unavailable in archive)ForumsTutorialsArt GalleryResourcesMaps

ItemUtils

12-02-2009, 12:44 AM#1
grim001
Requires Itemdex, ItemHolder, xebasic

This library was created as a collection of useful item handling functions and events. It contains several functions that should have already been natives or BJs, along with events to abstract away the annoying steps involved in detecting item dragging, trading, and usage. ItemUtils uses the values 0-5 to represent inventory slots and -1 for no slot.

Expand JASS:
12-02-2009, 06:44 PM#2
Rising_Dusk
I very much support this being one library with the events instead of two.
Collapse JASS:
// OnUnitPickupItem(ItemFunc)
//   Runs the specified ItemFunc when a unit picks up a powerup item.
//
// OnUnitPickupItem(ItemFunc)
//   Runs the specified ItemFunc when a unit picks up an item.
These two can probably be merged in the documentation.
12-02-2009, 07:03 PM#3
grim001
That was a typo which I just fixed. The first one was supposed to be "OnUnitPickupPowerup."
12-03-2009, 09:03 PM#4
Anitarf
I had a comment here about private triggers being declared in the calibration section, but I see now the calibration section was meant to end before that point, maybe that should be marked more clearly.

UnitGetRandomItem "leaks" items (if they are ever removed, their handle id will not get recycled), you should use a global item array or a local integer array that stores full item slots instead of items directly, which would also save on UnitItemInSlot calls for all that's worth.

What does UnitInventorySize return for null units? If 0, then most functions like UnitGetItemOfTypeSlot don't require the safety u==null check since they'll just skip their loop anyway.

Could the UnitSwapInventorySlots function be improved with a more low-level approach? Right now it does the whole UnitPickupItemToSlot procedure twice if there are two items, woldn't it be simpler to just fill the inventory up to the higher of the two slots with dummy items once, then drop the items in the two slots (whether they're both actual items or one of them is a dummy item doesn't matter), pick them up in reverse order, remove all dummy items from the unit, then run the events for the items in the slots in the end? Right now, when the drag event occurs for the first item, the second one isn't even in the unit's inventory yet when it probably should be.

The documentation for UnitAddItemToSlot is slightly ambigous, I actually understood it the wrong way the first time I read it. I suggest the following clarification:
// Adds the specified item to the specified slot of the specified
// unit. If an item is already in the specified slot, itthat item will be moved
// to the first free slot, or dropped if the inventory is full.


If ItemDex throws out bug messages when encountering something as normal as an item with charges, that should probably be fixed in ItemDex rather than requireing fixes in ItemUtils.

That's all I've got for now, looking good otherwise.
12-04-2009, 05:41 PM#5
grim001
Quote:
Originally Posted by Anitarf
UnitGetRandomItem "leaks" items
Fixed.

Quote:
Originally Posted by Anitarf
most functions like UnitGetItemOfTypeSlot don't require the safety u==null check since they'll just skip their loop anyway.
Fixed.

Quote:
Originally Posted by Anitarf
Could the UnitSwapInventorySlots function be improved with a more low-level approach?
Fixed.

Quote:
Originally Posted by Anitarf
The documentation for UnitAddItemToSlot is slightly ambigous
Fixed.

Quote:
Originally Posted by Anitarf
If ItemDex throws out bug messages when encountering something as normal as an item with charges, that should probably be fixed in ItemDex rather than requireing fixes in ItemUtils.
Vex has been ignoring all his system threads lately. Maybe he will revive someday. But for now, it works fine.
01-01-2010, 04:08 AM#6
Sinnergy
very nice, I'll just wait for the approval, I'm just confused on some stuffs in the readme inside the script
Quote:
By the way, please don't kill any items in your maps
did you mean by triggering only? or any attempts of killing an item (unit is ordered to attack an item; a user orders his/her hero to kill an item)?

UnitGetItemOfOfType , double "of" (itemOfOf)?

Collapse JASS:
// OnUnitPickupItem(ItemFunc)
//   Runs the specified ItemFunc when a unit picks up a powerup item.
//
// OnUnitPickupItem(ItemFunc)
//   Runs the specified ItemFunc when a unit picks up an item.
The same function names?
01-01-2010, 12:51 PM#7
Rising_Dusk
The first one is supposed to be "OnUnitPickupPowerup" and it's a typo in the documentation he apparently hasn't fixed.
01-01-2010, 09:40 PM#8
grim001
I thought I fixed that... OK, I'm going to update this sometime tonight.

Quote:
Originally Posted by Sinnergy
did you mean by triggering only?
Yes, I added something to the documentation to clarify that.

Quote:
Originally Posted by Sinnergy
double "of"
Fixed.

Quote:
Originally Posted by Sinnergy
The same function names?
Fixed.

OK, this thing should be ready for final review now.
01-24-2010, 09:45 PM#9
Anitarf
GetItemSlot seems to only return -1 for an item on the ground if that item has been picked up and dropped once, otherwise it returns 0. GetItemPrevSlot also returns 0 for items that have not yet been picked up.
01-25-2010, 09:04 AM#10
Anachron
I've made something similar for my current inventory system.
However, its not released to public yet and features the same functionality with fullscreeninventories aswell.

What I miss is some (optional) bridge to UnitMaxState and BonusMod.
02-01-2010, 12:20 PM#11
grim001
Quote:
Originally Posted by Anitarf
GetItemSlot seems to only return -1 for an item on the ground if that item has been picked up and dropped once, otherwise it returns 0. GetItemPrevSlot also returns 0 for items that have not yet been picked up.
Fixed it.

Quote:
Originally Posted by Anachron
What I miss is some (optional) bridge to UnitMaxState and BonusMod.
Your idea of what this library is supposed to do is confused if you think it needs that stuff.

Once again, should be ready for someone to look it over and approve it if no issues are found.
02-01-2010, 12:42 PM#12
Anitarf
Approved.
04-04-2011, 04:59 AM#13
SanKakU
i guess it might be useful...but can't you make some of the used libraries optional? i don't know what hiding items is for...
09-20-2011, 05:04 PM#14
Anitarf
I have a minor bug to report. The bug occurs when swapping items if the dragged item is removed in one of the event responses. Removing the dragged item cancels the swap, which means that the draggedto item remains where it was while ItemUtils thinks it is in a different slot now. This causes all sorts of errors in future drag events and elsewhere.

Until this is fixed, users shouldn't remove the dragged item in any OnUnitDragItem and OnUnitSwapItems event responses. Removing the draggedto item is safe, though, since that doesn't appear to cancel the item swap.


Edit:

The same error also occurs if a new item is put into an empty slot to which an item is being dragged. The bug also occurs if the dragged item is dropped from the inventory, not only if it's removed.

Another issue I noticed is that ItemUtils ignores drag events where the item is dropped onto itself, however detecting these events could be useful for some things like item stack splitting. Either drag events should run even when an item is dragged to the same slot or a separate event should be provided for such occasions.


Edit 2:

There is a bug in the event handling code:
Expand JASS:
If you're looping backwards, then the initial value of n should be $NAME$funcs_n. However, ideally the functions should be run in the order they were registered, so I think that this is a better correction:
Expand JASS:

Another small thing: In the OnDropItem function, perhaps the ClearItemOwnerDelayed should be called before running the events. The reason I mention this is that I use 0.0 timers on that event too (to find out where the dropped item ended up) and right now those timers expire before the ClearItemOwnerDelayed timer does, which means the item owner is not yet properly reset. Right now, when my timer expires, I have to compare the item's owner to the dropping unit rather than to null to establish that the item was dropped to the ground, which is a bit counter-intuitive. That also means that if another event function were to give the item back to the dropping unit, I'd have no way of knowing that.

This might remain a problem even if ClearItemOwnerDelayed is called before the event functions: I see nothing in the code that would ensure that if a unit reacquires an item it just dropped, the ClearItemOwnerDelayed_Sub function won't reset that item's owner anyway. The OnPickupItem function likely needs to be modified to account for that possibility.