2
\$\begingroup\$

In my game I have a simple loot system implemented. Enemies die and drop items which you can pick up. Picking up an item just draws that item's sprite in an inventory. All of this works properly if my inventory is open on the occasion of killing an enemy. However, if it is closed, i.e., if SetActive is set to false, then I get a strange NullReferenceException which I don't quite understand. Again, this happens if the inventory is closed when an enemy dies and generates an item, not when attempting to pick the item up. Attempting to pick the item up will work regardless of whether or not the inventory is open, insofar as the inventory was open when the enemies died and generated items. Error: NullReferenceException: Object reference not set to an instance of an object ItemPickup.Awake () (at Assets/Scripts/Items/ItemPickup.cs:16) UnityEngine.Object:Instantiate(GameObject, Vector3, Quaternion) ItemGenerator:GenerateItem() (at Assets/Scripts/Items/ItemGenerator.cs:40) ItemGenerator:Update() (at Assets/Scripts/Items/ItemGenerator.cs:32)

Here is the code.

Inventory scripts:

public class InventoryToggle : MonoBehaviour 
{
    GameObject inventory;

    bool inventoryIsOpen;

    void Awake()
    {
        inventory = transform.Find("Inventory").gameObject;
    }

    void Start()
    {
        // Deactivate in Start instead of Awake to allow other scripts to cache references beforehand
        inventory.SetActive(false);
    }

    void Update()
    {
        if (!inventoryIsOpen && Input.GetKeyDown(KeyCode.I))
        {
            inventory.SetActive(true);

            inventoryIsOpen = true;
        }

        else if (inventoryIsOpen && Input.GetKeyDown(KeyCode.I))
        {
            inventory.SetActive(false);

            inventoryIsOpen = false;
        }
    }
}

and

public class InventorySlotManager : MonoBehaviour 
{
    const int numSlots = 100;

    Item[] items = new Item[numSlots];
    public Image[] itemImages = new Image[numSlots];

    public void AddItem(Item item)
    {
        for (int i = 0; i < items.Length; i++)
        {
            if (items[i] == null)
            {
                items[i] = item;
                itemImages[i].sprite = item.itemSprite;
                itemImages[i].color = new Color(255, 255, 255);
                return;
            }
        }
    }
}

Attached to each enemy gameObject:

public class ItemGenerator : MonoBehaviour 
{
    EnemyHealth enemyHealth;
    ItemRandomizer itemRandomizer;
    public GameObject itemPrefab;

    ItemLabelDisplay itemLabelDisplay;

    bool itemGenerated;

    void Awake()
    {
        enemyHealth = GetComponentInParent<EnemyHealth>();
        itemRandomizer = new ItemRandomizer();
    }

    void Start()
    {
        // Item pool
        itemRandomizer.AddItemsToContainer(ItemDatabase.sword, 1);
        itemRandomizer.AddItemsToContainer(ItemDatabase.axe, 2);
    }

    void Update()
    {
        if (enemyHealth.isDead && !itemGenerated)
        {
            GenerateItem();
        }
    }

    void GenerateItem()
    {
        itemGenerated = true;

        GameObject generatedItem = Instantiate(itemPrefab, transform.position, Quaternion.identity);

        ItemClassAssociation itemClassAssociation = generatedItem.GetComponent<ItemClassAssociation>();

        itemClassAssociation.item = itemRandomizer.GetRandomItem();

        // Set the text in the item label to display the item's name
        itemLabelDisplay = generatedItem.GetComponentInChildren<ItemLabelDisplay>();
        itemLabelDisplay.itemName.text = itemClassAssociation.item.itemName;
    }
}

Attached to each Item's UI label

public class ItemPickup : MonoBehaviour, IPointerClickHandler 
{
    GameObject itemOnGround;
    ItemClassAssociation itemClassAssociation;
    InventorySlotManager inventorySlotManager;

