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.02k stars 2.12k forks source link

[Plan of Record] Improve MRTK ease of use: system facades, registrar interface, config ux #3545

Closed david-c-kline closed 5 years ago

david-c-kline commented 5 years ago

Plan of Record: Support Service Specific Scene Presence

Microsoft Mixed Reality Toolkit v2

This document describes a design change for the Microsoft Mixed Reality Toolkit v2 concerning how developer customers can choose the MRTK and its services.

The Issue: Usability

The MRTK team has received feedback from multiple sources that the current, single object presence in the application scene is difficult to use with regards to feature discovery and modularization. Multiple customers, both internal and external, have requested an alternative to beta 2’s MixedRealityToolkit object in the form of system specific locator components (ex: MixedRealityInputLocator) that can be additively included.

One piece of feedback, heard from multiple sources, points out the promise of additive vs. subtractive consumption of MRTK. The comments largely center around two themes:

  1. It is not easy, at a glance, to see if a scene is using a specific MRTK service. Many developers from whom we heard desire an individual service presence in the scene hierarchy.
  2. The root Mixed Reality Toolkit configuration profile makes it appear that all services are present in the application, even when disabled. There is a strong desire for scoping the profile inspector to show only installed and active services.

Solution: Enable Support for Individual Services to have Scene Presence

The solution defines the following changes:

  1. Provide service registration interfaces to enable customers to create a custom service locator and/or fully stand-alone service components
  2. Provide support for exposing active services and their configuration settings in the scene hierarchy at edit time via service facades
  3. Enable customers to create hybrid application architectures, if desired, that use any number of service locators in conjunction with any number of stand-alone services
  4. Create a light-weight service registry that maps service interfaces with the concrete type that manages service registration
  5. An updated scene hierarchy to contain the service registry, service facades, etc
  6. Modified MRTK project configuration flow to provide enhanced flexibility for customers when setting up their projects

Please see the Solution Details section of this document for information the described changes.

Goals

  1. Respond to direct customer feedback
  2. Provide customers with a choice of how they consume MRTK services
  3. Minimize required client code changes
  4. Preserve as much of the MRTK service locator functionality as possible (anticipating 90% or greater equivalence)

Non-Goals

  1. Removal of the service locator pattern
  2. Require developers to change how they wish to work

Solution Details

Service Registration Interfaces

To enable developers to create custom service locators and/or extend an existing service locator to add support for MRTK services, a set of interfaces will be created to support the registration and retrieval of service instances.

It is anticipated that there will either be one or three interfaces that will be created. The following sections describe the pros and cons of each implementation.

Single Registrar Interface

In the single interface approach, the MRTK will define IMixedRealityServiceRegistrar. This interface would define explicit methods for supporting objects implementing the following service types (listed by interface):

Each method (Register, Unregister, Get, CheckRegistration) will have a version that is type safe to the listed interfaces.

Pros Cons
Only one interface to implement Implementors may be required to implement stub methods
Can provide a single base class supporting all service types

Multiple Registrar Interfaces

In the multiple interface design, each type of service would have a corresponding registrar interface:

This approach would allow customers to support only the specific types of services necessary to build their component.

