5
\$\begingroup\$

I am currently developing a Bullet Hell (shoot-em-up) game for my school project.
I have implemented Object Pooling for my game to recycle mainly bullets. (Though I could use it to recycle enemies in the future if I do need to.)

Currently I have tested this object pooling on bullet and it has worked.
I am looking to receive feedback about my code in order to see if I can do anything about it to make it more efficient and cleaner.

ObjectPool.cs

using System;
using System.Collections.Generic;
using UnityEngine;
using System.Linq;

public class ObjectPool : Singleton<ObjectPool> {

    private List<GameObject> objectPool;

    private void Awake() {
        objectPool = new List<GameObject>();
    }

    /// <summary>
    /// Add a gameobject to the object pool.
    /// </summary>
    /// <param name="objToPool">The gameobject to add to the object pool.</param>
    /// <param name="deactivateObj">Deactivate this gameobject upon storing into the object pool.</param>
    public void AddToPool(GameObject objToPool, bool deactivateObj = true) {
        objectPool.Add(objToPool);

        // If we need to deactivate this gameobject.
        if (deactivateObj) {
            // Set it to not active.
            objToPool.SetActive(false);
        }
    }

    /// <summary>
    /// Fetch a gameobject from the pool, with the gameobject containing the desired component.
    /// </summary>
    /// <typeparam name="T">The type of the component.</typeparam>
    /// <param name="removeFromPool">True if the fetched gameobject needs to be removed from the pool.</param>
    /// <returns>The respective gameobject. (Null if none is found)</returns>
    public GameObject FetchObjectByComponent<T>(bool removeFromPool = true) where T : MonoBehaviour {
        GameObject fetchedObject = null;

        // Foreach game object in the object pool
        foreach (var gameObj in objectPool) {
            // If this gameobject has the desired component.
            if (gameObj.GetComponent<T>() != null) {
                // Fetch this object.
                fetchedObject = gameObj;
                // End loop (an object with the desired component is found.)
                break;
            }
        }

        // If an object is fetched and we need to remove it from the pool.
        if (fetchedObject != null && removeFromPool) {
            // Remove the fetched object from the pool.
            objectPool.Remove(fetchedObject);
        }

        return fetchedObject;
    }

    /// <summary>
    /// Fetch an array of gameobjects that contains the desired component.
    /// </summary>
    /// <typeparam name="T">The type of the component the gameobject must contain.</typeparam>
    /// <param name="maxSize">The max size of the array returned. (Negative for limitless)</param>
    /// <param name="removeFromPool">True to remove the respective fetched gameobject from the object pool.</param>
    /// <returns>The respective fetched game objects.</returns>
    public GameObject[] FetchObjectsByComponent<T>(int maxSize = -1, bool removeFromPool = true) where T : MonoBehaviour {

        List<GameObject> temp = new List<GameObject>();

        // Loop through the object pool as long as the size limit it not reached.
        for (int i = 0; i < objectPool.Count && i < maxSize; ++i) {
            // If this current object contains the desired component.
            if (objectPool[i].GetComponent<T>() != null) {
                // Add to the temporary list
                temp.Add(objectPool[i]);
            }
        }

        var fetchedObjects = temp.ToArray();

        // If we need to remove the fetched objects from the object pool, remove.
        if (removeFromPool) {
            RemoveObjectsFromPool(fetchedObjects);
        }

        return fetchedObjects;
    }

    /// <summary>
    /// Fetch an array of gameobject based on the given condition.
    /// </summary>
    /// <param name="condition">The condition to check on when fetching gameobjects.</param>
    /// <param name="maxSize">The maximum size of the array returned. (Negative for limitless.)</param>
    /// <param name="removeFromPool">True to remove the respective fetched gameobject from the object pool.</param>
    /// <returns>The respective fetched game objects.</returns>
    public GameObject[] FetchObjectsByCondition(Func<GameObject, bool> condition, int maxSize = -1, bool removeFromPool = true) {
        // Fetch all the matching objects.
        var fetchedObjects = objectPool.Where(condition).ToArray();

        // If an array size limit is given.
        if (maxSize >= 1) {
            List<GameObject> temp = new List<GameObject>();

            // Loop through the fetched objects, adding to the list as long as the list stays in it's given size limit.
            for (int i = 0; i < fetchedObjects.Length && i < maxSize; ++i) {
                temp.Add(fetchedObjects[i]);
            }

            fetchedObjects = temp.ToArray();
        }

        // If we need to remove the fetched objects from the object pool
        if (removeFromPool) {
            RemoveObjectsFromPool(fetchedObjects);
        }

        return fetchedObjects;
    }

    #region Util

    private void RemoveObjectsFromPool(GameObject[] objectsToRemove) {
        // For each given object.
        foreach (var gameObject in objectsToRemove) {
            // Remove the given object from the object pool.
            objectPool.Remove(gameObject);
        }
    }

