I’m a little conflicted here, so I want to find out why this would be a bad idea.
I have a Manager class which is setup as a singleton. Now within that class, I have a function which mostly gets called from other classes, to store data into an array defined in my Manager. Currently, that function is just public, and I call it like this:
// call from Awake() or OnEnable() in another class
Manager.Instance.MyFunction()
I was thinking about making MyFunction() static instead, is that a bad idea? Here’s my code below, maybe you can tell me if using a static function would be good/bad for this specific scenario:
public class HUDManager : MonoBehaviour {
// Singleton access
private static HUDManager _instance;
public static HUDManager Instance
{
get
{
if( !_instance )
{
_instance = FindObjectOfType( typeof( HUDManager )) as HUDManager;
}
return _instance;
}
}
public enum HUDType
{
MAIN = 0,
MENU,
CAMERA,
CONTROLS,
RACE_STATS,
LAPS,
LAPTIME,
SPEED,
FPS,
DEBUG,
NUM_OF_HUD_TYPES
}
public class HUDElement
{
public GameObject hudObject;
public GUITexture gTexture;
public HUDElement( GameObject hudObject, GUITexture gTexture )
{
this.hudObject = hudObject;
this.gTexture = gTexture;
}
}
private HUDElement[] HUDElementsArray = new HUDElement[(int)HUDType.NUM_OF_HUD_TYPES];
// =================================== AWAKE FUNCTION =================================== //
void Awake ()
{
if( !this.enabled ) return;
if( !_instance ) _instance = this;
}
// Thinking of making this static for easier access, is that a bad idea?
public void SetHUDElement( HUDType hudType, GameObject hudObject )
{
HUDElement incomingHUD = new HUDElement( hudObject, hudObject.GetComponent<GUITexture>() );
HUDElementsArray[(int)hudType] = incomingHUD;
if( !HUDElements.ContainsValue( incomingHUD ))
{
HUDElements.Add( hudType, incomingHUD );
}
else
Debug.LogWarning(incomingHUD + " already exist in dictionary");
}
All I want to know is if making that function static have any pros/cons, or if it’s just bad practice in general?
It’s not necessarily a ‘bad’ idea… it’s just mixing design patterns.
Which is weird.
The usefulness of a singleton is that it’s an actual object and can be referenced like an object. But if you plan on just using it as a static class, then why not just make it a static class?
It’s bad design to mix a static class and a singleton. But it’s not necessarily going to break anything.
In Unity, a singleton GameObject (a mish-mash of static and not-static unique to Unity) is very useful - for instance, Coroutines require a GameObject to run on, and you can’t access local variables from static functions (without an ‘instance’)
However, it’s a very good idea to make only private functions be non-static. Force the ‘user’ to call your controlling static functions - don’t let them access your instance at all. The singleton class uses its object as necessary (for Unity functions, coroutines, and data access through instance).
The more control each object has over itself, the easier it is for you to track down bugs when they happen.
In short - yes, I’d recommend making that function static.
Thanks for the info guys. I learned C# from scratch all while using Unity, and as you know, C# in Unity can be very different - in the way you code things - then regular C#…which is why I agree with both of you, and I am still just as conflicted hahaha!
But I think I might go with “mixing design patterns = bad”, because I already mix design patterns too much and I need to pick one and go with it.
The way Loius put it, they’re just describing a ‘static class’.
Static classes can have references to objects held in them to do special things. In Loius’s example, said object can be a gameobject. This isn’t really a singleton as GameObjects aren’t singleton, and the class wrapping is publicly known as static.
That methodology is fine enough. Though I’m not sure why you’d make the private methods non-static if the GameObject is the ref object… what use is an instance ref to a class with only static methods public?
The instance is used as a way to assign prefabs and such to the mostly-static class. Static variables can’t be changed in the Inspector (unless you write a custom editor, but that’s effort!), so public variables are used. But it’s generally a bad idea to let other classes know about my public variables (especially for controlling scripts), so we make it as hard as possible to get at them.
The class itself is like this: (simplified to show functionality)
class Game extends MonoBehaviour {
private static var instance;
var explosionPrefab : GameObject;
function Awake() {
if ( instance ) {
Debug.LogError("Only one Game can exist. This object will be ignored.");
return;
}
instance = this;
}
static function ExplodeAt( position : Vector3 ) {
instance.StartCoroutine( ExplodeCorou(position) );
}
static function GetExplosion() : GameObject {
return Instantiate( instance.explosionPrefab, Vector3.zero );
}
private function ExplodeCorou( pos : Vector3 ) {
var go : GameObject = GetExplosion();
yield WaitForSeconds(1);
Destroy( go );
}
}
Think of it as a static class, but you can initialize it in the Inspector instead of using a hundred Find functions on startup or having to recompile every time you want to change a number variable.
It forces its own singleness (even though you technically can create two Games, the second one will throw an error), and it does need its single class at times, so I think of it as a singleton.
now that’s just a weird mish-mash of patterns… you’ve mixed static-class, singleton, and component/composite.
Not saying it fails, it gets the job you want done. And in the game design world, getting the job done is more important.
Personally I create classes that aren’t components/monobehaviours, and just create and manage a gameobject that I can hook into. This I can then make a singleton or a static class independent of the component pattern. I don’t really like having to force break functionality implemented by another pattern, to get my current pattern working. If I’m doing that, I feel it’s time for another class.
using UnityEngine;
using System.Collections.Generic;
using com.spacepuppy.Utils;
namespace com.spacepuppy
{
public sealed class StateManager : System.IDisposable
{
#region Fields
private static StateManager _instance;
public static StateManager Instance
{
get
{
if(_instance == null) _instance = new StateManager();
return _instance;
}
}
public event System.EventHandler Update;
private GameObject _stateObject;
private UpdateEventHooks _updateHook;
private AbstractState _currentState;
#endregion
#region CONSTRUCTOR
private StateManager()
{
_stateObject = new GameObject("StateObject");
_updateHook = _stateObject.AddComponent<UpdateEventHooks>();
_updateHook.UpdateHook += this.OnUpdate;
UnityEngine.Object.DontDestroyOnLoad(_stateObject);
}
#endregion
#region Properties
public AbstractState Current
{
get { return _currentState; }
}
#endregion
#region Methods
public void GoToNullState()
{
var lastState = _currentState;
_currentState = null;
if (lastState != null !lastState.DoNotDestroyOnExit) UnityEngine.Object.Destroy(lastState);
}
public AbstractState GoToState<T>() where T : AbstractState
{
if (_currentState != null _currentState.GetType() == typeof(T)) return _currentState;
AbstractState lastState = _currentState;
_currentState = _stateObject.AddOrGetComponent<T>();
_currentState.Init(this);
if (lastState != null)
{
lastState.OnExitState(_currentState);
lastState.enabled = false;
}
_currentState.enabled = true;
_currentState.OnEnterState(lastState);
if (lastState != null !lastState.DoNotDestroyOnExit) UnityEngine.Object.Destroy(lastState);
return _currentState;
}
public AbstractState GoToState(System.Type tp)
{
if (!ObjUtil.IsType(tp, typeof(AbstractState))) return null;
if (_currentState != null _currentState.GetType() == tp) return _currentState;
AbstractState lastState = _currentState;
_currentState = _stateObject.AddOrGetComponent(tp) as AbstractState;
_currentState.Init(this);
if (lastState != null)
{
lastState.OnExitState(_currentState);
lastState.enabled = false;
}
_currentState.enabled = true;
_currentState.OnEnterState(lastState);
if (lastState != null !lastState.DoNotDestroyOnExit) UnityEngine.Object.Destroy(lastState);
return _currentState;
}
public AbstractState GoToState(string sStateName)
{
var tp = System.Type.GetType(sStateName);
return GoToState(tp);
}
#endregion
#region Event Hook
private void OnUpdate(object sender, System.EventArgs e)
{
if (this.Update != null) this.Update(this, e);
}
#endregion
#region IDisposable Interface
public void Dispose()
{
UnityEngine.Object.Destroy(_stateObject);
}
#endregion
}
}
this of course could also be accomplished as a static class, I just don’t do that because I need to be able to pass references of the StateManager around for various things (I left out huge chunks of this class as they weren’t pertinent).
That’s a solution I’ve honestly never thought of once I went into Unity. That Inspector is just so handy, I can hardly imagine a class with settings I can’t fiddle with. But I totally see the point of it.
So there’s your answer, I guess. Do what gets the job done, and whatever is most comfortable for you. As long as you can easily keep track of what you’re doing you’ll do great.
To tell you the truth guys, that’s a little over my head at the moment. I read through the StateManager code posted above and although I understand most of it, I definitively feel uncomfortable. I am not at that level yet
Louis hit it on the head I think, as long as I can easily keep track of what I am doing then I should go with what makes the most sense to me, while still writing relatively clean/efficient classes.
Thanks a lot for both of your tips and points of view guys, you have been very helpful!
After reading the codes above, a question just came to me:
The life scope of class static variable is “across different scenes”, is it? So if we implement the singleton pattern by inheriting MonoBehaviour and it exists in scene A, the first time the static var _instance will be assigned and used with no problem, but once another scene B gets loaded, the gameobject which the singleton script is attached to will be destroyed, while the value of _instance remains(is it?). When again scene A is loaded, and _instance is not null but points to a component already destroyed, we’ll be possibly getting into trouble with this.
Is my thinking correct? If this is the case, I guess we’ll need an OnDestroy function in the class that safely reset the _instance to null?
Of my years and years of programming, i do think a singleton is a bad design.
Sure its easy for quick things, just like its pretty easy to build a doghouse. But when your application grows and you end up needing 4+ singletons to work with each other and other unknown components, i find the singleton design to be lacking. (AudioManager, InputManager, GameManager, AppSettings, KeyboardMan, AccelerometerMan, OrientationMan, ButtonManager, ResourceMan, MessageManager, Notifications, EventMan… just some singletons I’ve seen frequently.)
Dependency Injection or the Inversion of Control seems to solve the issues of a singleton design but I haven’t bothered to write DIY di/ioc in unity. I ran across a method a while back but i considered myself very green with unity.
Scripts are always loaded and in memory on every scene regardless of what you do. So static vars are always in memory, hence why you must null it out if a scene doesn’t use it.
Anyways, there’s a generic design for a singleton on the wiki, i wrote one very similar because of the memory leak that our applications were displaying especially when going back and forth between a main menu and a game scene. http://wiki.unity3d.com/index.php?title=Singleton This one is close to our implementation.
The Singleton discussion is so all over the place everywhere! So many ppl have completely different opinions on Singletons that it’s hard for a junior programmer like myself to figure out when and where to use it sometimes.
lordofduct makes a good point though…“Any design, poorly, overly, or mis-used is going to be bad.”
And znoey, thanks for the link to the wiki, I found the generic Singleton design very interesting, and I am currently using it. So far so good
an overly used design doesn’t mean its bad, and a good design prevents poor use of itself. The composite pattern seems to be an all-in-one solution for gaming, but is a tooth-brush really a battle ship?!?
There’s only one gotcha on singletons that I’ve had to fix on numerous projects and that’s determining what the dependencies are for something that happens to be a singleton. Usually if two classes are tightly coupled for something that they accomplish, then neither of them should be a singleton.
The best use of a singleton that I’ve seen is always something that has no dependencies. A good example is the accelerometer on a droid or iOS device or some application settings that get used by everyone but don’t depend on anyone to accomplish their job. The hard part is for you (the programmer) to NOT use a singleton for something that seems like the highlander. “There can only be one!” which is something I see all the time on junior programmers. Its that very reason why i see so many “Managers” that are singletons as there should only be one for some specific task.
overly means, in excess, more than you ought to have
I didn’t say if you use something excessively, that makes the thing bad. It makes doing it excessively bad, as in using it where it isn’t needed and it becomes bloat and complication. And yes that is bad.
I should also point out, my StateManager code I posted is actually not a Singleton in my project. I used it as an example as the real Singleton object is to much to be showing here. I actually have a ‘Game’ singleton, who has a member that is the StateManager, and the StateManager is not a singleton.
But really, there is only one StateManager, and that is the use of a Singleton. So one could make it a Singleton. There’s no other purpose for the Singleton pattern than to enforce the existence of “only one”… like the highlander.
Do note the second part of the “intent”, encapsulation, is the whole “dependency” thing that znoey is going on about.
Hey guys, thanks for both your inputs/explanations, I really appreciate it!
So if I understand correctly, I probably made a junior programmer mistake myself by making all my managers Singletons? Let me elaborate a little:
I have my Main Manager class called Managers, attached to a Managers GameObject. Managers is a Singleton, and it caches references to all my other Managers: ControlsManager, CameraManager, HUDManager, ObjectManager etc…all these managers are also Singletons, and from what I read so far, maybe they shouldn’t be?
Since Managers caches references to all the other manager classes, I access each of them like this: Managers.Instance.Controls, Managers.Instance.HUD, Managers.Instance.Object etc…
But - and that’s where it gets a little confusing for me - all the other manager classes should always ONLY have ONE instance, hence why I made them Singletons. Is that the junior programmer mistake you talk about znoey (sorry if I read you wrong)?
lordofduct, thanks for the link, I’m reading through it right now
There may be no need for ControlsManager and CameraManager and the sort to be a singleton. Your Managers class gives you that global point of access.
That’s up to you to decide though. Is there something those managers do that makes it necessary they only exist once.
For instance in my example code above which is the StateManager… I actually don’t have it a Singleton, and instead have my ‘Game’ class the one true Singleton. There IS only one game, and there should only be one game… I need to enforce that. But really… there could be more than one statemanager down the line. Let’s say I decide to make my game a local multi-player game… one player could be in the ‘gameplay’ state, and another in the ‘pause menu’ state… I’d need two StateManagers for this.
All I’d have to do to get this working is where I get my StateManager I have a way to define which player I want the StateManager for. All my other code is pretty much ready for it because it references said StateManager like an object… it doesn’t care if it’s actually a Singleton or not, it just cares that it received a StateManager.
This is also why I brought up the bit about the ‘static methods’. If they’re static methods, you can’t do this. Because static methods are global… they aren’t on the object.
Thanks a lot, that made a lot of sense and really helped! Now I perfectly understand what you meant about static methods, and also understand when to use/not use a Singleton a lot better. I guess there could be more then 1 ControlsManager (player/AI), or even more then 1 CameraManager etc…the only one that can’t have another instance in my case is Managers, since it controls the game, in a way.
It’s a good day when I learn something new before mid-day hahaha!
