microsoft / MixedRealityToolkit-Unity

This repository is for the legacy Mixed Reality Toolkit (MRTK) v2. For the latest version of the MRTK please visit https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity
https://aka.ms/mrtkdocs
MIT License
6.01k stars 2.12k forks source link

Singleton not guaranteed to exist if called from Awake #1039

Closed jamesashley1 closed 7 years ago

jamesashley1 commented 7 years ago

The Singleton implementation uses eager instantiation, so it gets instantiated even if it doesn't actually get called -- that's just a c# purist problem. A more practical problem is that if it gets called from another Awake method, there's no guarantee it has been instantiated. You are rolling the dice.

This can be fixed by simply doing conditional (if null) instantiation in the Static Get method instead of in Awake.

keveleigh commented 7 years ago

Singleton has been a hot topic throughout the history of this repo, and, in fact, a version of it exists in our dev branch that works the way you describe: https://github.com/Microsoft/MixedRealityToolkit-Unity/blob/Dev_Unity_2017.2.0/Assets/HoloToolkit/Utilities/Scripts/SingleInstance.cs. This may be a good time to revisit our implementations.

jessemcculloch commented 7 years ago

I would agree @keveleigh, since the current implementation doesn't really match what we are calling it at all...

Railboy commented 7 years ago

@keveleigh Definitely prefer this implementation.

@jamesashley1 I feel it makes sense to assume a MonoBehaviour-based Singleton will be present in the scene prior to being called, even if that's impure behavior. But it might help to name it 'MonoSingleton' or something.

A Singleton that extends ScriptableObject would probably be the best of both worlds - a pure singleton, serialized data and no instantiation headaches.

StephenHodgson commented 7 years ago

A Singleton that extends ScriptableObject would probably be the best of both worlds - a pure singleton, serialized data and no instantiation headaches.

Definitely like this idea, but we would also want something that can also inherit from MonoBehavior as well.

StephenHodgson commented 7 years ago

Fixed by #1160