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

Remove XRDevice #405

Merged
merged 4 commits into from Oct 5, 2018
Merged

Remove XRDevice #405

merged 4 commits into from Oct 5, 2018

Conversation

toji
Copy link
Member

@toji toji commented Oct 2, 2018

Removes the concept of an XRDevice object from the spec, moving session creation to navigator.xr directly. WebGL context compatibility is also updated out of necessity. This change reinforces the existing decision to have the UA select the device that will be used if multiple are present, and uses that assumption to simplify the API surface.

Please note that some of the diffs in this PR look larger than they really are due to the fact that some blocks of text needed to be moved around to allow the document to read more clearly in the absence of the XRDevice object.

There are also some desired upcoming changes, such as supporting a Universal Rendering Path, updating session creation, or supporting an AR session type that are being studiously ignored for the sake of keeping this PR fairly targeted. This has the side effect of making it look like some parts of the API that we anticipate changing are being reinforced by this PR when in reality it's just trying to keep the document internally consistent as we update it.

@toji toji mentioned this pull request Oct 2, 2018
@toji
Copy link
Member Author

toji commented Oct 2, 2018

@RafaelCintron: Nell made the excellent suggestion of ensuring you have a chance to thumbs-up the changes to context compatibility here. It's not too much different, the XRDevice is now implied rather than explicit, but I'd still appreciate you looking at it.

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.

Overall looks good to me! I have a few minor comments, but this is a good simplification. And a good first step towards some of the other designs we've been working on.

explainer.md Outdated
* Display imagery on the device at the appropriate frame rate.
* Detect if XR capabilities are available.
* Query the XR devices capabilities.
* Poll the XR device and associate input device state.
Copy link
Member

Choose a reason for hiding this comment

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

associate -> associated

explainer.md Outdated

**Immersive**: Requested with the `immersive: true` dictionary argument. Immersive sessions present content directly to the `XRDevice`, enabling immersive presentation. Only one immersive session per XR hardware device is allowed at a time across the entire UA. Immersive sessions must be created within a user activation event or within another callback that has been explicitly indicated to allow immersive session creation.
Testing to see if the device supports the capabilities the application needs is done via the `navigator.xr.supportsSession` call, which takes a dictionary describing the desired functionality and returns a promise which resolves if the device can support those properties and rejects otherwise. Querying for support this way is necessary because it allows the application to detect what XR features are available without actually engaging the sensors or beginning presentation, which can incur significant power or performance overhead on some systems and may have side effects such as launching a status tray or storefront. Calling `navigator.xr.supportsSession` should also not interfere with any running XR applications on the system.
Copy link
Member

Choose a reason for hiding this comment

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

may have side effects such as launching a status tray or storefront

to

may have side effects such as launching a status tray, launching a storefront, or terminating another application's access to XR hardware.


If an `XRDevice` is available and able to create an immersive session, the application will usually want to add some UI to trigger activation of "XR Presentation Mode", where the application can begin sending imagery to the device. Testing to see if the device supports the capabilities the application needs is done via the `supportsSession` call, which takes a dictionary of the desired functionality and returns a promise which resolves if the device can create a session which supporting those properties and rejects otherwise. Querying for support this way is necessary because it allows the application to detect what XR features are available without actually engaging the sensors or beginning presentation, which can incur significant power or performance overhead on some systems and may have side effects such as launching a status tray or storefront.
**Non-Immersive (in-page)**: The default mode, but can be explicitly requested with the `immersive: false` dictionary argument. Non-immersive content does not have the ability to display on the XR device, but is able to access device tracking information and use it to render content on the page. This technique, where a scene rendered to the page is responsive to device movement, is sometimes referred to as "Magic Window" mode. It's especially useful for mobile devices, where moving the device can be used to look around a scene. Devices like Tango phones and tablets with 6DoF tracking capabilities may expose them via non-immersive sessions even if the hardware is not capable of immersive, stereo presentation.
Copy link
Member

