Can someone check this code for me?

Hello, i wrote this object pooling code with some help from my friend...can someone please check and tell me if this seems fine?..there are no errors shown, but the game gets laggy over time

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4.  
5. public class ECarSpawner : MonoBehaviour
6. {
7.     public GameObject car;
8.     int carNo;
9.     public float maxPos = 28f;
10.     public float delayTimer = 0.5f;
11.     float timer;
12.  
13.     //POOLING
14.     public int initialPoolObjects = 10;
15.     public List<GameObject> carPool;
16.  
17.     // Use this for initialization
18.     void Start()
19.     {
20.         timer = delayTimer;
21.         carPool = GenerateObjectPool();
22.  
23.     }
24.  
25.     // Update is called once per frame
26.     void Update()
27.     {
28.         timer -= Time.deltaTime;
29.         if (timer <= 0)
30.         {
31.             Vector3 carPos = new Vector3(Random.Range(28f,-18f), transform.position.y, transform.position.z);
32.             //Instantiate(car[carno], carPos, transform.rotation);
33.             SpawnCar(carPos, transform.rotation);
34.             timer = delayTimer;
35.         }
36.     }
37.  
38.     List<GameObject> GenerateObjectPool()
39.     {
40.         GameObject _car = null;
41.         List<GameObject> _carPool = new List<GameObject>();
42.         for (int i = 0; i < initialPoolObjects; i++)
43.         {
44.             _car = Instantiate(car);
45.             carPool.Add(_car);
46.         }
47.  
48.         return _carPool;
49.     }
50.  
51.     GameObject SpawnCar(Vector3 pos, Quaternion rotation)
52.     {
53.         GameObject _car = carPool.Find(c => c.activeInHierarchy == false);
54.  
55.         if (!_car)
56.         {
57.             _car = Instantiate(car);
58.             carPool.Add(_car);
59.         }
60.  
61.         _car.transform.position = pos;
62.         _car.transform.rotation = rotation;
63.         _car.SetActive(true);
64.  
65.         return _car;
66.     }
67.  
68.     void DestroyCar(GameObject car)
69.     {
70.         car.SetActive(false);
71.     }
72. }
73.  

e!

 

Sure, I edited your code and put notes inline:

Code (csharp):

1.  
2. using System.Collections;
3. using System.Collections.Generic;
4. using UnityEngine;
5.  
6. public class ECarSpawner : MonoBehaviour
7. {
8.  
9.     //@NOTE - LOD - use regions to organize your code
10.     #region Fields
11.  
12.     public GameObject carPrefab; //@NOTE - LOD - have more descriptive variable names
13.     public float maxPos = 28f;
14.     public float delayTimer = 0.5f;
15.  
16.     //@NOTE - LOD - explicitly define private, and organize them away from publics for clarity
17.     private float timer;
18.     //private int carNo; //@NOTE - LOD - remove unused variables, especially if private
19.  
20.     //POOLING
21.     public int initialPoolObjects = 10;
22.  
23.     //@NOTE - LOD - this should probably not be serialized, and encapsulate it so no one junks your pool
24.     [System.NonSerialized]
25.     private List<GameObject> carPool;
26.  
27.     #endregion
28.  
29.     #region CONSTRUCTOR
30.  
31.     // Use this for initialization
32.     void Start()
33.     {
34.         timer = delayTimer;
35.         carPool = GenerateObjectPool();
36.     }
37.  
38.     #endregion
39.  
40.     #region Methods
41.  
42.     // Update is called once per frame
43.     void Update()
44.     {
45.         timer -= Time.deltaTime;
46.         if (timer <= 0)
47.         {
48.             Vector3 carPos = new Vector3(Random.Range(28f,-18f), transform.position.y, transform.position.z);
49.             SpawnCar(carPos, transform.rotation);
50.             timer = delayTimer;
51.         }
52.     }
53.  
54.     List<GameObject> GenerateObjectPool()
55.     {
56.         //@NOTE - LOD - the '_' is a common private field denoter, using it for local variables to a function can be considered unreadable in some communities.
57.         //iT'D BE LIKE LOWER-CASING THE FIRST LETTER OF A SENTENCE AND UPPERING THE REST.
58.         List<GameObject> _carPool = new List<GameObject>();
59.         for (int i = 0; i < initialPoolObjects; i++)
60.         {
61.             var car = Instantiate<GameObject>(carPrefab); //@NOTE - LOD - you can inline variables
62.             carPool.Add(car); //@NOTE - LOD - and here is why I bring up the '_'... you've just added the cloned car to 'carPool' and not '_carPool'. When you later return this list and set 'carPool', all your work is gone
63.         }
64.  
65.         return _carPool;
66.     }
67.  
68.     GameObject SpawnCar(Vector3 pos, Quaternion rotation)
69.     {
70.         //@NOTE - LOD - same argument about the '_'
71.    
72.         //@NOTE - LOD - (== false) is stylistically odd, use the ! operator
73.         GameObject _car = carPool.Find(c => !c.activeInHierarchy);
74.  
75.         if (!_car)
76.         {
77.             _car = Instantiate(car);
78.             carPool.Add(_car);
79.             //@NOTE - LOD - you should consider putting a cap on your pool here, otherwise you could end up with an unlmited number of cached prefabs which is bad for memory
80.         }
81.  
82.         _car.transform.position = pos;
83.         _car.transform.rotation = rotation;
84.         _car.SetActive(true);
85.  
86.         return _car;
87.     }
88.  
89.     void DestroyCar(GameObject car)
90.     {
91.         //@NOTE - LOD - car was ambiguous from the local field 'car' above, but since I changed it, nolonger an issue
92.         car.SetActive(false);
93.     }
94.  
95.     #endregion
96.  
97. }
98.  

Some added notes.

You should probably divide the job of pooling and spawning rules away from one another and generalize this more. A pool doesn't necessarily care how/when things get spawned (your timer), only that things can be spawned and recycled. And a spawner shouldn't care what it's spawning (car), just that it spawns something on some rule (interval of timer).

This isn't a bad first attempt, though some heavy refactoring would need to be done to get it to what I'd consider a reusable/functional script.

On top of generalizing I'd probably want to separate available/in-use instances from one another so they're not all in one list. Add a cap to the number of managed objects. And have a way to track managed objects instead of just if they're inactiveInHierarchy (since what if I want to get an instance and need to disable it for some reason... now the manager thinks it's available for recycling).

Probably also want a way to signal to the car/object that it's being recycled so it can reset its internal state.


...

Other people will probably have other stuff to add to the topic.

 

Hey, thanks for taking out the time and working on this! I somewhat understand what you are trying to say..i created instantiate/destroy scripts initially but resorted to object pooling for feasibility and that is why spawning and pooling are together...Also,i have a new problem..I want to add an array to this(as i want 6 different enemy cars and not one), any idea if that is possible with object pooling?

 

Yes, but I'm heading out the door for holiday things.

I'll be around later.

 

Sure!,have all the fun...Merry Christmas