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

Explicitly spec out when requestReferenceSpace() can reject queries #651

Merged
merged 3 commits into from
May 17, 2019

Conversation

Manishearth
Copy link
Contributor

fixes #564

@Manishearth
Copy link
Contributor Author

r? @NellWaliczek

not sure if this is what you were looking for. I can also change the language to be stronger/weaker on this.

(I suspect we'll have to tweak this a bit as our permissions model comes in)

Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

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

This is a good initial pass, but we probably want to iterate on how we present these requirements a bit.

I think the first new line, about rejecting bounded/unbounded for inline, is perfect as-is. After that I feel like we might want something more along the lines of "In condition if the requirements for the type are not met, return null", and then point back at the type definitions somehow for individualized requirements. (I'm not sure what the right way to indicate that kind of "one-to-many" relationship in spec text is.)

For example: The bounded reference space should only be supported if the XR device can report a bounded area and accurately align with the floor. (And Nell and I have talked about limiting it to only supporting play areas above a certain size as well, but we don't have an exact recommendation for that yet.) That doesn't feel like criteria that belongs in this algorithm, though. I'd rather have it appear in the bounded ref space definition somehow, and have the algorithm refer back to it.

index.bs Outdated
1. If |type| is {{bounded-floor}} or {{unbounded}}, and |session|'s [=XRSession/mode=] is not {{XRSessionMode/inline}} the user agent may choose to return <code>null</code> if such experiences are not supported by the underlying [=/XR device=]
1. If |type| is {{local-floor}} or {{local}}, and |session|'s [=XRSession/mode=] is {{XRSessionMode/inline}} the user agent may choose to return <code>null</code> if such experiences are not supported by the underlying [=/XR device=] in non-immersive sessions

NOTE: All other combinations of {{XRReferenceSpaceType}}s and [=XRSession/mode=]s are guaranteed by this spec, provided the [=XRSession/mode=] itself is supported.
Copy link
Member

Choose a reason for hiding this comment

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

I can't imagine placing a note in the middle of an algorithm like this formats nicely? It should definitely be moved outside the algorithm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notes like this seem to work fine, they get indented. A couple specs do this

Copy link
Member

Choose a reason for hiding this comment

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

Well color me surprised! Today I learned something new.

@toji
Copy link
Member

toji commented May 16, 2019

Happened to be able to catch @bfgeek and ask him about this. (Presented as "I want my spec algorithm to have a virtual function, is that a thing?") He wasn't aware of any other specs that did something similar, and recommended a "manual V-Table dispatch" (because we are oh so painfully geeky) like so:
requestReferenceSpace algorithm:

  1. If the [=reference space is supported=] of type |type| etc etc...

Then, near the reference space definitions, define the support tests if they're simple enough or link off to a new algorithm if they're complex:
reference space is supported:

  1. If |type| is {{inline}} return true.
  2. If |type| is {{local}} return true if |session| is an [=immersive session=] or etc. etc...
  3. If |type| is {{bounded}} return if [=bounded reference spaces are supported=]

And then in a contextually relevant spot:
bounded reference spaces are supported:

  1. If the [=XR device=] cannot report boundaries, return false.
  2. If the [=XR device=] cannot identify the height of the user's physical floor, return false.
  3. Return true.

(Obviously I'm being pretty hand-wavy with all of that.) Sorry that this is turning out to be a bigger issue than it first appeared.

@Manishearth
Copy link
Contributor Author

Thanks, I don't mind being more explicit in spec text :) I wasn't quite comfortable with the compact text in the current PR either but I didn't know enough about the various criteria for support to write more. This seems good!

@Manishearth
Copy link
Contributor Author

Redid it. I'm not quite sure what the criteria for inline local and unbounded spaces should be, but I took a guess.

Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

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

This is great! A couple more tweaks, but I think we're close now. We'll definitely be coming back to these later and adding criteria around user consent and such once some of the privacy policies are landed, but this will give us a much better framework in which to define those restrictions. Thanks!

index.bs Outdated Show resolved Hide resolved
index.bs Outdated
1. If |type| is {{local}} or {{local-floor}}, and |session| is an [=immersive session=], return <code>true</code>.
1. If |type| is {{local}} or {{local-floor}}, and the [=/XR device=] supports reporting orientation data, return <code>true</code>.
1. If |type| is {{bounded-floor}} and |session| is an [=immersive session=], return if [=bounded reference spaces are supported=] by the [=/XR device=].
1. If |type| is {{unbounded}}, |session| is an [=immersive session=], and the [=/XR device=] supports reporting position and orientation data, return <code>true</code>.
Copy link
Member

Choose a reason for hiding this comment

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

I'm don't think this is an adequate requirement for unbounded, but I'm not sure how to describe it better. @thetuvix, do you have any thoughts about an appropriate support test here?

Copy link
Member

Choose a reason for hiding this comment

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

For the sake of moving this PR forward, I propose we change this to something along the lines of "supports stable tracking near the user over an unlimited distance, return true."

If there's a more accurate way to describe the requirements for an unbounded space then we can fix it up later, but that should be good enough for now and prevent implementers from saying "Oh, by this definition my Vive totally supports unbounded!"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@toji
Copy link
Member

toji commented May 17, 2019

Awesome, thanks!

@toji toji merged commit 13a978f into immersive-web:master May 17, 2019
@@ -877,6 +892,15 @@ Each {{XRBoundedReferenceSpace}} has a <dfn for="XRBoundedReferenceSpace">native

The <dfn attribute for="XRBoundedReferenceSpace">boundsGeometry</dfn> attribute is an array of {{DOMPointReadOnly}}s such that each entry is equal to the entry in the {{XRBoundedReferenceSpace}}'s [=XRBoundedReferenceSpace/native bounds geometry=] premultiplied by the {{XRRigidTransform/inverse}} of the [=XRSpace/origin offset=]. In other words, it provides the same border in {{XRBoundedReferenceSpace}} coordinates relative to the [=XRSpace/effective origin=].


<div class="algorithm" data-algorithm="bounded-space-supported">
Copy link
Member

Choose a reason for hiding this comment

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

@thetuvix are you on board with this language? If not, please feel free to propose an alternative when you update your tracking loss PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a reasonable approach. If the device can produce a floor-based coordinate system, but doesn't know boundaries, it should only support local-floor.

If the device somehow knows boundaries without knowing the floor, I suppose the UA could estimate the floor as with local-floor, but I don't know of any such devices today, so that would be more theoretical. We can always relax this later.


1. If |type| is {{inline}}, return <code>true</code>.
1. If |type| is {{local}} or {{local-floor}}, and |session| is an [=immersive session=], return <code>true</code>.
1. If |type| is {{local}} or {{local-floor}}, and the [=/XR device=] supports reporting orientation data, return <code>true</code>.
Copy link
Contributor

Choose a reason for hiding this comment

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

To confirm, apps should be able to get a local reference space even before the session becomes immersive?

Copy link
Member

Choose a reason for hiding this comment

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

Sessions do not "become" immersive, they are a single mode for their lifetime. Inline sessions should be able to create local reference spaces, assuming the UA has properly handled the privacy implications (ongoing discussion).

@AdaRoseCannon AdaRoseCannon added the ed:spec Include in newsletter, spec change label Jun 17, 2019
@Manishearth Manishearth deleted the request-ref branch August 13, 2019 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ed:spec Include in newsletter, spec change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

requestReferenceSpace() should talk more about when you can reject the request
5 participants