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月1日f1, if that matters.

Zeta
19.6k2 gold badges57 silver badges90 bronze badges
asked Jan 19, 2019 at 7:12
\$\endgroup\$
1
  • 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\$ Commented Jan 19, 2019 at 19:17

3 Answers 3

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.

answered Jan 22, 2019 at 20:18
\$\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.

answered Jan 19, 2019 at 19:10
\$\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

answered May 14, 2019 at 6:58
\$\endgroup\$
1
  • 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\$ Commented May 14, 2019 at 11:19

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.