-
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
Refactor getDevices into requestDevice #289
Changes from 1 commit
8cd20fa
d4df9c0
fb2af73
c44dcaa
66dde82
433b0a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,20 +54,21 @@ The basic steps any WebVR application will go through are: | |
|
||
The first thing that any VR-enabled page will want to do is request a `VRDevice` and, if present, advertise VR functionality to the user. | ||
|
||
`navigator.vr.requestDevice` returns a [`Promise`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) that resolves to an available device which matches the given filter criteria. A `VRDevice` represents a physical unit of VR hardware that can present imagery to the user somehow, referred to here as a "VR hardware device". On desktop clients this will usually be a headset peripheral; on mobile clients it may represent the mobile device itself in conjunction with a viewer harness (e.g., Google Cardboard or Samsung Gear VR). It may also represent devices without stereo presentation capabilities but more advanced tracking, such as Tango devices. | ||
`navigator.vr.requestDevice` returns a [`Promise`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) that resolves to a `VRDevice` if one is available. A `VRDevice` represents a physical unit of VR hardware that can present imagery to the user somehow, referred to here as a "VR hardware device". On desktop clients this will usually be a headset peripheral; on mobile clients it may represent the mobile device itself in conjunction with a viewer harness (e.g., Google Cardboard or Samsung Gear VR). It may also represent devices without stereo presentation capabilities but more advanced tracking, such as ARCore/ARKit enabled devices. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this was discuss in spec conversations, but I think it could help to have a disclaimer here: what happens when there are multiple headsets connected? or, if I unplug my Vive and plug in my Rift during a browser sesssion? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perhaps we don’t need to get verbose because there are a ton of headsets, but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Made the suggested changes, and added a section about the |
||
|
||
```js | ||
let vrDevice = null; | ||
|
||
navigator.vr.requestDevice().then((device) => { | ||
vrDevice = device; | ||
onVRAvailable(); | ||
navigator.vr.requestDevice().then(device => { | ||
onVRAvailable(device); | ||
}, (err) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can remove the parens (they aren’t used in the promise callback above) |
||
// Could not find any available VR hardware or an error occurred querying VR | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you want to say “device” instead of “hardware”? |
||
// hardware. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. personally, I’d like to see a |
||
}); | ||
``` | ||
|
||
If there are multiple VR hardware devices available the UA will need to pick which one to return. The UA is allowed to use any criteria it wishes to select which device is returned, including settings UI that allows users to manage device priority. Calling `requestDevice` should not trigger device selection UI, however, as this would cause many sites to display VR-specific dialogs early in the page lifetime when the user has not made any indication that they wish to use VR features. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, I see here you now cover the case of multiple VR devices. think you can add a heading to this section and link to it from above? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a comma after There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the device priority. it reminds me of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’d simplify this to say “user activation” explicitly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the sentence about prescribing UI behaviour (or advising against introducing UI) feels like non-normative text. do you agree? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that this isn't a spec proper the concept of "non-normative" is a little weird, but I've tried to make it a bit more apparent anyway. |
||
|
||
Future iterations of the API may add filter criteria to `requestDevice`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you feel this needs to be noted? (perhaps it could be in a “tip”-styled box) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you want to explicitly say There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
### Sessions | ||
|
||
A `VRDevice` indicates the presence of a VR hardware device but provides very little information about it. In order to do anything that involves the hardware's presentation or tracking capabilities the application will need to request a `VRSession` from the `VRDevice`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can probably shorten the first sentence to say There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a comma after There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
@@ -85,7 +86,11 @@ If a `VRDevice` is available and able to create an exclusive session, the applic | |
In the following examples we will focus on using exclusive sessions, and cover non-exclusive session use in the [`Advanced Functionality`](#non-exclusive-sessions-magic-windows) section. With that in mind, we ask here if the `VRDevice` supports sessions with `exclusive` access (the default), since we want the ability to display imagery on the headset. | ||
|
||
```js | ||
async function onVRAvailable() { | ||
let vrDevice = null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we do without the global variable? |
||
|
||
async function onVRAvailable(device) { | ||
vrDevice = device; | ||
|
||
// Most (but not all) VRDevices are capable of granting exclusive access to | ||
// the device, which is necessary to show imagery in a headset. If the device | ||
// has that capability the page will want to add an "Enter VR" button (similar | ||
|
@@ -614,17 +619,9 @@ partial interface Navigator { | |
readonly attribute VR vr; | ||
}; | ||
|
||
dictionary VRDeviceFilter { | ||
boolean exclusive = false; | ||
}; | ||
|
||
dictionary VRDeviceRequestOptions { | ||
required sequence<VRDeviceFilter> filters; | ||
}; | ||
|
||
[SecureContext, Exposed=Window] interface VR : EventTarget { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we have this (and others) exposed on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely want to get there, but I do feel it's worth having it's own issue. I don't expect it to be controversial, but any conversation that does crop up around it deserves its own space rather than being intermixed with this change. |
||
attribute EventHandler ondeviceschanged; | ||
Promise<VRDevice> requestDevice(optional VRDeviceRequestOptions options); | ||
attribute EventHandler ondevicechanged; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will documentation of this be handled in a separate PR? can you file an issue to track? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know I've asked this like a dozen different times, sorry lol… should this be past or activetense? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, this change is probably deserving of its own PR. but I think perhaps this needs to be a bit more nuanced. a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW: Changed this to |
||
Promise<VRDevice> requestDevice(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are there other APIs with similar property naming? the reason I ask is because like with the Permissions API, that all make sense? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would you be opposed to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both WebUSB and WebBluetooth use |
||
}; | ||
|
||
// | ||
|
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.
“present” -> “available” (to match usage elsewhere)
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.
technically, this is being advertised to the API/UA/developer
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.
"Advertise to the user" in this case meaning "Add a VR button to the DOM" or similar. I've added some text to that effect to the explainer.