ESOUI

ESOUI (https://www.esoui.com/forums/index.php)
-   AddOn Help/Support (https://www.esoui.com/forums/forumdisplay.php?f=164)
-   -   [Need help pls] Find slowdown bug at Advanced Filters & FCOItemSaver addons (https://www.esoui.com/forums/showthread.php?t=2461)

Baertram 11/23/14 06:29 AM

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

I'm the developer of FCOItemSaver and need some help please.

About my addon:
FCOItemSaver is an addon which allows you to mark items in your inventories/banks.
These marked items will show in the inventories at the left to the item's name (e.g. a red lock or a yellow coin, or both at the same icon):


You are able to filter these marked items by buttons at the bottom of the inventories. These buttons look like the icon itself and change their state and color if you click them (filter enabled: hide all marked items=green, Filter enabled and only show marked: show only marked items=yellow, Filter disabled: show all items including marked ones=red):


So you are able to hide items you have marked to prevent them from beeing deconstructed or selling them (if filter is enabled, set to green):


Used library:
ESO is able to use additional filters for the list dialogs (inventories e.g.) to hide/show items.
This is how your inventory tab "Junk" only shows items marked as junk e.g.!

The addon AdvancedFilters and FCOItemSaver both use this library: "libFilters"
libFilters registers custom made filters to the additional filters.

This library will queue all filters, coming from different addons (e.g. AdvancedFilters + AF Plugins, ItemSaver, FCOItemSaver, and maybe others), and enable/disable them at the different panels (inventory, banks, stores, mail, trading to players, crafting stations, enchanting station, guild store).
AdvancedFilters even uses some subfilters (like the different weapon, armor, material types, etc.).


Problem:
After the last update to the game (v1.5), and some following changes to the addon Advanced Filters, my addon FCOItemSaver will crash the game, or at least slow it really down!

If you have opened the bank/guild bank, at least one item is marked with an icon, and at least one of the filters is enabled (green), and you change the AdvancedFilters tabs then (from "All" to "Weapons", or from "Weapons" to "Armor", or from "Armor" to "Materials", etc.)

the game will slow down or crash.

The more often you change the tabs the slower the client becomes.
-> If you use Wykkyd's information bar you can watch the FPS drop to below 10 and the ping increase up to several seconds o0.

I tried to recode the registering and unregistering of the filters but all my changes do not solve the slow down problem :(

So I'm searching for some experienced LUA developer who can review the code of FCOItemSaver, AdvancedFilters & libFilters to see why the game slows down all of sudden!
Thank you very much in advance for your help!


How to rebuild the problem:
1. Download and install AdvancedFilters addon: http://www.esoui.com/downloads/info2...edFilters.html
2. Download my current version of addon FCOItemSaver from dropbox (unreleased beta with recode of registering/unregistering filters): https://dl.dropboxusercontent.com/u/..._3_1b_beta.zip
3.Delete/rename "FcoItemSaver.lua" file from "SavedVariables" if you have used my addon before!
3. Play the game with standard settings of FCOItemSaver
4. Go to the bank or guild bank and open the bank panel
5. Mark at least one item with any icon (e.g. the "lock" icon) by right clicking the item in your bank/inventory and choosing the context (e.g. "Lock", "Add to gear 1", or "Mark for research")
6. Enable the appropriate filter (lock icon = lock filter, helmet icon = helmet filter, research icon=reserach filter, coin icon=coin filter) at the bottom of the bank inventory, by clicking on the button.
The filter button at the bottom of the inventory must be "green"!
The chat will give you feedback about the filter state too.
7. Change AdvancedFilters tabs a bit (from "AlL" to "Weapons" to "Armor" back to "AlL" and then to "Materials" etc.

The game client should slow down very fast and each change of AdvancedFilters tabs should slow it down even more.
This will only happen if FCOItemSaver is enabled. If you disable the addon and do a /reloadui changing the tabs at AdvancedFilters will work very quick and stable.

Information on error finding:
If you click on the filter buttons at the bottom of the inventories the function "doFilter()" will be called to change the filter state and unregister (function unregisterAllFilters()) the previous filter (if it was set) + registering (function registerAllFilters()) the new filter.
It depends on the panel (p_FilterPanelId) where you are (deconstruction, bank, inventory, mail, etc.) and the button you've clicked (filterId).

The unique filter's name, used to register the filter with library "libFilters" will be depending on the panel where you currently are:

Possible filterIds:
1: Left button at the bottom (Standard: lock icon)
2: 2nd button from left at the bottom (Standard: helmet icon)
3: 3rd button from left at the bottom (Standard: research icon)
4: 4th button from left at the bottom (Standard: coins icon)

Possible p_FilterPanelIds:
INV_BAGS = 1
INV_BANK = 2
INV_GUILDBANK = 3
INV_STORE = 4
INV_DECONSTRUCTION = 5
INV_GUILDSTORE = 6
INV_MAIL = 7
INV_TRADE = 8
INV_ENCHANTING = 9

Unique filter names:
Player bank filter: "FCOItemSaver_PlayerBankFilterNew<p_FilterPanelId>_<filterId>"
Guild bank filter: "FCOItemSaver_GuildBankFilterNew<p_FilterPanelId>_<filterId>"
Deconstruction filter: "FCOItemSaver_DeconstructionFilterNew<p_FilterPanelId>_<filterId>"
Guild store filter: "FCOItemSaver_GuildStoreFilterNew<p_FilterPanelId>_<filterId>"
Vendor filter: "FCOItemSaver_FilterNew<p_FilterPanelId>_<filterId>"
...

Error searching:
I tracked it down to the "libfilters:RegisterFilter()" function somehow.
Unregistering the filters works quick and without problems.
But (re)registering the filters will slow everything down.
I wasn't able to see where exactly the slowdown happens.

It seems to happen if you have at least one of the FCOItemSaver filters registered and then change the AdvancedFilters tab -> The given filters stay registered and all the new AdvancedFilters filters (e.g. "Weapons") + subfilters (e.g. "Axe", "Sword", etc.) get registered.

See function libFilters:RegisterFilter -> arrays idToFilter and thisFilter ->
if filterType == LAF_BANK -> SetInventoryFilter(LAF_BANK) -> filtertype = LAF_BANK -> inventorytype = INVENTORY_BANK -> currentFilter holds the registered filters (including FCOItemSaver filter) -> new filters will be added in callback function for PLAYER_INVENTORY.inventories[inventoryType].additionalFilter afterwards, respecting the currentFilters too:
Lua Code:
  1. --handle already existing filters
  2.             if(currentFilter) then
  3.                 result = result and currentFilter(slot)
  4.             end

I hope someone of you can help me fix this bug so ppl can use my addon again without any problems.
Thank you very much!

merlight 11/23/14 07:24 AM

This looks so interesting that I can't resist ;)

The first thing I noticed is that libFilters has a few loops like this:
Lua Code:
  1. function(slot)                              
  2.             local result = true                    
  3.             for _,v in pairs(filters[filterType]) do
  4.                 if(v) then                          
  5.                     result = result and v(slot)    
  6.                 end                                
  7.             end                                    
  8.             return result                          
  9.         end

These should do an early return, once any of the filters returns false, the result is final. I will send changes to Randactyl later; but this little thing can only cure some symptoms, not the disease (if the disease is filters accumulating in the list). So I guess I'll need to dig deeper ;)

Baertram 11/23/14 07:34 AM

I hoped so @interesting :rolleyes:
Thx man! I hope it is not any small bug or typo which causes this issue...
Every help is very appreciated!

Even small findings can improve the overall performance :D
If I understand you right:
If any of the registered filters for the current filterType (e.g. LAF_BANK) returns false the other filters shouldn't be executed/needed anymore, because the item will be hidden.
So one could abort the for ... do loop already in between?

Edit:
I've added a chat output in libFilters function SetInventoryFilter() to see which filters are registered upon loading of the game (Advanced filters & FCOItemSaver enabled) and when you click on FCOItemSaver filter button or any button in your inventory (weapons, armory, materials, etc.).
I also added a counter variable to libfilters which increases by 1 in the function SetInventoryFilter() at this line:
Code:

if(v) then
                        cnt = cnt + 1


Upon loading of the game (checked via Bugeater addon to see extra chat output before addons are loaded finally) I have a list of 24518 times filter "AdvancedFilters_Dropdown_Filter". This looks a bit much for me?


Same if you click on one of the AdvancedFilters buttons, e.g. the "Weapons". The filter list will explode with another 51 entries adding the filter "AdvancedFilters_Button_Filter".

If I click through the other Advanced Filters tabs the dropdown filter will be added (if I choose only "Bows" for example).

After clicking around the filter counter increased to 33492 in total.
Only changing from quest items to junk view added 884 new entries...

Maybe your fix about the "false" filter results will fix a bit of these, but aren't these numbers too high in total? Or am I missing something why this values are that high? Is the function SetInventoryFilter() executed for every item in the bags and that multiple times? I thought it is only for the bags and only if a filter is registered (upon change of a given filter).

circonian 11/23/14 06:07 PM

Quote:

Originally Posted by Baertram (Post 13421)
I hoped so @interesting :rolleyes:
Thx man! I hope it is not any small bug or typo which causes this issue...
Every help is very appreciated!

Even small findings can improve the overall performance :D
If I understand you right:
If any of the registered filters for the current filterType (e.g. LAF_BANK) returns false the other filters shouldn't be executed/needed anymore, because the item will be hidden.
So one could abort the for ... do loop already in between?

Edit:
I've added a chat output in libFilters function SetInventoryFilter() to see which filters are registered upon loading of the game (Advanced filters & FCOItemSaver enabled) and when you click on FCOItemSaver filter button or any button in your inventory (weapons, armory, materials, etc.).
I also added a counter variable to libfilters which increases by 1 in the function SetInventoryFilter() at this line:
Code:

if(v) then
                        cnt = cnt + 1


Upon loading of the game (checked via Bugeater addon to see extra chat output before addons are loaded finally) I have a list of 24518 times filter "AdvancedFilters_Dropdown_Filter". This looks a bit much for me?


Same if you click on one of the AdvancedFilters buttons, e.g. the "Weapons". The filter list will explode with another 51 entries adding the filter "AdvancedFilters_Button_Filter".

If I click through the other Advanced Filters tabs the dropdown filter will be added (if I choose only "Bows" for example).

After clicking around the filter counter increased to 33492 in total.
Only changing from quest items to junk view added 884 new entries...

Maybe your fix about the "false" filter results will fix a bit of these, but aren't these numbers too high in total? Or am I missing something why this values are that high? Is the function SetInventoryFilter() executed for every item in the bags and that multiple times? I thought it is only for the bags and only if a filter is registered (upon change of a given filter).

I wrote something just like advanced filters but I didnt do any of the filtering like libFilters does. I'm trying to figure it all out. I do see one thing so far though:
Lua Code:
  1. -- in libfilters.lua
  2. -- this should be local
  3. filters = libFilters.filters

circonian 11/23/14 07:33 PM

I don't have much time & I am leaving for thanksgiving soon and will be gone for a week. I didn't find anything else wrong, but if all else fails...this may or may not be a better way to do it, just giving options.
I know this is not what your asking, but you could just filter the items yourself, eliminating libFitlers. I haven't used itemSaver or Advanced Filters so this may or may not cover everything you need to do in your addons.

Creating filters is not very hard, it can be done with 1 function & 1 line of code to change the filter. This is how the game does its filtering, I did it in an addon. The only problem I see is that since Advanced Filters is using libFilter & you would not be, Advanced filters might not filter your stuff correctly...but that could easily be solved. I wrote my own filter library (um never released it though wasn't really designed to track stuff for other addons also, maybe I will change that and release it when I get back next week).

Anyhow if your interested or for anyone else reading this creating filters is pretty easy. All you have to do is create a tabFilter & put it into the inventories tabFilter table, then call that inventories ChangeFilter(tabData) passing in your tabFitler....thats it.
Note: this has some extra code you would not need since you do not need a physical button...but advanced filters would so although some of this code (namely all of the parameters) are not used, I left some of the code from my addon in there to give everyone ideas on what you could do with it.

Warning: Spoiler


The default, built in, filterTypes that you can call are:
Lua Code:
  1. ITEMFILTERTYPE_ALL
  2. ITEMFILTERTYPE_WEAPONS
  3. ITEMFILTERTYPE_ARMOR
  4. ITEMFILTERTYPE_CONSUMABLE
  5. ITEMFILTERTYPE_CRAFTING
  6. ITEMFILTERTYPE_MISCELLANEOUS
  7. ITEMFILTERTYPE_JUNK

If you want to create your own filters to only display certain items all you have to do now is add a UNIQUE filter number to the items filterData table (found in /zgoo the item, goto dataEntry, data, filterData).
So say you create your own filterType:
Lua Code:
  1. local ITEMFILTERTYPE_FCOLOCKED = 137  -- whatever unique number
Then just place that number in the locked items filterData table, create a filterTab out of it, put the filterTab into the inventories tabFilters table, then when your ready call this (passing in your tabData filter)
Lua Code:
  1. PLAYER_INVENTORY:ChangeFilter(tabData)

You could create your own buttons to change the filters, add buttons to the inventory menu bar to do it, or hook the inventory menu bar buttons to change what they actualy show, whatever...

You would just have to handle the slotUpdate to handle when items are moved, sold, destroyed, looted, exc..to update its filterData values.
Oh and on initializing loop putting the filterData values into the tables too so they are there when the player logs in or /reloadui.

P.S. do be aware that bank inventory slots are not populated until the user opens the bank so you could not insert filterData until its populated..although you can do this to fix that problem:
Lua Code:
  1. PLAYER_INVENTORY:RefreshAllInventorySlots(iInventory)
Will refresh the slots, use it on the bank for example then the items (slots) will become available even before the user opens the bank.

So when you originally populate the slots on load you could do something like this:
Lua Code:
  1. local tSlots = PLAYER_INVENTORY.inventories[iInventory].slots
  2.     if not tSlot then
  3.         PLAYER_INVENTORY:RefreshAllInventorySlots(iInventory)
  4.         tSlots   = PLAYER_INVENTORY.inventories[iInventory].slots
  5.     end

circonian 11/23/14 11:44 PM

Ok, couldn't sleep so had some more time to look at it, here is "A" problem I see. This seems to be the one causing the most lag.
A zillion edits, sorry I hate type-o's and grammar mistakes and I make tons of them.

One of the problems is with libFilters

In this click of a single inventory tab libFilters runs all of this:

Side note: Edit: See my next post about Advanced Filters on this problem. I didn't dig this deep into it, there may be a reason, but you might also want to look at: Why is libFilters Unregistering AdvancedFilters_Dropdown_fitler twice in a row, then registering it, then unregistering it again, then registering it again ? It also shows the AdvancedFilters_Button_Filter being unregistered twice in a row, then reregistered?

All of that is in a single click of an inventory tab and those are supposed to be unique filter ID's are they not? so it has to be the same thing its unregistering & registering multiple times correct?
The main problem is updating the filter list after every single register & unregister event. It processes every single slot like 20-30 ?more? times (I got tired of counting them) in the click of a single button. If even more addons were using this or you guys were registering multiple (more) filters it would probably crash users instantly.
I'm not sure why everything is registered & unregistered all of the time....why aren't filters just registered, left in a table & called when needed...
Anyhow It can be fixed doing this (not saying its ideal, I'm no expert here)

The main idea would be to just zo_calllater the UpdateFilteredList() and put a lock on it..
So set a flag, I called it updateSet (as in the update is set to process), when its true a zo_calllater has already been made for the UpdateFilteredList() to run after however long.
This gives all filters a chance to register/unregister before it runs. So that UpdateFilteredList() only needs to run once.


First make a new local variable for our lock flag in libFilters:
Lua Code:
  1. local updateSet = false
That tells us whether or not an UpdateFilteredList(..) has already been called with a zo_calllater (to run sometime in the future). This way if the lock flag is already set, we wont keep calling UpdateFilteredList() over & over for no reason. You probably want to adjust the time, i put 100ms in there just as a random number to test it.


Changes: function libFilters:RegisterFilter( filterId, filterType, filterCallback )
Warning: Spoiler



Changes: function libFilters:UnregisterFilter( filterId )
Warning: Spoiler


** Do note I would make sure you call
Lua Code:
  1. -- This line
  2. updateSet = false
  3. -- before calling this line
  4. UpdateFilteredList(filterType)
This way if something has already called RegisterFilter or UnRegisterFilter while the UpdateFilteredList is running...the new register/unregister event will have a chance to call another zo_calllater'd UpdateFilteredList because it will miss the....UpdateFilteredList that is already in progress.

If you called updateSet = false after calling UpdateFilteredList() something could slip through the cracks.
wow...I hope that made sense :P

P.S. And don't forget, this should be local !
Lua Code:
  1. -- in libfilters.lua
  2. -- this should be local
  3. filters = libFilters.filters

circonian 11/24/14 01:14 AM

Advanced Filters
 
In advanced Filters I see this:

On a single click of an inventory button:
Oops, my picture is missing the first 3 lines:

Unregistering a filter: AdvancedFilters_Button_Filter
Unregistering a filter: AdvancedFilters_Dropdown_Filter
ResetToAll


* thats just d("xxxxx") code I added to AF

1) So ChangeFilter( self, filterTab ) gets called which calls
2) ResetToAll() gets called, which unregisters the AdvancedFilters_Dropdown_FIlter, and the AdvancedFilters_Button_Filter
That seems ok...

3) But then ResetToAll() selects the first item in the dropdown...which in turn fires:
4) OnDropDownSelect (through the function assigned to the dropdown items) which calls SetUpCallbackFilter, which attempts to unregister the AdvancedFilters_Dropdown_Filter again and then reregisters the AdvancedFilters_Dropdown_FIlter,

5) Then the OnClickedCallback fires, which again selects the first item in the dropdown...which starts the cycle all over:
6) Fires OnDropdownSelect, SetUpCallbackFilter, which unregisteres & reregisters AdvancedFilters_Dropdown_FIlter, filters again.

7) And then at the end of the OnClickedCallback...it has its own code to manually fire: SetUpCallbackFilter again...
Although this one might look like it is ok, its for the button this time, not the Dropdown_Filter BUT...it does end up attempting to Unregister the AdvancedFilters_Button_Filter again, which we already did at the very beginning in ChangeFilter(...), then it reregisters the button filter.

