[Need help pls] Find slowdown bug at Advanced Filters & FCOItemSaver addons

[merlight]

I tried with only backpack first (85/100 items), without marking anything with ItemSaver and all 4 icons red. Kept switching basic filters and AF subfilters for a few minutes. After a while it slowed down noticeably. Each click generated 2-6 calls to UpdateFilteredList in libFilters, causing up to 300k filter checks total (without early return optimization, which could hide the real problem, so I put it off for now).

As circonian found out, first problem is full inventory update after each register/unregister. Even when you know in advance you're going to register/unregister multiple filters, so you know the update is only needed after the last one, you have no way of telling libFilters to defer the update. This needs to be changed in libFilters. Either by a) suppress-updates parameter to register/unregister, or b) lock/unlock-for-updates functions, or c) require explicit update call. a) and b) would be backward-compatible, I like c) (it would allow changing some filter's settings/behavior without re-registering).


With a pair of greaves reserved for research, and the corresponding icon turned green, the delay with each tab switch is huge, even with "only" 50-100k filter checks, even on tabs other than apparel. I measured UpdateFilteredList and it took longer and longer, quickly went from ~20ms to >100ms per call as I was switching tabs (146/150 items in bank).


The slowdown is much bigger with Item Saver because AF filters are really simple and fast. Before I get to the real solution, you should try to optimize your FilterSavedItems* functions.
* call getSettingsIsFilterOn once per i, store the result in a local variable and use that in if/elseif conditions
* same with GetItemInstanceId, call once before the loop
* what's the purpose of SignItemId? Does the API sometimes return positive and sometimes negative value for the same item? Unless it does, get rid of it
* wrap debugMessage(..., true) calls in if (settings.deepDebug) then debugMessage(...) end; the way you have it now, you compose the whole message, which is quite expensive, only to throw it away if deepDebug is disabled.


Now to the core problem. libFilters in SetInventoryFilter chains additionalFilter. There's a comment saying additionalFilter vanishes when layout changes, which happens when you switch from Bank to Guild Store for example, but apparently it persists and gets chained after the next. This needs to be fixed, because now it creates an ever-expanding chain of functions doing the same thing. You can easily get to 50 filter calls per slot, and early return optimization only helps with items that don't pass some filter - those that pass all filters go through the whole chain.


Totally off-topic, bag slot indices start from 0:

-- replace
for slotIndex = 1, GetBagSize(bagId) do
-- with
for slotIndex = 0, GetBagSize(bagId) - 1 do

[Baertram]

Thx for checking the codes out too! I'll have a look at your comments, and the comments of Circonian too, to speed up FCOItemSaver where it is possible.
As I said I'm new to LUA and just have got it to work somehow (and was proud it worked and ppl liked it ;-) ).
Now it will be the time to recode it and speed up things I guess...

@Circonian:
The callback function for the filters (bags, bank) is FilterSavedItems(), which is called for each item in the invemtory and then calls the function getSettingsIsFilterOn() for each of the 4 possible filter icons. This is why it is called so often.
This function only checks if the "split filters for each panel" (inventory, bank, guild bank, trading, crafting, mails, etc.) setting is enabled or not, and if the relating filter is activated or not.
The information if the filter panel is activated and the item is marked with an icon is needed for each item to hide/show it.

I'll read all your comments slowly and check what you've found out about the callback functions, tables, fixes etc. Thank you so much for your input so far!!!

Originally Posted by merlight I tried with only backpack first (85/100 items), without marking anything with ItemSaver and all 4 icons red. Kept switching basic filters and AF subfilters for a few minutes. After a while it slowed down noticeably. Each click generated 2-6 calls to UpdateFilteredList in libFilters, causing up to 300k filter checks total (without early return optimization, which could hide the real problem, so I put it off for now). As circonian found out, first problem is full inventory update after each register/unregister. Even when you know in advance you're going to register/unregister multiple filters, so you know the update is only needed after the last one, you have no way of telling libFilters to defer the update. This needs to be changed in libFilters. Either by a) suppress-updates parameter to register/unregister, or b) lock/unlock-for-updates functions, or c) require explicit update call. a) and b) would be backward-compatible, I like c) (it would allow changing some filter's settings/behavior without re-registering). With a pair of greaves reserved for research, and the corresponding icon turned green, the delay with each tab switch is huge, even with "only" 50-100k filter checks, even on tabs other than apparel. I measured UpdateFilteredList and it took longer and longer, quickly went from ~20ms to >100ms per call as I was switching tabs (146/150 items in bank). The slowdown is much bigger with Item Saver because AF filters are really simple and fast. Before I get to the real solution, you should try to optimize your FilterSavedItems* functions. * call getSettingsIsFilterOn once per i, store the result in a local variable and use that in if/elseif conditions * same with GetItemInstanceId, call once before the loop * what's the purpose of SignItemId? Does the API sometimes return positive and sometimes negative value for the same item? Unless it does, get rid of it * wrap debugMessage(..., true) calls in if (settings.deepDebug) then debugMessage(...) end; the way you have it now, you compose the whole message, which is quite expensive, only to throw it away if deepDebug is disabled. Now to the core problem. libFilters in SetInventoryFilter chains additionalFilter. There's a comment saying additionalFilter vanishes when layout changes, which happens when you switch from Bank to Guild Store for example, but apparently it persists and gets chained after the next. This needs to be fixed, because now it creates an ever-expanding chain of functions doing the same thing. You can easily get to 50 filter calls per slot, and early return optimization only helps with items that don't pass some filter - those that pass all filters go through the whole chain. Totally off-topic, bag slot indices start from 0: -- replace for slotIndex = 1, GetBagSize(bagId) do -- with for slotIndex = 0, GetBagSize(bagId) - 1 do

[circonian]

While I'm thinking of it:

-- You register for this:
EVENT_MANAGER:RegisterForEvent(gAddonName, EVENT_INVENTORY_SINGLE_SLOT_UPDATE, FCOItemSaver_Inv_Update)
 
-- then in it you zo_calllater this:
--scanInventory(bagId, slotId)

Then you do stuff with bagId, slotId. Three things I see here.

1) When the player moves an item two slots are updated. First the slot the item was moved from, then the slot it was moved to. This means this event fires twice. One of the times the item is no longer even in the slot. You seem to have the same problem with what if a user destroys or sells an item? the item vanishes, the slot gets updated, but the item is not in the slot anymore. It looks like your running a bunch of code on items that no longer exist. Even if your code is handling it correctly, not causing an error when the item no longer exists, I don't see any reason to run the code if the item no longer exists in that slot. You should probably have something in there like:

function FCOItemSaver_Inv_Update( eventCode, bagId, slotId, isNewItem, itemSoundCategory, updateReason )
    if GetItemType(bagId, slotId) == ITEMTYPE_NONE then return end

This will cut back on how often this code runs a lot.


2) In FCOItemSaver_Inv_Update, why is this called with a zo_calllater? This serves no purpose? Delaying the call here doesn't do anything in this function or in scanInventory() ?

zo_callLater(function()
                debugMessage("Event 'Single inventory slot updated' executed now! bagId=" .. bagId .. ", slotIndex=" .. slotId, true)
                scanInventory(bagId, slotId)
            end, 250)

My Only guess I can come up with is that I see in ScanInventory() your doing something related to researchAssistant

--Automatic marking of ornate and/or researchable items (researchAssistant addon needed!) is activated?

If you are delaying to wait for researchAssistant to do something theres better ways. If you need to be informed of when researchAssistant does something (I have no clue what that addon even does, but), lets say you need to wait for it to finish marking items that are needed for research or something...ask the author to put in a custom callback that he can fire once the addon finishes that task & then your addon will know exactly when to run this code. Instead of calling it from the slotUpdate & delaying & hoping that researchAssistant has finished its task.


3) In FCOItemSaver_Inv_Update, do you really need to run this code on items that are NOT new items??
It looks like it does nothing but call scanInventory() which in turn does nothing but save an itemId if the item is ornate or researchable

markedItems[3][itemId] = true

Well if you check it & get that itemId when the item is new (when they first get it) you don't need to ever check that item again.

Seems like this whole function could just be:

--Inventory slot gets updated function
function FCOItemSaver_Inv_Update( eventCode, bagId, slotId, isNewItem, itemSoundCategory, updateReason )
    if not isNewItem and bagId ~= BAG_BACKPACK then return end
    if  GetItemType(bagId, slotId) == ITEMTYPE_NONE then return end
    
    scanInventory(bagId, slotId)
end

4) I would change this function:

--Scan the inventory for ornate and/or researchable items
local function scanInventory(p_bagId, p_slotIndex)

First you have it performing two very similar (practically identical) tasks...meaning you have a lot of code in there twice.
I only see two main tasks being done, I would create two separate functions out of them:

--Update ornate items
CheckForOrnate(bagId, slotId, any other info needed....)  
-- and
--Update researchable items
CheckForResearchable(bagId, slotId, any other info needed....)

Then rewrite scanInventory(bagId, slotId)
so that it ONLY works with a bagId, slotId. Theres no reason to write a bunch of code to make it scan the entire inventory when the bagId/slotId is nil, & do something else when their not. Make it only work when its given a bagId, slotId and if you want to update the entire inventory like on initialize...just loop through the inventory & pass scanInventory(bagId, slotId) the bag/slot's it needs updating each bag/slot.

[circonian]

Originally Posted by Baertram The callback function for the filters (bags, bank) is FilterSavedItems(), which is called for each item in the invemtory and then calls the function getSettingsIsFilterOn() for each of the 4 possible filter icons. This is why it is called so often. This function only checks if the "split filters for each panel" (inventory, bank, guild bank, trading, crafting, mails, etc.) setting is enabled or not, and if the relating filter is activated or not. The information if the filter panel is activated and the item is marked with an icon is needed for each item to hide/show it.

one other thing I thought of is that you should check to see if code is necessary to run or not before running it....this may be wrong, I have to leave soon, I don't have time to go back and look it up. but you said the getSettingsIsFilterOn() decides if something should be shown/hidden. and its looping through all the slots being called from wherever. I remember there was that CreateMarkerControl() something loop where it was firing tons of times as well and it looked like it was calling unecessary code to resize, anchor, change color, drawTier, exc...

At the very least, even if I was mistaken and all of that ControlMarker code is necessary...its defanitely not necessary if the control isn't visible. It does not matter what color, size, drawTier, anchor, exc...until it gets shown so visibility should be checked, and then if visible, setup the control in createmarker control.
Although I'm still believing that code shouldn't be in the callback.

[merlight]

Try this modified libFilters.lua if you will: https://gist.github.com/merlight/e45...libfilters-lua

It's a deep cut into how libFilters set additionalFilters, so it might not work at some places, I only checked at bank and wood decon. There's lot of debugging garbage, but hopefully I commented out enough so you can run it without my debug window.

[Baertram]

Originally Posted by circonian one other thing I thought of is that you should check to see if code is necessary to run or not before running it....this may be wrong, I have to leave soon, I don't have time to go back and look it up. but you said the getSettingsIsFilterOn() decides if something should be shown/hidden. and its looping through all the slots being called from wherever. I remember there was that CreateMarkerControl() something loop where it was firing tons of times as well and it looked like it was calling unecessary code to resize, anchor, change color, drawTier, exc... At the very least, even if I was mistaken and all of that ControlMarker code is necessary...its defanitely not necessary if the control isn't visible. It does not matter what color, size, drawTier, anchor, exc...until it gets shown so visibility should be checked, and then if visible, setup the control in createmarker control. Although I'm still believing that code shouldn't be in the callback.

CreateMarkerControl creates the texture controls for the icons next to the inventory slots. It also changes the look of the textures. Currently it is executed in the hook function of the inventory/banks/etc. lists.
I'm not sure if there is a better way or a better function to create and update each of the texture controls.
I've taken this source code from the original ItemSaver addon some months ago as I started FCOItemSaver.

Originally Posted by circonian While I'm thinking of it: -- You register for this: EVENT_MANAGER:RegisterForEvent(gAddonName, EVENT_INVENTORY_SINGLE_SLOT_UPDATE, FCOItemSaver_Inv_Update)   -- then in it you zo_calllater this: --scanInventory(bagId, slotId) Then you do stuff with bagId, slotId. Three things I see here. 1) When the player moves an item two slots are updated. First the slot the item was moved from, then the slot it was moved to. This means this event fires twice. One of the times the item is no longer even in the slot. You seem to have the same problem with what if a user destroys or sells an item? the item vanishes, the slot gets updated, but the item is not in the slot anymore. It looks like your running a bunch of code on items that no longer exist. Even if your code is handling it correctly, not causing an error when the item no longer exists, I don't see any reason to run the code if the item no longer exists in that slot. You should probably have something in there like: function FCOItemSaver_Inv_Update( eventCode, bagId, slotId, isNewItem, itemSoundCategory, updateReason )     if GetItemType(bagId, slotId) == ITEMTYPE_NONE then return end This will cut back on how often this code runs a lot.

Sounds logically. I only had a problem once where the item was moved from inventory to bank, this is why I asked for the bagId = BAG_BACKPACK. I didn't know any better way until today. Thanks.

Originally Posted by circonian 2) In FCOItemSaver_Inv_Update, why is this called with a zo_calllater? This serves no purpose? Delaying the call here doesn't do anything in this function or in scanInventory() ? zo_callLater(function()                 debugMessage("Event 'Single inventory slot updated' executed now! bagId=" .. bagId .. ", slotIndex=" .. slotId, true)                 scanInventory(bagId, slotId)             end, 250) My Only guess I can come up with is that I see in ScanInventory() your doing something related to researchAssistant --Automatic marking of ornate and/or researchable items (researchAssistant addon needed!) is activated? If you are delaying to wait for researchAssistant to do something theres better ways. If you need to be informed of when researchAssistant does something (I have no clue what that addon even does, but), lets say you need to wait for it to finish marking items that are needed for research or something...ask the author to put in a custom callback that he can fire once the addon finishes that task & then your addon will know exactly when to run this code. Instead of calling it from the slotUpdate & delaying & hoping that researchAssistant has finished its task.

Research Assistant is a handy addon marking your items with green/red/yellow icons (to the right of the name) to show you on first sight, if the item's trait is researchable, already known by any of your characters, etc.
You are right that I tried to wait for ResearchAssistant to finish some updating routines. ResearchAssistant is writing information about each item into the control's "data.dataEntry.researchassistant" table.
If the item is researchable you will find the string "researchable" in there. So I tried to do a ZO_callLater() here to wait for ResearchAssistant to fire it's own "OnInventoryUpdate" event callback function to update "data.dataEntry.researchassistant" tables first. Doing it this way worked in 90% of the cases so I did not change it to something else
The maintainer of ResearchAssistant coded it's own library libResearch meanwhile which is used inside the addon to check if items are researchable. I could use this library too to recheck the same item and get my information this way. But this would take more resources I guess then checking the control's data table like today?

Originally Posted by circonian 3) In FCOItemSaver_Inv_Update, do you really need to run this code on items that are NOT new items?? It looks like it does nothing but call scanInventory() which in turn does nothing but save an itemId if the item is ornate or researchable markedItems[3][itemId] = true Well if you check it & get that itemId when the item is new (when they first get it) you don't need to ever check that item again. Seems like this whole function could just be: --Inventory slot gets updated function function FCOItemSaver_Inv_Update( eventCode, bagId, slotId, isNewItem, itemSoundCategory, updateReason )     if not isNewItem and bagId ~= BAG_BACKPACK then return end     if  GetItemType(bagId, slotId) == ITEMTYPE_NONE then return end         scanInventory(bagId, slotId) end

I thought the same way like you, in the beginning. If you check the code you can see the IF item is new statement still in there, only commented.
The problem was: Items are only marked with "isNewItem" if the item will get into your inventory by trading, mail or looting. But if you get it by taking it from a bank/guild bank the item weren't checked properly for ornate and&or researchability. This is why I had commented teh if (not isNewItem) then return end line out.

[Baertram]

Originally Posted by merlight Try this modified libFilters.lua if you will: https://gist.github.com/merlight/e45...libfilters-lua It's a deep cut into how libFilters set additionalFilters, so it might not work at some places, I only checked at bank and wood decon. There's lot of debugging garbage, but hopefully I commented out enough so you can run it without my debug window.

I tested it in the inventory too. For now it seems to work fast and stable, and all filters I tried worked so far.
Thanks for the coding. I'll play around with it a bit.

After testing it at the bank, inventory, crafting stations, with and without AdvancedFilters and plugins: It seems to work and speed up things for the moment very well.
Am I allowed from your side to use your libFilters version as a current workaround for FCOItemSaver, until Randactyl will release a new one (or acknowledge your version)?

[circonian]

Originally Posted by Baertram CreateMarkerControl creates the texture controls for the icons next to the inventory slots. It also changes the look of the textures. Currently it is executed in the hook function of the inventory/banks/etc. lists. I'm not sure if there is a better way or a better function to create and update each of the texture controls. I've taken this source code from the original ItemSaver addon some months ago as I started FCOItemSaver. Sounds logically. I only had a problem once where the item was moved from inventory to bank, this is why I asked for the bagId = BAG_BACKPACK. I didn't know any better way until today. Thanks.