Pros Cons
Developers are free to implement only the required service type support Potentially mutltiple interfaces to implement
Develipers do not have to implement stub methods Cannot consume individual interface base class implementations (C# limitations)

Base Class Implementations

As with much of the MRTK, base classes will be provided to provide robust, reusable implementations of the registrar interface(s).

Providing the Registrar Instance to Services

When the registrar instantiates a service instance, it will provide itself as a constructor argument. The following example shows the parameter to be added to each service constructor.

public ServiceClass(IMixedRealityRegistrar registrar, ...);

A similar pattern will occur for data providers.

When a service requests registration of a data provider, it calls RegisterDataProvider, handing a reference to itself.

registrar.RegisterDataProvider<IMixedRealitySpatialObserver, IMixedRealitySpatialAwarenessSystem>(IMixedRealitySpatialAwarenessSystem system, Type observerType, ...);

The data provider's constructor is passed the provided reference in its constructor.

public SpatialObserver(IMixedRealitySpatialAwarenessSystem system, ...);

Service Facades

Service facades are an editor only monobehavior that exists in the application scene to give customers and easy, at-a-glance representation of the active services in the scene. When selected, the facade will display the service specific profile inspector and allow for easy configuration.

Backing these facades will be added to the scene and backed by the MRTK service locator. Developers not wishing these facades to be in their scene will be able to disable them in the MixedRealityToolkitConfiguration profile.

Hybrid Application Architecture

Hybrid applications architecture is a term intended to describe an application which utilizes any number of service locators and/or stand-alone service implementations.

To support hybrid architectures, MRTK will add a light-weight service registry that enables client code easy access to concrete service implementations.

Service Registry

The service registry in MRTK is an optional component that client code can use to acquire an instance of a desired service.

The registry can be thought of as a table that maps service interfaces to the object that manages the concrete implementation(s) .

The following table illustrates an example registry:

Registrar Interface
MixedRealityToolkitServiceLocator IBoundarySystem
CustomServiceLocator ISpatialAwarenessSystem
StandaloneInputSystem IInputSystem

The component managing the service registry can be attached to client script(s) or developers can choose to bypass the registry and hard-code references to the object managing service registration.

For example:

IServiceInterface service = MixedRealityToolkit.Instance.GetService<IServiceInterface>();

Scene Hierarchy

To help keep the scene hierarchy uncluttered, the default MRTK configuration process will create a Mixed Reality Toolkit parent object. This object will be the parent for the service locator and any enabled facades.

It is recommended that this object be the parent of objects created by registered services (ex: spatial awareness mesh objects).

The following image illustrates the default hierarchy when the MRTK service locator is used with the input and spatial awareness system facades enabled.

Hierarchy With Facades

Project Configuration UX

To make it easy and intuitive for customers to configure MRTK to suit their application's requirements, the Mixed Reality Toolkit menu's Configure item will display a selection dialog similar to the following illustration.

Configuration Dialog

david-c-kline commented 5 years ago

It would be entirely reasonable to have the scene objects display inspectors that aid discoverability. [Mixed Reality Toolkit Root Object] --[Input System Representation] ----"This component exists to improve discovery and only forwards events. If you wish to change the behaviors of the Input system - [Button to Profile Configuration] or adjust [Name of a few related classes] or look into [Related Interface]" --[Teleport System Representation] etc.

This is REALLY close to the intent of this proposal! The individual "managers" would provide access to the profiles.

Let's chat early next week and see how it lines up.

ryantrem commented 5 years ago

@StephenHodgson it doesn't seem like the example you gave above would work. As @davidkline-ms pointed out, the MixedRealityInputSystem has a bunch of accesses to MixedRealityToolkit.Instance (e.g. for the input system profile and the focus provider). I'd want to be able to have them as serialized fields on the standalone MonoBehaviour from your example, and pass these dependencies into the MixedRealityInputSystem. This is all I really mean when I'm asking for a more layered approach that allows me to use individual components and not need to use the whole MixedRealityToolKit mega component.

I really don't want to fork this discussion even more, but I keep seeing references to improved performance with the current approach. As far as I can tell, this is referring to having 1 MixedRealityToolKit component instead of ~6 individual MRTK related standalone system components within a scene that will typically have many hundreds of components.

StephenHodgson commented 5 years ago

it doesn't seem like the example you gave above would work.

Well I was giving a very basic example, but I disagree, it could, in theory be done. Not with the current MixedRealityInputSystem implementation because, yes you're right there's calls to the service locator; but that's why we provided an interface for people to implement their own versions of said system/service. The default implementations of the services were implemented to work with the current architectural patterns.

But you're right, simply wrapping them wouldn't work.

jamesashley1 commented 5 years ago

Here's a really easy way to make services "additive" instead of "subtractive":

        [MenuItem("CONTEXT/BaseMixedRealityProfile/Create Copy from Profile Values", false, 0)]
        protected static async void CreateCopyProfileValues()
        {
            profileToCopy = profile;
            ScriptableObject newProfile = CreateInstance(profile.GetType().ToString());
            profile = newProfile.CreateAsset("Assets/MixedRealityToolkit.Generated/CustomProfiles") as BaseMixedRealityProfile;
            Debug.Assert(profile != null);

            await new WaitUntil(() => profileToCopy != profile);

            Selection.activeObject = null;
            PasteProfileValues();
            Selection.activeObject = profile;

            if (!profileToCopy.IsCustomProfile)
            {
                // For now we only replace it if it's the master configuration profile.
                // Sub-profiles are easy to update in the master configuration inspector.
                if (MixedRealityToolkit.Instance.ActiveProfile.GetType() == profile.GetType())
                {
                    MixedRealityToolkit.Instance.ActiveProfile = profile as MixedRealityToolkitConfigurationProfile;
                    MixedRealityToolkit.Instance.ActiveProfile.IsBoundarySystemEnabled = false;
                    MixedRealityToolkit.Instance.ActiveProfile.IsCameraProfileEnabled = false;
                    MixedRealityToolkit.Instance.ActiveProfile.IsDiagnosticsSystemEnabled = false;
                    MixedRealityToolkit.Instance.ActiveProfile.IsInputSystemEnabled = false;
                    MixedRealityToolkit.Instance.ActiveProfile.IsSpatialAwarenessSystemEnabled = false;
                    MixedRealityToolkit.Instance.ActiveProfile.IsTeleportSystemEnabled = false;

                }
            }
        }
david-c-kline commented 5 years ago

Nice example, @jamesashley1. This will definitely help some customers.

StephenHodgson commented 5 years ago

When it comes to additive vs subtractive, one thing to note is that creating a new profile has everything disabled by default, where as copying a profile just makes a copy of whatever is enabled at the time.

david-c-kline commented 5 years ago

FYI: Added Goals and Non-Goals sections to the existing issue description.

david-c-kline commented 5 years ago

The proposal is in the process of being updated to replace "Manager" with "Locator". An FYI will be posted when that is completed.

david-c-kline commented 5 years ago

FYI: Proposal updated to replace Manager with Locator. Please note some captialization issues may be present as the update was a quick Find|Replace. :)

david-c-kline commented 5 years ago

We shouldn't need to change the base SDK to support that mode.

It is important to note that the proposal does not include removing any current components (i.e. MixedsRealityToolkit object). It formalizes a design that has been under discussion for some time now (providing support for custom service locator implementations) while adding individual locators for systems.

david-c-kline commented 5 years ago

Any feedback from folks on the following?

Thanks!

StephenHodgson commented 5 years ago

while adding individual locators for systems.

Why are we adding additional service locators? Wouldn't that add more complexity to the architecture, when the service locator is supposed to be the only one to begin with? (think of it as an application container)

This pattern uses a central registry known as the "service locator", which on request returns the information necessary to perform a certain task.

The application, not the component, should be the singleton. The application then makes an instance of the component available for any application-specific code to use.

keveleigh commented 5 years ago

