-
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 |
---|---|---|
|
@@ -42,43 +42,35 @@ VR provides an interesting canvas for artists looking to explore the possibiliti | |
|
||
The basic steps any WebVR application will go through are: | ||
|
||
1. Request a list of the available VR devices. | ||
2. Checks to see if the desired device supports the presentation modes the application needs. | ||
3. If so, application advertises VR functionality to the user. | ||
4. User performs an action that indicates they want to enter VR mode. | ||
5. Request a VR session to present VR content with. | ||
6. Begin a render loop that produces graphical frames to be displayed on the VR device. | ||
7. Continue producing frames until the user indicates that they wish to exit VR mode. | ||
8. End the VR session. | ||
1. Request a VR device that supports the presentation modes the application needs. | ||
1. If a device is available, application advertises VR functionality to the user. | ||
1. User performs an action that indicates they want to enter VR mode. | ||
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 say “triggered by user activation” explicitly? and can you link to this https://html.spec.whatwg.org/#triggered-by-user-activation 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. “enter VR mode” or “request presentation to a VR device”? |
||
1. Request a VR session from the device to present VR content with. | ||
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. “from the device with which to present”? 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. “from” -> “to”? |
||
1. Begin a render loop that produces graphical frames to be displayed on the VR device. | ||
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. “graphical frames” -> “visual frames”? admittedly, that’s not a whole lot better, but I can’t think of a more descriptive name atm. 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. we ought to be consistent with the “presented” vs. “displayed” vs. “requested” terminology |
||
1. Continue producing frames until the user indicates that they wish to exit VR mode. | ||
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. “producing” -> “sending”? 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 say “user activation” again here 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 mention anything about navigation here? IMO, it deserves a tiny mention 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 briefly mention the case of hardware error? the cases in which frames can not be send and/or not performantly? 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. admittedly, this probably isn’t the right place to get verbose, but an additional question I’d have upon reading this is, “What happens when the user takes off the VR device?” 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'm working on rewording this a bit, but I think this is the core of it: This section is to give a quick overview of the expected API use pattern, so it should favor brevity. I'll try to apply these suggestions throughout the rest of the doc, though. (Possibly as a follow up PR for clarity?) |
||
1. End the VR session. | ||
|
||
### Device enumeration | ||
### Acquiring a Device | ||
|
||
The first thing that any VR-enabled page will want to do is enumerate the available VR hardware and, if present, advertise VR functionality to the user. | ||
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. | ||
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. “present” -> “available” (to match usage elsewhere) 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. technically, this is being advertised to the API/UA/developer 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. "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. |
||
|
||
`navigator.vr.getDevices` returns a [`Promise`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) that resolves to a list of available devices. Each `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 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. | ||
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. Done |
||
|
||
```js | ||
let vrDevice = null; | ||
|
||
navigator.vr.getDevices().then(devices => { | ||
if (devices.length > 0) { | ||
// Use the first device in the array if one is available. If multiple | ||
// devices are present, you may want to provide the user a way of choosing | ||
// which device to use. | ||
vrDevice = devices[0]; | ||
onVRAvailable(); | ||
} else { | ||
// Could not find any VR hardware connected. | ||
} | ||
}, err => { | ||
// An error occurred querying VR hardware. May be the result of blocked | ||
// permissions by a parent frame. | ||
navigator.vr.requestDevice().then((device) => { | ||
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. nit: don't need the parentheses since this is a single, non-expanded argument 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. Done |
||
vrDevice = device; | ||
onVRAvailable(); | ||
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 think we might as well have this example pass 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. Done, though I still set the global variable in onVRAvailable simply because it's easier to track over the various example snippets than constantly passing the device around. |
||
}, (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 |
||
}); | ||
``` | ||
|
||
### Sessions | ||
|
||
A `VRDevice` indicates the presence of a VR hardware device but provides very little information about it outside of a name that could be used to select it from a list. 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`. | ||
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.
|
||
|
||
Sessions can be created with one of two levels of access: | ||
|
||
|
@@ -622,19 +614,24 @@ partial interface Navigator { | |
readonly attribute VR vr; | ||
}; | ||
|
||
[SecureContext, Exposed=Window] interface VR : EventTarget { | ||
attribute EventHandler ondeviceconnect; | ||
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 be updated to reflect the removal of these 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. Which documentation are you referring to in this case? There's not much in the way of docs for the 2.0 API yet. 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 thought I had read a passage on these events - but I was wrong. I was thinking of the spec. this is fine 👍 |
||
attribute EventHandler ondevicedisconnect; | ||
dictionary VRDeviceFilter { | ||
boolean exclusive = false; | ||
}; | ||
|
||
dictionary VRDeviceRequestOptions { | ||
required sequence<VRDeviceFilter> filters; | ||
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. they're simple, but can we add a sample code block showing usage of filters? 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. why's this 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. Agreed that we need example code that uses filters, I'll try to add that. As for the required on an optional, it's a pattern I've seen elsewhere before. Simply saying that the dictionary itself is optional, but if you include one then you must specify filters. (Otherwise why are you passing the dictionary at all?) 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. After discussion with the rest of the implementers we've decided to remove the filter criteria in the initial version so that we can have more detailed discussions about what's appropriate to filter on before we add it back in down the road. As such the need for example code is now moot. 😉 |
||
}; | ||
|
||
Promise<sequence<VRDevice>> getDevices(); | ||
[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; | ||
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. so we don't have the plural vs. singular and active vs. past tenses, do you think 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. on second thought, perhaps it's better to be explicit. but, if only one event is emitted per 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. shouldn't there be some navigator.vr.addEventListener('devicechange', evt => {
// Which VR device changed? And to which state did it change?
}); see the Permissions API for similar cases. 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'm not too opinionated on the exact verbiage (though I think "devices" is more accurate than "device", since there may in fact be more than one.) I don't know what we could appropriately surface in the event without brushing up against the fingerprinting concerns that prompted this change, though. If we deliver the devices in question via this event then developers could presumably just monitor the event to get a more complete view of the system than the query delivers. The purpose of the event is simply to indicate to developers that if they had previously asked for a device and didn't get an appropriate one back they may now want to try again. Given that, is there an appropriate bit of contextual information that you can think of that would be useful here without describing the available hardware explicitly? (It's a bit of an annoying song and dance, I know, but: Web.) 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. Changed to |
||
Promise<VRDevice> requestDevice(optional VRDeviceRequestOptions options); | ||
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. to avoid adding the new dictionary interface, 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 try to avoid 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. gotcha, makes sense. I assume that was the rationale behind this issue you filed: w3c/permissions#158 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. No, that had a different reason (and doesn't seem to be going anywhere). |
||
}; | ||
|
||
// | ||
// Device | ||
// | ||
|
||
[SecureContext, Exposed=Window] interface VRDevice : EventTarget { | ||
readonly attribute DOMString deviceName; | ||
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. this need to be removed? I assume that this is to avoid fingerprinting? without the ability to query or filter VR devices, this seems to perhaps be a premature change. I don’t have metrics in front of me (nor do I think it’d be easy to inspect existing WebVR content to determine), but I can see this complicating content. I know there’s an issue on file to handle fingerprinting. do you want to handle that in a separate PR? 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 you mention in the PR description that this change is to satisfy Apple’s fingerprinting concerns. I suppose this doesn’t completely. and if anything, hopefully the removal of this encourages developers to not sniff and exclude particular headsets (à la the abuse of after having thought about it, I think this change is acceptable. 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 would understand the impact to developer on making "responsive" webvr apps cross HMDs. I mean webvr apps are able to check 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 don't have a great answer for you, unfortunately. (This is something that WebGL in general struggles with.) But I can say with confidence that scaling scene complexity based on the device name is a bad idea. At the very least it would mean that you'd have to update your code every time a new device hit the market, and it's very easy to get wrong (Not only would browser possibly expose different names but developers might check for, say, the string "Vive" and load high-end assets not realizing that HTC has plans to use the Vive branding for standalone devices as well.) There's a variety of methods out there to determine if you're on a mobile device or not, so that could be used as a baseline performance differentiator. Beyond that you could ask users to pick a detail level (high, medium, low) or try to scale it dynamically as your experience runs. I wish I had a better answer, but this is a problem that extends well beyond WebVR and we wouldn't be doing the platform any favors by encouraging another form of user agent sniffing. |
||
readonly attribute boolean isExternal; | ||
|
||
Promise<void> supportsSession(optional VRSessionCreationOptions parameters); | ||
|
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.
mentioning availability is good, but what about permission? want to make a mention here?