0
\$\begingroup\$

In my game, the player picks up items. When every pickup is picked up, you can exit the level. This is how I coded it now, I don't like it and I am writing here to learn if there are better ways to do it. I am only gonna show the pickup manager code, as the pickup objects only add themselves on enable, and remove themselves on disable.

[DefaultExecutionOrder(-1)]
public class PickupManager : MonoBehaviour
{
    public static PickupManager Instance;

    private List<PickupableObject> objectsInstances = new List<PickupableObject>();

    private int objectsInLevel = 0;
    private int objectsRemoved = 0;

    private void OnEnable()
    {
        Instance = this;
    }

    private void OnDisable()
    {
        Instance = null;
    }

    public void Add(PickupableObject d)
    {
        if (!objectsInstances.Contains(d))
        {
            objectsInstances.Add(d);
            objectsInLevel++;
        }
    }

    public void Remove(PickupableObject d)
    {
        if (objectsInstances.Contains(d))
            objectsInstances.Remove(d);
    }

    public void ObjectPickedUp(PickupableObject d)
    {
        objectsRemoved++;
    }

    public float GetPercentage()
    {
        if (objectsInLevel > 0)
        {
            return (float)objectsRemoved / (float)objectsInLevel;
        }
        return 1;
    }
}

Cons:

  1. It is a Singleton, but at the same time not, as each of the scenes has a different one. I don't like it
  2. At the initialization of this script, it doesn't know how many pickups are there, until the end of the current frame

Pros:

  1. It works, but only if I do additional checks (in the first frame the number of picked up objects is the same as the number of pickups, 0)

I thought about another way to handle it, that is: making the manager spawn the pickups itself.

Cons:

  1. You can't see the pickups in the editor, unless you are running the game. Big no no as I need to place them perfectly, as each pickup has a different model.

Pros:

  1. The script doesn't need a public static instance
  2. The script knows the number of pickups at the very start and doesn't need workaround to work correctly

Is there a way to handle this better?

\$\endgroup\$
1
  • \$\begingroup\$ Instead of saying there is a better/ worse solution (there is always something better), I would reframe it and either pick one of the cons you would like to be gone or point out a flaw/ something that your current manager can't handle. If your code works, does the job and is even reuseable - don't overthink it and work on the rest of your game. As for your second con first point: you can execute functions in editor as well without play mode if that would already help you \$\endgroup\$
    – Zibelas
    Jan 8 at 19:50

1 Answer 1

1
\$\begingroup\$

You should do a bit of reading on code architecture, clean code and design patterns if you want to know how to make your code better on your own.

But to address your question, for a start, there are several things wrong with your Singleton. First of all, your Instance reference is publicly available, which means it can be changed from any other place in your program. Thankfully, property getters and setters don't have to have the same access modifiers, meaning that you can have a public getter, and a private setter.

private static PickupManager s_instance;
public static Pickup Manager Instance { get => s_instance; private set { s_instance = value; } }

Now the Instance is managed internally by your class, good. The next problem is there is nothing stopping the game from having multiple instances because when every PickupManager is enabled the Instance is set to that object. What I would do is move that functionality to the Awake method and check if an instance already exists, if it does then disable the component.

private void Awake()
{
    if (Instance != null)
    {
        this.enabled = false;
        return;
    }
    Instance = this;
}

And... now that that's out of the way, you don't need to make your manager a Signleton. Now it all comes down to this: will there be a fixed amount of pickups, or will more pickups be spawning during the level, will some be removed, etc. You should first consider what you actually want for your game.

Easiest solution is to have a fixed amount of pickups. You need an IPickup interface from which all pickups will inherit, and the interface should have a OnPickup event. The PickupManager will at it's Awake method find all GameObjects which have a component that inherits the IPickup interface, and place them in a Dictionary. For each pickup object as a key, there is a boolean pair which specifies if the pickup is picked up. The Manager should also listen to the OnPickup event for each item so that it can mark it as pickedup. One important thing about this is to not destroy the objects when they are picked up, but just not render their meshes and collision, and change the state of the script controlling the pickup.

using System.Collections.Generic;

public class PickupManager : MonoBehaviour
{
    private Dictionary<IPickup, bool> _pickups;

    private void Awake()
    {
         _pickups = new();
         foreach (IPickup pickup in Object.FindObjectsOfType<IPickup>())
         {
             _pickups.Add(pickup, false);
             pickup.OnPickup += (item) => { _pickups[item] = true; };
         }
    }

    public int PickedUpItems()
    {
        int ctr = 0;
        foreach (bool pickedup in _pickups.Values)
        {
            if (pickedup)
                ctr++;
        }
        return ctr;
    }
}

public interface IPickup
{
    public delegate void PickupEventHandler(IPickup item);
    public event PickupEventHandler OnPickup;
}

public class ExamplePickup : MonoBehaviour, IPickup
{
    public float SomeValue = 5f;
    public event IPickup.PickupEventHandler OnPickup;

    private void OnTriggerEnter(Collider col)
    {
        if (!col.transform.CompareTag("Player"))
            return;
        GetComponent<MeshRenderer>().enabled = false;
        GetComponent<BoxCollider>().enabled = false;
        OnPickup?.(this);
    }
}

Now if you want items to spawn during the game is to have a separate Spawner (factory) class which notifies the Manager of this changes. A step further would be to make all your pickups the same prefab, and have an Identifier component which specifies the look, functionality, properties, etc. of the item, and you would specify these using a custom EditorWindow. Then the items would be initialized by the Spawner class when the level is loaded, and registered to the PickupManager.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Consider using FindObjectsByType instead of FindObjectsOfType, so you can choose the unsorted version for better performance. \$\endgroup\$
    – DMGregory
    Jan 8 at 22:20

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .