4
\$\begingroup\$

I have an Asteroids clone I'm working on and I want to see if my object pooling is written correctly. The code works I just want to know if there is anything else I need to change to make it more efficient.

My first class is my ObjectPoolManager.cs. This holds the bullets and the asteroids.

public class ObjectPoolManager : MonoBehaviour
{
//Setup some objects to be pooled.
public GameObject BulletObj;
public ObjectPool Bullets;
public GameObject AstroidObj;
public ObjectPool Astroids;

    void Start()
    {
    BulletObj = new GameObject("Bullets");
    Bullets = gameObject.AddComponent<ObjectPool>();
    Bullets.objectToPool = (GameObject)Resources.Load("Prefabs/Bullet");
    Bullets.LateStart(BulletObj);
    AstroidObj = new GameObject("Astroids");
    Astroids = gameObject.AddComponent<ObjectPool>();
    Astroids.objectToPool = (GameObject)Resources.Load("Prefabs/Astroid");
    Astroids.LateStart(AstroidObj);
    }
}

Here is my ObjectPool.cs. This class holds the logic for objects that need to be called on by my MachineGun.cs to call bullets and my AsteroidsManager.cs to call asteroids.

public class ObjectPool : BaseClass {
    public GameObject objectToPool;
    [Range (1,45)]//This is done for an easy view in the level editor
    public int pooledAmount = 25;
    public bool willPoolGrow = false;
    public bool gravity = false;

    public List<GameObject> _pooledObjects;

    public void LateStart(GameObject ParentToGameObject)
    {
        initPool(ParentToGameObject);
    }

    private void initPool(GameObject ParentToGameObject)
    {
        _pooledObjects = new List<GameObject>();
        for (int i = 0; i < pooledAmount; i++)
        {
            GameObject obj = (GameObject)Instantiate(objectToPool);
            obj.GetComponent<Rigidbody>().useGravity = false;
            _pooledObjects.Add(obj);
            obj.SetActive(false);
            obj.transform.parent = ParentToGameObject.transform;
        }
    }

    public GameObject GetPooledObject() {
        for (int i = 0; i < _pooledObjects.Count; i++) {
            if (!_pooledObjects[i].activeInHierarchy) {
                return _pooledObjects[i];
            }
        }
        if (willPoolGrow) {
            GameObject obj = (GameObject)Instantiate(objectToPool);
            _pooledObjects.Add(obj);
            return obj;
        }
        return null;
    }
}

Here is the MachineGun.cs that shoots bullets:

public class MachineGun : MonoBehaviour {
    public ObjectPoolManager _objectPoolManager;//Sets up an object pool.
    public Transform gunPoint; //Tip of your gun

    GameObject projectile;//This is your bullet to fire. 
    public float rapidFireTimeCap;
    public float rapidFireTimeCurrent;
    public float rapidfireTimeMultiplier;
    public bool fireBullet = true;

    #region Fire Accuracy
    public float accuracyFire;//Accuracy of your gun 
    public Vector3 accuracyFireRotation;
    #endregion 

    // Use this for initialization
    void Start () {
        _objectPoolManager = GameObject.Find("ObjectPoolHolder").GetComponent<ObjectPoolManager>();
        gunPoint = transform.Find("FirePoint");//Get the point of the gun
    }

    // Update is called once per frame
    void Update () {
        RapidFire();
        rapidFireTimeCurrent += Time.deltaTime;
    }

    void RapidFire() {
        //Fire bullets none stop
        if (rapidFireTimeCurrent > rapidFireTimeCap && fireBullet) {
            rapidFireTimeCurrent = 0.0f;
            FireBullet();
        }
    }

    void FireBullet () {
        projectile = _objectPoolManager.Bullets.GetPooledObject();
        if (projectile != null){
            projectile.transform.position = gunPoint.transform.position;
            projectile.transform.rotation = gunPoint.transform.rotation;
            float zdiff = Random.Range(-accuracyFire,accuracyFire);
            //projectile.transform.Rotate( new Vector3 (0,0,zdiff));
            projectile.GetComponent<Projectile>().FireMe();
        }
    }
}

Here is the Projectile.cs that gets fired. In this call's FireMe() I don't really understand how it is working.

public class Projectile : BaseClass {
    public Vector3 fireDirection, speedReset;
    public Rigidbody myRigidbody;
    public float offScreenAmount = 1.0f;
    public IProjectile iProjectile;

