Skip to content

Remove XRSessionCreationOptions #566

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

Merged
merged 1 commit into from
Apr 16, 2019
Merged

Remove XRSessionCreationOptions #566

merged 1 commit into from
Apr 16, 2019

Conversation

toji
Copy link
Member

@toji toji commented Mar 21, 2019

As described in #542, this switches requestSession to a "primary type
plus options bag" pattern. However, since the dictionary only had a single
key after outputContext was moved this change ends up removing the
XRSessionCreationOptions dictionary entirely in favor of just passing in
the type enum directly.

In the future if additional options are needed for requestSession,
the expected pattern would be:

navigator.xr.requestSession('immersive-vr', { option: value });

As described in #542, this switches `requestSession` to a "primary type
+ options bag" pattern. However, since the dictionary only had a single
key after outputContext was moved this change ends up removing the
XRSessionCreationOptions dictionary entirely in favor of just passing in
the type enum directly.

In the future if additional options *are* needed for `requestSession`,
the expected pattern would be:

```
navigator.xr.requestSession('immersive-vr', { option: value });
```
@toji toji added the agenda Request discussion in the next telecon/FTF label Mar 21, 2019
@blairmacintyre
Copy link
Contributor

I would like to suggest that we consider moving forward with https://github.com/immersive-web/geo-alignment and use this as an example of possible options.

@NellWaliczek NellWaliczek added this to the Next Working Draft milestone Mar 25, 2019
@NellWaliczek NellWaliczek added the potential breaking change Issues that may affect the core design structure. label Mar 25, 2019
@toji toji removed agenda Request discussion in the next telecon/FTF labels Mar 29, 2019
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.

I'm on board with the direction of this change. One comment regarding not letting some XRReferenceSpace functionality slip through the cracks. Once we have closure on that, I'll sign off.


```js
function beginImmersiveSession() {
xrDevice.requestSession({ immersive: true, requiredReferenceSpaceType:'unbounded' })
Copy link
Member

Choose a reason for hiding this comment

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

So this sample code made it into the explainer, but never made its way into the spec because there was confusion about the overall permissions/features model. While we are working through the final shape of that design (oh, John..... pokepokepoke) can we at least file an issue to cover adding this functionality back in once that's complete?

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we can leave the options dictionary and put this in it ;)

@blairmacintyre
Copy link
Contributor

I like the idea of having the 1st parameter (session type) be explicit, but I'd still like to propose that we keep at least one "option" (I suggested geoaligned for requesting that the base coordinate system be geoaligned) and keep the optional XRSessionCreationOptions around. It seems to me that we will want this eventually, so why not keep it?

@NellWaliczek
Copy link
Member

@toji, please correct me if I'm misinterpreting... but you're not saying you think we don't need it. Just simply that we'll leave it off until a specific feature is actually merged into the spec that depends on it.

@blairmacintyre
Copy link
Contributor

Part of my motivation is that I suspect (we’ll, know) it will end up getting used, as people experiment with “next version” or “proposed” features.

I expect I’ll put a bunch of things behind prefs (rather than just expose them) in the WebXR Viewer after moving it over to the “real” spec, for example. So I’ll being using this immediately. This, I’d prefer to have at least the name and format of the object defined if we’ll be adding it.

@NellWaliczek
Copy link
Member

@blairmacintyre The format of the object will almost certainly be a dictionary since that's the general pattern on the web and as such seems like a safe assumption. (And as far as I'm aware, the idl type name shouldn't matter). That said, I do understand your concern. And my suspicion is that we're going to have something that proves the pattern before we hit CR, but I'm hesitant to put it in now so that we don't create a breaking change if we're wrong. Hence, my suggestion to file an issue against CR to enable the extensibility without holding up this PR. WDYT?

@blairmacintyre
Copy link
Contributor

Sounds reasonable to me, I'm not sure what to put in an issue.

I've been pushing for geo-orientation, for example, for a while; we have had it sitting in a repo (github.com/immersive-web/geo-orientation) for 7 months with no movement. The current proposal would need a little bit of work, may as well wait till after CR so we have a defined API to work against.

@toji
Copy link
Member Author

toji commented Apr 16, 2019

Filed #591. Nell confirmed offline that she's comfortable merging this once that issue was filed, so doing so now.

@toji toji merged commit 8f38f04 into master Apr 16, 2019
@toji toji deleted the no-request-options branch April 16, 2019 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
potential breaking change Issues that may affect the core design structure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants