search

immersive-web / webxr-polyfill

Use the WebXR Device API today, providing fallbacks to native WebVR 1.1 and Cardboard
Apache License 2.0
386 stars 86 forks source link

Add XRRay and update XRInputPose and XRInputSource attribute names #19

Closed jsantell closed 6 years ago

jsantell commented 6 years ago

Still need to actually test

jsantell commented 6 years ago

Fixed #14 and #13

toji commented 6 years ago

The change looks good in all technical respects, but I think we want to approach this a little more carefully. For one, XRRay hasn't actually been merged into the explainer yet, so I'd like to hold off on that portion of the change until we get group consensus. (Hopefully that'll come this week?)

Secondly, the input changes are necessary but if we land these changes now then the current samples will need to be updated and in doing so would break with Chrome 67/68's implementation. Those are known to be outdated at this point, so I'm happy to move past it, but ideally we'd want to provide some temporary shim layer that maps from the new behavior to the old while we still have those versions in the wild.

There's two logical places for that, and I'd love your opinion on it: We can bake that temporary shim layer into the polyfill so that anyone who uses the polyfill automatically gets the fixup, which seems good unless there's someone who wants to avoid the overhead of including the full polyfill? Alternatively, we could keep the polyfill focused on the latest spec state and I could implement a separate shim library that would probably live in the samples repo. Thoughts?

toji commented 6 years ago

FYI: The XRRay concept is now officially part of the explainer/spec, and the samples have been updated to use it. I'm in the process of rebasing this and making a few related minor updates, and then hopefully we can merge it.

jsantell commented 6 years ago

@toji sounds great! let me know when is good to orchestrate a new build/release with these changes.

toji commented 6 years ago

Okay, this has now been updated to reflect the current state of the spec and should be ready to merge whenever you get a chance to review.

One thing to note is the addition of a partial DOMPointReadOnly polyfill, which feels slightly out of place here and which I'm not exposing publicly for that reason. This is only there to ensure the polyfill continues to run on Edge and Safari, so I'm not inclined to make it into a full-fledged Geometry Interfaces polyfill.

jsantell commented 6 years ago

Looks good @toji! Just would like to confirm the default z value of -1 in an XRRay.prototype.direction, I can't find any docs to support that, but not sure if that's implicit from the WebGL coordinate system.

toji commented 6 years ago

There's nothing in the spec to say that should be a default, but given that it's required to be a normalized directional vector it should be something with a length of 1. Having a vector that points directly into the screen (OpenGL convention being -Z) is probably the most logical choice in that case. Likely not a big issue either way since we really should never report an XRInputPose with a default targetRay.