Object pooling utility #53
Conversation
DSDubit
requested review from
AndrewThompson85,
borntocompile,
george-mackayshore and
kkjamie
Utils/ObjectPool.cs
Outdated
|
|
||
| public static class ObjectPool | ||
| { | ||
| private abstract class AbstractPool |
There was a problem hiding this comment.
Given theres no implementation, this may as well just be an interface,
Utils/ObjectPool.cs
Outdated
| { | ||
| public abstract class PoolableComponent : MonoBehaviour | ||
| { | ||
| protected internal virtual void OnAddedToPool() |
There was a problem hiding this comment.
I'm not sure we need internal here, duck will currently always exist in the project's AssemblyCSharp assembly
Utils/ObjectPool.cs
Outdated
|
|
||
| namespace DUCK.Pooling | ||
| { | ||
| public abstract class PoolableComponent : MonoBehaviour |
There was a problem hiding this comment.
Correct me if i'm wrong, but the API here requires you to subclass this component in order to be poolable. If this is so can I suggest a slight API modification; make this one non abstract and we can just add this to our object if we want to pool it, otherwise I can see a lot of subclassing this just to add it, or unnecessary inheritance. Composition could be our friend here.
I also see opportunity to add a function here to return the object to the pool (destroy it without destroying it), that way we don't need access to the pool from within the object that is pooled. We can just go GetComponent<PoolableComponnet>().ReturnToPool()
If it's non abstract I'd probably think about renaming it. Soime suggestions PoolableBehaviour, PooledItem, PooledElement
Utils/ObjectPool.cs
Outdated
| } | ||
| } | ||
|
|
||
| public T Remove() |
There was a problem hiding this comment.
Perhaps Get would be a better name, as it sounds less destructive, theoretically this is where you would (without pooling) be creating the object, not removing it. Removal is just part of the pooling mechanism.
|
Looks promising 👍 |
| OnRemovedFromPool.SafeInvoke(); | ||
| } | ||
|
|
||
| public void CopyFrom(PoolableObject instance) { } |
There was a problem hiding this comment.
Why is this needed? To avoid specifically being able to copy a camera?
I'd expect CopyFrom to work but to interact with the pool.
There was a problem hiding this comment.
No - to mimic the behaviour of Unity's Instantiate(T original) - see ObjectPool.cs:67
There was a problem hiding this comment.
In that case it might be worth making this abstract or throw. Right now if I'm meant to expect pool.Instantiate(someObject); to work out of the box I may not know that I need to implement my own CopyFrom.
Especially given that sometimes (with the null coalescing) its going to use the original and working GameObject.Inantiate(original);
|
|
||
| namespace DUCK.Pooling | ||
| { | ||
| public class PoolableObject : MonoBehaviour |
There was a problem hiding this comment.
Minor bit of weirdness, PoolableObject inherits MonoBehaviour but this whole feature references objects as if that's the class you're dealing with but object is a little further up the inheritance tree.
There was a problem hiding this comment.
I don't follow - where does the ObjectPool reference objects as object/Object?
| return obj ?? Resources.Load<T>(resourcePath); | ||
| } | ||
|
|
||
| public static void Destroy(PoolableObject obj) |
There was a problem hiding this comment.
When an object is released to a pool it would be good to invoke a cleanup step.
There was a problem hiding this comment.
What would you be cleaning up? PoolableObject already expoded AddedToPool and RemovedFromPool events for any components on the gameObject to listen for - this is where I'd expect custom cleanup actions to take place
There was a problem hiding this comment.
I'd be cleaning up events :) and state. I didn't spot those add and remove methods though.
Not sure if publicly subscribe-able events will eventually prove problematic in a system that is dealing with objects that at any given time can be hard or soft destroyed.
Utils/ObjectPool.cs
Outdated
| { | ||
| var obj = (pooledObjects.Count > 0) | ||
| ? pooledObjects.Dequeue() | ||
| : null; |
There was a problem hiding this comment.
Might be worth considering letting the pool manage object creation as well since it handles the other end of the lifecycle and would be much more convenient for the end user.
Its not unusual for pools to return a new instance of the object where you have null if you think that would be less intuitive then a GetOrCreate() method would work.
There was a problem hiding this comment.
It does - ObjectPool is a private class so this will never be called by the end user. See the static Instantiate functions which are for external use - is that not what you mean? They perform the 'create or retrieve' step, either from a prefab or resourcePath
|
|
||
| namespace DUCK.Pooling | ||
| { | ||
| public static class ObjectPool |
There was a problem hiding this comment.
A few comments documenting how the pool works, when it should be used and any pitfalls would be good.
Part of those comments might be a some a warning against blind or excessive usage of a pool which could cause (assumption alert!) slow down of the GC or creating a large memory footprint and all the usual issues with premature optimisation.
There was a problem hiding this comment.
I tried to keep this relatively lightweight, so the pools aren't even created until you call ObjectPool.Instantiate - this will create and maintain a pool for the objects to return to when soft-destroyed via ObjectPool.Destroy, but is otherwise transparently identical to the standard Instantiate and Destroy functions, in that they're called as fallbacks if there isn't a pool to use.
I will, though, add guards to prevent a pooled object being pooled a second time, and to prevent the pool from returning destroyed objects, since it's still possible to hard-Destroy something while it's in the pool.
| CreatePool<T>(); | ||
| } | ||
|
|
||
| return obj ?? GameObject.Instantiate<T>(original); |
There was a problem hiding this comment.
Either line 72 needs removing or this line needs to be return GameObject.Instantiate<T>(original);. The reference to obj will always be null if the program reaches here.
|
@DSDubit & others, this PR is going stale. How do we move this along? |
No description provided.