Go to Page... |
Thread Tools | Display Modes |
11/25/14, 11:40 AM | #21 |
|
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:
|
11/25/14, 12:34 PM | #22 | |
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!!!
|
||
11/25/14, 01:27 PM | #23 |
|
While I'm thinking of it:
Lua Code:
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:
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:
Lua Code:
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:
Seems like this whole function could just be: Lua Code:
4) I would change this function: Lua Code:
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:
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. |
11/25/14, 01:39 PM | #24 | |
|
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. |
|
11/25/14, 02:58 PM | #25 |
|
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. |
11/25/14, 03:13 PM | #26 | ||||
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.
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?
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. |
|||||
11/25/14, 03:32 PM | #27 | |
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)? Last edited by Baertram : 11/25/14 at 04:28 PM. |
||
11/25/14, 06:24 PM | #28 | |||
|
Lua Code:
Lua Code:
|
|||
11/25/14, 06:28 PM | #29 | |
|
You need an official update. You need randactyl to fix it. |
|
11/25/14, 07:58 PM | #30 | |
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:
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.
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 Last edited by Baertram : 11/25/14 at 08:01 PM. |
||
11/25/14, 08:59 PM | #31 |
|
^^ 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. |
11/26/14, 11:13 AM | #32 |
|
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 |
11/26/14, 03:55 PM | #33 |
|
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 Last edited by Randactyl : 11/26/14 at 04:13 PM. |
11/26/14, 04:13 PM | #34 |
|
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 |
11/26/14, 06:50 PM | #35 | |
|
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. |
|
11/26/14, 06:56 PM | #36 | |
|
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. |
|
11/27/14, 07:21 PM | #37 |
|
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. |
11/28/14, 08:49 PM | #38 |
|
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. Last edited by Randactyl : 11/28/14 at 10:16 PM. |
11/29/14, 02:44 AM | #39 | ||
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
-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. Last edited by Baertram : 11/29/14 at 02:59 AM. |
|||
11/29/14, 03:12 AM | #40 | |
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:
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:
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.
|
||
ESOUI » AddOns » AddOn Help/Support » [Need help pls] Find slowdown bug at Advanced Filters & FCOItemSaver addons |
«
Previous Thread
|
Next Thread
»
|
Display Modes |
Linear Mode |
Switch to Hybrid Mode |
Switch to Threaded Mode |
|
|