Aren't these filterID's UNIQUE...why are they being unregistered at all, if you just re-register the exact same filter ?
Either way the exact same filter is getting unregistered & reregistered multiple times as we can see, unless I am missing something if the filterID's are not unique or being changed in the code somewhere & I missed it.


If you can get libFilter fixed so it doesn't call UpdateFilterList on every Register/Unregister, and you straighten out this sequence of code events in AdvancedFilters so it doesn't select the first item in the dropdown multple times and stop it from firing extra register/unregister events I think the two will solve all your problems.

circonian 11/24/14 02:07 AM

Quote:

Originally Posted by Baertram (Post 13421)
I hoped so @interesting :rolleyes:
Thx man! I hope it is not any small bug or typo which causes this issue...
Every help is very appreciated!

Even small findings can improve the overall performance :D
If I understand you right:
If any of the registered filters for the current filterType (e.g. LAF_BANK) returns false the other filters shouldn't be executed/needed anymore, because the item will be hidden.
So one could abort the for ... do loop already in between?

Yes, since your always doing:
Lua Code:
  1. result = result and <something else>
if result is already false, its always going to return false, regardless of what <something else> is.


Someone please correct me if I'm misunderstanding this part, but isn't the if statement unnecessary?
Lua Code:
  1. for _,v in pairs(filters[filterType]) do
  2.    if(v) then
  3.       result = result and v(slot)
  4.    end
  5. end

Doesn't v just represent functions in the table that tell if something should be displayed or not...since v is not a boolean (correct?) then wont the: if(v) always return true. If v did not exist it never would have made it to that part of the code, it would have broken out of the for loop before that.

So then couldn't he just do:
Lua Code:
  1. for _,v in pairs(filters[filterType]) do
  2.    result = result and v(slot)
  3.    if result == false then
  4.       return result
  5.    end
  6. end

circonian 11/24/14 02:23 AM

Anyways
 
So anyways, I hope that helps you guys out some. Goodluck getting it all straightened out !

Baertram 11/24/14 10:54 AM

Thank you very much for all your analysis Circonian!!!

Quote:

I didn't dig this deep into it, there may be a reason, but you might also want to look at: Why is libFilters Unregistering AdvancedFilters_Dropdown_fitler twice in a row, then registering it, then unregistering it again, then registering it again ? It also shows the AdvancedFilters_Button_Filter being unregistered twice in a row, then reregistered?

All of that is in a single click of an inventory tab and those are supposed to be unique filter ID's are they not? so it has to be the same thing its unregistering & registering multiple times correct?
This is EXACTLY what I've written in my post before your posts!
libFilters does unregister and reregister the filters about 30000! times in total, if you just switch around between some of the invetory tabs and subtabs of AdvancedFilters.