Choose a reason for hiding this comment

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

I know we don't want to change the text to inline yet because it's related to the upcoming AR mode PR.. but wow this paragraph will make a lot more sense at that point. (not asking for a change... just marveling)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup yup yup... sigh

explainer.md Outdated
Ensuring context compatibility with an `XRDevice` through either method may have side effects on other graphics resources in the page, such as causing the entire user agent to switch from rendering using an integrated GPU to a discrete GPU.
Ensuring context compatibility with an XR device through either method may have side effects on other graphics resources in the page, such as causing the entire user agent to switch from rendering using an integrated GPU to a discrete GPU.

If the system's underlying XR device changes (signaled by the `devicechange` event on the `navigator.xr` object) any previously set context compatibility bits will be cleared, and `makeXRCompatible` will need to be called again prior to using the context with a `XRWebGLLayer`.
Copy link
Member

Choose a reason for hiding this comment

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

I know we discussed this, but I can't recall where we landed... are active XRSession objects supposed to end if the devicechange event fires?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, yes, I see that below now. I wonder if it's worth mentioning something about it here too?

explainer.md Outdated
@@ -316,7 +294,7 @@ function onSessionEnd() {
}
```

The UA may end a session at any time for a variety of reasons. For example: The user may forcibly end presentation via a gesture to the UA, other native applications may take exclusive access of the XR hardware device, or the XR hardware device may become disconnected from the system. Well behaved applications should monitor the `end` event on the `XRSession` to detect when that happens.
The UA may end a session at any time for a variety of reasons. For example: The user may forcibly end presentation via a gesture to the UA, other native applications may take exclusive access of the XR hardware device, or the XR hardware device may become disconnected from the system. Additionally, if the system's underlying XR device changes (signaled by the `devicechange` event on the `navigator.xr` object) any active `XRSession`s will be ended. Well behaved applications should monitor the `end` event on the `XRSession` to detect when the UA forces the session to end.
Copy link
Member

Choose a reason for hiding this comment

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

It's probably worth calling out explicitly that this applies to immersive and non-immersive equally.

@@ -992,14 +970,6 @@ partial interface Navigator {

[SecureContext, Exposed=Window] interface XR : EventTarget {
attribute EventHandler ondevicechange;
Copy link
Member

Choose a reason for hiding this comment

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

Am I missing it? Where is the Event type definition?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no reason to have an XREvent, since there's only one navigator.xr object that it could possibly refer to and there's nothing else that I can think of that would be useful to communicate in this event. As a result it should just fire an Event.

This is already communicated in the spec but we've done a poor job of stating what event types should be fired in the explainer. I'll work on clarifying that.

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.

Thanks!

Copy link

@RafaelCintron RafaelCintron left a comment

Choose a reason for hiding this comment

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

LGTM aside from (hopefully) non-controversial nits.

explainer.md Show resolved Hide resolved
explainer.md Show resolved Hide resolved
@toji
Copy link
Member Author

toji commented Oct 5, 2018

Thanks Nell and Rafael!

I'm presuming that anyone who's opinionated on this will have had a chance to look since the PR was announced on the call Tuesday, so I'm inclined to merge this by EOD if there's no further comments.

@toji toji merged commit 75164fc into master Oct 5, 2018
@toji toji deleted the no-xrdevice branch October 5, 2018 23:24
AdaRoseCannon referenced this pull request Oct 17, 2018
Light estimation is one of the key features of AR platforms (ARCore, ARKit,...),
and is essential for creating realistic AR contents.

The LightEstimation of the AR platforms are tightly coupled with the "frame",
because we can estimate the light of the AR scene with the environment
(camera preview ) of the scene.

It consists of the intensity and the color,
and the Application can apply them into the intensity and the color of the ambient light.
Color is optional because some platforms may support only the intensity.
@jsantell jsantell mentioned this pull request Nov 7, 2018
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.

None yet

3 participants