Call it a locator, call it a Singleton, whatever you want. We're defining the functionality. This proposal aims to make standalone services locatable and accessible to a developer in a way that they choose and may be more familiar to them. The point is defining and building specific MonoBehaviours that can configure and run individual services. Think of it as the MixedRealityToolkit class, but specifically for the InputSystem, the BoundarySystem, etc. This way, a developer has the option of configuring their scene with individual GameObjects / MonoBehaviours, instead of just the one MRTK Service Locator. It's an added option, and the current method of having one single MRTK singleton to manage every service will still be an option.

StephenHodgson commented 5 years ago

Also I noticed that this proposal covers moving the data provider registry to the service, which I totally support doing, but this is already defined in another task/proposal.

david-c-kline commented 5 years ago

Also I noticed that this proposal covers moving the data provider registry to the service, which I totally support doing, but this is already defined in another task/proposal.

That was added to the proposal for completeness. Thx!

david-c-kline commented 5 years ago

Why are we adding additional service locators?

The intent is that service implementers would not have to implement support for loading and managing their data providers. It also creates a common object that can be used to quickly and easily change the specific system implementation.

That said, there is nothing preventing a service from implementing these interfaces (or not, if it doesn't need to load any additional services).

david-c-kline commented 5 years ago

Following the pattern that has been established with service and system interfaces, the locator interfaces are defined as a contract that can be relied upon without need for knowledge of the specific implementation.

StephenHodgson commented 5 years ago

The point is defining and building specific MonoBehaviours that can configure and run individual services

The whole purpose of MRTK v2 architecture was to break the tyranny of the MonoBehaviour. Even Unity is working towards this end goal with ECS and DOTS (just to name a few).

Think of it as the MixedRealityToolkit class, but specifically for the InputSystem, the BoundarySystem, etc.

Actually we should be thinking in terms of Interfaces and Contracts, which already support creating implementations based on needs (see my concrete example above). Classes are implementations of the contracts we setup. They can be implemented however the developer wants. If they wanna bind it to a monobehaviour they already can.

This proposal aims to make standalone services locatable and accessible to a developer in a way that they choose and may be more familiar to them.

The point is defining and building specific MonoBehaviours that can configure and run individual services.

What's so hard about calling MixedRealityToolkit.InputSystem from any script? Calling gameObject.GetComponent<InputSystem>() is arguably more expensive and falls back into the traps of race conditions and binding the application logic to the UI/UX presentation layer.

Arguably we started getting feedback before the whole thing was completed, without a solid SDK for people to utilize, and proper documentation. While yes, that early feedback is valuable, we also need to think about the root cause of the problem, and not treat the symptoms by simply covering it up with a fake layer of fluff (i.e. the GameObject references to things that shouldn't live in the presentation layer). This proposal sounds like someone got some feedback, then they came up with a solution for it, and proceeded to work their way backwards to the problem.

I understand it's a completely different workflow and that it's not immediately familiar to traditional Unity developers.

It's a culture shock, and I agree, maybe we should have had better teaching materials, and a solid SDK. It's like throwing someone from the 15th century into the modern era grocery store and asking them to pick up dinner. They have no clue where to begin! They're used to having to farm, and kill the cow, and butcher it all on their own. We shouldn't knock down the grocery store and plow a field for them just to help them adapt, but we should educate and ease them into the process of shopping, step by step.

One excellent solution to this was brought up by a someone for an article I plan on doing here in the future (that will hopefully make its way into documentation):

We're not trying to uphold some "aesthetic principle", but instead an objective, concrete, testable architecture that will unlock the potential of creators to quickly create enterprise level applications.

david-c-kline commented 5 years ago

Actually we should be thinking in terms of Interfaces

That is a large part of this proposal. The one place where MRTK does not have interfaces is the service locator.

david-c-kline commented 5 years ago

From the way the discussions are going, I think we can agree this isn't something that should be a part of the foundation. More likely this should be another extension for those wanting to only take parts of the MRTK and utilities to make that happen.

The discussion here, and threads I have received in email paints a different picture. Even if service specific locators are shipped as an extension, well known interfaces are necessary to enable consistent and reliable contracts for customer code to utilize.

to be used by those few who need only parts of the MRTK.

The purpose of modularizing MRTK is to enable developers to use only the portions of interest. In fact, the vast majority of the feedback we receive is related to use a subset of the feature set.

I agree we should support other patterns, but the foundation is a core to service the many and not the few.

That is what this proposal is about. Empowering as many developers as possible.

StephenHodgson commented 5 years ago

For the normal operation of the toolkit, the service locator doesn't have interfaces as it is never meant to be replaced like all the other components

💯This couldn't be stated better. There's no reason to replace the service locator, and this one is specific to Unity. Now, if this was Unreal, or Monogame, then they'd likely have a different requirement for their respective engines. This repository is focused on Unity specifically. Let's not forget that.

The purpose of modularizing MRTK is to enable developers to use only the portions of interest.

They could already do that.

That is what this proposal is about. Empowering as many developers as possible.

That should be the goal of the entire toolkit. Stating that it's what the proposal is about doesn't actually justify anything. It's a politically charged message that attempts to detract attention from the real discussion.

keveleigh commented 5 years ago

Each service could be ran individually by itself in its own sandbox so long as specific methods are driven by something whether or not it's the service locator or monobehaviour game loop messages

That is this proposal. We will provide this ability without forcing our customers to have to write it.

SimonDarksideJ commented 5 years ago

I've taken a step back and reviewed your proposal @davidkline-ms and believe it is the best path to support the priorities you mentioned above. It appears the Service Locator pattern is simply not conducive to the way that these partners are building solutions and it's hampering the adoption of the MRTK.

To that end I've created a proposal to take this proposal to it's ultimate conclusion and cut out the service locator pattern from the MRTK as it doesn't fit with the way developers need to work. https://github.com/Microsoft/MixedRealityToolkit-Unity/issues/3568

chrisfromwork commented 5 years ago

@davidkline-ms I still think that more detail is needed in this proposal around configuration menu usage when reconfiguring a project. I can imagine the following scenarios, and I think we should have a clear ux understanding of what will be possible/what users should avoid:

1) Developer has a global system manager and wants to convert a project to use a few individual system managers. 2) Developer has a few individual system managers and chooses to move to the global system manager 3) Developer has a few individual system managers and wants to add more individual system managers 4) Developer has a few individual system managers and would like to remove unused system managers

Do we know what will happen to a scene in going through these steps (i.e. What serialized fields do we expect to break in a scene)? What is our undo story if you remove system managers through this ui (Is it at all possible to revert a reconfiguration)? Are there any ui visual affordances that should be added so that users know they will be performing breaking changes (assuming a reconfiguration introduces breaking changes)?

chrisfromwork commented 5 years ago

nit: it looks like some of the provided images have also 404'd. Is it possible to get these fixed?

chrisfromwork commented 5 years ago

nit: Why is locator all lowercase in the interface/class names when its a new word?

SimonDarksideJ commented 5 years ago

Your point @chrisfromwork is why I'm most concerned about having a multitude of choice such as this in a single framework. Ultimately for any project you should have a single path and stick to it for the lifecycle of your project. There should never need to be a change, else it is likely to mess up your project completely.

Simply put as per David's proposal, the choice is only ever at the beginning and once chosen, you should not change.

My point is that if the project is giving advice for how to develop solutions, then there should only be one path. Choice in open source is to chose a different framework that works in a particular pattern.

That choice for MRTK, seems to lead to the Single GameObject pattern, leveraging the service approach built within the MRTK V2 but applying it in a way akin to the HoloToolkit that the enterprise developers are familiar with.

Offering such a drastic choice in the framework will only lead to misery. It would be like either mixing ECS and traditional unity development in a single project, or using both async and non async for the same work. The two methods are incompatible and miles apart.

keveleigh commented 5 years ago

nit: it looks like some of the provided images have also 404'd. Is it possible to get these fixed?

Fixed

david-c-kline commented 5 years ago

nit: Why is locator all lowercase in the interface/class names when its a new word?

search replace mistake

chrisfromwork commented 5 years ago

There should never need to be a change, else it is likely to mess up your project completely.

I think that new releases of the MRTK will eventually lead all projects to require some amount of refactoring. I also feel like porting existing projects to the MRTK will always have the potential for encountering configuration related breaks based on MRTK system service behaviors/forced scene changes. In this individual services based architecture, hopefully all reconfigurations are additive, resulting in new behavior and no pre-existing logic breaks. But I could also imagine scenarios where an extension service is provided through this menu and may need to be removed (Enterprises may decide that an extension doesn't meet their quality/security bar after some prototyping/internal assessment). We should have the foresight to assess what support we can provide for these scenarios, even if we choose to do nothing. We should also try and understand whether its possible to undo any of these configuration changes. It may be that someone already assessed what was possible when setting up the configuration tool before? It may also be felt that source control (git commit histories) should be the only mechanism for undoing things?

In regards to this proposal, I feel that the fallout of reconfiguring a scene to use the MRTK/different systems is not well documented. It may be well understood by contributors, but I'd like the proposal to capture these potential pain points so we understand what we are asking of end developers. And I think we should have explicit call outs in documentation stating what may happen when going through configuration. And guidance for trying to make the process easier.

SimonDarksideJ commented 5 years ago

I think that new releases of the MRTK will eventually lead all projects to require some amount of refactoring

That is certainly true where projects adopt the new service locator pattern. Like ECS it's a fundamental change in how you build a unity project, for the better.

This document Stephen published did go over a lot of detail for how the Service locator pattern should work, attributing to MVVM standards for readability https://docs.google.com/document/d/1B0BXCQq-Dn_xSmTUbHdWQ6aAx5K9IHmxq_E-OvV1AYc/edit?usp=sharing

In regards to configuration management, that's what the profiles are for, changes must be backwards compatible going forward, similarly, each service needs to be aware of it's dependencies and it's role in a project. However, this was core to the new approach to ensure that no one system is the MRTK, it's a sum of parts. SO that if you either want to replace or simply drop a system, the rest of the framework will carry on regardless without error.

the MRTK/different systems is not well documented

This is sadly true and both Stephen and I have to take a larger responsibility for that. As the original creators of this pattern for the MRTK, we did a fair amount of demo sessions and overviews with multiple teams, but we had little time to culminate proper documentation or guidance in the correct use and adoption of the framework. There were many reasons for this, mostly due to spending too much time building and not documenting as we go (although the code is highly documented in what it does, just now how to use)

I had started some documentation content in the documentation folder of this repo, however it became largely overlooked. Not because of anyone's fault, it just didn't make the front page. I also did a quickstart video and posted it, but didn't repeat that enough for the wider audience to appreciate (plus didn't have the time to dedicate to fully launch the channel due to working on the MRTK). More videos were planned but the project was in too much flux between beta's, which meant a lot of content would need to be repeatedly re-shot.

Now that the 2nd Beta is out and the framework is bedding in, I should have more time for more dedicated content to show people how to use it. Depending on which way this Proposal goes.

david-c-kline commented 5 years ago

reopening since I closed this on accident when commenting

markgrossnickle commented 5 years ago

Great thread!

I'm currently going through the process of migrating a couple projects from HTK to MRTK. It is definitely confusing at first. The main frustration I am hitting right now is jumping between profiles as mentioned by others. I can easily go between a level or two deep into profiles by using 2 inspector windows and locking one but beyond two levels its very difficult to jump between things in the scene. The breadcrumb idea could solve that to some extent. I do miss the ease of having game objects in the scene to search for and click on. @StephenHodgson Do you know how is Unity handling viewing/editing what is in the scene in ECS?