After your analysis of the AdvancedFilters functions for the dropdown boxes and the callback functions this seems to clear things up, why the number is that high.

Quote:

Aren't these filterID's UNIQUE...why are they being unregistered at all, if you just re-register the exact same filter ?
Either way the exact same filter is getting unregistered & reregistered multiple times as we can see, unless I am missing something if the filterID's are not unique or being changed in the code somewhere & I missed it.
Yes the filter IDs are qunique. But they are used with different content and the same ID.
Example: They get unregistered to remove let's say the "armor types" dropdown filter and reregister teh same filter ID with the "weapon types" dropdown filter, if you change from the inventory tab "Armor" to "Weapons". At least this is what I understand AdvancedFilters does.
If you do not unregister the old filter the callback function will try to hide "armor types" from the "weapons" tab?!

But you are right with the question: Why is it happening so often?


Quote:

Doesn't v just represent functions in the table that tell if something should be displayed or not...since v is not a boolean (correct?) then wont the: if(v) always return true. If v did not exist it never would have made it to that part of the code, it would have broken out of the for loop before that.
Yeah, you are right.

Quote:

Creating filters is not very hard, it can be done with 1 function & 1 line of code to change the filter.
I know. I have coded everything without libFilters in some older versions of FCOItemSaver. I've changed to use libFilters as the library was stable and AdvancedFilters has used it, because then the filters registered will work all together between different addons. And this is what I want. I don't want to recode AdvancedFilters :-) But thx for the information.


Result
I think the bug of the slowdown comes from AdvancedFilters (dropdown change functions) + libFilters (refresh of inventories after each filter registered, where the same filters get registered a thousand times upon one inventory tab change) then.

Bug fixing
I hope the addon author of libFilters and AdvancedFilters can take a look at Circonian's research and fix the un- & reregistering + needles inventory updating in those 2 addons.
-> Maybe we can talk about the changed enchanting filter in libFilters at the same time :)

I'll try to find a "workaround" solution for the inventory updates too which I can implement to fix things "roughly" for now.

Question
I do not understand why the errors in AdvancdFilters and libFilters, explained by Circonian above, don't slow down the game generally?

Example:
If you simply use AdvancedFilters addon, maybe together with some of the plugin addons, the changing of inventory tabs is fast and smooth. Even here the unregistering and reregistering of filters take place about thousands of times and the inventory is updated many times.

But at the moment you enable FCOItemSaver addon too, together with AdvancedFilters, the game gets sloooooooooow.
Is this only because of the updating of inventory contents after each filter reregistering, and because FCOItemSaver uses extra textures inside the inventories to show the marked items?

I'm wondering why the game does not slow down markable if you simply use AdvancedFilters without FCOItemSaver?


To All:
Thank you very very much for all the research into this problem so far!

Baertram 11/24/14 01:24 PM

