First set of active profile change fixes by david-c-kline · Pull Request #8050 · microsoft/MixedRealityToolkit-Unity

david-c-kline · 2020-06-12T21:20:46Z

This change is the first in a series of changes that will addresses the issues encountered when changing the active mixed reality toolkit configuration profile while the application is running.

The actual number of future PRs has not yet been determined. Test collateral and documentation will be included in the series as fixes enable them.

Failure to recreate the UIRaycastCamera

~~Change from Destroy to DestroyImmediate as the service locator expects the UIRaycastCamera to me immediately removed in order to correctly re-create it.~~

Handle data provider collection change during {Late]Update

~~This change handles the InvalidOperationException that is thrown when a collection is changed during enumeration, This prevents the code from incorrectly aborting.~~

Handle service collection change during ExecuteOnAllServices<In|Reverse>Order

~~This change replaces for with foreach and handles the InvalidOperationException that is thrown the services collection is changed during enumeration. This prevents the code from incorrectly aborting.~~

In this PR, the behavior of runtime profile change is modified to first take the request, then destroy current profile services after all LateUpdate()s of current profile services have been called, and finally initialize the new services associated with the new profile in the next frame before any Update() for a service is called. This solves the problem of potentially performing the profile change in the middle of an Update or LateUpdate of a service which causes a variety of problems. Delaying the initialization of new services to the next frame also helps to resolve the problems related to Destroy().

Testing

In editor using simulated hands
Toggle between two profiles on a click event
Confirm that interactable objects continue to function after re-acquiring the hand.
Attempt to use UnityUI
- Fails with far interaction
- Works with near interaction

david-c-kline · 2020-06-12T21:21:21Z

/azp run

azure-pipelines · 2020-06-12T21:21:35Z

Azure Pipelines successfully started running 2 pipeline(s).

david-c-kline · 2020-06-12T21:40:05Z

/azp run

azure-pipelines · 2020-06-12T21:40:18Z

Azure Pipelines successfully started running 2 pipeline(s).

julenka · 2020-06-12T21:50:58Z

+using UnityEngine;
+using UnityEngine.TestTools;
+
+namespace Microsoft.MixedReality.Toolkit.Tests


A test that I would like to see, going forward:

set the hololens 2 profile

Press a button, make sure it presses via poke

Set profile to be a hololens 1 profile

verify that poking does not press a button

Use head + airtap to press a button, verify that works.

Definitely! We should have extensive tests for ensuring this works once all of the issues have been uncovered.

Just to check @julenka you mean a test within this PR right?

I will see if that scenario can work with just the changes in this PR. It may require additional changes to fully support the requested test.

Looking at the profiles, there does not appear to be anything in the Default HoloLens1 profile that expressly disables near interaction.

I will update the profile as part of getting the above described test implemented.

david-c-kline · 2020-06-12T22:17:38Z

/azp run

azure-pipelines · 2020-06-12T22:17:53Z

Azure Pipelines successfully started running 2 pipeline(s).

keveleigh · 2020-06-12T23:02:17Z

