-
Notifications
You must be signed in to change notification settings - Fork 386
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
Conversation
Does this PR fully address the tracking loss handling, or is there substantially more to do? |
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. |
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. |
@NellWaliczek, @toji, @klausw: Are you good to merge these tracking loss/recovery explainer changes so I can follow on with spec changes? |
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.
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
|
||
#### 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: |
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.
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?
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.
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'sgripSpace
), pose matrices should continue to be provided even during tracking loss.
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.
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?
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.
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.
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 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 theviewer
reference space if you've recovered tracking in a disjoint new room, and so we want to be hardcore and returnnull
for just that new flavor ofXRSpace
. However, this decision won't surprise any existing apps - only apps that explicitly start usingXRAnchor
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.
spatial-tracking-explainer.md
Outdated
#### 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. |
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.
viewer
Is this phrasing compatible with the simplified spaces PR that just landed?
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 seems compatible to me - the concept of viewer remains a first-class concept in the spec.
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.
Why viewer
and not reference space
?
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.
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.
Co-Authored-By: Nell Waliczek <nhw@amazon.com>
Co-Authored-By: Nell Waliczek <nhw@amazon.com>
…webxr into tracking-loss-recovery
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. |
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.
Really happy with the direction this has been taking! And looking forward to the spec text changes :)
@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. |
…webxr into tracking-loss-recovery
Tracking spec squashed
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. |
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.
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.
@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.) |
I haven't been following this discussion closely, but I think the current version of the change looks good. Thanks for working on this! |
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! |
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.
Other than my confusion about the reset event, looking good.
@toji: I resolved the last Thanks everyone! |
This resolves #243 by addressing the Jan 2019 F2F discussion we had regarding #243 (comment).