    public void Awake() {
        myRigidbody = GetComponent <Rigidbody> ();
        myRigidbody.useGravity = false;
    }

    // Use this for initialization
    protected void Start () {
        speedReset =  Vector3.zero;
    }

    public void OnCollisionEnter(Collision collision)
    {
        if (collision.gameObject.tag == "Asteroid")
        {
            collision.transform.SendMessage("TakeDamage", 1, SendMessageOptions.DontRequireReceiver);//This is how I avoid a GameObject.getComponent()
            OnDisable();
        }
    }
    public void FireMe() {
        InvokeRepeating("OnDisable", 2f, 0f);//Once fired this will disable the bullet after a few secs
        gameObject.SetActive(true); // Need to make this true before we give the fire command.
        iProjectile.Fire(gameObject);//I don't really understand how this works? 
    }

    void OnDisable() {
        transform.rotation = Quaternion.identity;
        myRigidbody.velocity = speedReset;
        gameObject.SetActive(false);
    }
}

IProjectile.cs

public interface IProjectile : IDamage, IPoolable{ 
    void Fire(GameObject go);
}

BulletRegular.cs

public class BulletRegular : Projectile {

    void Awake() {
    iProjectile = new FireTypeStraight();
    base.Awake();
    }
}

FireTypeStraight.cs

public class FireTypeStraight : BaseClass, IProjectile
{
    public Vector3 fireDirection;

    public FireTypeStraight()
    {
        fireDirection = Vector3.forward * 500 ;
    }

    public void Fire(GameObject rb)
    {
        rb.GetComponent<Rigidbody>().AddRelativeForce(fireDirection);
    }

    public void CauseDamage()
    {
        throw new System.NotImplementedException();
    }

    public void Enable()
    {
        throw new System.NotImplementedException();
    }

    public void Disable()
    {
        throw new System.NotImplementedException();
    }
}
\$\endgroup\$
5
\$\begingroup\$

Ok, Few small things.

Hard coding is a bad idea.


Sure, it may not be that relevant in this case but it is good practice to take anything that can change...out, such as your prefabs. Next up.

magic strings are scary.

again, knowing unity and what you are doing means its not a big deal but again...it is better practice to avoid doing it all together. finally

Repetition leads to more places to change and is messy

in general remove the little bits of code repetition where you can, the component decoration and composition unity does can look a little bit hard on the eyes when stacked. so move some of that into helper methods where appropriate

as for unity SPECIFIC things, the way to avoid the distasteful pattern of publicly allowing access to fields is:


[SerializeField]private float someFloat;

this will still show up in the inspector, you can also use [HideInInspector] where needed

So with that in mind a few small changes:


public class ObjectPoolManager : MonoBehaviour
{
    //Setup some objects to be pooled.
    [SerializeField]private GameObject BulletObj;
    [SerializeField]private ObjectPool Bullets;
    [SerializeField]private GameObject AstroidObj;
    [SerializeField]private ObjectPool Astroids;

    [SerializeField]private string PrefabsDirectory = "Prefabs";

    [SerializeField]private string BulletPrefabName = "Bullet";
    [SerializeField]private string AsteroidPrefabName = "Asteroid";

    const string ASTEROIDS = "Asteroids";
    const string BULLETS = "Bullets";

    void Start()
    {

        BulletObj = LoadGameObjectResource(BulletPrefabName);
        Bullets = CreateObjectPool(BULLETS,BulletObj);

        AsteroidObj = LoadGameObjectResource(BulletPrefabName);
        Asteroids = CreateObjectPool(ASTEROIDS,AsteroidObj);

        Astroids.LateStart(AstroidObj);
    }

    ObjectPool CreateObjectPool(string name,GameObject objectToPool)
    {
        var pool = new GameObject(name).AddComponent<ObjectPool>();
        pool.objectToPool = objectToPool;
        return pool;
    }

    GameObject LoadGameObjectResource(string resource)
    {
        return (GameObject)Resources.Load(String.Format("{0}/{1}",PrefabsDirectory,resource));
    }

}

Is that not a little easier to understand? Hopefully as an added bonus seeing how little the differences are in creating multiple pools, you might want to even go so far as to make a pool manager that takes in a custom struct of the data and manage tonnes of pools e.g:


[Serializable]
public class ObjectPoolConfig
{
    [SerializableField]public string PrefabName;
    [SerializableField]public bool LateStart = false;
}

