Bank Deposit loading times [OUT OF COURSE CONTENT]

[Bahaa]

To keep this as simple as possible, I have followed Brian’s banking system to implement a banking system in my game. However, I have dealt with one major issue and have not been able to fix it so far, and that is… drum roll please…

**** THE BANK DEPOSIT LOADING TIMES ****

I tried adjusting the code myself, but this was way too complex for my understanding. I tried seeking help online, same problem, because the root issue is deep in the code itself. Can Brian or anyone who implemented this tutorial kindly help me out to Redraw the inventories only based on what needs to be transferred, and not the entire bank or inventory system? It would seriously help me, thanks in advance

In other words, I understand that the ‘Redraw()’ method is what is responsible for the entire process being slow. I was wondering if we can only Redraw the inventory spaces that have the items to be transferred, and the inventory spots the bank expects to have the items delivered to (i.e: count how many spots are needed for the amount of items, and only redraw these), rather than all 273 bank slots and 50 inventory slots of mine

[Brian_Trotter]

First of all, 273??

I do believe the issue is, indeed, reloading all the inventory slots every time an item is transferred.

I think the easiest solution (and I’m at work, so I don’t have the code in front of me) is to add a boolean skipRefresh to each of the add and remove related methods in Inventory. Essentially, any method that calls inventoryUpdated should take in as it’s LAST parameter

bool refresh=true

This becomes an optional parameter, where the default behavior is to refresh, but can be overridden by adding faslse to the end of the method.

If an internal Inventory method calls another method that leads to an inventory updated, then it also needs to pass refresh along the chain.

Then before calling any rinventoryUpdated, first check if refresh is true

Finally, at the end of the transfer routine, we need to call a new method on inventory whose sole job is to call inventoryUpdated (at which point the final redraw will happen).

[Bahaa]

@Brian_Trotter

First of all, 273??

For this one, the goal is to give the player the opportunity to store as many items as they desire in their bank. The final goal is to not get any complaints about inventory space in the end (I’m sure if we get the optimization issue resolved, the bank inventory size won’t matter)

Speaking of the code, if you have the time, can you please share the functional script with me? I don’t mean to come off as sloppy, but I genuinely did not have enough time recently to work enough on this project (nowadays I usually go crazy on it during the weekends)

[Brian_Trotter]

@Bahaa

Speaking of the code, if you have the time, can you please share the functional script with me?

It sort of defeats the idea of making that your challenge…

[Bahaa]

wait, that was a challenge? fine, I’ll give it a go. Might take a day or two though

[Brian_Trotter]

It’s your challenge. I have no deadlines and grade on a curve.

[Bahaa]

just trying to ensure we don’t close this question off. At the end of the day, what good is a decent-looking game without optimized performance, when no one can play it because it’s… well… slow?

[Brian_Trotter]

@Bahaa

just trying to ensure we don’t close this question off.

That’s actually a two week deadline, but only on Asks… I’ve moved the topic to Talk.

[Bahaa]

oh that’s fair then. Alright let’s get to it then, as soon as possible

[Bahaa]

OK so, I had some time to test it out… man do I have a problem:

Apart from ignoring the ‘RestoreFromJToken()’ (it calls the function, but placing the boolean as a parameter causes issues with the Interface) function in ‘Inventory.cs’, I called the boolean I set up at the start of ‘Inventory.cs’, known as ‘boolean refresh = true’, and then recalled in ‘InventoryUI.cs’, in the following functions:

In Inventory.cs, you have the following:

public bool refresh = true;

// Next changed function:

public bool AddToFirstEmptySlot(InventoryItem item, int number)
        {

            refresh = true;

            // If we picked up money, automatically send it to the pouch:
            foreach (var store in GetComponents<IItemStore>()) {

                number -= store.AddItems(item, number);

            }

            if (number <= 0) return true;

            // ----------------------------------------------------------

            int i = FindSlot(item);

            if (i < 0)
            {
                return false;
            }

            slots[i].item = item;
            slots[i].number += number;
            if (inventoryUpdated != null)
            {
                if (refresh == true) inventoryUpdated();
            }
            return true;
        }

// Next updated function:

public void RemoveFromSlot(int slot, int number)
        {
            refresh = true;
            slots[slot].number -= number;
            if (slots[slot].number <= 0)
            {
                slots[slot].number = 0;
                slots[slot].item = null;
            }
            if (inventoryUpdated != null)
            {
                if (refresh == true) inventoryUpdated();
            }
        }

// Next function:

public bool AddItemToSlot(int slot, InventoryItem item, int number)
        {
            refresh = true;
            if (slots[slot].item != null)
            {
                return AddToFirstEmptySlot(item, number); ;
            }

            var i = FindStack(item);
            if (i >= 0)
            {
                slot = i;
            }

            slots[slot].item = item;
            slots[slot].number += number;
            if (inventoryUpdated != null)
            {
                if (refresh == true) inventoryUpdated();
            }
            return true;
        }

// Next function:

public void RemoveItem(InventoryItem item, int number, bool refresh = true) {

            refresh = true;

            if (item == null) return;

            for (int i = 0; i < slots.Length; i++) {

                if (ReferenceEquals(slots[i].item, item)) {

                    slots[i].number -= number;
                    if (slots[i].number <= 0) {

                        slots[i].item = null;
                        slots[i].number = 0;

                    }

                    if (refresh == true) inventoryUpdated();

                    return;

                }

            }

        }

// Next Function:

public void TransferAllInventory(Inventory otherInventory)
        {
            refresh = false;
            if (otherInventory == null || otherInventory == this) return;
            for (int i = 0; i < inventorySize; i++)
            {
                if (slots[i].item == null) continue;
                for (int j = slots[i].number; j > 0; j--)
                {
                    if (otherInventory.HasSpaceFor(slots[i].item))
                    {
                        otherInventory.AddToFirstEmptySlot(slots[i].item, 1);
                        slots[i].number--;
                        if (slots[i].number <= 0) slots[i].item = null;
                    }
                    else break;
                }
            }
            if (refresh == true) {inventoryUpdated?.Invoke();
            otherInventory.inventoryUpdated?.Invoke();}
        }

And in ‘InventoryUI.cs’:

private void Awake() 
        {
            
            bool refresh = (GetComponent<Inventory>().refresh = true);

            // if we are dealing with the player Inventory, set it up (just don't do it for the Players' bank)
            if (isPlayerInventory) {
            
            selectedInventory = Inventory.GetPlayerInventory();
            if (refresh) selectedInventory.inventoryUpdated += Redraw;
        
            }
        
        }

// Next function:

public bool Setup(GameObject user, bool refresh = true) {

            if (user.TryGetComponent(out selectedInventory)) {

                // if the object has an inventory (our bank in this case), set the Selected Inventory Up, 
                // Subscribe to the Redraw() changes, and then redraw the inventory:
                if (refresh) selectedInventory.inventoryUpdated += Redraw;
                Title.text = selectedInventory.name;
                Redraw();
                return true;

            }

            return false;

        }

The problem is… my inventory size is now 25 slots (instead of 50), and nothing I pick off the ground is stored in the Inventory slots anymore. Please help

I have to say though, somewhere down the line of this attempt it did refresh at a significantly faster rate than usual, when I hit the ‘TransferAllToOtherInventory()’ function-triggering button, but I’m not sure why…

[Brian_Trotter]

@Bahaa

Apart from ignoring the ‘RestoreFromJToken()’

RestoreFromJToken needs to call inventoryUpdated in alll cases, so the parameter wouldn’t make sense anyways.

refresh shouldn’t be a global variable. It should be an optional parameter passed into the method only when we want it to be false.

For example:

public bool AddToFirstEmptySlot(InventoryItem item, int number, bool refresh = true)
        {

            // If we picked up money, automatically send it to the pouch:
            foreach (var store in GetComponents<IItemStore>()) {

                number -= store.AddItems(item, number);

            }

            if (number <= 0) return true;

            // ----------------------------------------------------------

            int i = FindSlot(item);

            if (i < 0)
            {
                return false;
            }

            slots[i].item = item;
            slots[i].number += number;
            if (inventoryUpdated != null)
            {
                if (refresh == true) inventoryUpdated();
            }
            return true;
        }

This is known as an optional parameter. This means that ou can call AddToFirstEmptySlot with or without the third parameter, meaning that you are adding functionality without forcing every call to these methods to update their signature.

If you call AddToFirstEmptySlot(item, number); then refresh is true automatically, and inventoryUpdated will fire. If, however, you call AddToFirstEmptySlot(item, number, false), then the inventoryUpdated will NOT fire, and at some point, you’ll have to call it manually.

Now the only case where you don’t want it to fire is in the bulk transfer for loop. Any other time you want it to fire, so won’t pass the parameter.

In the for loop, by calling it as false, it will be faster because it won’t be destroying a massive amount of UI gameObjects all in a single frame.

Once the for loop is finished, then you’ll need a method on Inventory that invokes InventoryUpdated, which will be called in that loop

public void ForceInventoryUpdatedEvent()
{
     inventoryUpdated?.Invoke();
}

[Bahaa]

OK umm… I wasn’t very clear on your previous response to be honest, so I set my ‘InventoryUI.cs’ code back to its default settings, and apart from every other function with ‘inventoryUpdated’ call having a ‘bool refresh = true’ parameter, my ‘TransferAllInventory’ at the end of ‘Inventory.cs’ had that parameter check set to false, right before invoking ‘inventoryUpdate’. I put the if (refresh == true) condition right before updating the inventory, as you can see below:

public void TransferAllInventory(Inventory otherInventory, bool refresh = true) {

            if (otherInventory == null || otherInventory == this) return;
            
            // List of Inventory Slots that will be updated (i.e: bank transfer):
            List<InventorySlot> changes = new List<InventorySlot>();

            for (int i = 0; i < inventorySize; i++) {

                if (slots[i].item == null) continue;

                int maxTransfers = Mathf.Min(slots[i].number, otherInventory.FreeSlots());

                if (maxTransfers > 0) {

                    otherInventory.AddToFirstEmptySlot(slots[i].item, maxTransfers);
                    slots[i].number -= maxTransfers;

                    if (slots[i].number <= 0) slots[i].item = null;
                    changes.Add(slots[i]);

                }

            }

            foreach (var modifiedSlot in changes) {

                // Calculate the slot index for the UI:
                int uiSlotIndex = Array.IndexOf(slots, modifiedSlot);

                // Update the UI for the specific slot:
                if (uiSlotIndex >= 0 && uiSlotIndex < transform.childCount) {

                    var itemUI = transform.GetChild(uiSlotIndex).GetComponent<InventorySlotUI>();
                    if (itemUI != null) itemUI.UpdateUI();

                }

            }

            changes.Clear();

            if (refresh == true) {
            inventoryUpdated?.Invoke();
            otherInventory.inventoryUpdated?.Invoke();
            }
        }

But now whilst it is significantly faster, my UI does not get updated unless something else gets picked up…

Edit: Changed it to ‘true’, and while there is a significant bump in speed, I’m still not sure how having an additional boolean did the trick. From my perspective, it didn’t seem to do anything extra tbh (P.S: I also tried batch processing, it did help, combined with other processing operations I’m still testing out).

Edit 2: One very interesting algorithm ChatGPT did suggest though, was Object Pooling, but when it got to ‘InventoryUI.Redraw()’ and tried modifying it, while there was a performance boost, the first 21 slots of my inventory, and the first 25 of my bank, are clickable, but nothing can stay in these slots

[Brian_Trotter]

There is no need to update additional UI…

Inventory should know nothing about the UI in the first place, the dependency is the other way round, UI depending on the inventory.

public void TransferAllInventory(Inventory otherInventory, bool refresh = true) {

            if (otherInventory == null || otherInventory == this) return;
            
            // List of Inventory Slots that will be updated (i.e: bank transfer):
            //List<InventorySlot> changes = new List<InventorySlot>();

            for (int i = 0; i < inventorySize; i++) {

                if (slots[i].item == null) continue;

                int maxTransfers = Mathf.Min(slots[i].number, otherInventory.FreeSlots());

                if (maxTransfers > 0) {

                    otherInventory.AddToFirstEmptySlot(slots[i].item, maxTransfers, false); without the false it will update inventoris.
                    slots[i].number -= maxTransfers;

                   // if (slots[i].number <= 0) slots[i].item = null;
                   // changes.Add(slots[i]);

                }

            }

           // foreach (var modifiedSlot in changes) {

                // Calculate the slot index for the UI:
               // int uiSlotIndex = Array.IndexOf(slots, modifiedSlot);

                // Update the UI for the specific slot:
               // if (uiSlotIndex >= 0 && uiSlotIndex < transform.childCount) {

                   // var itemUI = transform.GetChild(uiSlotIndex).GetComponent<InventorySlotUI>();
                  // if (itemUI != null) itemUI.UpdateUI();

               // }

            }

           //changes.Clear();

             inventoryUpdated?.Invoke();
            otherInventory.inventoryUpdated?.Invoke();
            }
        }

With this, the inventoryUI won’t be touched until the transfer is completed. Then at the end, both inventories signal it’s time to refresh.

[Bahaa]

Code is significantly faster, thanks Brian

***** BUT… *****

The item does not disappear from the inventory when it’s banked now. In other words, you can bank the item now, and whilst it does go to the bank, it makes the UI slot unusable for other items. So basically the item is now not only unusable for other slots, but it’s also clickable (but nothing happens), and everyone else has to move to the next inventory slot now, because it’s… well… occupied by a null item now.

I reversed my function and it works fine, so I know the issue is in the new ‘TransferAllInventory()’ function

Edit: It’s worse than I expected… Now I have this miraculous ability where if I bank something and pull it out, not only does it go to a new slot, but both items are now usable (OK I did some further re-testing. Only one item is usable, which is fine, but the UI refresh is a total disaster right now, and the slot remains occupied by a null item icon (the memory of the item that was there prior to banking my in-game stuff) unfortunately!), and the Debugs I place in the function don’t even get called to begin with for some reason.

As for stackable items, banking them keeps one item left to occupy the new ‘empty slot’ (it’s empty, but it’s occupied by the UI, and the only way to get rid of it, for both stackable and non-stackable, is to get that item back and then use it), and the rest is banked…

[Brian_Trotter]

The InventorySlotUI is just a pass through, which does nothing without the underlying inventory…

Here’s my copy of the Inventory system which is functioning correctly. Bear in mind that there are no changes to InventorySlotUI.

Inventory.cs

using System;
using UnityEngine;
using GameDevTV.Saving;
using System.Collections.Generic;
using System.Linq;
using GameDevTV.Utils;
using Newtonsoft.Json.Linq;

namespace GameDevTV.Inventories
{
    /// <summary>
    /// Provides storage for the player inventory. A configurable number of
    /// slots are available.
    ///
    /// This component should be placed on the GameObject tagged "Player".
    /// </summary>
    public class Inventory : MonoBehaviour, IJsonSaveable, IPredicateEvaluator
    {
        // CONFIG DATA
        [Tooltip("Allowed size")]
        [SerializeField] int inventorySize = 16;

        // STATE
        InventorySlot[] slots;

        public struct InventorySlot
        {
            public InventoryItem item;
            public int number;
        }

        public IEnumerable<InventorySlot> GetAllInventoryItems() =>
            slots.Where(s => s.item != null && s.number > 0).OrderByDescending(s => s.item.GetPrice());
        
        // PUBLIC

        /// <summary>
        /// Broadcasts when the items in the slots are added/removed.
        /// </summary>
        public event Action inventoryUpdated;

        /// <summary>
        /// Convenience for getting the player's inventory.
        /// </summary>
        public static Inventory GetPlayerInventory()
        {
            var player = GameObject.FindWithTag("Player");
            return player.GetComponent<Inventory>();
        }

        /// <summary>
        /// Could this item fit anywhere in the inventory?
        /// </summary>
        public bool HasSpaceFor(InventoryItem item)
        {
            return FindSlot(item) >= 0;
        }

        public bool HasSpaceFor(IEnumerable<InventoryItem> items)
        {
            int freeSlots = FreeSlots();
            List<InventoryItem> stackedItems = new List<InventoryItem>();
            foreach (var item in items)
            {
                if (item.IsStackable())
                {
                    if (HasItem(item)) continue;
                    if (stackedItems.Contains(item)) continue;
                    stackedItems.Add(item);
                }
                // Already seen in the list
                if (freeSlots <= 0) return false;
                freeSlots--;
            }
            return true;
        }

