Tracking loss and tracking recovery by thetuvix · Pull Request #559 · immersive-web/webxr · GitHub
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

Tracking loss and tracking recovery #559

Merged
merged 20 commits into from
Jun 26, 2019

Conversation

thetuvix
Copy link
Contributor

@thetuvix thetuvix commented Mar 12, 2019

This resolves #243 by addressing the Jan 2019 F2F discussion we had regarding #243 (comment).

@thetuvix thetuvix mentioned this pull request Mar 12, 2019
@NellWaliczek NellWaliczek added the agenda Request discussion in the next telecon/FTF label Mar 25, 2019
@toji toji removed agenda Request discussion in the next telecon/FTF labels Mar 29, 2019
@NellWaliczek NellWaliczek added this to the May '19 Working Draft milestone Apr 5, 2019
@cwilso
Copy link
Member

cwilso commented Apr 18, 2019

Does this PR fully address the tracking loss handling, or is there substantially more to do?

@thetuvix thetuvix marked this pull request as ready for review April 23, 2019 08:41
@thetuvix
Copy link
Contributor Author

Apologies for the delay! Just emerging from a bunch of non-WebXR craziness here on my end...

This PR up to now had covered the tracking loss half of the explainer and I've just pushed a commit to cover tracking recovery as well. This should bring the spatial tracking explainer up to date with where we landed after the F2F discussion regarding #243, including some of the changes that differed from the original proposal. (most notably that an unbounded reference space continues tracking its previous physical origin after tracking loss unless tracking recovers into a disjoint map fragment)

Pinging @NellWaliczek, @toji, @lincolnfrog, @Manishearth, @klausw, @darktears and @blairmacintyre to take a look at these explainer changes. If we agree on these explainer-level definitions, I can leave more precise feedback on #580, #589 and #597.

spatial-tracking-explainer.md Outdated Show resolved Hide resolved
@thetuvix
Copy link
Contributor Author

thetuvix commented May 7, 2019

Resolved outstanding comments and merge conflicts. This PR should now address the full set of explainer changes resulting from #243.

What will remain after this PR are the core spec language changes that encode this behavior on top of the native origin spec language @Manishearth added in #621. That core spec approach of defining each space in terms of the behavior of its native origin should make it far easier to specify this tracking loss and recovery behavior in a consistent way.

Once this is merged, I can open a follow-on PR to nail down that core spec language.

@thetuvix
Copy link
Contributor Author

thetuvix commented May 8, 2019

@NellWaliczek, @toji, @klausw: Are you good to merge these tracking loss/recovery explainer changes so I can follow on with spec changes?

Copy link
Member

@NellWaliczek NellWaliczek left a comment

Choose a reason for hiding this comment

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

Apologies for the delay in providing feedback and thank you so much for taking the time to get this written down. I've left a handful of comments/questions regarding the text, but have no concerns about the general design.

However, one thing that's been on my mind a lot lately is that the explainer and the spec are becoming divergent in an unmanagable way. I'd like us to get in the habit of PRs containing both sets of changes to minimize this difficulty. Would you be willing to combine both sets of changes into this PR? I promise I'll review it quickly!

spatial-tracking-explainer.md Outdated Show resolved Hide resolved
spatial-tracking-explainer.md Outdated Show resolved Hide resolved

#### Rays
An `XRRay` object includes both an `origin` and `direction`, both given as `DOMPointReadOnly`s. The `origin` represents a 3D coordinate in space with a `w` component that must be 1, and the `direction` represents a normalized 3D directional vector with a `w` component that must be 0. The `XRRay` also defines a `matrix` which represents the transform from a ray originating at `[0, 0, 0]` and extending down the negative Z axis to the ray described by the `XRRay`'s `origin` and `direction`. This is useful for positioning graphical representations of the ray.
However, once a pose is initially established for a viewer or input source, pose matrices should continue to be provided even during tracking loss. The `emulatedPosition` attribute of `XRPose` indicates that the position component of the retrieved pose matrix does not represent an actively tracked position. There are a number of reasons this might be the case. For example:
Copy link
Member

Choose a reason for hiding this comment

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

once a pose is initially established for a viewer or input source

Ok, so I see we're calling out these spaces as different than the generic XRSpace. Where do we define the behavior of the generic XRSpace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So far, all we have are reference spaces (including viewer spaces) and input source spaces. As we later define anchor spaces and other spaces, we'd need to augment the spec if we decide they behave differently than today's spaces.

For now, I've just changed this to:

However, once a pose is initially established for a space (e.g. a viewer reference space or an input source's gripSpace), pose matrices should continue to be provided even during tracking loss.

Copy link
Member

Choose a reason for hiding this comment

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

My hesitation with this new phrasing is that pose data is always queried the same way (through getPose()) regardless of the type of XRSpace being located. If future types inherited from XRSpace behave differently under tracking loss, we could end up in a situation where developers aren't prepared to handle those differences. In other words, you're making the statement that the only time this should return null is during initialization and that once a pose is returned it will always be non-null, but I'm not sure we'd take that stance for anchors in the future. Am I making sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I can see why we'd ever return non-null; assuming there is some way to know that the tracking has stopped, continuing to return the last know pose is the only option that makes sense to me. If it's optional, we'll get a pattern where developers (and libraries) will just mirror that last known pose anyway, and use "null" as a "use last know pose" flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two paths we may ultimately take with XRSpace tracking recovery for future spaces:

  • We do as @blairmacintyre says and just decide that for all future spaces, we might as well just keep returning a last-known pose once any pose is delivered, and so all future spaces just behave as today's spaces do.
  • We decide that it's simply too wrong to claim that we have any notion at all of where an XRAnchor is relative to the viewer reference space if you've recovered tracking in a disjoint new room, and so we want to be hardcore and return null for just that new flavor of XRSpace. However, this decision won't surprise any existing apps - only apps that explicitly start using XRAnchor objects and their resulting spaces will ever observe the new behavior. It seems OK for apps that explicitly opt in to a new scenario through new types to start getting more nuanced behavior from existing types, which we'd document at that point.

While I'm tempted toward the latter behavior to help stop apps from doing silly things when rendering anchored content, I believe the former is more consistent and easy to specify, and apps have the bit they need to not render at anchors when they're not actively tracked. (and as Blair points out, a naive abstraction layer above WebXR is likely to just implement "null" as "don't change the scene object's pose" anyway, and so there's a limit to how valuable being draconian is anyway) However, whatever decision we do want to make later, I think we can do so compatibly, regardless of what we say now.

#### Rays
An `XRRay` object includes both an `origin` and `direction`, both given as `DOMPointReadOnly`s. The `origin` represents a 3D coordinate in space with a `w` component that must be 1, and the `direction` represents a normalized 3D directional vector with a `w` component that must be 0. The `XRRay` also defines a `matrix` which represents the transform from a ray originating at `[0, 0, 0]` and extending down the negative Z axis to the ray described by the `XRRay`'s `origin` and `direction`. This is useful for positioning graphical representations of the ray.
However, once a pose is initially established for a viewer or input source, pose matrices should continue to be provided even during tracking loss. The `emulatedPosition` attribute of `XRPose` indicates that the position component of the retrieved pose matrix does not represent an actively tracked position. There are a number of reasons this might be the case. For example:
* A viewer with orientation-only tracking, whose position within an `eye-level` reference space represents neck modeling.
Copy link
Member

Choose a reason for hiding this comment

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

viewer

Is this phrasing compatible with the simplified spaces PR that just landed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems compatible to me - the concept of viewer remains a first-class concept in the spec.

Copy link
Member

Choose a reason for hiding this comment

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

Why viewer and not reference space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, a single space "having tracking" is a squishy shorthand for the well-defined notion of whether two particular spaces can be tracked relative to one another. I'm using that here in the explainer to get folks aligned with some digestible examples before we get specific in the following paragraphs.

When using that shorthand, though, I tend to find myself thinking of a viewer (e.g. my headset) as "having tracking", rather than thinking of my primary reference space itself (e.g. my floor) as "having tracking". While a viewer is a kind of reference space, being that generic here in this list of concrete examples felt like it would take away from readers grokking the overall concept.

spatial-tracking-explainer.md Outdated Show resolved Hide resolved
spatial-tracking-explainer.md Show resolved Hide resolved
spatial-tracking-explainer.md Outdated Show resolved Hide resolved
spatial-tracking-explainer.md Outdated Show resolved Hide resolved
spatial-tracking-explainer.md Outdated Show resolved Hide resolved
spatial-tracking-explainer.md Outdated Show resolved Hide resolved
@NellWaliczek
Copy link
Member

@thetuvix , I think we're aiming to get #640 and #642 merged by the end of day tomorrow. If so, do you have a rough idea when you'll be able to rebase this PR on top of them for another round of reviews?

thetuvix and others added 5 commits May 30, 2019 17:29
Co-Authored-By: Nell Waliczek <nhw@amazon.com>
Co-Authored-By: Nell Waliczek <nhw@amazon.com>
@thetuvix
Copy link
Contributor Author

I've rebased this PR on the latest explainer changes, updated it to account for the recent API changes, and addressed or commented on all of @NellWaliczek's feedback above.

I'm taking a look now at the spec language changes needed to encode these explainer changes formally.

Copy link
Member

@NellWaliczek NellWaliczek left a comment

Choose a reason for hiding this comment

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

Really happy with the direction this has been taking! And looking forward to the spec text changes :)

@thetuvix
Copy link
Contributor Author

@toji will be producing a first take on the core spec changes here, to save the time it will take me to get oriented from scratch in index.bs 😄

Our goal is to then get all this merged by the F2F next week.

@toji toji modified the milestones: May 2019, June 2019 Jun 7, 2019
@thetuvix
Copy link
Contributor Author

Thank you so much to @toji for pulling together spec text here! I think we're finally ready to merge.

Pinging @Manishearth, @klausw and @NellWaliczek to take a final look.

Copy link
Member

@NellWaliczek NellWaliczek left a comment

Choose a reason for hiding this comment

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

Yay! This is basically ready to go~! I had a few minor comments, but mostly nitpicky ones, so I'm gonna go ahead and approve.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
spatial-tracking-explainer.md Show resolved Hide resolved
spatial-tracking-explainer.md Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@toji
Copy link
Member

toji commented Jun 24, 2019

@thetuvix: We can proceed in one of two ways at this point. Since this is your branch it's going to be easiest for you to address any nits before we merge. If you're in a position to do so and feel like you understand the requests well enough to act on them, then feel free to go ahead and make any needed updates, after which I'll merge it. (Feel free to make the spec changes too! I'm happy to review them.)

