Mixedrealitytoolkit-unity: Getting intermittent null ref on SetGlobalListener

[genereddick]

Using the new MixedRealityCameraParent prefab, InputManager, etc. with updated code (both on master and 2017.2.0 versions) I'm getting an intermittent null ref exception on

InputManager.Instance.AddGlobalListener(gameObject)

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)

Specifically, the instance variable is null.

In the current version of the script, OnDisable and OnDestroy wrap the RemoveGlobalListener call in an if statement

private void OnDisable()
    {
        if (InputManager.Instance != null)
        {
            InputManager.Instance.RemoveGlobalListener(gameObject);
        }
    }

But OnEnable does not:

private void OnEnable()
    {
        InputManager.Instance.AddGlobalListener(gameObject);
    }

Presumably, this should be

private void OnEnable()
    {
       if (InputManager.Instance != null)
        {
            InputManager.Instance.AddGlobalListener(gameObject);
        }
    }

[StephenHodgson]

Because of the way we've updated the Singleton class, you shouldn't have to check if it's null during OnEnable because it will search the scene for you, and return the instance if it's present.

Sounds like maybe the InputManager is disabled or missing from your scene.

[genereddick]

My project has multiple scenes, and the issue seems be related to the timing of creating and destroying Singleton objects when switching scenes. But it is very difficult to debug, it is not even completely consistent in that, though the multiple event system warning (below) appears consistently, the InputManager.Instance errror happens between some scenes but not others and there isn't a clear pattern so the this may be completely wrong.

There is an EventSystem attached to the InputManager and there seems to be a moment when the previous scene's InputManager and its EventSystem haven't been destroyed yet and the new scene's InputManager and its EventSystem are being created. When the new scene loads, I first get a warning that:

Multiple EventSystems in scene... this is not supported
UnityEngine.EventSystems.EventSystem:OnEnable()

The incoming InputManager instance is null, and starts getting created by the get method of the Instance property, but searchForInstance = false, perhaps because the old instance is still in existence, so it jumps out of the loop and never gets created.

By the end, everything looks correct, there is only a single InputManager which is enabled and a single EventSystem.

[StephenHodgson]

I suggest bringing the input system into the next scene with you by calling DontDestroyOnLoad see if that helps.

[genereddick]

I can recreate this error with a minimal installation: three scenes including prefabs for MixedRealityCameraParent, DefaultCursor, InputManager. A button calling a minimal script to move between scenes, and a 3dTextPrefab.

For some reason, without the 3dTextPrefab, or if I disable the 3dTextPrefab in the current scene (the scene being exited) just before exiting, the error doesn't occur. Also, in this minimal installation, if I delay moving from the second to third scene, even by a couple of seconds, the error won't occur.

I can code around this by making InputManager.searchForInstance public and then checking for null && false in the OnEnable function of SetGlobalListener

  private void OnEnable()
    {
        if (InputManager.Instance == null && InputManager.searchForInstance == false) {
            InputManager.searchForInstance = true;
        }

        InputManager.Instance.AddGlobalListener(gameObject);
    }

But not sure of any additional implications this might have.

Calling DontDestroyOnLoad on the InputManager leads to an AssertionFailed in FocusManager. I'm not sure what additional issues might arise, in addition, at a minimum, it would make for additional work having to instantiate an InputManager, associate it with a cursor etc., in cases where I don't start on the first scene.

[StephenHodgson]

But not sure of any additional implications this might have.

Most likely not good ones. Singleton.searchForInstance should never be changed outside of the scope of that class.

I'll look into this a bit more and see what I can do.

[StephenHodgson]

Could you provide more details about how you're loading the next scene?

That could also be the issue.

[genereddick]

Changing scenes in this example case is from a button click event. Calling:

 var currentIndex = SceneManager.GetActiveScene().buildIndex;
 SceneManager.LoadScene(currentIndex + 1);

In my actual project I get the multiple event systems in a scene warning, but it is considerably more complex. In the minimal project (I can upload this, or make it available on dropbox if helpful) I only get the NullReferenceException. In the full project this error always happens, but in the minimal one only if I cycle the scenes quickly, so suggests a timing problem between when objects are being destroyed and created between scenes. This error is also new to the latest version and new Singleton, as this particular code in my project has been stable through many updates of the MRTK and Unity..

[genereddick]

This suggests that the issue is caused from the static class references in Singleton.cs which survive scene load

http://answers.unity3d.com/questions/41891/static-variables-between-scenes.html

[Alexees]

The problem is typical for Singletons that initialize themselves in Awake (as this one does). The call cycle is Awake and then immediately OnEnable is called if the script is enabled. Since Awake calls are arbitrary, but OnEnable is tied to it, there's no way to tell if the singleton is ready by the time another OnEnable is called. There are two solutions to this. Either move the Singleton in question up the Execution order table, or do the singleton lookup always in Start (which is guaranteed to be called after every Awake, hence every OnEnable, has been executed).
If you need to toggle your script. just skip the first OnEnable and use Start once for the first initialization.

The latest installment of the toolkit yields until the singleton is available, so you can also check that out.

[genereddick]

@Alexees I have a pretty good handle on managing the loading within a scene. The trouble I am having is between scenes with the InputManager and the new Singleton class.