        public int FreeSlots()
        {
            int count = 0;
            foreach (InventorySlot slot in slots)
            {
                if (slot.number == 0)
                {
                    count ++;
                }
            }
            return count;
        }

        /// <summary>
        /// How many slots are in the inventory?
        /// </summary>
        public int GetSize()
        {
            return slots.Length;
        }

        /// <summary>
        /// Attempt to add the items to the first available slot.
        /// </summary>
        /// <param name="item">The item to add.</param>
        /// <param name="number">The number to add.</param>
        /// <param name="refresh">optional - refreshes UI.  If false you will need to call TriggerInventoryUpdated() manually.</param>
        /// <returns>Whether or not the item could be added.</returns>
        public bool AddToFirstEmptySlot(InventoryItem item, int number, bool refresh = true)
        {
            foreach (var store in GetComponents<IItemStore>())
            {
                number -= store.AddItems(item, number);
            }
            if (number <= 0) return true;

            int i = FindSlot(item);

            if (i < 0)
            {
                return false;
            }

            slots[i].item = item;
            slots[i].number += number;
            if (refresh)
            {
                inventoryUpdated?.Invoke();
            }
            return true;
        }

        /// <summary>
        /// Is there an instance of the item in the inventory?
        /// </summary>
        public bool HasItem(InventoryItem item)
        {
            for (int i = 0; i < slots.Length; i++)
            {
                if (object.ReferenceEquals(slots[i].item, item))
                {
                    return true;
                }
            }
            return false;
        }

        /// <summary>
        /// Return the item type in the given slot.
        /// </summary>
        public InventoryItem GetItemInSlot(int slot)
        {
            return slots[slot].item;
        }

        /// <summary>
        /// Get the number of items in the given slot.
        /// </summary>
        public int GetNumberInSlot(int slot)
        {
            return slots[slot].number;
        }

        /// <summary>
        /// Remove a number of items from the given slot. Will never remove more
        /// that there are.
        /// </summary>
        public void RemoveFromSlot(int slot, int number, bool refresh = true)
        {
            slots[slot].number -= number;
            if (slots[slot].number <= 0)
            {
                slots[slot].number = 0;
                slots[slot].item = null;
            }
            if (refresh)
            {
                inventoryUpdated?.Invoke();
            }
        }

        /// <summary>
        /// Will add an item to the given slot if possible. If there is already
        /// a stack of this type, it will add to the existing stack. Otherwise,
        /// it will be added to the first empty slot.
        /// </summary>
        /// <param name="slot">The slot to attempt to add to.</param>
        /// <param name="item">The item type to add.</param>
        /// <param name="number">The number of items to add.</param>
        /// <returns>True if the item was added anywhere in the inventory.</returns>
        public bool AddItemToSlot(int slot, InventoryItem item, int number, bool refresh = true)
        {
            if (slots[slot].item != null)
            {
                return AddToFirstEmptySlot(item, number, refresh); ;
            }

            var i = FindStack(item);
            if (i >= 0)
            {
                slot = i;
            }

            slots[slot].item = item;
            slots[slot].number += number;
            if (refresh)
            {
                inventoryUpdated?.Invoke();
            }
            return true;
        }

        // PRIVATE

        private void Awake()
        {
            slots = new InventorySlot[inventorySize];
        }

        /// <summary>
        /// Find a slot that can accomodate the given item.
        /// </summary>
        /// <returns>-1 if no slot is found.</returns>
        private int FindSlot(InventoryItem item)
        {
            int i = FindStack(item);
            if (i < 0)
            {
                i = FindEmptySlot();
            }
            return i;
        }

        /// <summary>
        /// Find an empty slot.
        /// </summary>
        /// <returns>-1 if all slots are full.</returns>
        private int FindEmptySlot()
        {
            for (int i = 0; i < slots.Length; i++)
            {
                if (slots[i].item == null)
                {
                    return i;
                }
            }
            return -1;
        }

        /// <summary>
        /// Find an existing stack of this item type.
        /// </summary>
        /// <returns>-1 if no stack exists or if the item is not stackable.</returns>
        private int FindStack(InventoryItem item)
        {
            if (!item.IsStackable())
            {
                return -1;
            }

            for (int i = 0; i < slots.Length; i++)
            {
                if (object.ReferenceEquals(slots[i].item, item))
                {
                    return i;
                }
            }
            return -1;
        }

        public void RemoveItem(InventoryItem item, int number, bool refresh=true)
        {
            if (item == null) return;
            for (int i = 0; i < slots.Length; i++)
            {
                if (object.ReferenceEquals(slots[i].item, item))
                {
                    slots[i].number -= number;
                    if (slots[i].number <= 0)
                    {
                        slots[i].item = null;
                        slots[i].number = 0;
                    }
                    if(refresh) inventoryUpdated?.Invoke();
                    return;
                }
            }
        }

        public void TriggerInventoryUpdated()
        {
            inventoryUpdated?.Invoke();
        }
        public void RemoveItem(string itemID, int number)
        {
            InventoryItem item = InventoryItem.GetFromID(itemID);
            RemoveItem(item, number);
        }
        
        [System.Serializable]
        private struct InventorySlotRecord
        {
            public string itemID;
            public int number;
        }
    
 

        public bool? Evaluate(EPredicate predicate, string[] parameters)
        {
            switch (predicate)
            {
                case EPredicate.HasItem:
                    return HasItem(InventoryItem.GetFromID(parameters[0]));
                case EPredicate.HasItems: //Only works for stackable items.
                    InventoryItem item = InventoryItem.GetFromID(parameters[0]);
                    int stack = FindStack(item);
                    if (stack == -1) return false;
                    if (int.TryParse(parameters[1], out int result))
                    {
                        return slots[stack].number >= result;
                    }
                    return false;
                    
            }

            return null;
        }
        
        public void TransferAllInventory(Inventory otherInventory)
        {
            if (otherInventory == null || otherInventory == this) return;
            for (int i = 0; i < inventorySize; i++)
            {
                if (slots[i].item == null) continue;
                for (int j = slots[i].number; j >0; j--)
                {
                    if (otherInventory.HasSpaceFor(slots[i].item))
                    {
                        otherInventory.AddToFirstEmptySlot(slots[i].item, 1, false);
                        slots[i].number--;
                        if (slots[i].number <= 0) slots[i].item = null;
                    }
                    else break;
                }
            }
            inventoryUpdated?.Invoke();
            otherInventory.inventoryUpdated?.Invoke();
        }

        public JToken CaptureAsJToken()
        {
            JObject state = new JObject();
            IDictionary<string, JToken> stateDict = state;
            for (int i = 0; i < inventorySize; i++)
            {
                if (slots[i].item != null)
                {
                    JObject itemState = new JObject();
                    IDictionary<string, JToken> itemStateDict = itemState;
                    itemState["item"] = JToken.FromObject(slots[i].item.GetItemID());
                    itemState["number"] = JToken.FromObject(slots[i].number);
                    stateDict[i.ToString()] = itemState;
                }
            }
            return state;
        }

        public void RestoreFromJToken(JToken state)
        {
            if (state is JObject stateObject)
            {
                slots = new InventorySlot[inventorySize];
                IDictionary<string, JToken> stateDict = stateObject;
                for (int i = 0; i < inventorySize; i++)
                {
                    if (stateDict.ContainsKey(i.ToString()) && stateDict[i.ToString()] is JObject itemState)
                    {
                        IDictionary<string, JToken> itemStateDict = itemState;
                        slots[i].item = InventoryItem.GetFromID(itemStateDict["item"].ToObject<string>());
                        slots[i].number = itemStateDict["number"].ToObject<int>();
                    }
                }
                inventoryUpdated?.Invoke();
            }
        }

        public int SavePriority()
        {
            return 9;
        }
    }
    

}

[Bahaa]

That algorithm worked wonders when I fixed it. We went from over a minute to less than 10 seconds, thanks Brian (P.S: to avoid confusion, I did not blindly copy paste your Inventory script. I just cross-checked both scripts, and made changes accordingly)