    #endregion
}

I am currently using Unity 2018.3.1f1, if that matters.

\$\endgroup\$
  • 3
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers (even if it is your own), doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Feel free to post the improved variant in a new question, if you want to, or add it into your existing answer. \$\endgroup\$ – Zeta Jan 19 at 19:17
2
\$\begingroup\$

I'm going to look over this in further detail later this evening, but from an initial review I would not remove objects from the pool and add them back later. I would only remove from the pool if the object should never be used again.


Adding and Removing

It's easy to understand a fundamental concept of adding and removing objects from a collection. However, when your collection is meant to hold reusable data, why are you removing from it every time you want an object that isn't active. Instead, just set the object to an active state to ensure the same object cannot be pulled twice. Then only remove objects once you determine they are no longer needed. Some eligible criteria for removing objects from your pool are:

  1. Enough time has passed that you no longer need to store above n objects.
  2. Your scene has ended and you no longer need any of the objects.
    • The exception to this is if your objects are shared across scenes; revert to case 1.

I will add more later this evening along with an example to further assist.

\$\endgroup\$
0
\$\begingroup\$

I have added some improvement of my own after some further researching, though I am still welcoming further suggestions/improvements to my code.

using System;
using System.Collections.Generic;
using UnityEngine;
using System.Linq;

public class ObjectPool : Singleton<ObjectPool> {

    private HashSet<GameObject> objectPool;

    private void Awake() {
        objectPool = new HashSet<GameObject>();
    }

    /// <summary>
    /// Add a gameobject to the object pool.
    /// </summary>
    /// <param name="objToPool">The gameobject to add to the object pool.</param>
    /// <param name="deactivateObj">Deactivate this gameobject upon storing into the object pool.</param>
    public void AddToPool(GameObject objToPool, bool deactivateObj = true) {
        objectPool.Add(objToPool);

        // If we need to deactivate this gameobject.
        if (deactivateObj) {
            // Set it to not active.
            objToPool.SetActive(false);
        }
    }

    /// <summary>
    /// Fetch a gameobject from the pool, with the gameobject containing the desired component.
    /// </summary>
    /// <typeparam name="T">The type of the component.</typeparam>
    /// <param name="removeFromPool">True if the fetched gameobject needs to be removed from the pool.</param>
    /// <returns>The respective gameobject. (Null if none is found)</returns>
    public GameObject FetchObjectByComponent<T>(bool removeFromPool = true) where T : MonoBehaviour {
        GameObject fetchedObject = null;

        // Foreach game object in the object pool
        foreach (var gameObj in objectPool) {
            // If this gameobject has the desired component.
            if (gameObj.GetComponent<T>() != null) {
                // Fetch this object.
                fetchedObject = gameObj;
                // End loop (an object with the desired component is found.)
                break;
            }
        }

        // If an object is fetched and we need to remove it from the pool.
        if (fetchedObject != null && removeFromPool) {
            // Remove the fetched object from the pool.
            objectPool.Remove(fetchedObject);
        }

        return fetchedObject;
    }

    /// <summary>
    /// Fetch an array of gameobjects that contains the desired component.
    /// </summary>
    /// <typeparam name="T">The type of the component the gameobject must contain.</typeparam>
    /// <param name="maxSize">The max size of the array returned. (Negative for limitless)</param>
    /// <param name="removeFromPool">True to remove the respective fetched gameobject from the object pool.</param>
    /// <returns>The respective fetched game objects.</returns>
    public GameObject[] FetchObjectsByComponent<T>(int maxSize = -1, bool removeFromPool = true) where T : MonoBehaviour {

        HashSet<GameObject> temp = new HashSet<GameObject>();

        // For counting how many objects we already have in the temporary list.
        int i = 0;
        // Go through the object pool.
        foreach (var obj in objectPool) {
            // If we have already reached the max array size.
            if (i >= maxSize) {
                // Exit loop.
                break;
            }
            // If the current object contains the desired component.
            else if (obj.GetComponent<T>() != null) {
                // Add to the temporary list.
                temp.Add(obj);
                // An object has been added.
                ++i;
            }
        }

        var fetchedObjects = temp.ToArray();

        // If we need to remove the fetched objects from the object pool, remove.
        if (removeFromPool) {
            RemoveObjectsFromPool(fetchedObjects);
        }

        return fetchedObjects;
    }

