Bank improvements #1873
Bank improvements #1873
Conversation
Also adds performance improvements by making all items rather than finding items in the bank, like the core tags Layouts now do. Additionally, only renders the quest items and not the entire bank to further speed things up.
|
Main thing is I need to work out the best way to handle the scrolling when at the bottom of a section, as it always seems to jump up a bit. |
| } | ||
| public void register(EventBus eventBus) | ||
| { | ||
| potionStorage.setQuestBankTabInterface(questBankTabInterface); |
There was a problem hiding this comment.
With this being set in register, should it also be unset in unregister?
| private final BankSearch bankSearch; | ||
|
|
||
| private Potion[] potions; | ||
| public boolean cachePotions; |
There was a problem hiding this comment.
Naming/documentation
If I understand it correctly, it's a hint to the PotionStorage's onClientTick function that we should look for the potion storage widget & note down what types of potions are stored in it.
| private Potion[] potions; | ||
| public boolean cachePotions; | ||
| private boolean layout; | ||
| private Set<Integer> potionStoreVars; |
There was a problem hiding this comment.
List of potion ItemIDs(?), or varbits(?) that are interested to us?
|
|
||
| private Potion[] potions; | ||
| public boolean cachePotions; | ||
| private boolean layout; |
There was a problem hiding this comment.
Naming - as with the cachePotions, maybe an additional verb saying that "hey, this variable denotes that we'd like something to happen".
| boolean foundItem = false; | ||
| return count; | ||
| } | ||
| return potionStorage.count(itemId); |
There was a problem hiding this comment.
Can you not store the same potion both in the bank & in the potion storage?
| @@ -540,7 +539,8 @@ private boolean checkContainersOnPlayer(Client client) | |||
|
|
|||
| public boolean checkWithBank(Client client) | |||
There was a problem hiding this comment.
Maybe rename to checkAllContainers?
| @@ -607,7 +611,11 @@ private ItemAndLastUpdated[] containersToCheckDefault() | |||
| containers.add(QuestContainerManager.getEquippedData()); | |||
|
|
|||
| if (!equip) containers.add(QuestContainerManager.getInventoryData()); | |||
| if (shouldCheckBank) containers.add(QuestContainerManager.getBankData()); | |||
| if (shouldCheckBank) | |||
There was a problem hiding this comment.
imo the name of containersToCheckDefault is a bit misleading, I would personally move the logic of this function to the check function
| @@ -0,0 +1,247 @@ | |||
| /* | |||
| * Copyright (c) 2024, Adam <[email protected]> | |||
There was a problem hiding this comment.
Add yourself too - looks like you've done enough to warrant it
This PR allows for the potion storage to be considered for requirements, as well as for the bank filtering for removing/adding potions from it. The bank work is based on the way the core Bank Tags plugin has done it for Layouts.
In tandem based on this, this also moves the Quest Helper filter for the bank from moving real bank items around to instead just making items into the items we're interested in, now that it's an allowed method due to implementation in the core client. This generally means quite a lot less processing per bank event.
Widgets which are still made (section headers and dividers, numbers for quantities needed) will now be trimmed off the size of the bank. From across my accounts the max element in BANK_ITEM_CONTAINER is always 1248, BUT this is a bit sketchy as it's possible that 1. this might change, making quest helper cut stuff off improperly, or 2. It's already wrong for some people (maybe more possible with more bank extension things?). Maybe looking at capturing the size on load before doing anything may be a better play here to ensure it's correct in more scenarios.Finally, to help further with performance, I've removed the non-bank items from being part of quest filtered bank.
Across these changes I'm seeing a massive performance improvement when adding/removing items, so hopefully this is a good step in the right direction, in addition to it being good to support a new container.
Note: The quest bank can wiggle a little bit when removing items, hopefully not a big change needed to fix this but would be good to before releasing.