Mixedrealitytoolkit-unity: Consider refactoring InputManager, MixedRealityCameraParent, Cursor to reduce dependencies

[genereddick]

Currently scenes that have any of InputManager, MixedRealityCameraParent or a Cursor all have hard dependencies on the others.

If this is an absolute requirement, then the documentation should reflect this. Otherwise, this dependency chain along with Unity's uncertain load and unload order creates extra complexity around scene load and unload as well as enabling and disabling objects, or having scenes that might not require one of the components.

MixedRealityCameraParent without an InputManager throws:

NullReferenceException: Object reference not set to an instance of an object
    HoloToolkit.Unity.InputModule.SetGlobalListener.OnEnable () (at 
    Assets/HoloToolkit/Input/Scripts/Utilities/SetGlobalListener.cs:16)

InputManager without a cursor throws:

Couldn't find cursor for "InputManager.SimpleSinglePointerSelector".   UnityEngine.Debug:LogErrorFormat(Object, String, Object[])
    HoloToolkit.Unity.InputModule.SimpleSinglePointerSelector:FindCursorIfNeeded() (at     Assets/HoloToolkit/Input/Scripts/Focus/SimpleSinglePointerSelector.cs:135)
    HoloToolkit.Unity.InputModule.SimpleSinglePointerSelector:Start() (at     Assets/HoloToolkit/Input/Scripts/Focus/SimpleSinglePointerSelector.cs:51)

DefaultCursor (I assume the other versions as well) without an InputManager throws:

NullReferenceException: Object reference not set to an instance of an object
    HoloToolkit.Unity.InputModule.Cursor.RegisterManagers () (at     Assets/HoloToolkit/Input/Scripts/Cursor/Cursor.cs:183)
    HoloToolkit.Unity.InputModule.Cursor.Start () (at assets/HoloToolkit/Input/Scripts/Cursor/Cursor.cs:136)
 ```

NullReferenceException: Object reference not set to an instance of an object
HoloToolkit.Unity.InputModule.Cursor.UpdateCursorTransform () (at Assets/HoloToolkit/Input/Scripts/Cursor/Cursor.cs:289)
HoloToolkit.Unity.InputModule.Cursor.Update () (at Assets/HoloToolkit/Input/Scripts/Cursor/Cursor.cs:143)
```

I'm not sure of all the interconnections and implications for how to refactor these, but for example, the MixedRealityCameraParent's SetGlobalListener OnEnable method requires an InputManager or it will throw a nullreference exception. Wrapping this in an if statement to check for null would remove the error but at the risk of the MixedRealityCameraParent never being added to the InputManager's GlobalListener if an InputManager is later added to the scene.

In this case, you could have the InputManager check for the existence of a MixedRealityCameraParent in the scene and then add it if it has not already been added to the GlobalListener array.

[jbienzms]