    /// <summary>
    /// Fetch an array of gameobject based on the given condition.
    /// </summary>
    /// <param name="condition">The condition to check on when fetching gameobjects.</param>
    /// <param name="maxSize">The maximum size of the array returned. (Negative for limitless.)</param>
    /// <param name="removeFromPool">True to remove the respective fetched gameobject from the object pool.</param>
    /// <returns>The respective fetched game objects.</returns>
    public GameObject[] FetchObjectsByCondition(Func<GameObject, bool> condition, int maxSize = -1, bool removeFromPool = true) {
        HashSet<GameObject> temp = new HashSet<GameObject>();

        // For counting how many objects we already have in the temporary list.
        int i = 0;
        // Go through the object pool.
        foreach (var obj in objectPool) {
            // If we have already reached the max array size.
            if (i >= maxSize) {
                // Exit loop.
                break;
            }
            // If the current object meets the condition.
            else if (condition(obj)) {
                // Add to the temporary list.
                temp.Add(obj);
                // An object has been added.
                ++i;
            }
        }

        var fetchedObjects = temp.ToArray();

        // If we need to remove the fetched objects from the object pool
        if (removeFromPool) {
            RemoveObjectsFromPool(fetchedObjects);
        }

        return fetchedObjects;
    }

    /// <summary>
    /// Fetch a gameobject with the given condition.
    /// </summary>
    /// <param name="condition">The condition based on to fetch the gameobject</param>
    /// <param name="removeFromPool">True to remove this gameobject from the object pool.</param>
    /// <returns>The fetched gameobject by the respective condition. (Null if none is found.)</returns>
    public GameObject FetchObjectByCondition(Func<GameObject, bool> condition, bool removeFromPool = true) {

        GameObject fetchedObject = null;

        // Loop through the object pool.
        foreach (var obj in objectPool) {
            // If this object's condition meets the given condition.
            if (condition(obj)) {
                // Fetch this object.
                fetchedObject = obj;
                // Stop loop (object is found.)
                break;
            }
        }

        // Remove this object pool if required.
        if (removeFromPool && fetchedObject != null){
            objectPool.Remove(fetchedObject);
        }

        return fetchedObject;
    }

    #region Util

    private void RemoveObjectsFromPool(GameObject[] objectsToRemove) {
        // For each given object.
        foreach (var gameObject in objectsToRemove) {
            // Remove the given object from the object pool.
            objectPool.Remove(gameObject);
        }
    }

    #endregion
}

I have changed from using a List<T> to using a HashSet<T> since I read up that HashSet has been proven to be faster than a list.
(though it takes away the functionality of being able to access a list through it's index, but I do not need the functionality.)

EDIT

(Further improvement)

using System.Collections.Generic;
using UnityEngine;

public class ObjectPool<T> {

    private Dictionary<T, HashSet<GameObject>> objectPools;

    public ObjectPool() {
        objectPools = new Dictionary<T, HashSet<GameObject>>();
    }

    /// <summary>
    /// Add the object to the respective object pool
    /// </summary>
    /// <param name="type">The type of object to add.</param>
    /// <param name="obj">The object to add into the object pool.</param>
    /// <param name="deactiveObj">True to deactive this object when adding to the pool.</param>
    public void AddToObjectPool(T type, GameObject obj, bool deactiveObj = false) {
        // Get the respective object pool.
        var pool = FetchObjectPoolByType(type);
        // Add the object to the pool
        pool.Add(obj);

        if (deactiveObj) {
            obj.SetActive(false);
        }
    }

    /// <summary>
    /// Fetch the first inactive object in the object pool by type.
    /// </summary>
    /// <param name="type">The type of object to fetch.</param>
    /// <returns>An inactive object based on the given type. (Null if none was found)</returns>
    public GameObject FetchObjByType(T type) {
        // Get the respective bullet pool.
        var pool = FetchObjectPoolByType(type);

        return FetchAnyInactiveObjIfExists(pool);
    }

    #region Util

    private static GameObject FetchAnyInactiveObjIfExists(HashSet<GameObject> pool) {
        GameObject fetchObj = null;

        // Loop through the pool
        foreach (var obj in pool) {
            // If this object is not active.
            if (!obj.activeInHierarchy) {
                // Fetch this object
                fetchObj = obj;
                // Stop loop, an inactive object is fetched.
                break;
            }
        }

        return fetchObj;
    }

    private HashSet<GameObject> FetchObjectPoolByType(T type) {
        // If the pool does not exists yet.
        if (!objectPools.ContainsKey(type)) {
            // Create it
            objectPools.Add(type, new HashSet<GameObject>());
        }

        // Return the pool with given type.
        return objectPools[type];
    }

    #endregion
}

Rather than dumping everything into a single HashSet pool I have decided to use generic implementation to further separate into different object pools of respective types.
Also, I am now using an Object Pool manager to manage the different instances of ObjectPool.

\$\endgroup\$
-1
\$\begingroup\$

This script is pretty good. But I recommend using a Queue instead of a List. Try this custom class Object Pooling, Custom pool classs

\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! You can improve your answer by providing a reason for this suggestion. This will help the OP and others to learn from it. \$\endgroup\$ – AlexV May 14 at 11:19

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.