https://drive.google.com/drive/folders/1vakQ8nA3azEqa3V1bFXdl6UkfeYdf9Ba?usp=sharing
Looks like there are errors, I saw only briefly with the Pickups… There were index outof range exceptions.
[SKIP TO THE LAST COMMENT. I THINK I HAVE A BETTER IDEA THAN OBJECT POOLING]
Well, if you got any update ideas, please let me know 
@Brian_Trotter apologies for the lack of context on the previous message (I haven’t been sleeping well recently because of this project (unfortunately it’s out of my control as well), so I can be a little out of whack 
)
The ArgumentOutOfRangeException is because of this function in our ‘PickupFinder.cs’:
public InventoryItem GetItemInSlot(int slot)
{
return Targets[slot].GetComponent<Pickup>().GetItem();
}
the rest, as you can see from that video, is pure logical error
For the record, here’s my ‘InventoryUI.cs’ without Object Pooling:
using UnityEngine;
using GameDevTV.Inventories;
using TMPro;
using UnityEngine.Events;
using System.Linq;
using UnityEngine.Pool;
namespace GameDevTV.UI.Inventories
{
/// <summary>
/// To be placed on the root of the inventory UI. Handles spawning all the
/// inventory slot prefabs.
/// </summary>
public class InventoryUI : MonoBehaviour
{
// CONFIG DATA
[SerializeField] InventorySlotUI InventoryItemPrefab = null;
// BANK System CONFIG DATA:
[SerializeField] bool isPlayerInventory = true;
[SerializeField] TextMeshProUGUI Title;
// 3 bank <-> Inventory states:
// 'otherInventoryUI' (bank) is open,
// 'otherInventoryUI' (bank) is closed,
// YOU are the 'otherInventoryUI' (bank)
public bool IsOtherInventory => !isPlayerInventory;
public IInventorySource SelectedInventory => selectedInventory;
[SerializeField] private UnityEvent EnabledEvent;
[SerializeField] private UnityEvent DisabledEvent;
// CACHE
IInventorySource selectedInventory;
// LIFECYCLE METHODS
private void Awake()
{
// 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();
selectedInventory.inventoryUpdated += Redraw; // you need this inventory subscription
}
}
private void Start() // ORIGINAL START Function:
{
// Redraw the PLAYER INVENTORY, JUST NOT THE BANK!
if (isPlayerInventory) Redraw();
}
// PRIVATE
private void Redraw() // ORIGINAL REDRAW FUNCTION
{
// Re-creating the inventory from scratch, over each 'InventoryItemPrefab',
// literally that's it!
if (selectedInventory == null) return;
foreach (Transform child in this.transform)
{
Destroy(child.gameObject);
}
for (int i = 0; i < selectedInventory.GetSize(); i++)
{
var itemUI = Instantiate(InventoryItemPrefab, transform);
itemUI.Setup(selectedInventory, i);
Debug.Log($"Refresh done");
}
}
// PUBLIC
public bool Setup(GameObject user)
{
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 (to boot it up for the first time):
selectedInventory.inventoryUpdated += Redraw; // you ALSO need this inventory subscription
Title.text = selectedInventory.GetDisplayName();
Redraw();
return true;
}
return false;
}
private void OnEnable()
{
EnabledEvent?.Invoke();
}
private void OnDisable()
{
DisabledEvent?.Invoke();
}
public void TransferAllInventory()
{
if (selectedInventory != null)
{
selectedInventory.TransferAllInventory(GetOtherInventory());
}
}
private Inventory GetOtherInventory()
{
if (IsOtherInventory) return Inventory.GetPlayerInventory();
// return the FIRST other Inventory it finds (i.e: The bank), or null:
var otherInventoryUI = FindObjectsOfType<InventoryUI>().FirstOrDefault(ui => ui.IsOtherInventory);
//Check to see if it's not null (should never happen), if it's Active, and if the otherInventory's inventory is valid.
if (otherInventoryUI != null && otherInventoryUI.gameObject.activeSelf && otherInventoryUI.SelectedInventory != null)
{
Inventory otherInventory = otherInventoryUI.SelectedInventory as Inventory;
return otherInventory;
}
return null;
}
}
}
and my variant with Object Pooling:
using UnityEngine;
using GameDevTV.Inventories;
using TMPro;
using UnityEngine.Events;
using System.Linq;
using UnityEngine.Pool;
namespace GameDevTV.UI.Inventories
{
/// <summary>
/// To be placed on the root of the inventory UI. Handles spawning all the
/// inventory slot prefabs.
/// </summary>
public class InventoryUI : MonoBehaviour
{
// CONFIG DATA
[SerializeField] InventorySlotUI InventoryItemPrefab = null;
// BANK System CONFIG DATA:
[SerializeField] bool isPlayerInventory = true;
[SerializeField] TextMeshProUGUI Title;
// 3 bank <-> Inventory states:
// 'otherInventoryUI' (bank) is open,
// 'otherInventoryUI' (bank) is closed,
// YOU are the 'otherInventoryUI' (bank)
public bool IsOtherInventory => !isPlayerInventory;
public IInventorySource SelectedInventory => selectedInventory;
[SerializeField] private UnityEvent EnabledEvent;
[SerializeField] private UnityEvent DisabledEvent;
// CACHE
IInventorySource selectedInventory;
// OBJECT POOLING
private IObjectPool<InventorySlotUI> slotPool;
// LIFECYCLE METHODS
private void Awake()
{
// 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();
selectedInventory.inventoryUpdated += Redraw; // you need this inventory subscription
}
// OBJECT POOLING
slotPool = new ObjectPool<InventorySlotUI>
(
CreateSlot,
OnGetSlot,
OnReleaseSlot,
OnDestroySlot
);
}
private void Start() // ORIGINAL START Function:
{
// OBJECT POOLING
foreach (Transform child in transform)
{
Destroy(child.gameObject);
}
// Redraw the PLAYER INVENTORY, JUST NOT THE BANK!
if (isPlayerInventory) Redraw();
}
// OBJECT POOLING
private InventorySlotUI CreateSlot()
{
return Instantiate(InventoryItemPrefab, transform);
}
// OBJECT POOLING
private void OnGetSlot(InventorySlotUI slot)
{
slot.transform.parent = transform;
slot.gameObject.SetActive(true);
}
// OBJECT POOLING
private void OnReleaseSlot(InventorySlotUI slot)
{
slot.gameObject.SetActive(false);
slot.transform.parent = null;
}
// OBJECT POOLING
private void OnDestroySlot(InventorySlotUI slot)
{
Destroy(slot.gameObject);
}
// PRIVATE
private void Redraw() // ORIGINAL REDRAW FUNCTION
{
// Re-creating the inventory from scratch, over each 'InventoryItemPrefab',
// literally that's it!
// OBJECT POOLING
if (selectedInventory == null) return;
foreach (Transform child in this.transform)
{
// Destroy(child.gameObject);
// OBJECT POOLING
slotPool.Release(child.GetComponent<InventorySlotUI>());
}
for (int i = 0; i < selectedInventory.GetSize(); i++)
{
// var itemUI = Instantiate(InventoryItemPrefab, transform);
// OBJECT POOLING
var itemUI = slotPool.Get();
itemUI.Setup(selectedInventory, i);
Debug.Log($"Refresh done");
}
}
// PUBLIC
public bool Setup(GameObject user)
{
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 (to boot it up for the first time):
selectedInventory.inventoryUpdated += Redraw; // you ALSO need this inventory subscription
Title.text = selectedInventory.GetDisplayName();
Redraw();
return true;
}
return false;
}
private void OnEnable()
{
EnabledEvent?.Invoke();
}
private void OnDisable()
{
DisabledEvent?.Invoke();
}
public void TransferAllInventory()
{
if (selectedInventory != null)
{
selectedInventory.TransferAllInventory(GetOtherInventory());
}
}
private Inventory GetOtherInventory()
{
if (IsOtherInventory) return Inventory.GetPlayerInventory();
// return the FIRST other Inventory it finds (i.e: The bank), or null:
var otherInventoryUI = FindObjectsOfType<InventoryUI>().FirstOrDefault(ui => ui.IsOtherInventory);
//Check to see if it's not null (should never happen), if it's Active, and if the otherInventory's inventory is valid.
if (otherInventoryUI != null && otherInventoryUI.gameObject.activeSelf && otherInventoryUI.SelectedInventory != null)
{
Inventory otherInventory = otherInventoryUI.SelectedInventory as Inventory;
return otherInventory;
}
return null;
}
}
}
[REQUESTING A SOLUTION] Proposing an ‘Inventory Slot-based Refresh’ approach:
OK I spent a little time thinking about it, and I have a bit of an idea, which I’m not 100% sure of, but I’ll propose it anyway:
in ‘InventorySlotUI.cs’, we have this function:
public void OnPointerClick(PointerEventData eventData) {
if (LastUIClicked != this) {
LastUIClicked = this;
Invoke(nameof(TimesUp), .001f); // a very small timer means spam clicks are impossible, eliminating potential bugs coming out of that
Debug.Log($"{index} was clicked once");
}
else {
HandleDoubleClick();
}
}
what it does contain, is a variable called ‘index’, and this seems to give pretty accurate results each time it’s clicked
So my idea was, why don’t we, instead of destroying the entire inventory and recreating it each time, use that variable to get which slot needs to be destroyed and re-created? That way, we don’t rely on object pooling, and get accurate results consistently. The major Redraw can be done in the ‘OnEnable()’ on ‘InventoryUI.cs’, so it cleans everything up once, and once only, each time we enable that Inventory (and after that, each slot gets individually refreshed)
If it helps, this is what ‘TimesUp()’ contains (in ‘InventorySlotUI.cs’):
// 'TimesUp' checks if we're the last UI clicked (if true, clear 'LastUIClicked')
private void TimesUp() {
if (LastUIClicked == this) LastUIClicked = null;
}
and this is ‘HandleDoubleClick()’, also in ‘InventorySlotUI.cs’:
private void HandleDoubleClick()
{
TimesUp(); // avoids triple clicking from starting another 'HandleDoubleClick'
InventoryItem item = inventory.GetItemInSlot(index);
int number = inventory.GetNumberInSlot(index);
if (item == null || number < 1) return;
if (inventory.GetGameObject().CompareTag("Player"))
{
var otherInventoryUI = FindObjectsOfType<InventoryUI>().FirstOrDefault(ui => ui.IsOtherInventory);
if (otherInventoryUI != null && otherInventoryUI.gameObject.activeSelf && otherInventoryUI.SelectedInventory != null)
{
Inventory otherInventory = otherInventoryUI.SelectedInventory as Inventory;
TransferToOtherInventory(otherInventory, item, 1);
}
else if (item is EquipableItem equipableItem && inventory.GetGameObject().TryGetComponent(out Equipment equipment))
{
EquipItem(equipableItem, equipment, number);
}
}
else
{
TransferToOtherInventory(Inventory.GetPlayerInventory(), item, 1);
}
}
I’m not sure how to do it yet, but it’s food for thought
We will also rename the ‘Redraw()’ function to ‘InitialRedraw()’, and make sure it only works when the Inventory GameObject is enabled, and that’s it, and that can be done in an ‘OnEnable()’. After that, only the clicked slots and the destination slots are refreshed, nothing more
Edit 1: I did a little bit of try and error, and managed to get the correct index into ‘InventoryUI.cs’, by creating these in ‘InventoryUI.cs’:
// ---------------------------------------- OPTIONAL (USED IN 'InventorySlotUI.cs') ---------------------------
// INDEX
private int LastClickedIndex;
public int GetLastClickedIndex()
{
return LastClickedIndex;
}
public void SetLastClickedIndex(int LastClickedIndex)
{
this.LastClickedIndex = LastClickedIndex;
Debug.Log($"Last Clicked Index is: {LastClickedIndex}");
}
// ------------------------------------------------------------------------------------------------------------
and then getting them in ‘OnPointerClick()’ in ‘InventorySlotUI.cs’:
public void OnPointerClick(PointerEventData eventData)
{
if (LastUIClicked != this)
{
LastUIClicked = this;
Invoke(nameof(TimesUp), .001f); // a very small timer means spam clicks are impossible, eliminating potential bugs coming out of that
Debug.Log($"{index} was clicked once");
GetComponentInParent<InventoryUI>().SetLastClickedIndex(index);
}
else
{
HandleDoubleClick();
}
}
Next, I tried writing a ‘SlotBasedRedraw()’ function, which attempts to do exactly as it says. However, using it is a little confusing for me, so I’ll leave it here for review (and potentially instructions on what to do with it):
private void SlotBasedRedraw()
{
int index = GetLastClickedIndex();
InventorySlotUI slotToRedraw = null;
foreach (Transform child in this.transform)
{
// For optimization purposes, only destroy the clicked Slot, otherwise continue
var slotUI = child.GetComponent<InventorySlotUI>();
if (slotUI != null && slotUI.GetSlotIndex() == index)
{
slotToRedraw = slotUI;
break;
}
}
// Destroy the found slot, if it exists
if (slotToRedraw != null)
{
slotToRedraw.Setup(selectedInventory, index);
}
// Instantiate the new slot, only for the specified index
if (index < selectedInventory.GetSize())
{
var itemUI = Instantiate(InventoryItemPrefab, transform);
itemUI.Setup(selectedInventory, index);
}
}
(But this will potentially return errors with the new pickup system, because there’s no slots in there, so that’s another problem to think of)
The next step will be to Integrate this, but because I don’t know who goes where exactly, with our current setup, this will be quite challenging for me. But I know that before doing so, I will have done the ‘Redraw()’ in ‘OnEnable()’ for the inventories only, and a ‘SlotBasedRedraw()’ for when we click the inventory slot, both for the transmitter and the receiver (so this ‘SlotBasedRedraw()’ will need to be updated again)
(I’m not sure if we need to optimize drag and drop too (we probably will), but for point and click, this seems like a fair result)
But I also have a serious problem with the fact that the ‘PickupWindow’ has no slots when we open it, so I need to also find a way to avoid this system entirely for that inventory
TLDR:
I want to implement a clicked-slot based individual refresh (i.e: Only the slot that you click, and the slot that it’s supposed to go towards, should be refreshed. That’s it!).
The main challenge I have, is the countless references from external scripts to this. It makes understanding who goes where quite hard
This means you likely will want a completely different InvenorySlotUI script for the PickupWindow…
OK so… What do I do now? I know that’s a vague question, but I’m truly confused (and lost. I don’t even know where to start right now), and lag spikes are terrible on the bank (and Object pooling from yesterday didn’t exactly work ), and as much as I’d love to reduce the quantity, on the long run in the game this will ultimately need to be reversed. Unfortunately this is a topic I can’t just ignore (unless I want the steam ratings to go from Very Positive to Mixed, or Overwhelmingly Negative)
To be honest, I’m not sure
I don’t know why the pooling didn’t work for you. It might be something with the IPointerClickHandler code, or it might be something else altogether… Add to that my time is still fairly restricted (Doc says I can sit for an hour at the computer, then I gotta sit in my recliner for a couple…
I’m not sure what I changed yesterday, but now the pooling isn’t working for me either. I’ll take a look at it later.
No worries, take your time and get some rest (although I’m lowkey convinced a slot-based refresh approach is better on the long run, but hey I’m not messing with systems I don’t fully understand just yet,. As long as the performance improves to reduce lag spikes to almost zero, then I’m good. I’m probably not touching inventories after this anyway (apart from the visual bug with the quiver slot UI… and the coins UI has the same visual bug. We’ll discuss these another time))
In the meanwhile I’ll be investigating other bugs that I kept delaying. I fixed two rare bugs I accidentally caught today 
(from my own systems. This has nothing to do with any course content)
If whatever you try next doesn’t work, then I’ll send you all my scripts relevant to inventory transfers, if that helps
@Brian_Trotter please don’t close this topic until Object Pooling is done. I tried it on my own (a while ago… I deleted the code for it to keep it all clean after it failed), but nothing worked for me
In the meanwhile I’m trying to get something else to work
As I’m not the sole arbiter of closure when it comes to Ask topics, I’ve converted this to a Talk.
I would suggest most of your topics at this point should be Talks. We try to constrain our Ask topics to direct course issues to save wear and tear on our Teaching Assistants. Every Unity TA gets the list of Asks that don’t have a solution and we have to use the Tags and Question Contexts to find ours. This is even harder with the new site reboot.
Hello Brian. Understandable, I’ll just tag you and leave it as is under “Talk” topics moving forward
But please, if you got any updates about this topic, whenever you got a chance, let me know (I’m not ignoring this topic, but because I truly have no clue how Object Pooling works, I figured I’d watch and learn just this time. In the meanwhile, I’m trying other stuff for other systems)
This topic was automatically closed 20 days after the last reply. New replies are no longer allowed.