I tried this zo_callLater workaround with 500ms. It is annoying as the filters won#t update after the click, but half a second later.
It seems to bring a way better performance as you switch tabs in inventories.
But only as long as you do not enable all FCOItemSaver filters (4 possible) in the inventories.
If all 4 filters are enabled each click will last for about 3 seconds again before something happens :(

So using the updateFlag variable is not a working workaround alone. It seems the behaviour of AdvancedFilters needs to be fixed too.



Quote:

Originally Posted by circonian (Post 13436)
Ok, couldn't sleep so had some more time to look at it, here is "A" problem I see. This seems to be the one causing the most lag.
A zillion edits, sorry I hate type-o's and grammar mistakes and I make tons of them.

The problem is with libFilters

In this click of a single inventory tab libFilters runs all of this:

Side note: Edit: See my next post about Advanced Filters on this problem. I didn't dig this deep into it, there may be a reason, but you might also want to look at: Why is libFilters Unregistering AdvancedFilters_Dropdown_fitler twice in a row, then registering it, then unregistering it again, then registering it again ? It also shows the AdvancedFilters_Button_Filter being unregistered twice in a row, then reregistered?

All of that is in a single click of an inventory tab and those are supposed to be unique filter ID's are they not? so it has to be the same thing its unregistering & registering multiple times correct?
The main problem is updating the filter list after every single register & unregister event. It processes every single slot like 20-30 ?more? times (I got tired of counting them) in the click of a single button. If even more addons were using this or you guys were registering multiple (more) filters it would probably crash users instantly.
I'm not sure why everything is registered & unregistered all of the time....why aren't filters just registered, left in a table & called when needed...
Anyhow It can be fixed doing this (not saying its ideal, I'm no expert here)

The main idea would be to just zo_calllater the UpdateFilteredList() and put a lock on it..
So set a flag, I called it updateSet (as in the update is set to process), when its true a zo_calllater has already been made for the UpdateFilteredList() to run after however long.
This gives all filters a chance to register/unregister before it runs. So that UpdateFilteredList() only needs to run once.


First make a new local variable for our lock flag in libFilters:
Lua Code:
  1. local updateSet = false
That tells us whether or not an UpdateFilteredList(..) has already been called with a zo_calllater (to run sometime in the future). This way if the lock flag is already set, we wont keep calling UpdateFilteredList() over & over for no reason. You probably want to adjust the time, i put 500 in there just as a random number to test it, thats probably to long.


Changes: function libFilters:RegisterFilter( filterId, filterType, filterCallback )
Warning: Spoiler



Changes: function libFilters:UnregisterFilter( filterId )
Warning: Spoiler


** Do note I would make sure you call
Lua Code:
  1. -- This line
  2. updateSet = false
  3. -- before calling this line
  4. UpdateFilteredList(filterType)
This way if something has already called RegisterFilter or UnRegisterFilter while the UpdateFilteredList is running...the new register/unregister event will have a chance to call another zo_calllater'd UpdateFilteredList because it will miss the....UpdateFilteredList that is already in progress.

If you called updateSet = false after calling UpdateFilteredList() something could slip through the cracks.
wow...I hope that made sense :P

P.S. And don't forget, this should be local !
Lua Code:
  1. -- in libfilters.lua
  2. -- this should be local
  3. filters = libFilters.filters


Randactyl 11/24/14 03:20 PM

Hey guys,

I just wanted to drop in and say I've been keeping tabs on this development. I've gotten your PMs, Baertram and circonian, I've just been swamped with midterms and backlogged school work.

I'll be on break soon and I'll read through everything and make changes. Thank you for doing all of this detective work in the first place!

Baertram 11/24/14 03:49 PM

Thx for giving a shout Randactyl. Take your time please. Ppl will be able to play without addons too :p

I'm currently testing around with AdvancedFilters and libFilters. If I find a working solution I'll send you a pm with more information.

circonian 11/24/14 03:58 PM

Quote:

Originally Posted by Baertram (Post 13446)
I tried this zo_callLater workaround with 500ms. It is annoying as the filters won#t update after the click, but half a second later.
It seems to bring a way better performance as you switch tabs in inventories.
But only as long as you do not enable all FCOItemSaver filters (4 possible) in the inventories.
If all 4 filters are enabled each click will last for about 3 seconds again before something happens :(

So using the updateFlag variable is not a working workaround alone. It seems the behaviour of AdvancedFilters needs to be fixed too.

Yeah, that 500ms was just a random number I used...its obviously way to long, 100 might even be to big.
half second later...haha...that is still quicker than what I was getting pushing tab buttons without it ;P

Also yes you are right, its not a complete fix by itself, but it was like 50 times faster than without it & I was using 500ms which is waaaayyy to long. I didn't check with all 4 filters I was just doing the 1 filter like you stated.
Quote:

Originally Posted by Baertram (Post 13446)
I know. I have coded everything without libFilters in some older versions of FCOItemSaver. I've changed to use libFilters as the library was stable and AdvancedFilters has used it, because then the filters registered will work all together between different addons. And this is what I want. I don't want to recode AdvancedFilters :-) But thx for the information.

I understand, but no matter what the fact that the way libFitlers does its filtering, by calling additional filter functions to check EVERY single filter on EVERY single slot to see if the item should be displayed or not means that the more filters used (by any/all addons) the more lag every addon is going to get when a filter is changed. The more filters registered, the more addons using it, the more times every single slot has to be checked when the user pushes a button.
Which is why I originally posted about how to do the filters the other way. Then you only need to handle slotUpdate to update items "as needed" when a slot is updated. When you change a filter none of the slots need to be checked because the filterData is already up to date. The change is instant.

Quote:

Originally Posted by Baertram (Post 13446)
because then the filters registered will work all together between different addons.

As I said I never used libFilters or any of these addons and when I was looking at all of this I wasn't really paying attention to what was getting filtered & what wasn't, so I may be way off in my assumptions here, but from the code that I did look at...If ANY filter returns false then the item will not be displayed correct? So I don't really see how this makes different addons filters work together. That seems to me like they actually work against each other.
If you want item 1,2, & 3 to show, but not item 4, 5, & 6...your filter returns true on the first 3 & false on the next 3. But then if another addon has filters registered that does the opposite: returning false for the first 3 items & true for the last 3...Since each item had a false return somewhere, wont all of the items end up hidden? Thus your addon did not get what it wanted Items 1, 2, & 3 were hidden?


Quote:

Originally Posted by Baertram (Post 13446)
Question
I do not understand why the errors in AdvancdFilters and libFilters, explained by Circonian above, don't slow down the game generally?

Because, my guess from the things that I did test...is that the problem seemed to occur (for me at least) as I continued to click buttons, the faster I clicked the worse it got. Because there are so many updates going on & it they take so long to finish running yet...as I continue pushing buttons its starting more & more updates even though the original updates haven't even finished yet. So like you said if its updating every slot 40-50 times on a single push & I push the button again before its finished, now theres 40-50 more updates running on every single slot, then push it again now 120-150 updates running on every single slot, exc...
So why doesn't it slow the game down generally? Because eventually all of those updates finish running & the lag goes away....if that's what you meant by that.



Quote:

Originally Posted by Baertram (Post 13446)
Example:
If you simply use AdvancedFilters addon, maybe together with some of the plugin addons, the changing of inventory tabs is fast and smooth. Even here the unregistering and reregistering of filters take place about thousands of times and the inventory is updated many times.

But at the moment you enable FCOItemSaver addon too, together with AdvancedFilters, the game gets sloooooooooow.
Is this only because of the updating of inventory contents after each filter reregistering, and because FCOItemSaver uses extra textures inside the inventories to show the marked items?

I'm wondering why the game does not slow down markable if you simply use AdvancedFilters without FCOItemSaver?

As for that part I have no clue. I was only checking the sequence as you had stated the visible problem from, with 1 itemSaver filter on & clicking AdvancedFilter tabs. After going through libFilters & AdvancedFilters and trying to document what I found on those two I just didn't have time to look into ItemSaver. So any other connections to the problem caused by ItemSaver I didn't get a chance to look for.
Edit: As an after thought to this question, it could it possibly be what I said above..by turning on itemSaver filters as well more & more filters get registered causing more & more updates to process on every single slot.

Sasky 11/24/14 04:06 PM

Quote:

Originally Posted by circonian (Post 13440)
Yes, since your always doing:
Lua Code:
  1. result = result and <something else>
if result is already false, its always going to return false, regardless of what <something else> is.


Someone please correct me if I'm misunderstanding this part, but isn't the if statement unnecessary?
Lua Code:
  1. for _,v in pairs(filters[filterType]) do
  2.    if(v) then
  3.       result = result and v(slot)
  4.    end
  5. end

Doesn't v just represent functions in the table that tell if something should be displayed or not...since v is not a boolean (correct?) then wont the: if(v) always return true. If v did not exist it never would have made it to that part of the code, it would have broken out of the for loop before that.

So then couldn't he just do:
Lua Code:
  1. for _,v in pairs(filters[filterType]) do
  2.    result = result and v(slot)
  3.    if result == false then
  4.       return result
  5.    end
  6. end

The IF would return true as long as v isn't NIL or boolean false. If it's NIL, it shouldn't even be iterated over. If it's a function it can't be false.

As far as the short circuit, it could be simplified even more if v(slot) always returns a boolean:
Lua Code:
  1. function(slot)
  2.     for _,v in pairs(filters[filterType]) do
  3.         if not v(slot) then
  4.             return false
  5.         end
  6.     end
  7.     return true
  8. end
If you hit false, it'll carry the false throughout, and the rest of the filters aren't necessary (as merlight noted) so you can immediately exit. Similarly, this also means a true return will only happen if and only if it iterates through the whole loop. And every time it processes the whole loop it'll return true, so the running assignment is unnecessary. Not as much of an optimization as early exit, but does save some assignment operations.

circonian 11/24/14 04:39 PM

Quote:

Originally Posted by Sasky (Post 13450)
The IF would return true as long as v isn't NIL or boolean false. If it's NIL, it shouldn't even be iterated over. If it's a function it can't be false.

Yeah I just wasn't sure if v was actually functions like I thought or if it could ever possibly be something else.

Quote:

Originally Posted by Sasky (Post 13450)
As far as the short circuit, it could be simplified even more if v(slot) always returns a boolean:
Lua Code:
  1. function(slot)
  2.     for _,v in pairs(filters[filterType]) do
  3.         if not v(slot) then
  4.             return false
  5.         end
  6.     end
  7.     return true
  8. end
If you hit false, it'll carry the false throughout, and the rest of the filters aren't necessary (as merlight noted) so you can immediately exit. Similarly, this also means a true return will only happen if and only if it iterates through the whole loop. And every time it processes the whole loop it'll return true, so the running assignment is unnecessary. Not as much of an optimization as early exit, but does save some assignment operations.

Oh, nice one.

Baertram 11/24/14 06:24 PM

Quote:

Originally Posted by circonian;
As I said I never used libFilters or any of these addons and when I was looking at all of this I wasn't really paying attention to what was getting filtered & what wasn't, so I may be way off in my assumptions here, but from the code that I did look at...If ANY filter returns false then the item will not be displayed correct? So I don't really see how this makes different addons filters work together. That seems to me like they actually work against each other.
If you want item 1,2, & 3 to show, but not item 4, 5, & 6...your filter returns true on the first 3 & false on the next 3. But then if another addon has filters registered that does the opposite: returning false for the first 3 items & true for the last 3...Since each item had a false return somewhere, wont all of the items end up hidden? Thus your addon did not get what it wanted Items 1, 2, & 3 were hidden?

Correct. And this is how it should work. If I got several addons (FCOItemSaver, AdvancedFilters, ItemSaver, etc.) all filters should apply to one item.
Example:
I mark a medium piece of armor (some gloves) with the "Gear set 1" icon from FCOItemSaver and put the medium armor on my bank. At the bank I enable FCOItemSaver's "Gear" filter so the item will be hidden from the bank inventory.
This applies to the inventory tabs "All" and "Armor".

If I switch to the AdvancedFilters subtabs for "Armor" I can choose different armor types (light, medium, heavy) and the parts (head, body, feet, gloves, etc.) by buttons & dropdown box.
I select all medium armor (by button=1 additional filter) and gloves (by dropdown box=another filter).
In total 3 filters are active now: FCOItemSaver "gear sets", AdvancedFilters button "medium armor", AdvancedFilters dropdown "gloves".
All gloves of medium armor will be shown now, except the ones marked with FCOItemSaver's "Gear set 1" icons.
This behaviour (multiple filters of different addons apply to the same item) is wished (at least for FCOItemSaver :-) ).

You could even mark an item with 2 different icons in FCOItemSaver and enable one filter but set the other filter to "show only marked". They will work against each other too. The item will be shown because "show only marked" is set, which weights more then filter enabled (hide item).

Quote:

Originally Posted by circonian;
As for that part I have no clue. I was only checking the sequence as you had stated the visible problem from, with 1 itemSaver filter on & clicking AdvancedFilter tabs. After going through libFilters & AdvancedFilters and trying to document what I found on those two I just didn't have time to look into ItemSaver. So any other connections to the problem caused by ItemSaver I didn't get a chance to look for.
Edit: As an after thought to this question, it could it possibly be what I said above..by turning on itemSaver filters as well more & more filters get registered causing more & more updates to process on every single slot.

Could be. I noticed that the really noticeable slowdown happens only at banks & guild banks. At your local vendor, crafting stations and inventory the fixed version I've tried today was working with a better performance. Maybe it is because of the amount of items stored at the banks (up to 500).

For today I'll stop trying to find a workaround. The performance was increased now, but it is still too laggy and slow.

circonian 11/24/14 11:46 PM

Item Saver
 
On Item Saver I see this:
Now I didn't have tons of time to spend on this one, so forgive me if I missunderstood the code I glanced at or if theres more going on that I noticed, but...on a single click of an inventory tab I got so much spam on functions that are firing I couldn't even fit it all in the chat window and some serious lag . Thats even after I maxed out the chat window buffer MaxHistoryLines...which is at 1,000. I see what you mean now about having all 4 filters on.

I see this running over & over & over


This part here:
Lua Code:
  1. --Choose the filter type
  2. local function getSettingsIsFilterOn(p_filterId, p_filterPanel)
Looks like it is getting called a lot...a lot. I couldn't really tell where it was coming from, there's so many calls to it all over. But I saw no other function before it firing to call it, so it must be getting called from a hook somewhere? It would probably be easier for you to track down being more familiar with the code.

Either way, What does getSettingsIsFilterOn() do. It looks like it just checks to see if certain filters are turned on or not for certain bags/inventories ? Could you just store all of the filter states for each of the inventories in a table, then use it wherever needed instead of calling this function everywhere? As filter states change just update the table. It looks like that is what its trying to do, but the question is, is there a reason it gets called so many times?



I also see this spamming a lot, scrolling through the entire buffer.

Create marker control is set as part of your callback on inventory row items and is whats calling the other functions after it.
Almost all of the code I see getting called from the callback on the inventory is doing stuff like setting dimensions, anchors, texture, colors, setting the tiers, exc.... I may be wrong do they move or change colors? Even if they do those don't seem like they should be part of the callback code. The callback itself is not what changes the color/size, so just change the color/size whenever the user pushes a button or whatever causes the color/size change, not every time the callback fires. These controls could probably be created & setup 1 time, then hidden/shown & change colors/sizes ONLY as needed.

As for MyGetItemInstanceId, MyGetItemDetails, & SignItemId are getting called from that it looks like.


I've never used prehooks so I may misunderstand whats going on here, but just in case I thought I would point this out. In your CreateMarkerControl (which is called in a callback from the inventory hook, so it fires a lot), it looks like it is constantly creating more & more & more prehooks everytime the function makes it into that part of the code? If for whatever reason it only makes it into that part of the code once then it doesn't need to be in the callback code. It should be called elsewhere to create the prehook then never called again?
Lua Code:
  1. if (controlId==1 and parent:GetParent() == ZO_ListDialog1ListContents) then
  2.  
  3.             --Pre-Hook the handler for mouse button up, to show context menu at mouse button 2, but keep the normal OnMouseUp handler as well
  4.             --parent:SetHandler("OnMouseUp", function(control, button, upInside)
  5.             --PreHookHandler( "OnMouseUp", parent, function(control, button, upInside)
  6.             ZO_PreHookHandler(parent, "OnMouseUp", function(control, button, upInside)
  7.                 --d("Clicked: " ..control:GetName() .. ", MouseButton: " .. tostring(button))
  8.                 --button 1= left mouse button / 2= right mouse button
  9.                 if button == 2 and upInside then
  10.                     --Add the menu items to the popup dialog row (parent)
  11.                     ClearMenu()
  12.                     for i = 1, gFCONumFilterIcons, 1 do
  13.                         zo_callLater(function() AddMark(parent, i, false) end, 50)
  14.                     end
  15.                 end
  16.             end)
  17.         end


One other thing I noticed, I don't think this is a big problem I never even saw this function fire, but while I'm at all of this (you did say even little fixes will help) in this function there is a lot of if/elseif statements:
Lua Code:
  1. --Check the weapon type of a weapon
  2. local function checkWeaponType(p_weaponType, p_check)
  3. d(colorDrkOrange.."checkWeaponType")
  4.     if (p_check == nil or p_check == '') then
  5.         p_check = '1hd'
  6.     end
  7.     if (p_weaponType ~= nil) then
  8.  
  9.         if     (p_check == '1hd') then
  10.  
  11.             if (p_weaponType == WEAPONTYPE_AXE or
  12.                 p_weaponType == WEAPONTYPE_DAGGER or
  13.                 p_weaponType == WEAPONTYPE_HAMMER or
  14.                 p_weaponType == WEAPONTYPE_SWORD) then
  15.                 return true
  16.             end
  17. -- exc....more code, lots of elseif statements --

it looks like you could just create a local table out of all of that info and then just check the table inside your function to see what to return. It would be a lot faster.
Like:
Lua Code:
  1. local p_Check_table = {
  2.    ["1hd"] = {
  3.       [WEAPONTYPE_AXE] = true,
  4.       [WEAPONTYPE_DAGGER] = true,
  5.       [WEAPONTYPE_HAMMER] = true,
  6.       [WEAPONTYPE_SWORD] = true,
  7.    },
  8. --- exc...do this for all of the p_check types
  9.    }
  10. }
  11. -- then do something like:
  12. local function checkWeaponType(p_weaponType, p_check)
  13.    -- ... code ... --
  14.    if (p_weaponType ~= nil) then
  15.       if p_Check_table[p_check] and p_Check_table[p_check][p_weaponType] then
  16.          return true
  17.       end
  18.    end
  19. --.. was there more code? I'm getting to lazy to go back and look.. if so do it too.. --
  20.    return false
  21. end

In the function:
local function AddMark(rowControl, markId, isEquipmentSlot)
I see
Lua Code:
  1. if (    rowControl:GetParent() ~= ZO_StoreWindowListContents
  2.         and rowControl:GetParent() ~= ZO_BuyBackListContents
  3.         and rowControl:GetParent() ~= ZO_PlayerInventoryQuestContents
  4.         --and rowControl:GetParent() ~= ZO_SmithingTopLevelImprovementPanelInventoryBackpackContents
  5.         and rowControl:GetParent() ~= ZO_SmithingTopLevelImprovementPanel
  6.         and rowControl:GetParent() ~= ZO_SmithingTopLevelResearchPanelResearchLineListList
  7.         and rowControl:GetParent() ~= ZO_SmithingTopLevelCreationPanelPatternListList
  8.         and rowControl:GetParent() ~= ZO_SmithingTopLevelCreationPanelMaterialListList
  9.         and rowControl:GetParent() ~= ZO_SmithingTopLevelCreationPanelStyleListList
  10.         and rowControl:GetParent() ~= ZO_SmithingTopLevelCreationPanelTraitListList
  11.         and rowControl:GetParent() ~= ZO_AlchemyTopLevelInventoryBackpackContents
  12.         and rowControl             ~= ZO_AlchemyTopLevelSlotContainer
  13.         --and rowControl:GetParent() ~= ZO_EnchantingTopLevelInventoryBackpackContents
  14.         and rowControl             ~= ZO_EnchantingTopLevelRuneSlotContainer
  15.         and rowControl             ~= ZO_EnchantingTopLevelExtractionSlotContainer
  16.         and rowControl:GetParent() ~= ZO_MailInboxMessage
  17.         and rowControl:GetParent() ~= ZO_MailSend
  18.         and rowControl             ~= ZO_ApplyEnchantPanel
  19.         and rowControl             ~= ZO_SoulGemItemChargerPanel
  20.         and rowControl:GetParent() ~= ZO_QuickSlotListContents) then
first, there are some of them missing the :GetParent() is that correct?

Either way that should really be changed to something like this, so you only have to call rowControl:GetParent() 1 time.
Lua Code:
  1. local parent = rowControl:GetParent()
  2.     --Check if the right click menu should be updated. Only allowed panels and menus apply
  3.     if (    parent ~= ZO_StoreWindowListContents
  4.         and parent ~= ZO_BuyBackListContents
  5.         and parent ~= ZO_PlayerInventoryQuestContents
  6.         --and parent ~= ZO_SmithingTopLevelImprovementPanelInventoryBackpackContents
  7.         and parent ~= ZO_SmithingTopLevelImprovementPanel
  8.         and parent ~= ZO_SmithingTopLevelResearchPanelResearchLineListList
  9.         and parent ~= ZO_SmithingTopLevelCreationPanelPatternListList
  10.         and parent ~= ZO_SmithingTopLevelCreationPanelMaterialListList
  11.         and parent ~= ZO_SmithingTopLevelCreationPanelStyleListList
  12.         and parent ~= ZO_SmithingTopLevelCreationPanelTraitListList
  13.         and parent ~= ZO_AlchemyTopLevelInventoryBackpackContents
  14.         and rowControl             ~= ZO_AlchemyTopLevelSlotContainer
  15.         --and parent ~= ZO_EnchantingTopLevelInventoryBackpackContents
  16.         and rowControl             ~= ZO_EnchantingTopLevelRuneSlotContainer
  17.         and rowControl             ~= ZO_EnchantingTopLevelExtractionSlotContainer
  18.         and parent ~= ZO_MailInboxMessage
  19.         and parent ~= ZO_MailSend
  20.         and rowControl             ~= ZO_ApplyEnchantPanel
  21.         and rowControl             ~= ZO_SoulGemItemChargerPanel
  22.         and parent ~= ZO_QuickSlotListContents) then
Although it would be a lot better if you just made a table out of the allowed panels and menus & then just check the table to see if parent and/or rowControl is in the table, if it is then it is an allowed panel. You could do it several ways, one would be getting the control names, storing them in the table with a value of true (if they are allowed panels) then checking the table to see if the panel is allowed.

I don't really know anything about these panels or menus. I do see your saying those panels up there are NOT allowed, but I don't know any other panel names to use so i'm going to use them and pretend they are allowed in my example.
You could do :
Lua Code:
  1. local tAllowedPanels = {
  2.    ["AlchemyTopLevelInventoryBackpackContents"] = true,
  3.    ["SmithingTopLevelCreationPanelTraitListList"]     = true,
  4.    ["SmithingTopLevelResearchPanelResearchLineListList"] = true,
  5. -- exc...--
  6. }
Then grab the parents name & see if it is an allowed panel by checking your table:
Lua Code:
  1. -- instead of doing this
  2. --local parent = rowControl:GetParent()
  3. -- and that big if statement
  4.  
  5. -- you could just do this
  6. local parentName = rowControl:GetParent():GetName()
  7. if tAllowedPanels[parentName] then
  8. -- do whatever --
  9. end


I see the same thing for this below, should probably do the same thing here with rowControl:GetName()
Lua Code:
  1. if (settings.autoMarkAllWeapon == false) then
  2.    if (    rowControl:GetName() == 'ZO_CharacterEquipmentSlotsMainHand'
  3.    or rowControl:GetName() == 'ZO_CharacterEquipmentSlotsOffHand'
  4.    -- lots more rowControl:GetName()


What about this:
Lua Code:
  1. local function UpdatePlayerInventory()
  2. ---...code...---
  3. -- It calls:
  4. PLAYER_INVENTORY:UpdateList(1)
If libFilters is handling the filtering why is your code calling UpdateList ?

circonian 11/24/14 11:54 PM

Anyhow I hope I'm not spamming this thread to much with all of this stuff, hopefully it helps you find a fix to the problem.

circonian 11/24/14 11:57 PM

Quote:

Originally Posted by Baertram (Post 13446)
I tried this zo_callLater workaround with 500ms. It is annoying as the filters won#t update after the click, but half a second later.

Just change it to like 100ms or less even would probably work, I shouldn't have put 500 on there. I should have changed it to a lower number before I posted that code. I wasn't looking for an optimal setting I was just messing around making sure I put a big enough number in there to see if it helped and it did. A lot.

Edit:
I just tested it again with 50ms and it was still plenty long enough that the UpdateFilteredList() only got called once while switching tabs. Heck I took it all the way down to 1ms and it still seemed to filter just fine only calling UpdateFilteredList once. Going that low though of course is not necessary & probably not a good idea.

merlight 11/25/14 11:40 AM

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:
Lua Code:
  1. -- replace
  2. for slotIndex = 1, GetBagSize(bagId) do
  3. -- with
  4. for slotIndex = 0, GetBagSize(bagId) - 1 do

Baertram 11/25/14 12:34 PM

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!!!

Quote:

Originally Posted by merlight (Post 13464)
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:
Lua Code:
  1. -- replace
  2. for slotIndex = 1, GetBagSize(bagId) do
  3. -- with
  4. for slotIndex = 0, GetBagSize(bagId) - 1 do


circonian 11/25/14 01:27 PM

While I'm thinking of it:

Lua Code:
  1. -- You register for this:
  2. EVENT_MANAGER:RegisterForEvent(gAddonName, EVENT_INVENTORY_SINGLE_SLOT_UPDATE, FCOItemSaver_Inv_Update)
  3.  
  4. -- then in it you zo_calllater this:
  5. --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:
Lua Code:
  1. function FCOItemSaver_Inv_Update( eventCode, bagId, slotId, isNewItem, itemSoundCategory, updateReason )
  2.     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() ?
Lua Code:
  1. zo_callLater(function()
  2.                 debugMessage("Event 'Single inventory slot updated' executed now! bagId=" .. bagId .. ", slotIndex=" .. slotId, true)
  3.                 scanInventory(bagId, slotId)
  4.             end, 250)
My Only guess I can come up with is that I see in ScanInventory() your doing something related to researchAssistant
Lua Code:
  1. --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
Lua Code:
  1. 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:
Lua Code:
  1. --Inventory slot gets updated function
  2. function FCOItemSaver_Inv_Update( eventCode, bagId, slotId, isNewItem, itemSoundCategory, updateReason )
  3.     if not isNewItem and bagId ~= BAG_BACKPACK then return end
  4.     if  GetItemType(bagId, slotId) == ITEMTYPE_NONE then return end
  5.    
  6.     scanInventory(bagId, slotId)
  7. end

4) I would change this function:
Lua Code:
  1. --Scan the inventory for ornate and/or researchable items
  2. 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:
Lua Code:
  1. --Update ornate items
  2. CheckForOrnate(bagId, slotId, any other info needed....)  
  3. -- and
  4. --Update researchable items
  5. 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 11/25/14 01:39 PM

Quote:

Originally Posted by Baertram (Post 13465)
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 11/25/14 02:58 PM

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 11/25/14 03:13 PM

Quote:

Originally Posted by circonian (Post 13468)
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.

Quote:

Originally Posted by circonian (Post 13467)
While I'm thinking of it:

Lua Code:
  1. -- You register for this:
  2. EVENT_MANAGER:RegisterForEvent(gAddonName, EVENT_INVENTORY_SINGLE_SLOT_UPDATE, FCOItemSaver_Inv_Update)
  3.  
  4. -- then in it you zo_calllater this:
  5. --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:
Lua Code:
  1. function FCOItemSaver_Inv_Update( eventCode, bagId, slotId, isNewItem, itemSoundCategory, updateReason )
  2.     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.

Quote:

Originally Posted by circonian (Post 13467)
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() ?
Lua Code:
  1. zo_callLater(function()
  2.                 debugMessage("Event 'Single inventory slot updated' executed now! bagId=" .. bagId .. ", slotIndex=" .. slotId, true)
  3.                 scanInventory(bagId, slotId)
  4.             end, 250)
My Only guess I can come up with is that I see in ScanInventory() your doing something related to researchAssistant
Lua Code:
  1. --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?

Quote:

Originally Posted by circonian (Post 13467)
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
Lua Code:
  1. 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:
Lua Code:
  1. --Inventory slot gets updated function
  2. function FCOItemSaver_Inv_Update( eventCode, bagId, slotId, isNewItem, itemSoundCategory, updateReason )
  3.     if not isNewItem and bagId ~= BAG_BACKPACK then return end
  4.     if  GetItemType(bagId, slotId) == ITEMTYPE_NONE then return end
  5.    
  6.     scanInventory(bagId, slotId)
  7. 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 11/25/14 03:32 PM

Quote:

Originally Posted by merlight (Post 13469)
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 11/25/14 06:24 PM

Quote:

Originally Posted by Baertram (Post 13470)
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:
Lua Code:
  1. if bagId =~ BAG_BACKPACK then return end
Then you will probably still want that in there.


Quote:

Originally Posted by Baertram (Post 13470)
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:
Lua Code:
  1. -- You do \have to include the library file in your .txt file, see instructions on my portal page for libItemInfo
  2.  
  3. --Put this line at the top of your code file to register the library
  4. local LII = LibStub:GetLibrary("LibItemInfo-1.0")
  5.  
  6.  
  7. -- Then Call this function & pass in either a link
  8. LII:NeedForResearch(Link)
  9.  
  10. -- or a bagId & slotId, it will work either way, that's it.
  11. LII:NeedForResearch(BagId, SlotId)



Quote:

Originally Posted by Baertram (Post 13470)
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 11/25/14 06:28 PM

Quote:

Originally Posted by Baertram (Post 13471)
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 11/25/14 07:58 PM

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:
Lua Code:
  1. -- You do \have to include the library file in your .txt file, see instructions on my portal page for libItemInfo
  2.  
  3. --Put this line at the top of your code file to register the library
  4. local LII = LibStub:GetLibrary("LibItemInfo-1.0")
  5.  
  6.  
  7. -- Then Call this function & pass in either a link
  8. LII:NeedForResearch(Link)
  9.  
  10. -- or a bagId & slotId, it will work either way, that's it.
  11. 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.


Quote:

Originally Posted by circonian (Post 13473)
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 11/25/14 08:59 PM

^^ 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 11/26/14 11:13 AM

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 11/26/14 03:55 PM

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 11/26/14 04:13 PM

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 11/26/14 06:50 PM

Quote:

Originally Posted by Randactyl (Post 13491)
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 11/26/14 06:56 PM

Quote:

Originally Posted by merlight (Post 13493)
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 11/27/14 07:21 PM

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 11/28/14 08:49 PM

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 11/29/14 02:44 AM

Quote:

Originally Posted by merlight (Post 13483)
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

Quote:

Originally Posted by merlight (Post 13483)
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 11/29/14 03:12 AM

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):
Lua Code:
  1. local BACKPACK                  = ZO_PlayerInventoryBackpack
  2. local BANK                      = ZO_PlayerBankBackpack
  3. local GUILD_BANK                = ZO_GuildBankBackpack
  4. local DECONSTRUCTION            = ZO_SmithingTopLevelDeconstructionPanelInventoryBackpack
  5. local IMPROVEMENT               = ZO_SmithingTopLevelImprovementPanelInventoryBackpack
  6. local MAIL_SEND                 = ZO_MailSend
  7. local PLAYER_TRADING            = ZO_Trade
  8. local ENCHANTING_STATION        = ZO_EnchantingTopLevelInventoryBackpack
  9. 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:
Lua Code:
  1. --See if we are sending a mail
  2.     if (not MAIL_SEND:IsHidden()) then
  3.         -- register additional filter for LAF_MAIL
  4.        elseif (not PLAYER_TRADING:IsHidden()) then
  5.         -- register additional filter for LAF_TRADE
  6.        ...
  7.       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.

Quote:

Originally Posted by Randactyl (Post 13520)
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.


merlight 11/29/14 10:56 AM

Quote:

Originally Posted by Randactyl (Post 13520)
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?

Exactly. It's always INVENTORY_FRAGMENT (top-level control ZO_PlayerInventory) with different layout fragment added to the scene. ingame/inventory/backpacklayouts.lua

Quote:

Originally Posted by Randactyl (Post 13520)
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.

For example in an add-on named NeverSellRawMaterialsToNPC (but allow sending to friends). We don't have to come up with every possible usage, but if someone else comes with an idea and can use libFilters to implement it, that'd be great ;)

