immersive-web / webxr

Repository for the WebXR Device API Specification.
https://immersive-web.github.io/webxr/
Other
2.98k stars 382 forks source link

Combine XRStationaryReferenceSpaceSubtype into XRReferenceSpaceType #514

Closed ddorwin closed 5 years ago

ddorwin commented 5 years ago

https://github.com/immersive-web/webxr/pull/502 merged subtype into XRReferenceSpaceOptions, and have type and (optional) subtype are now in the same dictionary.

In addition to having a member that only applies when the type is set to one value, it's not clear what value there is in having subtypes for "stationary" rather than just making three types for stationary. It seems that it would be simpler for developers to only need to think about choosing the type(s) they support. This would also simplify the spec a little bit as described below.

XRStationaryReferenceSpace exists only to provide the subtype member. We probably don't need this (see https://github.com/immersive-web/webxr/issues/513). Even if we did, it would probably make sense to provide the type, which would now include the existing subtypes, in the base XRReferenceSpace.

I don't think there is a need to have a 1:1 mapping of type to XRReferenceSpace sub-interfaces. Thus, we can probably remove XRStationaryReferenceSpace and XRUnboundedReferenceSpace, further simplifying the API and spec. Sub-interfaces can always be added later if necessary. The spec and applications will need to be updated to take advantage of new attributes anyway.

toji commented 5 years ago

I know that when you reached out to me internally about this I let the conversation drop prior to fully resolving this, so sorry for that.

First off, I agree that in the spirit of #513 (and a desire to do a general clean-up pass over the whole API) we could probably drop the subtype member and then reduce the reference space types other than XRBoundedReferenceSpace to simply return an opaque XRReferenceSpace type. It shouldn't be too disruptive later to re-introduce those types either. The only thing it should affect in real applications is people doing typeof tests and that wouldn't be a meaningful way to differentiate the types anyway, so the risk of breaking content is low.

As for the type/subtype division at creation time, I'm not sure I see as much benefit in bikeshedding that particular decision? I think we want to keep the arguments as a dictionary for future expansion, so we're not reducing the number of types there. Merging names like "stationary-floor-level", while verbose, could certainly work. But I don't think it's any different in terms of cognitive load for developers, so I'd lean towards not changing the already established API shape in favor of concentrating on other known issues.

(Actually, I'll make an argument for going the other way and saying that it might be kind of nice if we determined a default subtype for "stationary" reference spaces. Probably 'eye-level'? At that point the division between type and subtype is more justified from an ergonomics standpoint.)

NellWaliczek commented 5 years ago

(Actually, I'll make an argument for going the other way and saying that it might be kind of nice if we determined a default subtype for "stationary" reference spaces. Probably 'eye-level'? At that point the division between type and subtype is more justified from an ergonomics standpoint.)

I had intentionally not set a default because I was hoping it would cause developers to think for a moment about their intentions, but practically speaking it probably just makes them pick something random to get past it. Given that, I'm open to picking a default.

As for the main point of this issue.... giant sigh It's certainly unfortunate to be revisiting these sorts of design details at this point in time. However, I do think that David's got a reasonable point. The current API shape doesn't feel entirely Javascript-like (hardly surprising given how many years I spent writing almost exclusively C++). I'd be ok with removing the XRStationaryReferenceSpace and XRUnboundedReferenceSpace types for now so long as the behavior isn't changing. (btw, I think @thetuvix will get a kick out of how much that ends up accidentally looking like the WMR APIs even though it's not really the same). I agree with the point that the two types can be easily added later and that I'm not super worried about devs doing typeof checks since they called the apis to create the reference spaces to begin with. It does beg the question... should the XRBoundedReferenceSpace also be collapsed into the base and its properties made nullable? That feels... a bit more icky to me... but I need to think on it a bit more...

As for combining type and subtype... that one makes be a bit nervous because it potentially limits our ability to extend this api over time. I need to think on it a bit more...

NellWaliczek commented 5 years ago

Aaaah! Blair's comment on another issue just made me realize this isn't going to work as is. The requestReferenceSpace()function accepts an array of reference space descriptions and returns the first one that is allowed. We need to decide to either keep the "type" property or keep the types themselves.

NellWaliczek commented 5 years ago

Though I also see that it is incorrect in the spec :(

thetuvix commented 5 years ago

I'm fine with having less interfaces defined and relying on attributes to work out these details, especially to fit in better with other web APIs.

@NellWaliczek:

I had intentionally not set a default because I was hoping it would cause developers to think for a moment about their intentions, but practically speaking it probably just makes them pick something random to get past it.

Do we have evidence that developers will pick randomly between "eye-level" and "floor-level"? Certainly a developer will have a very different experience laying out their scene if Y=0 is at the user's head vs. at the floor. The goal was indeed for developers to pick the one that matches how they're already expecting to reason about the world.

We could try to just run with one as a default, since developers will quickly realize when they find themselves in "eye-level" but they'd wanted Y=0 on the floor. However, I'd hate to have developers not notice the extra eye vs. floor attribute and think they have to jump all the way to "bounded" to get Y=0 onto the floor, since that will exclude UAs on devices that allow for standing but not walking around. If we're making the dictionary optional, I'd rather just flatten this fundamental app choice into the base enum.

Consider that, today, the spatial tracking explainer refers to these 6 types/subtypes as peer rows within the tables at the bottom, with each representing a unique class of app that is being built. We should ensure that any alternate design does not depress usage of the stationary/floor-level row, which is just as meaningful as the others and offers greater device compat that bounded.

Reference Space Examples

Type Subtype Examples
identity - In-page content preview
- Click/Drag viewing
stationary position-disabled - 360 photo/video viewer
stationary eye-level - Immersive 2D video viewer
- Racing simulator
- Solar system explorer
stationary floor-level - VR chat "room"
- Action game where you duck and dodge in place
- Fallback for Bounded experience that relies on teleportation instead
bounded - VR painting/sculpting tool
- Training simulators
- Dance games
- Previewing of 3D objects in the real world
unbounded - Campus tour
- Renovation preview

XRReferenceSpace Availability

Guaranteed The UA will always be able to provide this reference space

Hardware-dependent The UA will only be able to supply this reference space if running on XR hardware that supports it

Rejected The UA will never provide this reference space

Type Subtype Inline Immersive
identity Guaranteed Guaranteed
stationary position-disabled Hardware-dependent Guaranteed
stationary eye-level Hardware-dependent Guaranteed
stationary floor-level Hardware-dependent Guaranteed
bounded Rejected Hardware-dependent
unbounded Rejected Hardware-dependent
toji commented 5 years ago

Fixed by #587.