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

We should distinguish viewerSpace from other XRSpaces #581

Closed
Manishearth opened this issue Apr 5, 2019 · 4 comments
Closed

We should distinguish viewerSpace from other XRSpaces #581

Manishearth opened this issue Apr 5, 2019 · 4 comments
Labels
fixed by pending PR A PR that is in review will resolve this issue.
Milestone

Comments

@Manishearth
Copy link
Contributor

Currently session.viewerSpace produces an XRSpace, however while session.viewerSpace says what the space should do, there's no indication on XRSpace itself that it may have such behavior.

Especially once we start more explicitly speccing what the pose of an given space is (an outcome I hope will come out of the discussion in #565 , one which I'm willing to help write spec text for), we'll kinda need a way to differentiate between viewerSpace XRSpaces and other XRSpaces.

There are two ways to do this:

  • either we define XRViewerSpace as a new thing. This is similar to what Chromium does internally, though it's not a real interface, just a subclass
  • we have a is_viewer_space internal flag on XRSpace, and session.viewerSpace constructs one with that flag set, everything else does not.

I kinda prefer the former since over time that will make it easier to write stuff in the specs for each space.

@cwilso cwilso added this to the May 2019 Working Draft milestone Apr 15, 2019
@NellWaliczek
Copy link
Member

So the interesting thing about this issue is that it highlights a really important distinction. What we define at the API/IDL layer is often a stripped down or simplified model of the implementation concepts. Just because we keep the API simple because it's easier to add API surface area later, doesn't mean we can get away with the spec text not having concrete definitions of the different "subtypes".

In other words, the spec text ought to contain a clearly defined "viewer space" type that should not exist in the idl.

@Manishearth
Copy link
Contributor Author

Sounds good. I wrote this issue before the PR for consolidating most of the reference spaces into a single type happened, so I wanted to match that model, but having an internal type also works. I can make a PR for this once #587 lands

@Manishearth
Copy link
Contributor Author

This will be fixed by https://github.com/immersive-web/editor-collab/pull/38 once #587 lands

Manishearth added a commit to Manishearth/webxr that referenced this issue May 1, 2019
@cwilso cwilso added the fixed by pending PR A PR that is in review will resolve this issue. label May 1, 2019
Manishearth added a commit to Manishearth/webxr that referenced this issue May 1, 2019
@toji toji modified the milestones: April 2019, May 2019 May 2, 2019
@toji
Copy link
Member

toji commented May 7, 2019

Fixed by #621

@toji toji closed this as completed May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed by pending PR A PR that is in review will resolve this issue.
Projects
None yet
Development

No branches or pull requests

4 participants