I like the idea of moving away from Monobehavior due to ECS and DOTS, but would this scriptableObject setup be easily translated to ECS any easier than Monobehavior? @StephenHodgson Is there a clear path from scriptableObject to ECS? That would be the main reason in my mind for finding a way to make this current setup work.

I haven't used the toolkit enough to comment on how easy it will be to configure and extend. It seems like you could enable/disable whatever you need at first glance. So I can't speak to that goal of this proposal.

I don't like the idea of two different configurations but perhaps I am missing a key part of this proposal because I am a bit unclear on a few things:

  1. Are we getting rid of scriptableObjects profiles or not?

I see this:

The feedback isn’t around POCO and decoupling services from MonoBehaviors.

But also this:

We have an unwieldy and over-engineered set of nested profiles and a frustratingly restrictive architecture.

Are we just swapping top level profiles with GameObjects and keeping low level profiles? Or how are we adding game objects without removing profiles?

  1. Why do we need multiple locators? That seems like a messy solution to fixing the issue of wanting components to be standalone. Could we instead have the ability to define the interface being used as a property on the gameObject and if it is not there then look in the central locator?

  2. Why are we using strings to Register/GetServices? Wouldn't it be more optimized to store interface implementation by their type and then just call GetService(type)? That would also nix the need to pass namespace.

I'm sure I'm missing some things so I apologize if these thoughts/questions aren't helpful. I'll be sure to add more as I progress further and hit any bottlenecks.

StephenHodgson commented 5 years ago

I'm so glad you joined the conversation Mark, and these are all great thoughts and questions you've brought up.

I do miss the ease of having game objects in the scene to search for and click on.