I am out of town atm, I don't remember what your code said, but if your code originally had something like:

if bagId =~ BAG_BACKPACK then return end

Then you will probably still want that in there.

Originally Posted by Baertram Research Assistant is a handy addon marking your items with green/red/yellow icons (to the right of the name) to show you on first sight, if the item's trait is researchable, already known by any of your characters, etc. You are right that I tried to wait for ResearchAssistant to finish some updating routines. ResearchAssistant is writing information about each item into the control's "data.dataEntry.researchassistant" table. If the item is researchable you will find the string "researchable" in there. So I tried to do a ZO_callLater() here to wait for ResearchAssistant to fire it's own "OnInventoryUpdate" event callback function to update "data.dataEntry.researchassistant" tables first. Doing it this way worked in 90% of the cases so I did not change it to something else The maintainer of ResearchAssistant coded it's own library libResearch meanwhile which is used inside the addon to check if items are researchable. I could use this library too to recheck the same item and get my information this way. But this would take more resources I guess then checking the control's data table like today?

No, it would be far easier to use a library to check yourself & much better than waiting for researchAssistant. I don't know anything about libResearch, I'm sure its great, but I don't know anything about it so I can not speak towards it...but I have a library libItemInfo that tells users information about items on a single call, like is an item 1h, is it jewelry, is it researchable, does the user need it for research, exc...tons of stuff you can find out with a single call. For example to check if an item is needed for research by the user all you have to do is:

-- You do \have to include the library file in your .txt file, see instructions on my portal page for libItemInfo
 
--Put this line at the top of your code file to register the library
local LII = LibStub:GetLibrary("LibItemInfo-1.0")
 
 
-- Then Call this function & pass in either a link
LII:NeedForResearch(Link)
 
-- or a bagId & slotId, it will work either way, that's it.
LII:NeedForResearch(BagId, SlotId)

Originally Posted by Baertram I thought the same way like you, in the beginning. If you check the code you can see the IF item is new statement still in there, only commented. The problem was: Items are only marked with "isNewItem" if the item will get into your inventory by trading, mail or looting. But if you get it by taking it from a bank/guild bank the item weren't checked properly for ornate and&or researchability. This is why I had commented teh if (not isNewItem) then return end line out.

Um, I don't have the code or stuff here to look at it again with, but that sounds like there must have been something else wrong. It is true if you moved an item from a bank to your inventory it would not be new so the code would not run, but I thought I rememberd you were storing this...whatever it was your checking for here....into a table? So once it is checked and stored in the table the item never needs to be checked for that data again. Are you not gathering all of this data when the addon loads? From all inventories? THEN AFTER THAT you only need to worry about new items, because you already have all of the information about the old items.

[circonian]

Originally Posted by Baertram I tested it in the inventory too. For now it seems to work fast and stable, and all filters I tried worked so far. Thanks for the coding. I'll play around with it a bit. After testing it at the bank, inventory, crafting stations, with and without AdvancedFilters and plugins: It seems to work and speed up things for the moment very well. Am I allowed from your side to use your libFilters version as a current workaround for FCOItemSaver, until Randactyl will release a new one (or acknowledge your version)?

I don't know what changes he made, I've not looked at it, but keep in mind if you release that with your addons, depending upon the changes he made, could this mess up other addons who are using libFilters if this version of the library gets loaded first. And if it doesn't get loaded first, then it doesn't matter if you include it or not, it wont get used and it wont help your addon.

You need an official update. You need randactyl to fix it.

[Baertram]

No, it would be far easier to use a library to check yourself & much better than waiting for researchAssistant. I don't know anything about libResearch, I'm sure its great, but I don't know anything about it so I can not speak towards it...but I have a library libItemInfo that tells users information about items on a single call, like is an item 1h, is it jewelry, is it researchable, does the user need it for research, exc...tons of stuff you can find out with a single call. For example to check if an item is needed for research by the user all you have to do is:

-- You do \have to include the library file in your .txt file, see instructions on my portal page for libItemInfo
 
--Put this line at the top of your code file to register the library
local LII = LibStub:GetLibrary("LibItemInfo-1.0")
 
 
-- Then Call this function & pass in either a link
LII:NeedForResearch(Link)
 
-- or a bagId & slotId, it will work either way, that's it.
LII:NeedForResearch(BagId, SlotId)

