This repository has been archived by the owner on Aug 11, 2024. It is now read-only.
Feature/diagnostics system port #156
Merged
Conversation
This file contains 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
Hiding the field from the inspector makes it impossible to set the canvas in editor, which is generally ok, but the Awake method is checking whether canvas is null before getting it. Since it cannot be set in inspector this if will also be true.
Mainly adjusting namespaces and class names that have changed. Added the visual profile component which replaces the DiagnosticsHandler that used to live in XRTK/SDK.
|
XRTK/SDK PR 28 is part of this: XRTK/com.xrtk.sdk#28 |
This was referenced May 4, 2019
|
Can we review and update the outstanding tasks prior to merging? Platforms must be tested prior to merge
Great work on this PR btw :D |
|
You also need to refresh your submodules as they are showing out of date and could cause issue. |
|
Thanks1 I can test iOS later today and check the submodules. |
|
Just a note, make sure the Submodule checkouts are always the latest development checkout sha. |
|
/azp run |
|
Pull request contains merge conflicts. |
# Conflicts: # Submodules/SDK # Submodules/WindowsMixedReality # XRTK-Core/Packages/com.xrtk.core/Inspectors/Profiles/MixedRealityDiagnosticsSystemProfileInspector.cs
...-Core/Packages/com.xrtk.core/Definitions/DiagnosticsSystem/MixedRealityDiagnosticsProfile.cs
Outdated
Show resolved
Hide resolved
...-Core/Packages/com.xrtk.core/Definitions/DiagnosticsSystem/MixedRealityDiagnosticsProfile.cs
Show resolved
Hide resolved
XRTK-Core/Packages/com.xrtk.core/Services/DiagnosticsSystem/MixedRealityDiagnosticsSystem.cs
Outdated
Show resolved
Hide resolved
XRTK-Core/Packages/com.xrtk.core/Services/DiagnosticsSystem/MixedRealityDiagnosticsSystem.cs
Outdated
Show resolved
Hide resolved
XRTK-Core/Packages/com.xrtk.core/Services/DiagnosticsSystem/MixedRealityDiagnosticsSystem.cs
Outdated
Show resolved
Hide resolved
XRTK-Core/Packages/com.xrtk.core/Services/MixedRealityToolkit.cs
Outdated
Show resolved
Hide resolved
...ore/Packages/com.xrtk.core/Services/DiagnosticsSystem/XRTK.Services.DiagnosticsSystem.asmdef
Outdated
Show resolved
Hide resolved
|
Tested on iOS. Behaves the same as on Android. |
|
I'll test it on Lumin soon. |
StephenHodgson
suggested changes
May 7, 2019
|
Getting this warning when building to device and running in the editor: |
cleaned up the system and visualizer classes a bit
…xedRealityDiagnosticsSystem.cs
…requests Feature/diagnostics change requests
|
@StephenHodgson thank you. I'll look into the shader warning. Pretty sure I know what's up. |
|
@StephenHodgson Added missing shader to XRTK.Core/StandardAssets/Shaders. The warning is gone now. |
|
@FejZa just need to resolve the conflicts and we should be good to go. |
SimonDarksideJ
approved these changes
May 8, 2019
StephenHodgson
approved these changes
May 8, 2019
XRTK-Build-Bot
pushed a commit
that referenced
this pull request
May 12, 2019
* Remove [HideInInspector] attribute for CanvasUtility canvas field Hiding the field from the inspector makes it impossible to set the canvas in editor, which is generally ok, but the Awake method is checking whether canvas is null before getting it. Since it cannot be set in inspector this if will also be true. * Commit submodules * Port latst DiagnosticsSystem updates from MRTK Mainly adjusting namespaces and class names that have changed. Added the visual profile component which replaces the DiagnosticsHandler that used to live in XRTK/SDK. * Fix DiagnosticsService constructor params * Fix asset menu path to create diagnostics profile * Resolve PR review comments * removed a bunch of stuff that wasn't used anymore. cleaned up the system and visualizer classes a bit * removed stray meta * Update XRTK-Core/Packages/com.xrtk.core/Services/DiagnosticsSystem/MixedRealityDiagnosticsSystem.cs * Add missing shader for visual profiler
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
XRTK - Mixed Reality Toolkit Change Request
Overview
Ported the latest implementation of the MRTK DiagnosticsSystem over and made necessary Adjustments. There is some open TODOs:
OnDiagnosticsChanged()inMixedRealityDiagnosticsSystem.csis never executed and the event does not fire respectively. Implementing this will allow to do runtime changes to settings of the diagnostics system, since the Visual Profiler implementsIMixedRealityDiagnosticsSystemHandler.Support for all XRTK platforms Needs to be verified. E.g. while it works on Android mobile AR it actually Displays in 3D space. Probably for mobile s screen space overlay solution would be more appropriate. Also I could not verify Lumin support.
Target of the change:
Changes: