Mixedrealitytoolkit-unity: Why have SingleInstance?

[StephenHodgson]

Only difference between SingleInstance and Singleton is that it doesn't depend on the instance being set in Awake.

What's the benefit?

I propose removing SingleInstance and replacing its usage with Singleton.

[keveleigh]

The benefit is that a dev can reference a SingleInstance anywhere in code, without the Awake restriction. This is the way Singleton used to be, and there are devs who prefer that pattern. See #1039, for example.

[keveleigh]

It's a restriction because I can't reference a Singleton in any Awake function with the guarantee that it exists yet.

[StephenHodgson]

Hmm, so then we should replace Singleton with SingleInstance?

[keveleigh]

I personally think we should, because it simplifies the experience on the developer's side (no need to call base.Awake() if I want my own Awake(), as well as being able to call the Instance anywhere in my code). I know others disagree though, as seen in the other issue referenced above and the fact that it was changed in the first place.

There's also some discussion from @darax about the two at the bottom of #486 (along with the rest of the thread discussing Singleton, some of which seems to have been fixed already).

[StephenHodgson]

I agree. My only goal here is to reduce the amount of complexity and redundancy.

[darax]

I was disappointed last year when I discovered that the HoloToolkit singleton was changed. There were a cascade of issues that were created by making that change, discovered slowly over time. While creating MR250, I uncovered more scripts that didn't fully comply with the new behavior. I also frequently find myself wanting to access a singleton during an Awake() function.

I had a choice:

1. Revert singleton to its old behavior.
2. Create a new singleton like class with the old behavior.
3. Fix the scripts that weren't 'complying' and fully comply myself

In order to avoid breaking a bunch of scripts that might rely on the new behavior, I chose option 2. I agree it is sub optimal to have two 'singleton' classes, and maybe I should have picked option 3. I simply couldn't bring myself to do it.

[StephenHodgson]

I could look into this and see if I can get the classes to work with the current Singleton.

In the meantime, this issue does indeed come up often. We should look into what actions we should do to help this out.

https://github.com/Microsoft/MixedRealityToolkit-Unity/issues/1031#issuecomment-333630887

I think the biggest part of the problem is that we're attaching instances to GameObjects in the first place.

Ideally these instances should live on their own and not be tied to scene object references, but alas this is Unity, and in order to have a script run it needs to be a Component that you've got to attach to a GameObject that has to be enabled in your scene.

It's a split in the traditional paradigm of how software engineers usually treat programming, and is usually one of the biggest gripes traditional software engineers have with using Unity.

[darax]

Both approaches fall apart if the game object is disabled. I would much rather use the approach in SingleInstance because it is simpler and sets fewer traps.

(or am I wrong, and Awake() gets called for disabled objects?)

[StephenHodgson]

Awake does not get called for disabled GameObjects.

[timGerken]

In the event the SingleInstance is on a disabled game object (to start with), Instance should allow for finding zero instances (without spamming the console window).

[StephenHodgson]

Well this is annoying as well.

Apparently OnEnable or even Start _could_ be called before other GameObject's Awake call.

http://answers.unity3d.com/questions/1081660/onenable-called-before-awake.html

[StephenHodgson]

@timGerken btw if you don't wanna spam the console but still check if the Instance is valid you can always call AssertIsInitialized or IsInitialized before calling the actual Instance

[timGerken]

OnEnable may not be safe, but I'm pretty sure Start is.

MonoBehaviour.Start

The Awake function is called on all objects in the scene before any object's Start function is called. This fact is useful in cases where object A's initialisation code needs to rely on object B's already being initialised; B's initialisation should be done in Awake while A's should be done in Start.

[StephenHodgson]

Nope. Start is also not safe. Remember this is called on a _per GameObject basis_.

[keveleigh]

Why is Start also not safe? Your link only talks about OnEnable and Awake

https://docs.unity3d.com/Manual/ExecutionOrder.html

Note that for objects added to the scene, the Awake and OnEnable functions for all scripts will be called before Start

[StephenHodgson]

Why is Start also not safe?

Because each of the Unity APIs are called on a per object basis, and even though one object's Start function has been called, another object may still get their Awake called much later.

That specific link does call out Awake but it's easy to search for other examples with Start.

[timGerken]

The documentation for Start clearly says that Awake is called for all objects in the scene _before_ any object's Start method is called.

[keveleigh]

even though one object's Start function has been called, another object may still get their Awake called much later.

That statement is only true for objects instantiated during runtime.

[StephenHodgson]

I think we should write a test to make a definitive statement.

If this was the case, then we wouldn't have the need for the Script Execution Order.

[StephenHodgson]

The documentation for Start clearly says that Awake is called for all objects in the scene before any object's Start method is called.

[From Documentation]

Note that for objects added to the scene, the Awake and OnEnable functions for all scripts will be called before Start, Update, etc are called for any of them. Naturally, this cannot be enforced when an object is instantiated during gameplay.

Yeah this _should_ be the case, but it doesn't actually seem to be if we're getting scripts OnEnable and Start functions called anyway.

[StephenHodgson]

Oh I see, Awake and OnEnable always get called before Start.

So potentially one scripts Awake and another's OnEnable could be but not their Start functions.

[StephenHodgson]

Okay so how come we don't assign an Instance in Singleton if we don't find one like in @timGerken's lastest change to SingleInstance?

Was there a reason why we assign the Instance in Awake instead of when we ask for the Instance?

        private static T _Instance;
        public static T Instance
        {
            get
            {
                if (_Instance == null)
                {
                    T[] objects = FindObjectsOfType<T>();
                    if (objects.Length == 1)
                    {
                        _Instance = objects[0];
                    }
                    else if (objects.Length > 1)
                    {
                        Debug.LogErrorFormat("Expected exactly 1 {0} but found {1}", typeof(T).ToString(), objects.Length);
                    }
                }
                return _Instance;
            }
        }

[timGerken]

Yes, because the SingleInstance object may be on a GameObject that is disabled at scene load time. If that's the case, you'd expect instance to be null and want to do a null check without spamming the console. It might also be instantiated in a scene that is loaded at a later time. Also requiring a null-check.

[StephenHodgson]

I understand why you changed it, I just want to know why we don't apply to changes to Singletion

[timGerken]

We probably should.

[StephenHodgson]

I'm just trying to remove a largely duplicated class, that essentially does the same thing.

[StephenHodgson]

We can keep the Awake assignment, but still do the more expensive FindObjectsOfType<T>(); if our instance is null.

[StephenHodgson]

What if we move away from using the Singleton pattern?

[darax]

We will (or at least I will) still want to have a convenient way to create globally accessible app state. That will probably carry with it the same lifetime problems under a different name.

The concern about FindObjectsOfType being expensive is valid when someone calls .Instance frequently when the object is either not in the scene or disabled. I wonder if we could, instead, document the performance drawback of frequently polling for an instance to become available. Or we could rename SingleInstance to Singleton and Singleton to PollableSingleton.

[StephenHodgson]

It's only expensive if FindObjectsOfType doesn't return anything each time, correct?

[StephenHodgson]

Could we also only have it call FindObjectsOfType once, and every subsequent time only return null unless Awake sets it?

[darax]

that would be okay. So long as it is done in a way where if I forget to base.Awake() the only thing I lose is setting the instance. base.Awake() will need to tolerate the instance being set from FindObjectByType<> .

[StephenHodgson]

Check out that PR and let me know your thoughts.

[darax]

that looks good.