But this won't check if the item will be researchable with any of your characters, or am I wrong?
ResearchAssistant got some pretty nice features. I'll have a look at the libResearch then and see if I can use it.

Originally Posted by circonian I don't know what changes he made, I've not looked at it, but keep in mind if you release that with your addons, depending upon the changes he made, could this mess up other addons who are using libFilters if this version of the library gets loaded first. And if it doesn't get loaded first, then it doesn't matter if you include it or not, it wont get used and it wont help your addon. You need an official update. You need randactyl to fix it.

Yep. As far as I know libFilters is only used in AdvancedFilters, Item Saver and FCOItemSaver so far. But maybe other addons use it too.

I've implemented some of the parts merlight and you have pointed me to into FCOItemSaver.
I don't find the time to recode everything for now. But this is on my todo list for the future, so it will become faster and faster ;-)
If any1 wants to test the version, including libFilters fixed by merlight, you can download it here:
https://dl.dropboxusercontent.com/u/..._0_4_0beta.zip

[merlight]

^^ pretty much this. It certainly deserves more thorough testing.

I haven't done any changes to the interface I suggested earlier. Instead of controlling updates with new parameters or functions, I reduced the amount of updates triggered from within Register/UnregisterFilter by delaying them. One can never be 100% sure, but I think this is unlikely to break other add-ons.

Second change was a much deeper cut (and much more needed). I removed SetInventoryFilter (which was the culprit, always expanding the chain of filters) and SetFilterByFragment calls from RegisterFilter. Instead of hooking additionalFilter for the proper inventory from RegisterFilter, I hook it once from InitializeLibFilters in tables where it should persist - that means backpack layout fragment's layoutData table for BACKPACK, and the inventory table itself for BANK and GUILDBANK. This change might break something in ways I can't foresee.

I didn't have time to test these changes in actual gameplay yet, and I usually prefer doing that for about a week before releasing anything but the simplest changes. Plus the only add-on with libFilters I use is AF; I installed Item Saver only because of this thread.

Ah, now I see you've put it up with big green credits and a less-big red warning... perhaps you should've done it the other way around - big green warning and less-big red credits! jk Well let's see how many users try it out and return undaunted.

[merlight]

Today I hit some new (to me at least) issues with FCO Item Saver at BLACKSMITHING station. I still have those greaves reserved for research in bank, but couldn't pick them for research when the research icon was green (with red/yellow I could). Also sometimes hovering over them in decon tab didn't show the blue highlight.

There're a few things wrong with your mouse handlers.

1) The biggest evil is if control:GetHandler(e) != myHandler then PreHook(e, control, myHandler) end
Someone else might hook the handler after you, and you'll end up hooking it again. If you need to keep track of what you already hooked, have an alreadyHooked[control][eventName] map. You can use the control object directly as the key, no need to GetName().

1a) After you will have fixed this, you can get rid of eventHandlers table and GetEventHandler/SetEventHandler/PreHookHandler completely.
Use ZO_PreHookHandler(control, handlerName, hookFunction) instead.

2) You're hooking "OnMouseEnter" on whole lists (e.g. DECONSTRUCTION_BAG) and there you hook OnMouseEnter of it's children. I know what led you there - rows are created later as needed - but the correct solution is to do it in dataType.setupCallback (checking alreadyHooked) or dataType.pool.m_Factory.

3) FCOItemSaver_InventoryItem_OnMouseEnter doesn't call the original handler if g_noOnMouseEvents ~= false.
Just saying, this bug will go away if you do point 1a

[Randactyl]

Hello everyone!

I've spent the morning sifting through all of this and I think the best path forward is to use the version of libFilters merlight came up with and modified by Baertram to include the second enchanting filter.

Since there are not very many users of this library (only myself and Baertram to my knowledge) I'm going to go ahead and release it as 1.0r10. I've read over the code a few times and there isn't anything that jumps out at me that would cause an issue. If we do find a problem, it can be easily fixed

[Randactyl]

Additionally:

I just put up the new version of libFilters. I'm going through AF in order to familiarize myself with the debug outputs that have been added, but I'm curious to see if either Baertram or circonian can drop the new version of libFilters into their already working debug edits of AF and see if the problem still exists? I'm working toward this myself too, would just be speedier results if one of you were available to test as well

[merlight]

Originally Posted by Randactyl I just put up the new version of libFilters. I'm going through AF in order to familiarize myself with the debug outputs that have been added,

I'd better clean that up, some of it are hacks I used to identify hooks by calling them via /script with no args.