If you aren't able to address Nell's feedback soon for whatever reason, though, I'm happy to merge this now and I'll immediately follow it up with a nit-fix PR, since that would be more effective than trying to juggle branches any further.

Which would you prefer?

(Oh, and FYI: There's a warning about the branch not being able to rebase due to conflicts, but if I set the merge mode to "squash and merge" then it's not an issue, which is what we've traditionally been doing anyway to keep the master commit log relatively clean, so don't worry about trying to fix the rebase.)

@klausw
Copy link
Contributor

klausw commented Jun 24, 2019

Thank you so much to @toji for pulling together spec text here! I think we're finally ready to merge.

Pinging @Manishearth, @klausw and @NellWaliczek to take a final look.

I haven't been following this discussion closely, but I think the current version of the change looks good. Thanks for working on this!

index.bs Outdated Show resolved Hide resolved
@thetuvix
Copy link
Contributor Author

thetuvix commented Jun 25, 2019

Just updated the spec in response to the feedback from @NellWaliczek's and @Manishearth.

Please take a final look at my spec updates here, especially to the core switch logic on line 852.

Thanks!

Copy link
Member

@NellWaliczek NellWaliczek left a comment

Choose a reason for hiding this comment

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

Other than my confusion about the reset event, looking good.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@thetuvix
Copy link
Contributor Author

@toji: I resolved the last reset goof and cleaned up the remaining feedback nits. I believe we're ready to merge now!

Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handling tracking loss
7 participants