It seems to be related to the persistence of static variables between scenes (c.f. link above, this one http://answers.unity3d.com/questions/1169580/static-variable-reset-at-loadlevel.html, et.al.), as well as timing, but it is maddeningly inconsistent.

If I throw a bunch of debug statements, I can see the chain:

_The searchForInstance value is true so the first scene's InputManager.Instance is created._
InputManager Singleton static searchForInstance is: True
HoloToolkit.Unity.InputModule.SetGlobalListener:OnEnable()

_My code to change scenes runs_
Exiting scene 0
GameManagers.SceneLevelManager:LoadNextScene()

_The new scene loads and now the old InputManager is still around with its EventSystem as well as the
new scene's InputManager and EventSystem, throwing the warning below_
Multiple EventSystems in scene... this is not supported
UnityEngine.EventSystems.EventSystem:OnEnable()

_The old InputManager is removed, but the static class variable is still set, so the new InputManager doesn't have the Instance assigned_
InputManager Singleton static searchForInstance is: False
HoloToolkit.Unity.InputModule.SetGlobalListener:OnEnable()

_And the exception below is thrown_
NullReferenceException: Object reference not set to an instance of an object
HoloToolkit.Unity.InputModule.SetGlobalListener.OnEnable () (at Assets/HoloToolkit/Input/Scripts/Utilities/SetGlobalListener.cs:17)

[StephenHodgson]

Great detail on the repro steps.

The old InputManager is removed, but the static class variable is still set, so the new InputManager doesn't have the Instance assigned
InputManager Singleton static searchForInstance is: False
HoloToolkit.Unity.InputModule.SetGlobalListener:OnEnable()

I wonder if calling OnDestroy on the old InputManager is also unsetting our instance for the new InputManager as well. searchForInstance _should_ be true again, and should properly find it. This is strange.

[edit]
Actually it could be that the Awake for the new Input Manger is called before the OnDestroy of the old one. Could you test that out and see what the results are?

In either case I suggest keeping the InputManager between scenes and also adding the DontDestroyOnLoad the parent GameObject that it's on and the same for any other managers that complain of null refs.

[genereddick]

I'll test.

One other thing I noticed, the Singleton class has the following comment related to OnDestroy (similar to OnAwake):

    /// Base OnDestroy method that destroys the Singleton's unique instance.
    /// Called by Unity when destroying a MonoBehaviour. Scripts that extend
    /// Singleton should be sure to call base.OnDestroy() to ensure the
    /// underlying static Instance reference is properly cleaned up.

However, none of the singleton classes in the MRTK, including InputManager actually override OnDestroy, though they do override Awake of course.

I'm not sure if this is important, and, at least in my test regarding the issue above it did not seem to matter, however, without it the debugger never steps into the Singleton's OnDestroy method.

[genereddick]

OK this test has an OnDestroy override in InputManager to make sure the Singleton base destroy method is called, although its presence doesn't materially impact the errors.

The timing of Destroy and Awake seems correct. Interestingly the InputManager, even after the error thrown in GlobalListener, does eventually get started up, the cursor works etecetera. Also, the errors seem to compound the more scenes that get loaded, especially if I move through them rapidly. I can also mitigate some issues by using SceneManager.LoadSceneAsync and waiting for the load.

Again, all of this is inconsistent and seems to change a bit from one run to another.

SCENE 0
InputManager Singleton static searchForInstance is: True
InputManager Awake is called: IM #1
InputManager Start is called: IM #1
Exiting scene 0

SCENE 1
InputManager OnDestroy is called: IM #1

Multiple EventSystems in scene... this is not supported

InputManager Singleton static searchForInstance is: True
NullReferenceException: Object reference not set to an instance of an object

InputManager Awake is called: IM #2
InputManager Start is called: IM #2
Exiting scene 1

InputManager OnDestory is called: IM #2

[StephenHodgson]

I really appreciate you taking the time to test this out and explore the problem a little deeper.

However, none of the singleton classes in the MRTK, including InputManager actually override OnDestroy, though they do override Awake of course.

Hmm, I'm pretty sure if they don't need to override then base OnDestroy it should be called by default.

private override void OnDestroy()
{
    base.OnDestroy
}

The above code is redundant, and shouldn't require the need to override it, but I could be wrong.

however, without it the debugger never steps into the Singleton's OnDestroy method.

Depending on where you put your breakpoint and if the instance was the the same, you may or may not have hit it because it was a different instance.

I'll test this out.

[genereddick]

OK, I can see debug statements getting logged from the Singleton OnDestroy, so it is getting called. Just that, for me at least, the debugger won't break in the Singleton class However, I can step into the Singleton if I add a protected override OnDestroy in the InputManager.

[genereddick]

I have a working theory that seems to explain the behavior (sorry, behaviour) and the inconsistency.

The two objects interacting are the MixedRealityCameraParent gameobject and attached script SetGlobalListener and the InputManager gameobject with the attached InputManager script inheriting from Singleton.

Awake is effectively random as to when it is called. But, so is OnDestroy. OnEnable can have dependencies as to when called. Static values are preserved across scene loads.

Two scenes each with a MixedRealityCameraParent and an InputManager. When the transition between the scenes is called, the order in which the two objects are destroyed is important.

If the camera is destroyed first the SetGlobalListener OnDestroy checks for null on the InputManager.Instance.

if (InputManager.Instance != null)

Because this is not yet null, the InputManager.Instance.RemoveGlobalListener function is called. Next the InputManager is destroyed and the Singleton static values are reset and ready for the new scene. All is OK.

If the InputManager is destroyed first, the gameobject is destroyed and the Singleton static values reset and ready for the new scene. Now the camera is destroyed. SetGlobalListener calls its OnDestroy method which checks

if (InputManager.Instance != null)

Now though, the Instance is null and searchForInstance has been reset to true. So in the Singleton Instance property, we hit this:

public static T Instance {
        get {
           if (instance == null && searchForInstance)  {
                searchForInstance = false;
                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;
        }
    }

This resets searchForInstance to false. But, there is nothing to return from FindObjectsOfType as the InputManager has already been destroyed. The new scene loads but the static value of searchForInstance survives the scene transition.

Now, if the InputManager loads before the MixedRealityCameraParent, the InputManager Singleton Awake method works properly, reassigns the Singleton Instance property, the searchForInstance value is still false but that is now correct, and all is again OK.

However, if the MixedRealiityCameraParent loads first, then the SetGlobalListener OnEnable gets called, but because the static searchForInstance is still false and the instance is still null, accessing InputManager.Instance throws the nullreference exception.

private void OnEnable() {
        InputManager.Instance.AddGlobalListener(gameObject);
  }

After this the InputManager Awake is called and you still get a functioning inputmanager. Though I think things are still in a bad state as the MixedRealityCameraParent is never added to the AddGlobalListener call.

This seems to mactch what I'm seeing, especially when I include the timing of the SetGlobalListener OnEnable, OnDisable and OnDestroy functions.

[StephenHodgson]

@genereddick did the latest singleton update #1208 fix this issue you're having?

Now you'll need to go though and remove all the Singleton objects in the new scenes you're transitioning to. You may also want to put the DontDestroyOnLoad on your cursor as well.

[genereddick]

Testing this it does solve the issue nullreference issue.

However, it raises other issues:

It requires that only the first scene in your project can contain InputManager or MR Camera gameobjects as they both have Singleton scripts attached so the DontDestroyOnLoad means they all carry across all your scenes.

The Cursor association on InputManager is lost on the following scene -- I know this gets reset using FindCursorIfNeeded, but at a performance cost (I'm not sure how major though).

"Cursor hasn't been explicitly set on \"{0}.{1}\". We'll search for a cursor in the hierarchy, but"
                    + " that comes with a performance cost, so it would be best if you explicitly set the cursor.",

Any objects in your own code that inherit from Singleton will have the same issue.

You can no longer run a scene after the first scene without writing additional code to Instantiate instances of the MR Camera and InputManager if they don't exist.

I don't know enough about Unity coding practices to speak with any certainty, but from what I've seen choosing to have objects survive scene load seems like it is a deliberate choice and not expected behavior.

Perhaps most MR projects aren't using multiple scenes so these aren't issues that come up much, or perhaps I'm missing some way to mitigate against the additional issues that are created?

[StephenHodgson]

I think that's more of a design choice, and how complicated you want your applications to be.

I've generally always used a single scene for all my applications, and just enable/disable objects as needed.

There are use cases for having multiple scenes if your game has lot's of levels and such, but you could also write your own version that just loads what you need in the scene you've already got open.

Again it goes back to design choice. I suppose we could add a bool to give users the choice to have these objects persist, but in my opinion you wouldn't want to destroy a valid singleton instance for any reason (even when transitioning scenes) so I would expect it to the the correct behavior.

As for the Cursor, you could also add DontDestroyOnLoad if you want it to also persist.

You can no longer run a scene after the first scene without writing additional code to Instantiate instances of the MR Camera and InputManager if they don't exist.

I'm sure a little helper script could fix this issue.
We could also make a virtual method to override that that spawns a prefab in the scene if it's not found.

[genereddick]

OK. I've also mostly used single scenes, but for larger projects I'm not sure how well this scales -- maybe just an unwarranted assumption on my part from looking at other Unity projects. Perhaps I'm also abusing Singletons in my project.

It might be helpful to have an project in the HoloToolkit-Examples that uses multiple scenes so that cross scene issues can be detected.

One additional question about this is the hard dependency between the MixedRealityCameraParent's SetGlobalListener script and the InputManager. Because of the lack of a null check on the OnEnable function, if you don't have an InputManager in the scene -- for example if you have a short-lived splash scene that doesn't allow interactivity -- it will throw an error.

  private void OnEnable()
    {
        InputManager.Instance.AddGlobalListener(gameObject);
    }

Maybe this is necessary as other assets in the MixedRealityCameraParent, such as MixedRealityTeleport, depend on it, but if so the dependency should be make explicit (maybe it is, haven't looked at the documentation recently). Otherwise, seems like they could be refactored to allow for separate inclusion in a scene.

[StephenHodgson]

This is a very good point. We should revisit what dependencies we have in the prefabs and make sure we explicitly put those in the docs. You could open a new issue for that if you like, but I think the main topic of this thread has been mostly resolved.

[genereddick]

Thanks for the help on this!