-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Mrtk development serialization fix #2981
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
Merged
david-c-kline
merged 8 commits into
microsoft:mrtk_development
from
SimonDarksideJ:mrtk_development_SerializationFix
Oct 26, 2018
Merged
Mrtk development serialization fix #2981
david-c-kline
merged 8 commits into
microsoft:mrtk_development
from
SimonDarksideJ:mrtk_development_SerializationFix
Oct 26, 2018
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1: Cleared out invalid serialised "services" from the DefaultMixedRealityToolkitConfigurationProfile.asset 2: Removed serialization functions from the MixedRealityToolkitConfigurationProfile script Continuing to address other comments noted in the issue, including the double use of "IsCoreSystem"
* Added success / fail return to RegisterService call * Updated Internal use of RegisterService to capture success / Fail response
32 tasks
|
incorporated the IsAssignableFrom fix from @StephenHodgson s other PR #2976 |
Assets/MixedRealityToolkit-SDK/Profiles/DefaultMixedRealityToolkitConfigurationProfile.asset
Outdated
Show resolved
Hide resolved
Assets/MixedRealityToolkit/_Core/Services/MixedRealityToolkit.cs
Outdated
Show resolved
Hide resolved
StephenHodgson
suggested changes
Oct 26, 2018
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just a few minor changes.
…lkitConfigurationProfile.asset Co-Authored-By: SimonDarksideJ <[email protected]>
…ssignableFrom(type))
…m/SimonDarksideJ/MixedRealityToolkit-Unity into mrtk_development_SerializationFix
StephenHodgson
previously approved these changes
Oct 26, 2018
Assets/MixedRealityToolkit/_Core/Services/MixedRealityToolkit.cs
Outdated
Show resolved
Hide resolved
|
That felt like a rapid fire round |
StephenHodgson
approved these changes
Oct 26, 2018
david-c-kline
approved these changes
Oct 26, 2018
|
confirmed fixed 2979 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
@davidkline-ms noted an exception that was being raised in a UWP build on both Windows10 and HoloLens in #2979.
On investigation, this was actually caused by poorly serialised data in the DefaultMixedRealityConfigurationProfile asset. When the Core Services get reordered for Initialization, these serialised versions were also being detected and as they were being serialized by their concrete types and not their interface, the system failed to identify them as core systems.
This had two effects:
It was also noted by others that there was some duplicated core system type checking, which needs cleaning up
Resolution
Several changes have been made to resolve and reduce the risk of this happening in the future
Changes
Tested on