Update systems and demos to reference the input system via the service registry #4315
Conversation
david-c-kline
requested review from
Railboy,
keveleigh,
luis-valverde-ms and
wiwei
|
/azp run mrtk_pr |
|
Azure Pipelines successfully started running 1 pipeline(s). |
david-c-kline
changed the title
Assets/MixedRealityToolkit.SDK/Features/UX/Scripts/Cursors/CursorModifier.cs
Show resolved
Hide resolved
Assets/MixedRealityToolkit.Services/InputSystem/Utilities/CanvasUtility.cs
Show resolved
Hide resolved
Assets/MixedRealityToolkit.Services/InputSystem/InputSystemGlobalListener.cs
Show resolved
Hide resolved
Assets/MixedRealityToolkit.Services/InputSystem/InputSystemGlobalListener.cs
Outdated
Show resolved
Hide resolved
Assets/MixedRealityToolkit/Interfaces/InputSystem/IMixedRealityGazeProvider.cs
Show resolved
Hide resolved
| return inputSystem; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This overall change looks fine, it's is weird to me though that we go from one singleton (i.e. MixedRealityToolkit) to another (MixedRealityServiceRegistry) - in this case now we put the onus of caching and reaching out to all of the consumers (i.e. this exact code is copy pasted in a lot of the things here)
If every single caller is handling caching it almost feels like MixedRealityServiceRegistry.TryGetService should cache as well (i.e. why not share some of this code at a lower level)
There was a problem hiding this comment.
we go from one singleton (i.e. MixedRealityToolkit) to another (MixedRealityServiceRegistry)
MixedRealityServiceRegistry is a static c# class that has no scene presence (it is not a Unity object).
If every single caller is handling caching it almost feels like MixedRealityServiceRegistry.TryGetService should cache as well (i.e. why not share some of this code at a lower level)
Interesting thought. This is actually very similar to the original pattern where many scripts would attempt to get the service if a previous instance had not yet been found (ex: was null)
There was a problem hiding this comment.
As an aside, caching is an optimization and is not required. Any script can request a system at the time it is needed.
|
Alrighty, done with this set of comments. |
|
/azp run mrtk_pr |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run mrtk_docs |
|
Azure Pipelines successfully started running 1 pipeline(s). |
4 participants
This change completes the migration of MRTK code from directly calling into the MixedRealityToolkit object to obtain instances of core services. While this is still a viable pattern for client code, the prefered method is to use MixedRealityServiceRegistry.TryGetService().
This completes the work started with #4099 and is related to the ongoing effort to fully implement #3545.