Quote:

Originally Posted by Randactyl (Post 13520)
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.

Basically what I was thinking. B) looks like a bit more work for AF, but non-overlapping LAF filters might be worth it. libFilters could even tell you which LAF is active on INVENTORY_BACKPACK, it can save the value in layoutData when it hooks additionalFilter.

Quote:

Originally Posted by Randactyl (Post 13520)
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.

Even without help from libFilters or trying to figure out which LAF is active, registering all LAF types every time would impose marginal overhead. Compared to the previous version, which caused several inventory refreshes per click and chained filters ad infinitum, we could perhaps do some Folding@Home during the time saved.


Quote:

Originally Posted by Baertram (Post 13530)
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?)

Agreed. I already wrote one modification inspired by event registration in the API, this would bring it further. RegisterForEvent fails silently if the event has already been registered, UnregisterForEvent fails silently if it hasn't been registered. Register/UnregisterFilter could work in the same way, only returning success/failure status.

Randactyl 11/29/14 12:24 PM

Thanks for the detailed replies.

Quote:

Originally Posted by merlight (Post 13535)
Agreed. I already wrote one modification inspired by event registration in the API, this would bring it further.

I saw your comment mentioning this. So you have a working modification already? I do like this idea.

circonian 11/29/14 05:13 PM

