- Home /
Variable will not change?
I'm making an inventory system and here I have a script that's placed on every inventory slot. The function 'SetItem' is called from another script that manages the actual list of items in an array... But for some reason, that function simply will not change the 'thisItem' variable to the item passed in. I've checked in numerous ways that the item being passed in isn't null and I've tried setting the item within the script in various different ways and that all works. What doesn't work is that one specific function and I am absolutely lost as to why... Any help would be appreciated.
public class ItemSlot : MonoBehaviour {
public Item thisItem;
public GameObject thisIcon;
public void Start(){
thisIcon.SetActive(false);
}
public void Update(){
if (thisItem != null){
thisIcon.SetActive(true);
thisIcon.GetComponent<Image>().sprite = thisItem.icon;
} else {
thisIcon.SetActive(false);
}
}
public void SetItem(Item item){
thisItem = item;
}
public void RemoveItem(){
thisItem = null;
}
}
Please provide the script where you call SetItem()
Answer by Bunny83 · Jan 20, 2020 at 06:44 PM
Like mentioned in the comment above we need to know how you actually call your method. You might not call it properly or you not call the method on the right object. This is a common mistake. Since we don't know how the method is called we can't tell what went wrong.
Though a few things we can tell immediately:
The "thisItem" variable is public. So it can be changed from anywhere without you noticing it. Unless you require it to be public you might want to make it private. That way you know no other script could modify it except through the SetItem method.
The whole usage of the Update callback is pretty nonsense. You should move that code into your SetItem / RemoveItem method. Personally I would probably use a property instead.
My class would probably look like this:
public class ItemSlot : MonoBehaviour
{
private Item m_Item;
[SerializeField]
private GameObject thisIcon;
public Item thisItem
{
get { return m_Item;}
set {
m_Item = value;
thisIcon.SetActive(m_Item != null);
if (m_Item != null && thisIcon != null)
thisIcon.GetComponent<Image>().sprite = m_Item.icon;
}
}
public void Start()
{
thisItem = null;
}
public void SetItem(Item item)
{
thisItem = item;
}
public void RemoveItem()
{
thisItem = null;
}
}
Using a property has the advantage that you can place a Debug.Log statement inside the getter and/or the setter to see every access to your property. Thanks to the callstack of each log you can actually trace back who changed the property / field.
Also keep in mind to create meaningful log messages. In addition to the log message you can pass an additional context object. This helps to track down wrong object references as mentioned above. Just use a log statement like this inside your setter as the first line:
Debug.Log("thisItem has been set to a new value", gameObject);
If the "Item" class has some field that would help to identify which item it is you could print it as well. Keep in mind that you could set it to null as well.
Debug.Log("thisItem has been set to a new value: " + ((value != null)?value.name:"null"), gameObject);
This assumes that Item has a field / property called "name".
Note that I pass the scripts "gameObject" as context object. Now when you do a single left click on the debug message in the console, the Unity editor will highlight that object in the sceneview / projectview. This helps to identify which object you actually working with. A common mistake is that you might be referencing a prefab in the project instead of the actual slot object in the scene. Another mistake might be that the actual slot object has already been destroyed and you still use a reference to the destroyed object.
ps: You might want to change the type of the field "thisIcon" from "GameObject" to "Image". Of course If you assigned an object previously you have to re-assign it in the inspector. However now you don't need to use GetComponent each time. To enable / disable the gameobject you can simply use
thisIcon.gameObject.SetActive( newState );
ins$$anonymous$$d. Every component has a reference to it's owning gameobject, so that access is essentially free.
Okay so here is the script where I call SetItem. Essentially I have a list of items and an array of item slots that gets generated when the game runs. Then, I call assignslots, which runs through the length of the current list of items (which is three as I call inventory.add 3 times) and it calls SetItem on the corresponding ItemSlot.
public class Inventory : $$anonymous$$onoBehaviour { public int inventorySize;
public List<Item> inventory;
public ItemSlot[] itemSlots;
public Item testItem;
public void Start(){
CreatePlayerInventory();
AssignSlots();
}
public void CreatePlayerInventory(){
if (transform.childCount > 0){
foreach (Transform child in transform){
GameObject.Destroy(child.gameObject);
}
}
itemSlots = new ItemSlot[inventorySize];
for (int i = 0; i < inventorySize; i++){
GameObject newSlot = Resources.Load<GameObject>("Item_Slot");
Instantiate(newSlot, transform);
itemSlots[i] = newSlot.GetComponent<ItemSlot>();
}
}
public void AssignSlots(){
inventory.Add(testItem);
inventory.Add(testItem);
inventory.Add(testItem);
// Item[] tempList = new Item[inventory.Count];
for (int i = 0; i < inventory.Count; i++){
if (inventory[i] != null){
itemSlots[i].SetItem(inventory[i]);
}
}
}
}
Those 3 lines don't make much sense and probably don't do what you think they do:
GameObject newSlot = Resources.Load<GameObject>("Item_Slot");
Instantiate(newSlot, transform);
itemSlots[i] = newSlot.GetComponent<ItemSlot>();
Your "newSlot" variable points to the prefab asset in your project. The Instantiate method returns the cloned object. It does nothing to the source object that you pass in. So what you do is storing the same prefab asset reference in each of your array elements. So as I already assumed you call your method on the prefab object in the project and not on your actual slot objects in your scene. You have to do:
ItemSlot slotPrefab = Resources.Load<GameObject>("Item_Slot").GetComponent<ItemSlot>();
for (int i = 0; i < inventorySize; i++){
itemSlots[i] = Instantiate(slotPrefab, transform);
}
Haha yeah, that seemed to fix it, thank you. I understand what you mean, I wrote what I had before trying specifically not to do what you just described. Looks like I had it a little off though. I appreciate it!