Regarding LAF_ENCHANTING2, perhaps it would deserve a better name. Or rename the first mode to LAF_ENCHANTING_EXTRACTION. And apply both in a single AddItemData hook.

[Randactyl]

Originally Posted by merlight I'd better clean that up, some of it are hacks I used to identify hooks by calling them via /script with no args. Regarding LAF_ENCHANTING2, perhaps it would deserve a better name. Or rename the first mode to LAF_ENCHANTING_EXTRACTION. And apply both in a single AddItemData hook.

If you could, that'd be great!

Yes, those constants need more descriptive names as you suggest. I was planning on revisiting it once all the fires are out, as libFilter's description page could probably use a refresh as well.

[merlight]

Found another issue with the new libFilters - Advanced Filters don't work at all at vendor Sell tab, mail Send tab, and probably other places. Reason is there are different additionalFilters applied to backpack contents depending on where you access it from : MENU_BAR, BANK (deposit), STORE (vendor sell), TRADING_HOUSE (guild store sell), MAIL (send), PLAYER_TRADE. For example PLAYER_TRADE must hide bound items, that's what additionalFilter does.

Previously whenever you called libFilters:RegisterFilter(x, LAF_BAGS, y), it hooked PLAYER_INVENTORY.inventories[INVENTORY_BACKPACK].additionalFilter, no matter where you were. In my version this no longer happens, additionalFilter is assigned from a backpack layout fragment when that fragment is shown. So when you go to an NPC vendor, BACKPACK_STORE_LAYOUT_FRAGMENT is shown and only LAF_STORE filters applied.

There are 2 possible solutions:
a) either include LAF_BAGS filters in all other backpack filters, maintaining backward compatibility, or
b) change Advanced Filters to register LAF_STORE, LAF_MAIL, LAF_TRADE in addition to LAF_BAGS.

[Randactyl]

Yeah I noticed that as well. I'm working right now on making Advanced Filters behave responsibly - I think making it use the correct filters for the correct layouts is the way to go.

Edit: actually, it's really the "same inventory" in any of those contextual fragments just with special flavor like a label saying "Sell" instead of "Inventory" and the buttons to navigate through that fragment, right?

Kinda just thinking out loud here:

So then we would want to include all of the LAF_BAGS filters by default? (at least for Advanced Filters, anyway)

I'm trying to think of when I would want different filters in Sell vs. Mail vs. Inventory.

I don't think it would be good to always register/unregister filters for every scene type.
Option B could involve something like me choosing the correct LAF by hooking the display of a scene just like libFilters uses to determine which LAF to apply.
Option A sounds to be the easiest, but I have a gut feeling that is not the best thing to do.

I'm hesitant to dive into these changes because I'm trying to figure out which would be the best option performance wise since that is what all of this discussion was borne out of. I just don't know enough about performance impacts of the different options to make a call out of the gate.

[Baertram]

Originally Posted by merlight Today I hit some new (to me at least) issues with FCO Item Saver at BLACKSMITHING station. I still have those greaves reserved for research in bank, but couldn't pick them for research when the research icon was green (with red/yellow I could).

Hey merlight,

this behaviour is wanted:

The "research icon" at the bottom of your inventory is not related to researching at crafting station only! I created this filter for marking researchable items, that's right. But you can even change this icon in the settings. It only relates to the appropriate marked items.
Marked items with research icon (or let's say we changed it to a "duck" symbol) -> Will be filtered by clicking the icon at the bottom of inventories showing the same symbol ("research" or "duck").

Researching marked items is not possible if they are marked with any of the icons, as long as you:
a) won't enable the setting "Allow marked items for research"
b) if this setting a) is enabled and the item is marked, you need to disable the appropriate filter (yellow or red state) at the deconstruction panel inventory bottom (by pressing the appropriate filter button icon)
If a) and b) apply you will be able to select the item at the research popup.

It was implemented this way because ppl wanted a save way to secure the marked items. Only filtering them at deconstruction would allow to research, and deconstruct them this way.
So I decided to block the marked items totally and allow the players to set the wished behaviour in the settings.
Easiest way (for me) to allow researching of marked items:
-Enable setting "Allow researching of marked items"
-if you don't have enabled the anti-deconstruction settings, which prevents marked items to be selected (see below about your other "bug"):
Enable the filters at deconstruction panels too (to avoid deconstruction of items)
-At the research popup right-click the item you wish to research and disable the mark
-Now the item will get green and will be selectable for researching

