Singleton class inheritance not working

Hi all,

I'm having problems with my Singleton class that I am trying to make so other static classes (such as GameManager) can inherit from them and automatically be singletons. Below is the Singleton class I have made and below that is the beginning of the GameManager script. Basically it seems that its not working at all. The GameManager GameObject does not got into the DontDestroyOnLoad parent folder in the hierarchy once the game is loaded and none of the test debug text is executing . For the life of me I cannot work out why. Do I need to call the Singleton maybe in the GameManager script to enable it?

Also I suspect the FindObjectOfType<T>() is not optimal...

Any help would be greatly appreciated.

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4.  
5. public class Singleton<T> : MonoBehaviour where T : MonoBehaviour {
6.  
7.     private static T instance;
8.  
9.     public static T Instance
10.     {
11.         get
12.         {
13.                 Debug.Log("test1");
14.             if (instance == null)
15.             {
16.                 instance = FindObjectOfType<T>();
17.                 Debug.Log("test2");
18.             }
19.             else if (instance != FindObjectOfType<T>())
20.             {
21.                 Destroy(FindObjectOfType<T>());
22.                 Debug.Log("test3");
23.             }
24.             DontDestroyOnLoad(FindObjectOfType<T>().gameObject.transform.root);
25.             return instance;
26.         }
27.     }

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4.  
5. public class GameManager : Singleton<GameManager> {

 

I personally don't use FindObjectOfType.

Instead I use the awake and enable methods to make sure only 1 exists, and if not, destroy it.

Here's my Singleton, though it's a little more complex as I have added several features to it... including defining the lifetime of the Singleton.
https://github.com/lordofduct/spacepuppy-unity-framework/blob/master/SpacepuppyBase/Singleton.cs

This is the editor:
https://github.com/lordofduct/space...eEditor/Base/Inspectors/SingletonInspector.cs

 

FindObjectOfType is a really weird way to handle singletons. If you are going to use it, you should call it once and cache the result, there is no gaureentee it will return the same instance each time it's called.

The typical 'Unity way' to handle a singleton is to put a check in Awake to see if the instance already exists, and take action as appropriate.

So your code becomes.

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4.  
5. public class Singleton<T> : MonoBehaviour where T : MonoBehaviour {
6.  
7.     public static T instance {get; private set;}
8.  
9.     protected virtual void Awake (){
10.         if (instance != null){
11.              Debug.LogError("A instance already exists");
12.              Detroy(this); //Or GameObject as appropriate
13.              return;
14.         }
15.         instance = this;
16.     }
17. }

TL;DR; Singleton control belongs in the constructor (ie Awake), not in the accessor.

 

Thanks for the feedback guys, you are right. I had an inkling while following the course the way they were teaching was incorrect (main reaso I posted here). It did seem weird using the FindObjectofType as I had a feeling that would cause issues with more than one Singleton. Also as you say putting the code in the getter seems odd. I will move the code into the Awake methods as you suggest and see what happens

 

The major issue I've had with the standard Unity Singleton model is that if for example a button's onclick event calls into the Singleton, the Awake() event never gets called, so you start to get duplicates.

The alternative is to have even more scripts as glue between the button's event and the Singleton, creating more administrative nightmares.

Neither prospect is ideal or giving the developer options and should not force a certain design pattern on the developer.

Hopefully I've missed something obvious and someone helpful person will point out what.

 

What is the standard unity singleton model? Unity doesn't have a standard singleton that I know of.

Why has the Awake event never been called? Your gameobject is enabled right?

I don't know what your design is... so I don't know what extra glue you're adding.

You're not forced to use a specific design pattern. Again, unity doesn't have a standard singleton model that I know of. You don't have to use the Singleton pattern at all if you don't want to.

Not exactly sure what you're even referring to exactly at this point... if you could let me know, I may try to help out.

 

???

Why are you creating a new instance in OnClick?

You have two general options for singletons.

• Assign an instance in Awake, and check there to make sure you only have a single instance.
• Create the instance inside a property of the instance variable, and check there you only have a single instance.

 

I'm not. Unity is creating a new singleton instance behind the scenes when the OnClick event calls it. You'll only notice this if you dump out instanceID's from the singleton's constructor. You wont notice this if you just check ID's in awake.

I already do that, that's why I said standard unity singleton. But onclick is not triggering the awake function on the singleton, thus not checking for duplicates.


Just to recap for clarification.
A standard unity singleton gets created by being attached to a GameObject in an empty boot scene. It has the usual public instance handle and in Awake() has anti-duplication methods and DontDestroyOnLoad.
Everything is perfect, calling this will work fine and there will be only one instance of the singleton when called from code anywhere in the project and from across scenes.

Another scene is loaded and the single Singleton persists as you'd expect.

A UI button is clicked, the onclick event points to a method on the Singleton script.
Boom.. a 2nd instance had now just been created of the singleton but because Awake is not triggered, it can't test for duplicates and destroy them.

Again, unless you dump out instance ID's in the singleton's class constructor you might never notice there's a 2nd instance created.

 

I feel like there might be a bug in your code; I've never seen OnClick randomly duplicate objects for no reason. Maybe you could post an example of what you're doing in OnClick? Why does "the onclick event points to the GameObject that hosts the singleton"? The point of a Singleton is that you access it through the class name, like "MySingleton.instance.DoTheThing()". You shouldn't need to reference the actual GameObject inside a button.

 

Yeah that was a mistake in my last post, I'm accessing the Singleton Script, not the GameObject. (I've updated that post)
The code I'm running in OnClick() on the Singleton could be an empty method, it still behaves the same.

 

I still don't know why you keep calling this a 'Standard Unity Singleton'... there is NO standard unity singleton.

If your singleton code isn't working... ok, show us the code, maybe we can see what's wrong with it.

 

The overwhelming majority of Unity singletons are implemented using a variation of the same design pattern. This involves using a public instance variable, checking for duplicates and calling DontDestroyOnLoad in Awake(). Most of which is unique to Unity. This absolutely qualifies as being described as a "standard unity singleton" and everyone else seems to get this.

You seem to have a chip on your shoulder mate and that was obvious from your first response to my question. I don't care for that attitude or your help.

 

There must be something weird with your Singleton code; I can't get that to happen.

 

Pffffff.... hahahahahahahahaha.

OK mate.

 

Are you printing out the Singleton instance Id in the constructor (not awake) ?

Just for a sanity check I'll make another check of the code.

 

Oh wait that might explain it. Are you saying you have a constructor in a MonoBehaviour? You're not supposed to use constructors with MonoBehaviours because Unity will call them constantly at unexpected times, like every time it recompiles or loads a new scene. For MonoBehaviours, you should do anything constructory in Awake().

See this: http://answers.unity3d.com/questions/862032/c-constructor-in-monobehaviour.html

It should throw a warning in the console when you do it... don't just ignore all those yellow caution icons like most people do.

 

Not normally no, for the very reasons you mention.

Removing the contractor shows the same issue.

Line 1 Awake() is being triggered when the singleton is first created with an ID of -57558

Line 2 shows when the button accesses it via OnClick and it's got the same ID.

Line 3 But another button on another scene in line uses a different instance.



I

 

Show us a code example. Because the issues you are talking about just don't happen in my singleton implementation. No matter which method I use.

(I also agree that there is no such thing as a 'standard Unity singleton'. I have about three different implementations that I use depending on the situation.)

 

I've written a new simplified class to isolate the issue that manifests the same problem.

Setup
The singleton script below lives on a GameObject called "SingletonScriptHost" that sits on the first "boot" scene by itself.
After starting, scenes are additively loaded and unloaded.
One of those scenes has a button on it that has its OnClick event tied to TestSingleton.ChangeScene();


What happens
The debug output shows that the first instance of the singleton was created right at the very start of the game execution (as expected) and it obtained the instance ID of -10546 as shown when its TestSingleton.Awake() method.
Later on the 3rd scene when the Button is clicked, the Id is now -4826, which means a new instance of the Singleton exists. Not surprising since singleton duplication checks are in awake() method that the button never triggers.

Code (CSharp):

1. using System;
2. using System.Collections;
3. using System.Collections.Generic;
4. using UnityEngine;
5.  
6.  
7. public class TestSingleton : MonoBehaviour
8. {
9.  
10.     public static TestSingleton Instance = null;
11.  
12.     private void Awake()
13.     {
14.         Debug.Log("TestSingleton.Awake() GetInstanceID=" + this.GetInstanceID().ToString());
15.  
16.         if (Instance == null)
17.         {
18.             Instance = this;  //This is the first Singleton instance. Retain a handle to it.
19.         }
20.         else
21.         {
22.             if (Instance != this)
23.             {
24.                 Destroy(this); //This is a duplicate Singleton. Destroy this instance.
25.             }
26.             else
27.             {
28.                 //Existing Singleton instance found. All is good. No change.
29.             }
30.         }
31.  
32.         DontDestroyOnLoad(gameObject);
33.     }
34.  
35.     public void ChangeScene(string sceneName)
36.     {
37.         Debug.Log("TestSingleton.ChangeScene() GetInstanceID=" + this.GetInstanceID().ToString());
38.     }
39.  
40. }

As I mentioned in the first line of my OP; If the issue is due to relying on singleton duplication detection in Awake() that simply won't work with Button.OnClick(), then I'll have to change my implementation.

Sure there's other implementations of Singletons, I never said or implied there wasn't. If you look around the internet, 90% of the implementations of Unity singletons use exactly the same overall design and thus fulfill the definition of the word standard.

 

from the moment I saw the title of this thread I knew something was wrong. Singletons and Inheritance should not even belong in the same sentence.

Singletons are designed to solve a very, very specific design problem and as a cost they violate almost every other programming principal in the book. The fact that they are misused in the Unity community to such a degree should be criminal. They support inheritance poorly, they can't be polymorphic, and they are a pain in both multi-threading and unit testing as well. I also die a little inside when a see a singleton coupling code.

so for that reason I rarely implement Singletons, only for the case it was actually designed for. ScriptableObjects are so much better in terms of access, testing, persistance, polymorphism, threading, coupling, race conditions, and yes... inheritance. and if MonoBehaviour messages are needed making a runner to run the scriptableObject is dead simple.

spread the word, end the tyranny of the singleton

 

To be fair, I did kinda accidently derail and extend the OP's original post. That's my fault. Sorry!

 

I don't see anything obviously wrong with the code, although it's somewhat redundant.

Sure.

You've got the basic Awake style, which is the same pattern you used.

Code (CSharp):

1. static Singleton instance;
2.  
3. void Awake () {
4.     if (instance) {
5.         Debug.LogError("An instance already exists", instance);
6.         Destroy (this);
7.     }
8.     instance = this;
9.     DontDestroyOnLoad(GameObject);
10. }

Then you can run with a lazy pattern

Code (CSharp):

1. Singleton _instance;
2.  
3. Singleton instance {
4.     get {
5.         if (_instance) return _instance;
6.         GameObject clone = new GameObject ("Singleton");
7.         DontDestroyOnLoad (clone);
8.         _instance = clone.AddComponent<Singleton>();
9.         return _instance;
10.     }
11. }

The other patterns are variations of these two. Sometimes I don't need a GameObject at all, so you can just use the lazy pattern with a vanilla new.

Often I don't want true singletons, I want a new instance to replace the old one. Great to set up scene specific singletons.

Of course often Singletons are a bad idea altogether. But that's another thread.

 

I tried to read and follow what this post was going on about.. Is it resolved or not?

 

fascinating how people complicateing simple things

 

That's something which should have raised some attention.

According to your explanation, the SingletonScriptHost lives in the initial boot scene and the button in another scene (SceneX).
Is that correct?

So in the editor, the button in SceneX can only access the Scene X gameObjects (but there's no way for the SingletonScriptHost to be assigned) and besides that, all the assets.

If you had another instance in SceneX, this could be wired in the editor but the reference would be missing once it calls 'Awake' due to its destruction and the callback wouldn't be available anymore, so the button qouls ignore it.

Based on that, I assume you've accidently wired up a prefab. That's another 'instance' that exists more or less inactively and that one wouldn't run Awake or any other methods, thus sitting around and responding as a 'valid' subscriber.

Just a little note for the ones that might find this useful:

That implementation should be used with care.
I'd still combine this one with an Awake() approach. Otherwise you can just drag another instance into your scene or someone uses AddComponent etc.., Sure, the getter will only ever get the one that has been set up lazily, but the other instances could still mess around.

 

I was thinking the same thing about the button referencing in the wrong scene. I asked if this was ever resolved, before I went into explaining how to maybe fix that up.. Anyhow

 

Ah, this is your problem. There's one TestSingleton on the game object in the first scene, and there is one (I'm guessing) on a prefab in your project folder that you're dragging onto the button. So it's not actually duplicating anything; you're just directly referencing the one that's sitting on the prefab rather than the one you put into a scene. Like I was saying earlier, you pretty much never want to grab a copy of the Singleton component or drag it anywhere onto any fields; the whole point of a Singleton is that you only access it through the class name and its static "Instance" field. You're bypassing that entirely by dragging a copy onto the button and calling its methods directly. The button should have its own script with an OnClick that it calls, like this:

Code (csharp):

1. public class ButtonScript : MonoBehaviour {
2.     public void OnClick()
3.     {
4.         TestSingleton.Instance.ChangeScene("Test");
5.     }
6. }

Then there's no need to drag a copy of the TestSingleton object anywhere; you don't have to care where it is, you just access it through the static Instance variable.

 

Cool, that's exactly the suggestion I was waiting to make if he still hadn't gotten it

 

Found your issue.

You have UnityEvent instances that are referencing TestSingleton in subsequently loaded scenes.

You say that come scene 3, the id changes. Yeah... because the button in scene 3 is referencing the singleton you put in scene 3.

SURE, you destroyed said Singleton. But it was already created. And here's the thing... the 'Destroy' method only destroys the unity engine side of things (the C++ object). The mono/C# object still exists in memory, and it'll get cleaned up by garbage collection when it becomes eligible.

BUT, because your various UnityEvents (OnClick) is referencing that object, the mono/C# object isn't getting cleaned up.

Anyways, even if it did... it would have no object to reference. Because UnityEvent is referencing the object directly, not the Singleton.Instance property.

This is why I have a 'SingletonProxy' that I use for such things... you tell it the type of the singleton, and it reaches out for it. So that you don't have this overlapping issue.
https://github.com/lordofduct/space.../blob/master/SpacepuppyBase/SingletonProxy.cs

...

See, this is why I asked how YOU are implementing it, and to give an example of the problem actually happening. Just because you're using a likeness of a common implementation, we don't know exactly how you implemented or what bugs may exist in your specific implementation.

Oh wait... sorry, forget everything I said. It's all just a chip on my shoulder.

Or was that chip on yours?

 

That's the first thing that comes to mind, but the event system takes care of destroyed objects even though they do technically still exist in memory in that case.

 

I haven't checked the implementation of the event system to be sure.

In general terms if you keep a reference to a MonoBehaviour it's possible to call code on it after it's been destroyed.

 

Ah yeah if there is a Singleton in every scene and you're directly referencing one, it's the same problem as directly referencing a prefab. Still, same solution: don't directly reference Singletons anywhere, use Singleton.Instance.

 

Yup, I'm aware of that. However, the event system apparently takes these objects into account and prevents any calls to these objects even though a non-Unity-API touching method would run just fine.

 

I owe you a pint, that's exactly what's gone wrong.

 

Indeed thanks. I had identified this as an option in my first reply but it seemed to get ignored by subsequent posts and things started to go around in circles. I got there in the end though.

 

So the problem is solved, I just have to use the second method I mentioned at the start and use script to glue the UI events to the singleton. I really wanted to avoid this sort of design, but hey ho.

Thanks again @Suddoha and everyone else that helped.
Sometimes the most obvious mistakes stare you right in the face!.

 

Thanks!