Quote:

Originally Posted by Randactyl (Post 13491)
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 :)

I don't use it, I use my own libItemInfo for research information. It would require to many changes to put it into my addon just to test it.
I just got back from out of town though, if no one has tested it yet I could whip something up to test it with.

circonian 11/29/14 05:39 PM

I just downloaded new copies of FCOItemSaver & Advanced filters. Whatever you guys did while I was gone seems to have made a huge improvement.

merlight 11/29/14 06:18 PM

Quote:

Originally Posted by merlight (Post 13535)
Quote:

Originally Posted by Baertram (Post 13530)
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?)

Agreed. I already wrote one modification inspired by event registration in the API, this would bring it further. RegisterForEvent fails silently if the event has already been registered, UnregisterForEvent fails silently if it hasn't been registered. Register/UnregisterFilter could work in the same way, only returning success/failure status.

I have to revise my previous answer. UnregisterFilter already fails silently if you try to unregister something that's not even there, so nothing to change here. RegisterFilter, on the other hand, fails with an error message if you try to register something twice - and if you think about it, that makes perfect sense. Failure is definitely the correct resolution in such case - you wouldn't want it to replace a previously registered filter without even saying duh? And when it fails, a message telling you what's wrong is invaluable.

Quote:

Originally Posted by Randactyl (Post 13536)
I saw your comment mentioning this. So you have a working modification already? I do like this idea.

Yes, I updated the gist.

libFilters.lua : https://gist.github.com/merlight/e45...libfilters-lua
This is based upon r10, without debugging stuff. I removed ENCHANTING2 because it overlaps with ENCHANTING (on EXTRACTION tab both would have to be called), instead there are LAF_ENCHANTING_CREATION and LAF_ENCHANTING_EXTRACTION. FCO Item Saver will probably want to register both.

I also exposed RequestInventoryUpdate, an enhanced scheduleListUpdate which, when called with e.g. LAF_STORE and LAF_MAIL, will only cause one update (my original scheduleListUpdate would use different callbackNames and thus cause two updates).

libFilters_tags.lua : https://gist.github.com/merlight/e45...lters_tags-lua
This is the above + the change from filterId to filterTag.

Randactyl 11/30/14 12:34 AM

Okay, I used libFilters_tags as the base for r11. I added a function to get the LAF that was applied last (libFilters:GetCurrentLAF()) so an addon can apply its generic filters to whichever LAF is active. This is what I'm using in Advanced Filters to choose which LAF to register my inventory filters for.

Other uses such as Item Saver should work fine as normal just registering for the one or two LAFs they want.

merlight 11/30/14 04:58 AM

I was wondering what bloody editor adds a space at every empty line ... until I found out it's github :eek: Next time I link a gist, I should note to use "Raw" source ;)

merlight 11/30/14 07:26 AM

Ok I was reluctant to inject LAF type into inventories/layoutData, I thought there might be a better solution. Saving currentLAF in runFilters looked good, unfortunatelly there's a problem with it.

I'm at Bank Deposit tab. When I put something in the bank, it triggers filtering of both the backpack and bank inventories (due to SLOT_UPDATE events I guess). Afterwards currentLAF == LAF_BANK, although I'm still at Deposit tab and subfilters stop working until I switch the top filter twice without touching any subfilter in-between.

merlight 11/30/14 08:22 AM

Injected filterType into inventories, added inventoryType argument to GetCurrentLAF : libFilters.lua / gist

Required change in AF:
Lua Code:
  1. local function SetUpCallbackFilter( self, filterTag )
  2.     local inventoryType = GetCurrentInventoryType()
  3.     local laf = libFilters:GetCurrentLAF(inventoryType)

edit: updated with LAF_IMPROVEMENT from below ;)
edit2: oops, now really

Baertram 11/30/14 08:22 AM

Quote:

Originally Posted by Randactyl (Post 13544)
Okay, I used libFilters_tags as the base for r11. I added a function to get the LAF that was applied last (libFilters:GetCurrentLAF()) so an addon can apply its generic filters to whichever LAF is active. This is what I'm using in Advanced Filters to choose which LAF to register my inventory filters for.

Other uses such as Item Saver should work fine as normal just registering for the one or two LAFs they want.

@merlight
Thx for the rework on libFilters again. I'll tets around with FCOItemSaver and the libfilters with filterTags.

@Randactyl:
Would it be possible to add the following LAF to libfilters too, to make it more "complete" for all the different panels?

Lua Code:
  1. LAF_IMPROVEMENT = 13

This one would be used at crafting stations at the improvement tab. The tab already got registered standrd filters to only shown items you are able to improve at the current crafting station type and which are able to be improved yet.

I think we cannot use it the same way with LAF_DECONSTRUCTION like LAF_ENCHANTING_CREATION and LAF_ENCHANTING_EXTRACTION are used (ENCHANTING.enchantingMode selects the index entry from array enchantingModeToFilterType to get the appropriate LAF for the filterType),
because the active panel at SMITHING can only be distinguished by the panel names (deconstructionPanel, improvementPanel, refinePanel, researchPanel):IsHidden() function and not by help of a variable like smithingMode. Am I right?