Good catch @genereddick. I was actually coming here to open a ticket on SetGlobalListener using the value returned from .Instance without actually checking for an instance. This (and many of the others you've pointed out) are a continuing struggle with the Singleton class.

The value returned by Singleton.Instance should _always_ be checked for null before using it, but unfortunately many classes in the toolkit still assume this property will not be null and start accessing the value using blindly. For example, SetGlobalListener.cs makes the following call without a check:

InputManager.Instance.AddGlobalListener(gameObject);

Instead, that line should probably be:

if (InputManager.Instance != null)
{
     InputManager.Instance.AddGlobalListener(gameObject);
}
else
{
     Debug.LogWarning("SetGlobalListener used but no InputManager found in the scene. This object will not be registered, even if an InputManager is added.");
}

Since this sort of check needs to happens a lot, we could look at adding a method to Singleton like this:

Singleton.IfInstance(Action action, string message)

And it could be called like this:

InputManager.IfInstance(()=>
{
    InputManager.Instance.AddGlobalListener(gameObject);
}, "SetGlobalListener used but no InputManager found in the scene. This object will not be registered, even if an InputManager is added.");

The message parameter could also have a default value since we know the type of Singleton being searched for. In that case, message would be optional and only needed to give information about how the scene is affected without the service.

I don't know if this is a worthwhile pattern, especially based on how much code it saves. But it may serve as a reminder to not just access .Instance blindly.

Anyway, just a thought. Thanks again for catching these genereddick.

[StephenHodgson]

@jbienzms The Singleton class was updated to make calling .Instance blindly a little better, but I agree. We should be calling AssertIsInitialized before making any blind calls.

[StephenHodgson]

Currently if the Instance is null we search the scene one time to find a suitable instance, then stop.

        /// <summary>
        /// Returns the Singleton instance of the classes type.
        /// If no instance is found, then we search for an instance
        /// in the scene.
        /// If more than one instance is found, we throw an error and
        /// no instance is returned.
        /// </summary>
        public static T Instance
        {
            get
            {
                if (!IsInitialized && searchForInstance)
                {
                    searchForInstance = false;
                    T[] objects = FindObjectsOfType<T>();
                    if (objects.Length == 1)
                    {
                        instance = objects[0];
                        instance.gameObject.GetParentRoot().DontDestroyOnLoad();
                    }
                    else if (objects.Length > 1)
                    {
                        Debug.LogErrorFormat("Expected exactly 1 {0} but found {1}.", typeof(T).Name, objects.Length);
                    }
                }
                return instance;
            }
        }

[jbienzms]

Yessir. I know Singleton has gone through a lot of work and has made major improvements over the past year. I especially like the work done to the Instance property itself as well as Awake.

In this particular scenario it may make sense to call AssertIsInitialized. Assuming you actually do want to throw an assertion (which may be the right action to take in this case for SetGlobalListener). But sometimes it's OK to proceed without the instance and just log a warning instead of asserting. I guess that's the path I was trying to simplify, but of course that path can also be solved for by just testing the Instance property value first before using it.

I noticed you've been a major contributor to the improvements with Singleton. Thank you!

[keveleigh]

I believe the blind Instance use in early functions (Awake, OnEnable) is by-design, because of the above Singleton changes Stephen posted. If you're getting a null ref there, your scene isn't set up properly. Is there a scenario where InputManager won't exist in Awake but might later?

https://github.com/Microsoft/MixedRealityToolkit-Unity/issues/1189#issuecomment-337378890

There's also IsInitialized that could be checked, but checking it in Awake() is incompatible with the singleton searching changes.

[jbienzms]

If you're getting a null ref there, your scene isn't set up properly.

I fully agree. The problem, as genereddick pointed out, is that there is no clue to the developer that the scene is setup incorrectly. For example, if you use the MixedRealityCameraParent prefab but don't ever add an InputManager to your scene you end up with a NullReferenceException instead of a helpful message informing you that an InputManager is required to use this prefab.

This is not the fault of the MixedRealityCameraParent prefab so much as it is the fault of the SetGlobalListener script that is part of that prefab. But again, the technical details of where the fault occurs aren't as important as the fact that the developer doesn't clearly know how to fix them.

There's also IsInitialized that could be checked, but that check is incompatible with calling .Instance in the early functions before it gets set in its own Awake().

Could you elaborate on that a little? I'm reading that to mean that IsInitialized is valid sometimes but not always and I'd like to understand that better.

@keveleigh, any thoughts on how we can help this particular issue? Specifically, do you have any suggestions on how we could improve the scripts or the prefab so that if a user adds a MixedRealityCameraParent but forgets to add an InputManager the Unity log messages could guide them to do the right thing?

[keveleigh]

If we change this to objects.Length != 1, that'll print out a message telling the user what was expected to exist but doesn't.

https://github.com/Microsoft/MixedRealityToolkit-Unity/blob/59726cdc3c0afccb8b8c3dc8d846a936e0ca1a3a/Assets/HoloToolkit/Utilities/Scripts/Singleton.cs#L38

Could you elaborate on that a little?

If Instance is called and the singleton exists in the scene but its Awake hasn't been called yet, the Instance call will search the scene and set itself. If IsInitialized is checked in a script's Awake, there's a chance Instance will be null (thus returning false) without the script searching the scene.

[jbienzms]

If we change this to objects.Length != 1, that'll print out a message telling the user what was expected to exist but doesn't.

That _would_ generate a warning in the log window that an InputManager was expected but not found. Currently there would also still be a NullReferenceException which could still be confusing. Don't get me wrong, this is _absolutely_ a step in the right direction. I would just really like to see this debug message:

"MixedRealityCameraParent requires an InputManager in the scene but none was found."

I don't think that message is possible because MixedRealityCameraParent is a prefab (not a script) and it's actually SetGlobalListener doing the work. I think to enable that level of guidance in a debug message would require either a configurable message on SetGlobalListener or to have MixedRealityTeleport interact with InputManager instead of using SetGlobalListener. Then the message would be:

"MixedRealityTeleport requires an InputManager in the scene but none was found."

Which isn't as clear as saying the prefab requires it, but it is technically more accurate.

I do realize this probably sounds like a nit(pick), and maybe it is. But as genereddick points out, helpful guidance doesn't just affect the MixedRealityCameraParent prefab. Similar guidance would be helpful for when InputManager is used without a Cursor and DefaultCursor is used without an InputManager.

It's true that a log message saying "An InputManager was requested but not found" could certainly point someone in the right direction. But there are some cases where Singletons are searched for and it's OK if they're not found and log messages would still be generated in this case.

Sorry, not trying to be difficult here. Just playing devils advocate. Thanks for the conversation.

[keveleigh]

"MixedRealityCameraParent requires an InputManager in the scene but none was found."

is totally possible!

leads to

My preference would be to find a way to do this in Singleton, though. This would be a very commonly reused bit of code, which is why we added the log in Singleton when more than one instance was found. I'm not sure the best way to get the GameObject name in there though.

Of course, if we're going through and refactoring anyway, like this Issue asks, perhaps it wouldn't be too bad to follow this pattern everywhere.

[StephenHodgson]

have MixedRealityTeleport interact with InputManager instead of using SetGlobalListener

That's exactly what we should be doing. The SetGlobalListner class is just a easy helper script anyway.

[StephenHodgson]

How does this look?

        public static T Instance
        {
            get
            {
                if (!IsInitialized && searchForInstance)
                {
                    searchForInstance = false;
                    T[] objects = FindObjectsOfType<T>();

                    switch (objects.Length)
                    {
                        case 0:
                            Debug.LogErrorFormat("An {0} was requested but not found in the scene.", typeof(T).Name);
                            break;
                        case 1:
                            instance = objects[0];
                            instance.gameObject.GetParentRoot().DontDestroyOnLoad();
                            break;
                        default:
                            Debug.LogErrorFormat("Expected exactly 1 {0} but found {1}.", typeof(T).Name, objects.Length);
                            break;
                    }
                }
                return instance;
            }
        }

Wish there was a way to know who called it.

[jbienzms]

is totally possible!

This looks perfect! But my question is, where did the name field come from that is used in your Debug.LogError message? And how did it get set to the string "MixedRealityCameraParent"?

Wish there was a way to know who called it.

Precisely. That would be ideal. But without doing a stack walk I don't think that's possible. Especially with a property. One of the reasons I was suggesting the IfInstance method above was to provide more information about the caller or the request. But again that might be overkill.

I believe @keveleigh may be proposing we add a name (or requester?) field to SetGlobalListner? If so that could help with the prefab issue because that field could be set as part of the MixedRealityCameraParent prefab. But that doesn't help with the other two mentioned in this issue and I agree it would be much nicer to solve this in Singleton itself. I just don't see a way to solve it through the .Instance property alone.

It may be worth discussing at this point that what we're really trying to solve for with the Singleton class is effectively Inversion of Control (or IoC). And here's a great MSDN Article that compares IoC Containers and Singletons.

It's probably overkill, but one possible solution would be to actually use an IoC Container. If we did, the Singleton base class would simply call Container.Register when it's activated. As with other IoC systems, the Container would then be responsible for making sure there is only once instance (or named instance, etc.)

Even if we didn't opt for a full IoC container, we could still opt for using some of the patterns. For example, instead of a .Instance _property_ we could opt for a .GetInstance _method_. This would allow .GetInstance to thrown an exception when the service is missing, and another method .GetInstanceOrDefault would work like the .Instance property does today. If these were methods instead of properties, we could also have optional string parameter(s) that describe the caller and / or use case. That information could then be propagated into any log or exception messages.

Anyway, just thinking out loud here again.

[StephenHodgson]

It may be worth discussing at this point that what we're really trying to solve for with the Singleton class is effectively Inversion of Control (or IoC). And here's a great MSDN Article that compares IoC Containers and Singletons.

Yeah I've been a big fan of IoC lately. I def like where you're going with this.

[keveleigh]

I believe @keveleigh may be proposing we add a name (or requester?) field to SetGlobalListener?

Even better! That name comes off the script's gameObject, so we get it for free!

I haven't yet had a change to thoroughly look through the IoC stuff yet, but it definitely looks interesting.

[wiwei]

Closing issues older than 180 days. If this is still an issue, please reactivate with recent information.