then by being a serializable, unity will make all the fields available in the inspector, now you can set up a more modular system.


foreach(var config in ObjectPoolConfigurations) //this is the list of the configs...
{
    var objectToPool = LoadGameObjectResource(config.PrefabName);
    var pool = CreateObjectPool("PoolNameHere",BulletObj);
    pool.objectToPool = objectToPool;
    if(config.LateStart)
        pool.LateStart(objectToPool)
    ObjectPools.Add(pool);
}

so now, in the actual editor you just set "Bullets,False" - "Asteroids,True" and you can add new pool as easy as that. Nothing hardcoded. ...so yeah that's a small bit of homework if you want to make it more modular.


onto the ObjectPool class:

So, similar story with the private fields and so forth. aside form that, I have always hated the line:

GameObject obj = (GameObject)Instantiate(objectToPool);

equally as bad as the resource load line above, take both. make yourself some extension methods ro a helper class or something. For the non unity-ers you may be wondering why so extreme for one line, well, because once you create a gameObject in unity there is a 90% chance you will add or get a component. making an easier method to create a configured gameObject in a well phrased single function is a good idea.

as for error checking...... you are able to call "GetPooledObject" and if you didn't call LateStart, the list would be null and throw an exception, lazy load your list.


List<GameObject> PooledObjects { get { return _pooledObjects ?? (_pooledObjects = new List<GameObject>(); ) } }


so basically, if the pooledObjects doesn't exist, create it, then...return it. If it already exists...just return it.

this is just me being a bit picky, (as i am a linq fan) but you can simplify the GetPooledObject to:


PooledObjects.Where(x=>!x.activeInHierarchy).First();


I would also argue you could separate out the pool item from the look a little.


void AddPoolItem(){}

and then use:

for(int i = 0; i < pooledAmount; i++) AddPoolItem();

easier on the eyes...


MachineGun is next....

privatize fields as before. variablize the magic strings, either make them passable parameters or constants. (will you ALWAYS have "FirePoint" in your scene and will it always be called that?)

as for unity specific, GameObject.Find is SLOWWWWWWWWW. and a bad idea to traverse the whole tree to find something. Id made the poolManager a singleton, have it set itself on the Awake() function,


public static ObjectPoolManager Instance { get; private set; }

and then:

void Awake()
{
    if(Instance != null)
        throw new Exception("Trying to create second Pool manager, only one allowed per scene") //
    else Instance = this;
}

then in this script you basically say ObjectPoolManager.Instance, no searching, no problem, everything that needs pooling can access it.

Rapid fire seems fine, although unnecessary comments are....unnecessary. code should speak for itself, if it doesn't..rename it. Comments are reserved to complex things that cannot be distilled further or domain specific logic like explaining maths functions in code etc, RapidFire....rapidFires? huh, who knew :P

as for unity specific, you never put a constraint or a check to ensure that the gameObjects in the Bullets pool have a bullet script. yet in fireBullet you make that assumption...code could break. ESPECIALLY if you are loading from a resource and not managing the scripts additions personally....


onto projectile...

...SendMessage, ////This is how I avoid a GameObject.getComponent()

SendMessage is WORSE than a GetComponent! avoid it if you can. read up on event systems or signals.


I love me a good interface but...what's it for? you are not using that IProjectile in any logically path i see. Hell your "Projectile class doesn't even implement "IProjectile"! Maybe rethink the interface name if it refers to a fire type.....

\$\endgroup\$
  • \$\begingroup\$ Is it supposed to be [SerializableField]? It's trowing an error. [SerializeField] works. \$\endgroup\$ – Funlamb Jul 2 '15 at 4:08
  • \$\begingroup\$ In ObjectPoolManager you're missing the word return in the LoadGameObjectResource(string resource) method. It took me a while to see that. \$\endgroup\$ – Funlamb Jul 2 '15 at 4:31
  • \$\begingroup\$ Yes and Yes, good catch. editing code in notepad from memory is rarely a good idea. Unfortunately I was away from my dev machine when i wrote that... \$\endgroup\$ – apieceoffruit Jul 2 '15 at 13:18
  • \$\begingroup\$ Wow I just went through all of this information. Thank you so much. I'll be updating with the revised code in another post. \$\endgroup\$ – Funlamb Jul 4 '15 at 5:53

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.