Based on merlight's libfilters with filter tags this should be the needed additions:

Lua Code:
  1. --LAF_IMPROVEMENT
  2. libFilters.filters = {
  3.     [LAF_BAGS] = {},
  4.     [LAF_BANK] = {},
  5.     [LAF_GUILDBANK] = {},
  6.     [LAF_STORE] = {},
  7.     [LAF_DECONSTRUCTION] = {},
  8.     [LAF_GUILDSTORE] = {},
  9.     [LAF_MAIL] = {},
  10.     [LAF_TRADE] = {},
  11.     [LAF_ENCHANTING_CREATION] = {},
  12.     [LAF_ENCHANTING_EXTRACTION] = {},
  13.     [LAF_IMPROVEMENT] = {},
  14. }
  15.  
  16. --this one doesn't set the filter, but it IS the filter
  17. --since this is a PreHook using ZO_PreHook, a return of true means don't add
  18. local function ImprovementFilter( self, bagId, slotIndex, ... )
  19.     return not runFilters(LAF_IMPROVEMENT, bagId, slotIndex)
  20. end
  21.  
  22.  
  23. local inventoryUpdaters = {
  24.     BACKPACK = function()
  25.         PLAYER_INVENTORY:UpdateList(INVENTORY_BACKPACK)
  26.     end,
  27.     BANK = function()
  28.         PLAYER_INVENTORY:UpdateList(INVENTORY_BANK)
  29.     end,
  30.     GUILD_BANK = function()
  31.         PLAYER_INVENTORY:UpdateList(INVENTORY_GUILD_BANK)
  32.     end,
  33.     DECONSTRUCTION = function()
  34.         SMITHING.deconstructionPanel.inventory:HandleDirtyEvent()
  35.     end,
  36.         IMPROVEMENT = function()
  37.                 SMITHING.improvementPanel.inventory:HandleDirtyEvent()
  38.         end,
  39.     ENCHANTING = function()
  40.         ENCHANTING.inventory:HandleDirtyEvent()
  41.     end,
  42. }
  43.  
  44. local filterTypeToUpdaterName = {
  45.     [LAF_BAGS] = "BACKPACK",
  46.     [LAF_BANK] = "BANK",
  47.     [LAF_GUILDBANK] = "GUILD_BANK",
  48.     [LAF_STORE] = "BACKPACK",
  49.     [LAF_DECONSTRUCTION] = "DECONSTRUCTION",
  50.     [LAF_GUILDSTORE] = "BACKPACK",
  51.     [LAF_MAIL] = "BACKPACK",
  52.     [LAF_TRADE] = "BACKPACK",
  53.     [LAF_ENCHANTING_CREATION] = "ENCHANTING",
  54.     [LAF_ENCHANTING_EXTRACTION] = "ENCHANTING",
  55.         [LAF_IMPROVEMENT] = "IMPROVEMENT",
  56. }
  57.  
  58. unction libFilters:InitializeLibFilters()
  59.     if self.IS_INITIALIZED then return end
  60.     self.IS_INITIALIZED = true
  61.  
  62.     -- PLAYER_INVENTORY.inventories[INVENTORY_BACKPACK].additionalFilter
  63.     -- is reset every time a different backpack layout fragment is shown,
  64.     -- therefore it needs to be hooked in each fragment's layout data
  65.     hookAdditionalFilter(LAF_BAGS, BACKPACK_MENU_BAR_LAYOUT_FRAGMENT)
  66.     hookAdditionalFilter(LAF_BAGS --[[ correct, not LAF_BANK ]], BACKPACK_BANK_LAYOUT_FRAGMENT)
  67.     hookAdditionalFilter(LAF_STORE, BACKPACK_STORE_LAYOUT_FRAGMENT)
  68.     hookAdditionalFilter(LAF_GUILDSTORE, BACKPACK_TRADING_HOUSE_LAYOUT_FRAGMENT)
  69.     hookAdditionalFilter(LAF_MAIL, BACKPACK_MAIL_LAYOUT_FRAGMENT)
  70.     hookAdditionalFilter(LAF_TRADE, BACKPACK_PLAYER_TRADE_LAYOUT_FRAGMENT)
  71.  
  72.     -- other inventories seem to never reset additionalFilter
  73.     hookAdditionalFilter(LAF_BANK, PLAYER_INVENTORY.inventories[INVENTORY_BANK])
  74.     hookAdditionalFilter(LAF_GUILDBANK, PLAYER_INVENTORY.inventories[INVENTORY_GUILD_BANK])
  75.  
  76.     ZO_PreHook(SMITHING.deconstructionPanel.inventory, "AddItemData", DeconstructionFilter)
  77.         ZO_PreHook(SMITHING.improvementPanel.inventory, "AddItemData", ImprovementFilter)  
  78.     ZO_PreHook(ENCHANTING.inventory, "AddItemData", EnchantingFilter)
  79. end


Btw:
What is this function doing: HandleDirtyEvent() ?
Found at SMITHING.deconstructionPanel.inventory

Randactyl 11/30/14 01:50 PM

Can't get to my computer for about an hour, but just for my understanding:

This new method would result in both LAF_(GUILD)BANK and LAF_BAGS being registered? Couldn't I just as easily in Advanced Filters check to see if the current LAF is one for the banks and also register for BAGS?

Of course, this leads to currentLAF really being a misnomer since those situations have more than one currrently applied. Should be something more like lastLAF :cool::D

edit: Ohh I see how it will work for Bank/Bag. Current inventory and therefor current LAF will change every time change filter runs (clicking one of the big default filters). Does it also trigger when switching between deposit/withdraw?

Also, I'm not sure what GetCurrentInventoryType() will return for store, mail, trade, etc? I'm pretty sure it will be INVENTORY_BACKPACK and we wouldn't end up with the LAF_STORE etc.
Lua Code:
  1. function GetCurrentInventoryType()
  2.     return g_currentInventoryType
  3. end

merlight 11/30/14 02:52 PM

Apparently I wrote that, but had to look it up nonetheless :D g_currentInventoryType is assigned in OnEffectivelyShown hooks on "inventory display controls", it can only be INVENTORY_BACKPACK, INVENTORY_BANK or INVENTORY_GUILD_BANK.

When you're at the bank, tabs switch between ZO_PlayerInventory (INVENTORY_BACKPACK) and ZO_PlayerBank (INVENTORY_BANK). PLAYER_INVENTORY.inventories[INVENTORY_BACKPACK].appliedLayout == BACKPACK_BANK_LAYOUT_FRAGMENT.layoutData the whole time, btw.

When you withdraw/deposit an item, filters get run on both BACKPACK and BANK inventories, so it's not always true that the last filterType from runFilters is the filterType of the currently displayed inventory control.

g_currentInventoryType matches the displayed control, and you can ask libFilters what's the current LAF for this inventoryType - for BANK and GUILD_BANK it's trivial, for BACKPACK the answer is in appliedLayout.

Randactyl 12/02/14 03:12 PM

Test time!

All done right after a /reloadui. Zgoo used to view data.

Open relevant fragment, then choose one of the main filters. Results follow.

Inventory:
1. /zgoo GetCurrentInventoryType() -> INVENTORY_BACKPACK
2. /zgoo PLAYER_INVENTORY.appliedLayout.libFilters_filterType -> LAF_BAGS
3. /zgoo PLAYER_INVENTORY.inventories[GetCurrentInventoryType()].libFilters_filterType -> LAF_BAGS

Bank (withdraw):
1. INVENTORY_BANK
2. nil
3. LAF_BANK

Guild Bank (withdraw):
1. INVENTORY_GUILD_BANK
2. nil
3. LAF_GUILDBANK

Store, Deconstruction, Guild Store, Mail, Trade, Enchanting Creation/Extraction, Improvement:
1. INVENTORY_BACKPACK
2. nil
3. nil

So there is an issue with the hooking function that is supposed to run on init?

circonian 12/02/14 10:29 PM

Quote:

Originally Posted by Randactyl (Post 13597)
Test time!

All done right after a /reloadui. Zgoo used to view data.

Open relevant fragment, then choose one of the main filters. Results follow.

Inventory:
1. /zgoo GetCurrentInventoryType() -> INVENTORY_BACKPACK
2. /zgoo PLAYER_INVENTORY.appliedLayout.libFilters_filterType -> LAF_BAGS
3. /zgoo PLAYER_INVENTORY.inventories[GetCurrentInventoryType()].libFilters_filterType -> LAF_BAGS

Bank (withdraw):
1. INVENTORY_BANK
2. nil
3. LAF_BANK

Guild Bank (withdraw):
1. INVENTORY_GUILD_BANK
2. nil
3. LAF_GUILDBANK

Store, Deconstruction, Guild Store, Mail, Trade, Enchanting Creation/Extraction, Improvement:
1. INVENTORY_BACKPACK
2. nil
3. nil

So there is an issue with the hooking function that is supposed to run on init?

I was gone for a week for thanksgiving and kind of lost track of what you guys are doing and I'm not 100% up on what it is you were expecting from this test. But, a few weeks ago I separated my filtering feature from my JunkIt addon and had been working on writing my own filter library for a separate addon release (only because I wanted the experience & I have fun doing this stuff). The point is some problems that I ran into may or may not be related or helpful, you may already know these things, but I'm going to point them out just in case it helps.

I don't know what you were expecting from that test, but some things I learned may apply to your test on the backpack. I had originally wrote my library to hook into the: ZO_InventoryManager:ShouldAddSlotToList so that as items were added to the inventory lists I would run custom filters against it. It worked fine for the backpack, bank, & guild bank.

Everything worked fine until I started final testing and realized the different layout views for the backpack: Mail, trade, store, exc.. had some problems. When trying to track it down I noticed that, for example, when the mail is opened for the first time the list is populated, and when it is closed ZO_InventoryManager:ShouldAddSlotToList would run again and repopulate the list of items reverting back to the default backpack view. After the mail had been opened 1 time, ZO_InventoryManager:ShouldAddSlotToList would NOT run again when the mail was opened, only when closed. Why would it not run every time the mail was opened, but only run on close?? My best guess seemed to be that the layoutData was applied when it was opened (on the already populated/current backpack list), but the list is only repopulated the first time a layout view is opened and when the layout view is closed (in order to revert back to the default backpack view). I could not figure out a way to fix it & had to scrap it in favor of using the additional filters in the layoutData for the different backpack layouts like is done in libfilters. I could find no other way of doing it other than using those layout additional filters for: backpack, trade, mail, trading house, & vendor.