Originally Posted by merlight Also sometimes hovering over them in decon tab didn't show the blue highlight.

This is wished too. At least the following things should apply:
-If the setting "Anti-Deconstruction" is enabled the marked items at the deconstruction panels won't be selectable by mouse (this is why the blue frame is missing).
-The right click menu for those items won't show the "Add to deconstruction slot" entry any more
-Mouse Drag&drop to deconstruction slot won't work anymore
-Keybindings to add the item to the deconstruction slot won't work anymore
If an item won#t be deconstructable, because the setting is enabled and the item is marked, the chat will give you a feedback entry too.

This applies to "Anti-destroy", "Anti-maiL" and "anti-trade" the same way, if the appropriate settings are enabled.

The way it is working now will allow players using a gamepad (which emulates the keybindings on button press) to save the items as well. I tried several ways to get only the keybindings disappear, but this will f**k up most of the keybindings and related in strange behaviours.
So the current solution will work pretty well for mouse, keyboard AND gamepads.

If you want to disable this anti-destruction behaviour you can do this in the settings (at the bottom), or just unmark the item by right-click menu.


I copied the pre-hook functions from another addon that was trying to disable the keybindings etc. (see the discalimer info by "saykoaddon" inside my addon source at line 4166).
I'll check if changing the pre-hook methods to your solution will still work they way it is intended.
The biggest problem was to make the keybindings not work anymore on mouse enter of marked items, but make them work again for all other non-marked items.
This was quite difficult as the keybindings must work for ALL other panels, and even needs to be checked for events like mouseenter, mouseexit, mousewheel, mousedown, etc.

[Baertram]

Just my thoughts about it, without having feedback for the performance differences between option a) and b) :

I'd choose option B) to only register the filters needed at the current panel (vendor, trade, mail, etc.).
You can see if the current panel is opened by checking the following constants (not a complete list):

local BACKPACK                  = ZO_PlayerInventoryBackpack
local BANK                      = ZO_PlayerBankBackpack
local GUILD_BANK                = ZO_GuildBankBackpack
local DECONSTRUCTION            = ZO_SmithingTopLevelDeconstructionPanelInventoryBackpack
local IMPROVEMENT               = ZO_SmithingTopLevelImprovementPanelInventoryBackpack
local MAIL_SEND                 = ZO_MailSend
local PLAYER_TRADING            = ZO_Trade
local ENCHANTING_STATION        = ZO_EnchantingTopLevelInventoryBackpack
local VENDOR_SELL             = ZO_StoreWindowListSellToVendorArea

e.g. if you click the button, or change the dropdown of AdvancedFilters, check if one of the ZO_ variables is currently visible:

--See if we are sending a mail
    if (not MAIL_SEND:IsHidden()) then
        -- register additional filter for LAF_MAIL
       elseif (not PLAYER_TRADING:IsHidden()) then 
        -- register additional filter for LAF_TRADE
       ...
      end

Avoid registering the filters twice by checking if the filter is already registered libfilters:IsFilterRegistered(filterId) in advance (btw: wouldn't this even make sense to be added inside the libfilters:registerFilter function by default?)
I'm not sure if the button's OnClicked or the dropdown's OnSelectItem is the best position to check this.

I bet this is less performance consumting then always enabling the LAF_BAGS filters too, which would result in another check (callback function for the filter) for all inventory items.

Originally Posted by Randactyl Yeah I noticed that as well. I'm working right now on making Advanced Filters behave responsibly - I think making it use the correct filters for the correct layouts is the way to go. Edit: actually, it's really the "same inventory" in any of those contextual fragments just with special flavor like a label saying "Sell" instead of "Inventory" and the buttons to navigate through that fragment, right? Kinda just thinking out loud here: So then we would want to include all of the LAF_BAGS filters by default? (at least for Advanced Filters, anyway) I'm trying to think of when I would want different filters in Sell vs. Mail vs. Inventory. I don't think it would be good to always register/unregister filters for every scene type. Option B could involve something like me choosing the correct LAF by hooking the display of a scene just like libFilters uses to determine which LAF to apply. Option A sounds to be the easiest, but I have a gut feeling that is not the best thing to do. I'm hesitant to dive into these changes because I'm trying to figure out which would be the best option performance wise since that is what all of this discussion was borne out of. I just don't know enough about performance impacts of the different options to make a call out of the gate.