    void Awake()
    {
        itemOnGround = transform.root.gameObject;
        itemClassAssociation = itemOnGround.GetComponent<ItemClassAssociation>();
        inventorySlotManager = GameObject.FindGameObjectWithTag("Inventory").GetComponent<InventorySlotManager>();
    }

    public void OnPointerClick(PointerEventData eventData)
    {
        PickUpItem();
    }

    void PickUpItem()
    {
        inventorySlotManager.AddItem(itemClassAssociation.item);

        Destroy(itemOnGround, 0.1f);
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I know this isn't the Code Review Exchange, but if I may offer a suggestion, the Update() method in your InventoryToggle class can be cleaned a bit by only checking the Input once: if (Input.GetKeyDown(KeyCode.I)) { inventory.SetActive(!inventoryIsOpen); inventoryIsOpen = !inventoryIsOpen; } \$\endgroup\$ – Daevin Jul 28 '17 at 17:47
4
\$\begingroup\$

FindGameObjectWithTag skips objects which are not active.

That means that if your inventory is disabled, this will return null:

GameObject.FindGameObjectWithTag("Inventory")

So this line in ItemPickup's Awake() method:

inventorySlotManager = GameObject.FindGameObjectWithTag("Inventory")
                                 .GetComponent<InventorySlotManager>();

becomes:

inventorySlotManager = (null).GetComponent<InventorySlotManager>();

And trying to call an instance method on a null reference triggers a NullReferenceException.

Things you can do to fix this:

  • Separate your inventory manager (model) from the visual inventory interface (view). Then you can toggle on/off or even delete the visual interface without upsetting the underlying system and game logic of the slot manager. This separation of concerns can set you up well for future iteration, if you ever need to change your UI presentation without upsetting all the code that depends on your inventory system.

  • Provide another method of getting access to the inventory slot manager, such as a static reference (eg. a singleton pattern), or a central game manager that's never toggled off that can direct requests for common dependencies.

\$\endgroup\$
  • \$\begingroup\$ I believe Unity uses C#6 now (forum.unity3d.com/threads/existing-c-6-users.417796/…), so you can use the ?. functionality. That way, doing inventorySlotManager = GameObject.FindGameObjectWithTag("Inventory")?.GetComponent<InventorySlotManager>(); will just end up with inventorySlotManager = null;. Check the Null-Conditional Operator section here for more information. \$\endgroup\$ – Daevin Jul 28 '17 at 16:51
  • \$\begingroup\$ @Daevin I believe that's only available in Unity2017, which is just one side or the other of the cusp of "not being beta" (i.e. I don't know if it's still beta and about to not-be or was made not-beta fairly recently). I know that I look forward to many of the 4.6 runtime features (co/contra-variance, hnnng!). \$\endgroup\$ – Draco18s Jul 28 '17 at 17:00
  • \$\begingroup\$ @Daevin this use of ? also solves only the problem "my script throws an exception" and leaves us with the problem "my script lacks a valid reference to the dependency it needs" \$\endgroup\$ – DMGregory Jul 28 '17 at 17:03
  • \$\begingroup\$ @DMGregory Why is inventorySlotManager null when an enemy dies and the inventory is deactivated and not when I try to pick up items and the inventory is deactivated. I don't see any dependency there. I would think the latter would happen... Also, do you have any pointers on how to implement option 1. Does Unity's API have a simple way to disable the visible part of a gameObject while leaving its logic unaffected? Thanks much! \$\endgroup\$ – embracethefuture Jul 28 '17 at 17:17
  • 1
    \$\begingroup\$ Because the offending code is in the Awake method, which is called when the item spawns, not when you attempt to pick it up. If you had your inventory open when the item spawned, then closed it before picking it up, then the reference you stored earlier in Awake is still valid by the time the pointer click fires. As for how to separate them: the clearest way is to simply make them separate GameObjects. You also have the ability to set components enabled or disabled one-by-one, so you could turn off just the renderer, but this is both fiddly and doesn't give you the separation of concerns wins. \$\endgroup\$ – DMGregory Jul 28 '17 at 17:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.