I think people forget that you can navigate profiles (aka ScriptableObjects) in the project window with the same ease as the scene hierarchy (In fact I don't see this being any different).

Do you know how is Unity handling viewing/editing what is in the scene in ECS?

They've got specialized components that go on the GameObject to do fast lookups (similar to what we do with the interface handlers, but way more involved).

Is there a clear path from scriptableObject to ECS?

I'm pretty sure Unity is already defining this with their "data driven" approach, so yes the scriptable objects fit that criteria. As for the exact path? I'm not quite too sure about. Maybe some inside knowledge from Aras Pranckevičius would help? Unity is really big with scriptable objects atm and I don't see that going away anytime soon. Essentially they're containers that help split the data from the core application logic.

Why are we using strings to Register/GetServices? Wouldn't it be more optimized to store interface implementation by their type and then just call GetService(type)? That would also nix the need to pass namespace.

In my fork, I actually only made it so you can only register by type, and got rid of the name altogether. (it was just a helper to find a unique instance anyway).

restrictive architecture

nested profiles

Yeah this can def be much better, but I also don't think throwing them all into a massive inspector window helps much either.

markgrossnickle commented 5 years ago

Those were quotes from the thread. I was confused by some chatter saying profiles aren't being debated and others saying they are the problem.

david-c-kline commented 5 years ago

Thanks for the feedback, @markgrossnickle! I called out your numbered questions and added some comments. Hope they help clarify things.

Are we getting rid of scriptableObjects profiles or not?

That is not the intent. Scriptable objects and profiles will still remain.

Why do we need multiple locators?

This was proposed to allow developer customers an easy way to swap service implementations (like they can with the current MixedRealityToolkit component without needing to reconfigure their profile settings. The multiple locators would exist as an option for developers who wish to have the service swapping capability yet see individual objects in the scene,

Why are we using strings to Register/GetServices? Wouldn't it be more optimized to store interface implementation by their type and then just call GetService(type)? That would also nix the need to pass namespace.

Strings were in the original class definition and allow developer customers to add a friendly name to an instance (mostly for the Additional Service Providers profile). Using Type objects is definitely a good suggestion and one we should consider standardizing around.

That brings up a good question: do we need service friendly names at all?

markgrossnickle commented 5 years ago

@davidkline-ms Thanks for the clarification! If Profiles are not out, how will the in-scene gameObject representation of the profiles work? I thought the locator code shown above was going onto the profile ScriptableObjects? Where does the monobehavior locator code come into play and how does that easily tie into the profile code?

Understanding how the gameObjects will work with profiles may clear up multiple locators for me too. Keeping profiles but giving the ability to customize without reconfiguring profiles is a bit confusing.

I'd vote no on friendly names as they can be error prone and are slow for comparison (depending on implementation).

michael-house commented 5 years ago

Great discussion! Might be getting a bit lengthy and needs separating into a few separate topics. However, my two cents. Like many here, I came into Unity from a non-Unity developer background. So I often like to do things the "non-Unity" way. However, that doesn't really meet the requirements of our target audience.

For the most part, heavy use of interfaces and dependency injection allow for most of the logic to be outside of Unity. While also allowing implementation inside of Unity for those that want to do it that way.

For example having an interface and base class inside of a non-Unity MRTK core, but still allowing an implementation inside of Unity.

interface IMyInterface { string Name { get; } void DoStuff(); }

class MyBaseClass : IMyInterface { string _name; string Name => _name;

void DoStuff() { //Do some stuff } }

And in Unity, you can implement this with encapsulation of the base class:

class MyUnityClass : MonoBehaviour, IMyInterface { MyBaseClass baseClass = new MyBaseClass(); string Name => baseClass.Name;

void DoStuff() { baseClass.DoStuff(); } }

And of course, it allows purely Unity side implementation, which will be passed back down into the non-Unity "managers/systems" via dependency injection.

This is similar to Stephens example, but having the encapsulating class also implement the interface to ensure it doesn't miss any functionality.

SimonDarksideJ commented 5 years ago

Just chiming in to @markgrossnickle & @michael-house 's feedback, thanks for jumping in on the debate.

Are we getting rid of scriptableObjects profiles or not?

Scriptable objects are core to the service approach as they are the main configuration / data model for each service, as they provide a single instance of the runtime of the service that can be observed. It means we have standard classes to describe the data used in a service and manage it at runtime (where the service actively cooperates with its data)

Why are we using strings to Register/GetServices?

This was an enabler for developers to reuse the service locator for their own projects. Core services do not use name, only type because invariably there should only be one instance of a service. But in a consumer project, the string name was added to allow a developer to register multiples of the same class and be able to identify a specific on by name. This was optional of course.

Meaning you could either:

Why do we need multiple locators?

This is one point of contention I have with the proposal, and hence why I proposed the MRTK itself drop the locator pattern. Following standard architecture, there should only ever be ONE locator service within a project, a single place to find, register and recover the state of a service at any point in time.
Multiple locators breaks the base pattern of the locator / DI pattern. As you are either:

Either you know or you don't and if you don't you wouldn't know which of the multitude of locators to ask the question of, unless you are going to have a locator of locators, which is just silly.

If as this proposal suggests, a service should be standalone linked to a GameObject, then you already have a located instance of that service in the scene and you would fall back to the traditional methods for selecting that service, as has been done in the HTK and Unity projects going back years. A "locator" would not improve this situation.

It's like saying in a single town you need multiple phone directories, one for each zip code. But if you don't know which zip code the number you want is in, then you would have to fall back to asking each and every one (e.g. GameObject.FindObjectofType), or have a separate master phone directories to know roughly which zip the number could be in (which ultimately is flawed as people move how and take their number with them)

but I am but one voice in the wilderness. But my simple point is you fall back to the Unity pattern (which is what has been asked for) or you use the locator, there is no middle ground without even more complex architecture. Plus what would be using these new locators?

And thanks for your Feedback @michael-house , that code pattern is exactly what we followed for the Service locator / DI style pattern used currently in the MRTK. Applying standards based approaches in a Unity way, allowing us to avoid the overuse of MonoBehaviours and turn the majority of services in to pure C#, which has a huge performance boost in a Unity project, in excess of 80% improvement

markgrossnickle commented 5 years ago

The shiproom call cleared up some things for me.

Agree all services should be able to be able to drag-and-drop and that the framework should be additive. I understand now the gameObjects are facades and I think that is a great solution for typical unity devs. Perhaps it could be toggled off for those who dislike the added gameObjects in the scene but overall I think most people will want it at first.

The only pushback I have is I want to avoid having to know which service components are registered to and I want to avoid coupling between services. Perhaps we can still do that with this proposal but I'm wondering if we can achieve everything above without the added complexity by automating the inclusion of a light weight locator in the scene whenever a service is added to the scene.

That could be annoying to some but if we change the locator to be light weight and additive I wouldn't think anyone would have a serious complaint. The locator at that point would have no hard coded values and just get/register interfaces and display a list of the added services in the inspector.

I feel the annoyance of that light weight locator being added to scenes automatically would be a lesser evil than the complexity added of multiple locators. But again, that's just my two cents and I realize I'm still new to this framework and am not privy to all of the feedback.

Great thread, I'll try to stop making it longer now that I understand it and said my piece. Cheers for the discussion!

StephenHodgson commented 5 years ago

This is one point of contention I have with the proposal, and hence why I proposed the MRTK itself drop the locator pattern. Following standard architecture, there should only ever be ONE locator service within a project, a single place to find, register and recover the state of a service at any point in time.

Agreed. The point of the locator is to act as an application container and keep everything in sync with each other. The conductor in the orchestra as it were. It also serves as your phone book to know how to call up your friends when you need to tag team a problem.

I understand the desire and feedback that other consumers of the toolkit have their own "service locator" type of application registry, but I still don't understand why they can't create their own implementations of services based on their service locator?

They are not going to be able to consume the services created specifically for the toolkit. They will need to create implementations of those services that fit their requirements, and we enabled that with the contracts/interfaces. That's precisely what we meant when we described the architecture as being open. You take the service contracts and implement them to your specific needs. It's like saying that you want to take services written specifically in Unity and put them in Unreal. It's not going to work out of the box. There's dependencies in the implementation that require you to be dependant on certain things.

I don't think we should sacrifice the architecture to achieve something that's already possible to do.

david-c-kline commented 5 years ago

Thanks for the well stated feedback, @markgrossnickle! The team will look into what it would take to have a light-weight locator (has been recently called a registry in internal conversations).

I believe we have a good feel for the various opinions as captured in this thread (and in email / direct communications). The proposal will be updated in the near future to reflect this feedback.

mr0ng commented 5 years ago

@davidkline-ms a few thoughts:

Proposal pros:

Proposal cons:

Question/proposal: Is there a way we can use something similar to your selection UI/UX to just enable/disable features in the current MRTK profiles? Basically a UX to toggle stuff we already have in MRTK 2. And as others have suggested - we can also include an option to display facade gameobjects in the scene, representative of the MRTK features that are enabled. For example, a spatial awareness object in the scene. It would be optional, and purely to help devs get over MRTK 2 culture shock. It would be nice if those gameobjects had the ability to point to settings that devs can modify - for example being able to tweak spatial mapping settings during runtime - but unsure if that would introduce monobehaviours and break patterns. Just a few thoughts/ideas - not sure if they are founded or realistic.

SimonDarksideJ commented 5 years ago

Is there a way we can use something similar to your selection UI/UX to just enable/disable features in the current MRTK profiles

This was my alternate proposal some time back (which I sadly deleted for reasons unknown). Simply stating, that in order to give developers more access to the services in the scene, we can have SDK components that provide an editor shim that does not impact the overall operation of the framework.

I've asked this proposal to be split in to two, because there are two different requirements here, each requiring different solutions but as yet I've not had a response to that request.

blgrossMS commented 5 years ago

Catching up on this thread. Sorry that my post is a bit tangential to the architectural disucssion. I think the proposed architecture is definitely the right direction we want to see MRTK move in based on feedback we have given. I'll work on some more actionable feedback on the architecture proposal :)

I think a design principle the MRTK should have is: "How can we create components that could easily be adopted by Unity".

A lot of what me and my team want out of MRTK are systems and abstraction that Unity doesn't currently provide, but should perhaps at one point. What I we want out of MRTK is:

I'd also like to address some misconceptions I've seen in this thread: Some thoughts and a couple misconceptions I would like to point out that have been brought up: Unity is abandoning MonoBehaviours This just simply isn't true, it is not a deprecated feature. Yes they have ECS/DOTS, most existing codebases are not going to swap entirely from a GameObject MonoBehavior system to an ECS/DOTS system. I believe it is more likely that most Unity projects will have a hybrid of existing GameObject/Prefab model today with using the ECS for high performance work (but not all work..).

Additionally, if you look at any of the ECS samples, they use GameObjects and prefabs to layout elements in the scene and then at runtime convert the GameObjects into entites, which is a much better workflow.

Enterprise C# developers don't like Unity/think it's current behavior is bad. Everything is accessible by everything

I suppose at this point I am an enterprise C# developer, and I love working in Unity for a lot of the reasons mentioned above. We have in our codebase definitely had issues with over coupling system, but I believe asmdef's and architectural guidelines help solve that. Complaints I have heard from people who do not like Unity due to it's component nature usually just come from a world where everything is just raw code. I think it's just a matter of doing things differently. If MRTK is building an SDK for Unity, it should be Unity centric. I have found the most success in doing things the Unity way rather than fighting against it. If there is feedback about how they are doing things I think it is better to work with the Unity team to best identify patterns/issue areas.

MRTK should be handling 90% and the developers 10% I find this absurd. It must be a very simple project if that is the case. MRTK has its purposes as I listed above. It should be extending Unity to make it simpler to write code in the engine.

SimonDarksideJ commented 5 years ago

Thanks for your feedback @blgrossMS On one point though

Unity is abandoning MonoBehaviours, This just simply isn't true, it is not a deprecated feature.

We're not saying Unity are deprecating MonoBehaviours. Just that MonoBehaviours are the children of evil powers and should be avoided if you can :D ECS, Like XRTK's service structure are a way to move past MB's for the majority of operations that are not UI based.

MRTK should be handling 90% and the developers 10% Agreed, this is a bit lofty, but that was the aim. We are far from it currently, mainly due to the lack of a Mature SDK. But the intent is to have the majority of the tools available that are used in the majority of projects ready off the shelf to reduce the needs for custom code. It's always the dream, but we're closer to 50/50 (or 40/60 tbh) currently.

Just a few additions

StephenHodgson commented 5 years ago

most existing codebases are not going to swap entirely from a GameObject MonoBehavior system to an ECS/DOTS system

Yup, and I think the MRTK is that middle stepping stone to get there. It helps ease developers into the workflow and thinking that they'll need for future projects that utilize them.

It should be extending Unity to make it simpler to write code in the engine.

Agreed, but what about those developers that aren't writing code? You'd be abandoning those creators that just want to focus on getting content made.

If MRTK is building an SDK for Unity, it should be Unity centric.

Agreed, and all presentation layer GameObjects would be. Infact, many people are forgetting that the entire event system is based on and extends the built-in features already.

The only thing the MRTK does is make that clear distinctions between Presentation layer and Core application layer. The SDK should entirely based on MonoBehaviors with interface event handlers.

david-c-kline commented 5 years ago

Quick note: This proposal has been refined and will be updated shortly.

david-c-kline commented 5 years ago

Thank you to everyone who provided feedback (here, on email, in a call, in person).

The following is the plan of record.

Plan of Record: Support Service Specific Scene Presence

Microsoft Mixed Reality Toolkit v2

This document describes a design change for the Microsoft Mixed Reality Toolkit v2 concerning how developer customers can choose the MRTK and its services.

The Issue: Usability

The MRTK team has received feedback from multiple sources that the current, single object presence in the application scene is difficult to use with regards to feature discovery and modularization. Multiple customers, both internal and external, have requested an alternative to beta 2’s MixedRealityToolkit object in the form of system specific locator components (ex: MixedRealityInputLocator) that can be additively included.

One piece of feedback, heard from multiple sources, points out the promise of additive vs. subtractive consumption of MRTK. The comments largely center around two themes:

  1. It is not easy, at a glance, to see if a scene is using a specific MRTK service. Many developers from whom we heard desire an individual service presence in the scene hierarchy.
  2. The root Mixed Reality Toolkit configuration profile makes it appear that all services are present in the application, even when disabled. There is a strong desire for scoping the profile inspector to show only installed and active services.

Solution: Enable Support for Individual Services to have Scene Presence

The solution defines the following changes:

  1. Provide service registration interfaces to enable customers to create a custom service locator and/or fully stand-alone service components
  2. Provide support for exposing active services and their configuration settings in the scene hierarchy at edit time via service facades
  3. Enable customers to create hybrid application architectures, if desired, that use any number of service locators in conjunction with any number of stand-alone services
  4. Create a light-weight service registry that maps service interfaces with the concrete type that manages service registration
  5. An updated scene hierarchy to contain the service registry, service facades, etc
  6. Modified MRTK project configuration flow to provide enhanced flexibility for customers when setting up their projects

Please see the Solution Details section of this document for information the described changes.

Goals

  1. Respond to direct customer feedback
  2. Provide customers with a choice of how they consume MRTK services
  3. Minimize required client code changes
  4. Preserve as much of the MRTK service locator functionality as possible (anticipating 90% or greater equivalence)

Non-Goals

  1. Removal of the service locator pattern
  2. Require developers to change how they wish to work

Solution Details

Service Registration Interfaces

To enable developers to create custom service locators and/or extend an existing service locator to add support for MRTK services, a set of interfaces will be created to support the registration and retrieval of service instances.

It is anticipated that there will either be one or three interfaces that will be created. The following sections describe the pros and cons of each implementation.

Single Registrar Interface

In the single interface approach, the MRTK will define IMixedRealityServiceRegistrar. This interface would define explicit methods for supporting objects implementing the following service types (listed by interface):

Each method (Register, Unregister, Get, CheckRegistration) will have a version that is type safe to the listed interfaces.

Pros Cons
Only one interface to implement Implementors may be required to implement stub methods
Can provide a single base class supporting all service types

Multiple Registrar Interfaces

In the multiple interface design, each type of service would have a corresponding registrar interface:

This approach would allow customers to support only the specific types of services necessary to build their component.

Pros Cons
Developers are free to implement only the required service type support Potentially mutltiple interfaces to implement
Develipers do not have to implement stub methods Cannot consume individual interface base class implementations (C# limitations)

Base Class Implementations

As with much of the MRTK, base classes will be provided to provide robust, reusable implementations of the registrar interface(s).

Providing the Registrar Instance to Services

When the registrar instantiates a service instance, it will provide itself as a constructor argument. The following example shows the parameter to be added to each service constructor.

public ServiceClass(IMixedRealityRegistrar registrar, ...);

A similar pattern will occur for data providers.

When a service requests registration of a data provider, it calls RegisterDataProvider, handing a reference to itself.

registrar.RegisterDataProvider<IMixedRealitySpatialObserver, IMixedRealitySpatialAwarenessSystem>(IMixedRealitySpatialAwarenessSystem system, Type observerType, ...);

The data provider's constructor is passed the provided reference in its constructor.

public SpatialObserver(IMixedRealitySpatialAwarenessSystem system, ...);

Service Facades

Service facades are an editor only monobehavior that exists in the application scene to give customers and easy, at-a-glance representation of the active services in the scene. When selected, the facade will display the service specific profile inspector and allow for easy configuration.

Backing these facades will be added to the scene and backed by the MRTK service locator. Developers not wishing these facades to be in their scene will be able to disable them in the MixedRealityToolkitConfiguration profile.

Hybrid Application Architecture

Hybrid applications architecture is a term intended to describe an application which utilizes any number of service locators and/or stand-alone service implementations.

To support hybrid architectures, MRTK will add a light-weight service registry that enables client code easy access to concrete service implementations.

Service Registry

The service registry in MRTK is an optional component that client code can use to acquire an instance of a desired service.

The registry can be thought of as a table that maps service interfaces to the object that manages the concrete implementation(s) .

The following table illustrates an example registry:

Registrar Interface
MixedRealityToolkitServiceLocator IBoundarySystem
CustomServiceLocator ISpatialAwarenessSystem
StandaloneInputSystem IInputSystem

The component managing the service registry can be attached to client script(s) or developers can choose to bypass the registry and hard-code references to the object managing service registration.

For example:

IServiceInterface service = MixedRealityToolkit.Instance.GetService<IServiceInterface>();

Scene Hierarchy

To help keep the scene hierarchy uncluttered, the default MRTK configuration process will create a Mixed Reality Toolkit parent object. This object will be the parent for the service locator and any enabled facades.

It is recommended that this object be the parent of objects created by registered services (ex: spatial awareness mesh objects).

The following image illustrates the default hierarchy when the MRTK service locator is used with the input and spatial awareness system facades enabled.

Hierarchy With Facades

Project Configuration UX

To make it easy and intuitive for customers to configure MRTK to suit their application's requirements, the Mixed Reality Toolkit menu's Configure item will display a selection dialog similar to the following illustration.

Configuration Dialog

SimonDarksideJ commented 5 years ago

Ok, Sorry, but the proposal does not meet and far exceeds the "goal" of this request.

It simply states this proposal is to "Plan of Record: Support Service Specific Scene Presence"

However, this "proposal" still does to many things and in order to be successful should still be broken in to multiple parts.

It still contains two fundamental changes in the way MRTK operates:

  1. To enhance the scene management for developers to gain a better understanding of what is running in the scene (which is honest feedback and something the SDK should provide without changing the architecture)
  2. An improved ability to navigate configuration as key elements are too deep in the configuration hierarchy (which again is great feedback and likely will either need extensions to the configuration profiles or amendments to ease navigation, which still wouldn't need changes to the foundation, except maybe some editor inspector enhancements)
  3. Fundamentally changing and extending how services register their presence and enable standalone services to operate. This according to the proposal will enhance the foundation to support this workflow.

WE seriously cannot have a SINGLE task and expect it to fully detail the outcomes for each of these proposals. Seems we are beyond debate now and should just push ahead, so please setup three independent "Tasks" outlining each of the requirements and the proposal delivery mechanism for each. This "Request" is simply too large for any one change.