My point is, that if you were doing this:
Lua Code:
  1. Inventory:
  2.  1. /zgoo GetCurrentInventoryType() -> INVENTORY_BACKPACK
  3.  2. /zgoo PLAYER_INVENTORY.appliedLayout.libFilters_filterType -> LAF_BAGS
  4.  3. /zgoo PLAYER_INVENTORY.inventories[GetCurrentInventoryType()].libFilters_filterType -> LAF_BAGS
and storing data in the backpack inventory it may have been wiped out when the other layouts were applied for Mail, trade, vendor, trading house because they also use the backpack, but with a different view.


As for the others (I don't know how you guys handled this in your code, but) Enchanting does not use the backpack. It uses: ZO_CraftingInventory, as do all of the other craft stations. In mine I wrote separate filters for each crafting station, research, & improvement. For the crafting stations (refine/deconstruction) I rewrote: ZO_CraftingInventory:EnumerateInventorySlotsAndAddToScrollData to add additional checks & run the custom registered filters as it was enumerating the crafting inventories.

For deconstruction I used: ZO_SmithingResearchSelect:SetupDialog to hide things from the research dialog box.

and for the improvement filter I used: ZO_SharedSmithingImprovement_CanItemBeImproved to run my registered filters to see if we should allow an item to be improved or not. If an addons registered filters say the item should be hidden I just returned that the item can not be improved.

The only tough one seems to be hiding things from the provisioning stations since it seems all code only refers to recipes with recipeListIndex, recipeIndex. I found a way around it & I've got it working, I just need to do some final testing to double check everything.

circonian 12/02/14 10:36 PM

PLAYER_INVENTORY:UpdateList
 
I do have one question though. I glanced at some changes you guys made & I'm unclear on why libfilters is doing:
Lua Code:
  1. PLAYER_INVENTORY:UpdateList(INVENTORY_BACKPACK)

Since libFilters is using the layoutData.additionalFilter, which runs when the layout is applied...the list gets Committed/automatically updated after the layout is applied, so why call UpdateList ever?

merlight 12/03/14 04:00 AM

@Randactyl
I'm running out of ideas. Tried to reproduce what BigM wrote about Sell in comments, but for me it works (now). It'll be some stupid obvious bug, obvious bugs are hard to find, they look like features :D


Quote:

Originally Posted by circonian (Post 13600)
I do have one question though. I glanced at some changes you guys made & I'm unclear on why libfilters is doing:
Lua Code:
  1. PLAYER_INVENTORY:UpdateList(INVENTORY_BACKPACK)

Since libFilters is using the layoutData.additionalFilter, which runs when the layout is applied...the list gets Committed/automatically updated after the layout is applied, so why call UpdateList ever?

Yes and no. Advanced Filters change filters when the tab changes, in which case the inventory list is rebuilt anyway, so in AF this step could be avoided. But Item Saver has icons in the bottom of the inventory, which when toggled change the rules, and so they need to force inventory refresh.

merlight 12/03/14 04:37 AM

Quote:

Originally Posted by Randactyl (Post 13597)
Test time!

All done right after a /reloadui. Zgoo used to view data.

Open relevant fragment, then choose one of the main filters. Results follow.

Repeated, only instead of Zgoo I dumped the values to debug window. (summary: everything looks ok to me)

Inventory:
1. GetCurrentInventoryType() -> INVENTORY_BACKPACK
2. PLAYER_INVENTORY.appliedLayout.libFilters_filterType -> LAF_BAGS
3. PLAYER_INVENTORY.inventories[GetCurrentInventoryType()].libFilters_filterType -> LAF_BAGS

Bank (withdraw):
1. INVENTORY_BANK
2. nil (this was unexpected, I thought it would apply layout immediately when opening bank; however, while on withdraw tab, appliedLayout is irrelevant, since it only applies to backpack)
3. LAF_BANK

Bank (deposit):
1. INVENTORY_BACKPACK
2. LAF_BAGS (now it's ok)
3. LAF_BAGS

Guild Bank (withdraw):
1. INVENTORY_GUILD_BANK
2. nil
3. LAF_GUILDBANK

Guild Bank (deposit):
1. INVENTORY_BACKPACK
2. LAF_BAGS
3. LAF_BAGS

Store (buy, sell):
1. INVENTORY_BACKPACK
2. LAF_STORE
3. LAF_BAGS (this is ok, because LAF from appliedLayout takes precedence)

Mail (send):
1. INVENTORY_BACKPACK
2. LAF_MAIL
3. LAF_BAGS (same as above)


Quote:

Originally Posted by Randactyl (Post 13597)
So there is an issue with the hooking function that is supposed to run on init?

I see correct values there. For me AF now works in bank, sell, mail.
The only issue I'm aware of now is that when I switch the top filter from e.g. Weapons to All and back to Weapons, subfilter dropdown doesn't appear.

merlight 12/03/14 05:34 AM

Quote:

Originally Posted by Baertram (Post 13551)
Btw:
What is this function doing: HandleDirtyEvent() ?
Found at SMITHING.deconstructionPanel.inventory

Didn't notice this question before.

Lua Code:
  1. function ZO_SharedCraftingInventory:HandleDirtyEvent()
  2.     if self.control:IsHidden() or ZO_CraftingUtils_IsPerformingCraftProcess() then
  3.         self.dirty = true
  4.     else
  5.         self:PerformFullRefresh()
  6.     end
  7. end

Baertram 12/03/14 08:56 AM

Ah, thanks for the function explanation. Really dirty :-)

Edit:
I've found the error... as I thought it was a simple one, but hard to find.
As libFilters was updated and LAF_ENCHANTING and LAF_ENCHANTING2 were removed and changed to new LAF_ENCHANTING_EXTRACTION and LAF_ENCHANTING_CREATION constants the constant's integer value changed too.
Inside my addon I forgot to update them properly and my maximum value of available panels did not increase as LAF_IMPROVEMENT was added later.
libFilters:UnregisterFilter() did silently fail then as the tag of the filter contains the panelId in FCOItemSaver...


Error description (SOLVED!)
I'm currently rewriting FCOItemSaver to speed up and use the new libFilters correctly.

Changed:
-Only the enabled filter settings (bags, bank, guild bank, vendor, etc.) will register according filters
-The filters and buttons at the inventories will only be registered if the panel is shown, otherwise only LAF_BAGS is registered (if enabled)
So at the start of the addon only the LAF_BAGS filter will registered
-The filter buttons will be created/changed (OnClicked function, colors, etc.) as the panel is shown
The inventory filter buttons will be used for all the panels where ZO_Inventory is used (banks disposal, mail, trade, guild store sell, vendor, inventory). This is done in function "CheckFilterButtonsAtCurrentPanel".
-I'm choosing the current panel by help of events and PreHooking function "OnMouseUp" at the top menu buttons at some stations, or using the scene callback for the mail_send part (thx to merlight).

It is working together with AdvancedFilters and seems to be ok so far.

The only part not working is the LAF_IMPROVEMENT one :-( It must be some silly "feature" like merlight has written above :cool:

LAF_IMPROVEMENT should be enabled as I click the improvement button at a crafting station (preHook OnMouseUp function at line 5638) or open up the crafting station (SMITHING is shown) and the improvement tab is already shown from the last usage of the improvement parts.
The current panel will be selected in function "checkActivePanel".

Error:
If I click the improvement button it will start to repeat the called function "FCOItemSaver_PreHookButtonHandler()" every second again?
This will go on until I click the deconstruction button (line 5631).
If I do not /reloadui or click the deconstruction button the game client might crash after a while.

I'm not able to see why this prehooked button is behaving that bad :-(
All other buttons react as aspected and will only run the code inside once.
I've even implemented a variable to prevent this repeating (preventerVars.gButtonChanging) but it is not working either.

Inside the chat type "/fcois deepdebug" (and use an addon to see the "pre addon loading text" maybe) to enable the debug output.

Here is the version causing this bug:
https://dl.dropboxusercontent.com/u/...ENT_button.zip

I'd be glad if someone could help me where this "repeating" button click error comes from.

btw:
I've not changed all the right click menu hook, self made pre hook functions, and that other stuff (debugMessage function etc.) circonian and merlight have pointed me to before. I focused on removing the unnecessary filters and buttons and inventory updates first.
So please don#t review this code again :)

merlight 12/03/14 09:57 AM

Quote:

Originally Posted by Baertram (Post 13611)
Error:
If I click the improvement button it will start to repeat the called function "FCOItemSaver_PreHookButtonHandler()" every second again?
This will go on until I click the deconstruction button (line 5631).
If I do not /reloadui or click the deconstruction button the game client might crash after a while.

I'm not able to see why this prehooked button is behaving that bad :-(
All other buttons react as aspected and will only run the code inside once.
I've even implemented a variable to prevent this repeating (preventerVars.gButtonChanging) but it is not working either.

Weird. I'm not going to test now, Dortene called I should come defend some elder scrolls...
I'd rather divert you from the idea of hooking buttons' OnMouseUp completely.

This is SMITHING:SetMode:
Lua Code:
  1. function ZO_Smithing:SetMode(mode)
  2.     if self.mode ~= mode then
  3.         self.mode = mode
  4.  
  5.         CRAFTING_RESULTS:SetCraftingTooltip(nil)
  6.         self.refinementPanel:SetHidden(mode ~= SMITHING_MODE_REFINMENT)
  7.         self.creationPanel:SetHidden(mode ~= SMITHING_MODE_CREATION)
  8.         self.improvementPanel:SetHidden(mode ~= SMITHING_MODE_IMPROVEMENT)
  9.         self.deconstructionPanel:SetHidden(mode ~= SMITHING_MODE_DECONSTRUCTION)
  10.         self.researchPanel:SetHidden(mode ~= SMITHING_MODE_RESEARCH)
  11.  
  12.         KEYBIND_STRIP:UpdateKeybindButtonGroup(self.keybindStripDescriptor)
  13.         TriggerTutorial(self.GetTutorialTrigger(self, GetCraftingInteractionType(), mode))
  14.     end
  15. end
It readily offers two ways of catching when a panel is shown/hidden. You can either hook SMITHING:SetMode, or OnEffectivelyShown/OnEffectivelyHidden handlers on each panel you're interested in. You won't need to check whether mouse button == 1 and upInside and lastClickedButton, and you don't need to delay it with zo_callLater - you know what's being shown right there. The code should really be simpler.


All times are GMT -6. The time now is 10:23 AM.

vBulletin © 2024, Jelsoft Enterprises Ltd
© 2014 - 2022 MMOUI