-
Notifications
You must be signed in to change notification settings - Fork 322
FIX: Trackedposedriver stops tracking (ISXB-1555) #2189
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
base: develop
Are you sure you want to change the base?
Conversation
I have no XR expertise or devices, will look for an XR QA to add |
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2189 +/- ##
===========================================
+ Coverage 67.80% 67.81% +0.01%
===========================================
Files 367 367
Lines 53513 53526 +13
===========================================
+ Hits 36282 36301 +19
+ Misses 17231 17225 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
Added @evelinastaniulyte , she's on a hackweek right now but will be able to help us out next week |
@@ -629,6 +631,54 @@ public void Components_TrackedPoseDriver_DoesNotEnableOrDisableReferenceActions( | |||
Assert.That(trackingStateInput.action.enabled, Is.False); | |||
} | |||
|
|||
[UnityTest] | |||
[Category("Components")] | |||
public IEnumerator CanUseTrackedPoseDriverWithoutTrackingAction() |
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.
Thanks for adding this test.
@@ -432,6 +432,7 @@ protected virtual void Awake() | |||
protected void OnEnable() | |||
{ | |||
InputSystem.onAfterUpdate += UpdateCallback; | |||
InputSystem.onDeviceChange += OnDeviceChanged; |
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.
The only thing I am thinking around this one if it truly captures what would drive an action resolve (or might cause unnecessary checking)
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.
How come (before my regression of this) it worked then?
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.
The tracking was never set to None, but always kept at Position & Rotation when no tracking action was assigned. Like that there was no need to reevaluate the tracking state.
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.
There will be some amount of unnecessary checking for sure, if any (not trackable) device is connected/disconnected for instance. But I am wondering if that's so critical, it probably depends on how frequently deviceChange events happen.
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.
Since this is a HUP I think we would be fine with this and see it as an optimization if it could be reduced in the future.
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.
Seems like there are regressions in functional tests of tracked pose driver. I think these needs to be fixed.
yes, it caught a case where the device was added before the initialisation of the TrackedPoseDriver started, that's handled now. |
Was any dev testing done? It’s not mentioned in the description |
No. I wasn't able to test on the device unfortunately. |
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.
LGTM
@ritamerkl When an app runs in the HMD as non-XR, it is usually because of missing XR Plugin Management settings. Could you try testing against the repro project in ISXB-155 after following the setup steps here? @evelinastaniulyte In addition to Rita verifying the user's project, could you pick one of the XR templates to test with this? If so, then tell me which template you pick so that I can test with a different one. @ritamerkl Do these code changes affect screen space input? If so then Evelina or I should test with AR Mobile Template. |
Unfortunately I do not have access to a Quest device today and I will be on PTO next week. Does someone in the XR team have the opportunity to test on a device? That would be awesome, since it's a HUP and great to land this rather sooner than later.
No, screen space input should not directly be affected. |
Description
This PR fixes HUP bug ISXB-1555, where the TrackedPoseDriver will not update the position of a gameobject when no tracking action is assigned to the TrackedPoseDriver.
This issue was caused by a change which allowed stopping the tracking when no device is connected.
The fix is to not just disable the tracking once on setup of the TrackedPoseDriver, but adjust the tracking state whenever a device changed and potentially the tracking can be enabled again.
Testing status & QA
No testing yet.
Overall Product Risks
Please rate the potential complexity and halo effect from low to high for the reviewers. Note down potential risks to specific Editor branches if any.
Comments to reviewers
Please describe any additional information such as what to focus on, or historical info for the reviewers.
Checklist
Before review:
Changed
,Fixed
,Added
sections.Area_CanDoX
,Area_CanDoX_EvenIfYIsTheCase
,Area_WhenIDoX_AndYHappens_ThisIsTheResult
.During merge:
NEW: ___
.FIX: ___
.DOCS: ___
.CHANGE: ___
.RELEASE: 1.1.0-preview.3
.After merge: