-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Refactor WMRUtilities to support Unity legacy XR and XR SDK #6989
Refactor WMRUtilities to support Unity legacy XR and XR SDK #6989
Conversation
for Unity backend-specific implementations
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
...it.Providers/WindowsMixedReality/Shared/Definitions/IWindowsMixedRealityUtilitiesProvider.cs
Show resolved
Hide resolved
Assets/MixedRealityToolkit.Providers/WindowsMixedReality/Shared/WindowsMixedRealityUtilities.cs
Show resolved
Hide resolved
...ts/MixedRealityToolkit.Providers/WindowsMixedReality/WindowsMixedRealityUtilitiesProvider.cs
Show resolved
Hide resolved
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.
Overall changes seem alright to me modulo some naming things (and also probably seeing the other side of the changes) but I feel good about reasonable decisions being made here with the feedback.
1569030
to
44b4228
Compare
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
@@ -24,12 +24,12 @@ public struct HolographicFrameNativeData | |||
public uint MaxNumberOfCameras; | |||
|
|||
/// <summary> | |||
/// The current native root <see href="https://docs.microsoft.com/uwp/api/windows.perception.spatial.spatialcoordinatesystem">ISpatialCoordinateSystem</see>). | |||
/// The current native root <see href="https://docs.microsoft.com/uwp/api/windows.perception.spatial.spatialcoordinatesystem">ISpatialCoordinateSystem</see>. |
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.
is this the root pointer for all native access? not sure what "root" means here
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.
It's the ISpatialCoordinateSystem
at the root of the scene. Happy to use a different word if something else makes it more clear!
The current native root ISpatialCoordinateSystem
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.
ignore me.... i missed the <see ... /> part of the comment. it is fine as is
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.
Error:BaseWindowsMixedRealityCameraSettings.cs(54,17): error CS0246: The type or namespace name 'WindowsMixedRealityCameraSettingsProfile' could not be found (are you missing a using directive or an assembly reference?)
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.
@LongForever1993 Where and when are you seeing that error? Please file an issue if it's consistent.
.../MixedRealityToolkit.Providers/WindowsMixedReality/Definitions/HolographicFrameNativeData.cs
Show resolved
Hide resolved
...it.Providers/WindowsMixedReality/Shared/Definitions/IWindowsMixedRealityUtilitiesProvider.cs
Show resolved
Hide resolved
Assets/MixedRealityToolkit.Providers/WindowsMixedReality/Shared/WindowsMixedRealityUtilities.cs
Outdated
Show resolved
Hide resolved
Assets/MixedRealityToolkit.Providers/WindowsMixedReality/Shared/WindowsMixedRealityUtilities.cs
Outdated
Show resolved
Hide resolved
Assets/MixedRealityToolkit.Providers/WindowsMixedReality/Shared/WindowsMixedRealityUtilities.cs
Show resolved
Hide resolved
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. a few comments / questions.
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
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.
Overview
Refactors the static
WindowsMixedRealityUtilities
to take in a provider, which can be set in platform-specific data providers. This allows the utilities class to be called generically regardless of the Unity XR backend.Changes