+                }
+                catch(InvalidOperationException)
+                {
+                    Debug.Log("Data provider collection was changed during Update.");


It feels like try/catching exceptions due to the collection changing instead of more completely managing the lifecycle could lead to unexpected behavior.

For example (but please correct me if I'm wrong here!):

In update, the profile changes

This exception is thrown, and (as far as I can tell), the new providers don't get Update called on them and instead this method bails

LateUpdate is called on all (now, the new) data providers

If a data provider does something in LateUpdate to follow up on something that's expected to be done in Update, and I think there's a fairly strong promise that system lifecycle happens in a specific order, the provider could get into a weird state.

Also it looks like only Update and LateUpdate are wrapped. What if I try to change the profile when a system is enabled / disabled / reset (maybe not a common scenario, but I'm not sure)?

To follow up on this, I remember we had a conversation a while back about more explicitly handling where / when the profiles are torn down and started up. Something like:

New profile is set

Old services are torn down

All remaining service lifecycle code is cancelled for this frame

Next frame, new services are spun up

Normal service lifecycle resumes

All this to say I really think handling the lifecycle of all this more explicitly and completely might lead to not having to spot-fix specific issues one at a time (for example, if we wait until the next frame to try to recreate the UIRaycastCamera, would we have to make the DestroyImmediate race condition change? or does the frame boundary complete the clean-up and leave the scene fresh for the new services?).

if we wait until the next frame to try to recreate the UIRaycastCamera, would we have to make the DestroyImmediate race condition change?

that appears to be an issue based on Unity indicating the object exists after Destroy and before cleanup. MRTK is checking the next frame and, seeing that the object exisst, does not recreate it.

Super appreciate the feedback here!

All remaining service lifecycle code is cancelled for this frame

I'll see what i can do to ensure complete update/cleanup between changes. It may introduce an extra frame, or so, between shutdown and restart.

What if I try to change the profile when a system is enabled / disabled / reset.

The [Late]Update try/catch is only for data providers. Services are covered by the handling in ExecuteOnAllServices* methods, which are called in most (very few exceptions) situations.

An earlier incarnation of this change had exception handing for all data provider methods (enable, disable, etc) and that was originally deemed unneccessary.

Happy to revisit!

…rofile250_part1

david-c-kline · 2020-06-19T19:47:11Z

/azp run

azure-pipelines · 2020-06-19T19:47:27Z

Azure Pipelines successfully started running 2 pipeline(s).

…rofile250_part1

azure-pipelines · 2020-07-31T01:39:14Z

Azure Pipelines successfully started running 2 pipeline(s).

MaxWang-MS · 2020-07-31T03:11:32Z

/azp run

azure-pipelines · 2020-07-31T03:11:47Z

Azure Pipelines successfully started running 2 pipeline(s).

MaxWang-MS · 2020-07-31T19:18:54Z

/azp run

azure-pipelines · 2020-07-31T19:19:09Z

Azure Pipelines successfully started running 2 pipeline(s).

wiwei · 2020-08-04T19:28:17Z

        /// <inheritdoc/>
        public override void Initialize()
        {
+            ReadProfile();


It feels like this order is a bit off - ReadProfile needs BoundaryProfile (i.e. if it's null, it will crash right?)

Or is the check of BoundaryProfile == null on line 59 not needed?

@davidkline-ms

Let me double check. I believe all of the systems that have this method may behave the same.

wiwei · 2020-08-04T19:33:02Z

+                }
+                else
+                {
+                    ResetConfiguration(value);


If I happened to pass the same profile in that matches the current one, we'd still want to reset in that case right?

Co-authored-by: Will <wiwei@microsoft.com>

wiwei · 2020-08-04T19:36:57Z

+                }
+                else
+                {
+                    ResetConfiguration(value);


Do you know why in the else case were and synchronously resetting things within this call, but for the new thing we do that async on the next update?

It does feel a bit weird that we have a divergence of behavior between these two (it feels like we should be consistent with when we chose to tear things down and restart things).

Suggest adding detailed comment describing the scenarios.

wiwei · 2020-08-04T19:45:50Z

            if (IsActiveInstance)
            {
                LateUpdateAllServices();
+                if (newProfile != null)


Can you add some comments for why we're doing RemoveCurrentProfile here and then InitializeNewProfile in Update?

It's obvious to folks working on this right now but the order actually is worth documenting.

wiwei · 2020-08-04T19:48:33Z

+        /// <remarks>
+        /// When setting the ActiveProfile, all currently running services will be destroyed and those specified in
+        /// the new profile will be instantiated and initialized. A noticable application hesitation may occur
+        /// during this process.


There's going to be time between LateUpdate and Update where MRTK is destroyed across calls to update the ActiveProfile - it woudl be good to capture that in the remarks.

Also would be good to mention that any scripts that have higher priority than the MRTK object will run before this has a chance to set things, so to be careful with accessing MRTK state if you happen to have such a thing.

wiwei · 2020-08-04T19:50:10Z


        private void Update()
        {
+            if ((CoreServices.InputSystem == null) ||


Can you add a comment on this (i.e. designed to avoid running in cases where the MRTK instance is reinitializing)

wiwei · 2020-08-04T19:52:37Z

-
            base.Initialize();
+
+            if (!Application.isPlaying || !XRDevice.isPresent) { return; }


Can you explain a bit why we swapped the order here?

@davidkline-ms

I will address in a separate pr

initialization needs to happen right away, otherwise profiles do not get read correctly.

going to open a separate PR for the boundary system issues

wiwei · 2020-08-04T19:52:50Z

+        /// <inheritdoc />
+        private void OnDestroy()
+        {
+            if (GazeCursor != null && !GazeCursor.Equals(null))


Can you leave a comment about why we both check against null and also against null?

wiwei · 2020-08-04T20:38:28Z

                    execute(services[i]);
                }
-
+                


Suggested change

david-c-kline

minor comment

david-c-kline · 2020-08-04T22:42:01Z

+                }
+                else
+                {
+                    ResetConfiguration(value);


Suggest adding detailed comment describing the scenarios.

Split boundary system fixes from #8050

davidkline-ms added 2 commits June 12, 2020 12:15

ensure the uiraycastcamera is properly recreated

29e7b4b

handle collection change during enumeration

6730a36

david-c-kline added the Profile and Inspectors label Jun 12, 2020

david-c-kline added this to the MRTK 2.5.0 milestone Jun 12, 2020

david-c-kline requested review from CDiaz-MS, Zee2, julenka, keveleigh and wiwei June 12, 2020 21:20

david-c-kline self-assigned this Jun 12, 2020

add first profile change test

5ff8b27

julenka reviewed Jun 12, 2020

View reviewed changes

davidkline-ms added 2 commits June 12, 2020 15:13

remove unused files

c1926a9

test cleanup

8da0ed0

keveleigh reviewed Jun 12, 2020

View reviewed changes

wiwei reviewed Jun 15, 2020

View reviewed changes

Comment thread Assets/MRTK/Core/Services/MixedRealityToolkit.cs Outdated

wiwei reviewed Jun 15, 2020

View reviewed changes

Comment thread Assets/MRTK/Core/Services/MixedRealityToolkit.cs Outdated

wiwei reviewed Jun 15, 2020

View reviewed changes

Comment thread Assets/MRTK/Core/Services/BaseDataProviderAccessCoreSystem.cs Outdated

davidkline-ms added 2 commits June 19, 2020 12:26

Merge remote-tracking branch 'upstream/mrtk_development' into changeP…

f2ec6d3

…rofile250_part1

restore for() loops

d3dc55a

davidkline-ms added 2 commits June 22, 2020 09:32

Merge remote-tracking branch 'upstream/mrtk_development' into changeP…

03cf860

…rofile250_part1

add remarks doc comment for MRTK.ActiveProfile

ea587f2

Update GazeProvider.cs

931b872

Fix test problem

7ed8919

MaxWang-MS requested review from Troy-Ferrell, keveleigh, thalbern and wiwei July 31, 2020 19:32

wiwei reviewed Aug 4, 2020

View reviewed changes

Comment thread Assets/MRTK/Core/Services/MixedRealityToolkit.cs Outdated

Update Assets/MRTK/Core/Services/MixedRealityToolkit.cs

caf3b2d

Co-authored-by: Will <wiwei@microsoft.com>

wiwei reviewed Aug 4, 2020

View reviewed changes

Comment thread Assets/MRTK/Core/Services/MixedRealityToolkit.cs

wiwei reviewed Aug 4, 2020

View reviewed changes

Comment thread Assets/MRTK/Core/Services/MixedRealityToolkit.cs

execute(services[i]);

}

wiwei Aug 4, 2020

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

david-c-kline commented Aug 4, 2020

View reviewed changes

david-c-kline mentioned this pull request Aug 4, 2020

Split boundary system fixes from #8050 #8255

Merged

david-c-kline pushed a commit that referenced this pull request Aug 5, 2020

Merge pull request #8255 from davidkline-ms/boundaryFix

d2878d0

Split boundary system fixes from #8050

mrtk-bld added a commit that referenced this pull request Aug 5, 2020

Docs for commit d2878d0 (Split boundary system fixes from #8050)

44ac5c1

MaxWang-MS mentioned this pull request Aug 6, 2020

Enabling runtime profile switching #8260

Merged

wiwei closed this Aug 6, 2020

david-c-kline deleted the changeProfile250_part1 branch August 7, 2020 23:20


		base.Initialize();

		if (!Application.isPlaying \|\| !XRDevice.isPresent) { return; }

Conversation

david-c-kline commented Jun 12, 2020 • edited by MaxWang-MS Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

david-c-kline commented Jun 12, 2020

Uh oh!

azure-pipelines Bot commented Jun 12, 2020

Uh oh!

david-c-kline commented Jun 12, 2020

Uh oh!

azure-pipelines Bot commented Jun 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

david-c-kline commented Jun 12, 2020

Uh oh!

azure-pipelines Bot commented Jun 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

david-c-kline commented Jun 19, 2020

Uh oh!

azure-pipelines Bot commented Jun 19, 2020

Uh oh!

azure-pipelines Bot commented Jul 31, 2020

Uh oh!

MaxWang-MS commented Jul 31, 2020

Uh oh!

azure-pipelines Bot commented Jul 31, 2020

Uh oh!

MaxWang-MS commented Jul 31, 2020

Uh oh!

azure-pipelines Bot commented Jul 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

david-c-kline commented Jun 12, 2020 •

edited by MaxWang-MS

Loading