Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix profiles reset and services reinitialized when already running #646

Merged
merged 1 commit into from
Jun 19, 2020

Conversation

FejZa
Copy link
Contributor

@FejZa FejZa commented Jun 19, 2020

XRTK - Mixed Reality Toolkit Pull Request

Overview

Fixes the issue reported by @SimonDarksideJ in the hands PR #552 (comment)

Simulated Hands inspector spams console if looked at in runtime

Replication Steps:

Open Demo Scene
Run on any platforms
Select the MRT Game Object
Navigate to "Input System profile"
Expand / touch the IMixedRealityInputDataProvider list
See error SPAM
Error Details

Object reference not set to an instance of an object
  at XRTK.Providers.Controllers.Simulation.BaseSimulatedControllerDataProvider.RefreshSimulatedDevices () [0x00001] in C:\LocalDevelopment\XRTK-Core\XRTK-Core\Packages\com.xrtk.core\Runtime\Providers\Controllers\Simulation\BaseSimulatedControllerDataProvider.cs:129 
  at XRTK.Providers.Controllers.Simulation.BaseSimulatedControllerDataProvider.Update () [0x00008] in C:\LocalDevelopment\XRTK-Core\XRTK-Core\Packages\com.xrtk.core\Runtime\Providers\Controllers\Simulation\BaseSimulatedControllerDataProvider.cs:111 
  at XRTK.Services.MixedRealityToolkit.UpdateAllServices () [0x00095] in C:\LocalDevelopment\XRTK-Core\XRTK-Core\Packages\com.xrtk.core\Runtime\Services\MixedRealityToolkit.cs:1479 
UnityEngine.Debug:LogError(Object)
XRTK.Services.MixedRealityToolkit:UpdateAllServices() (at Packages/com.xrtk.core/Runtime/Services/MixedRealityToolkit.cs:1483)
XRTK.Services.MixedRealityToolkit:Update() (at Packages/com.xrtk.core/Runtime/Services/MixedRealityToolkit.cs:697)
Expected Behaviour:
No errors while navigating the inspector, whether editor or runtime

Changes

What would happen is that upon selecting / unfolding any data provider configuration in the profiles, while the application is in play mode, causes a full profile reset. This destroys all services and does a full reinitialization of the toolkit. As a side effect of this, the playspace would also get destroyed and recreated. It would then not appear in DontDestroyOnLoad collection anymore but in the base scene itself.

To prevent these issues, I disabled resetting profiles when the application is running in editor play mode.

@FejZa FejZa added Bug Something isn't working Ready for review PR finished primary development, open for review labels Jun 19, 2020
@FejZa FejZa self-assigned this Jun 19, 2020
Copy link
Contributor

@SimonDarksideJ SimonDarksideJ left a comment

Choose a reason for hiding this comment

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

What an odd issue, surprised we haven't hit it before.

@FejZa
Copy link
Contributor Author

FejZa commented Jun 19, 2020

Right? I'd like @StephenHodgson to verify this as well as it's his territory and I can't oversee side effects of this change. The question is what happens to changes made to profiles during play mode. I think they are saved to the assets nonetheless but XRTK will not automatically reinitialize when play mode is left. We might want to disable profile editing when in playmode at all.

@SimonDarksideJ
Copy link
Contributor

I'm fine merging this. Unity isn't really meant to be edited at runtime and resets everything back when you finish playing anyway. So I think this is fine. If you need to reset in runtime, you still can, just not while in the editor

@SimonDarksideJ SimonDarksideJ merged commit 9f38fb3 into development Jun 19, 2020
@SimonDarksideJ SimonDarksideJ deleted the fix/runtime-profiles-reset branch June 19, 2020 11:45
@StephenHodgson
Copy link
Member

I guess this is fine for now, but I think it's just a temp fix for whatever you were running into. Can you give me a detailed bug report about it and a commit Sha I can checkout to reproduce it?

@SimonDarksideJ
Copy link
Contributor

You mean like the detailed log in this PR and the linked issue comment in the description?

But you don't needs SHA, simply comment out the 4 lines and try the aforementioned replication steps linked

@StephenHodgson
Copy link
Member

I'd like a Sha commit just cause it was a lot going on.

I didn't see the rest as I was doing a quick reply from the app. Didn't see the rest of the thread. Sorry.

I'll do my best to check it out when I have a chance. Just seems like a cover up of an issue that could have been fixed a proper way. Again I think it's fine for now but we do want the ability to reset profiles at runtime later as we add more features

@FejZa
Copy link
Contributor Author

FejZa commented Jun 21, 2020

It's a valid fix I think as I don't see any use case and scenario where you'd want a full reset of the service locator and all services etc. while the application is running. Also this affects only in-editor work. You can still do anything as before in a built player. This is only about fiddling with profiles in the inspector while the game is running.

@StephenHodgson
Copy link
Member

I'd still like to understand what was happening and why it wasn't a problem before now.

@FejZa
Copy link
Contributor Author

FejZa commented Jun 22, 2020

I'll post a recording of what is going on over on Discord

@StephenHodgson
Copy link
Member

After reviewing this should be fine. I agree with Dino's original assessment. We probably shouldn't be resetting the service locator in playmode while in the editor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Ready for review PR finished primary development, open for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants