-
Notifications
You must be signed in to change notification settings - Fork 142
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
chore: Swap webvr to webxr polyfill packages #273
base: main
Are you sure you want to change the base?
Conversation
…w Device API is more likely the better thing to do though
WebVR to WebXR
[KJSL] Swap webvr ro webxr polyfill packages
[KJSL] Swap webvr ro webxr polyfill packages
[KJSL] Add WebXR immersive VR support
chore: Swap webvr ro webxr polyfill packages
…olyfill if webxr immersive is not available)
chore: Swap webvr ro webxr polyfill packages (continue to use webvr p…
… operation to play/pause and exit VR/AR session)
chore: Swap webvr for webxr polyfill packages (adding basic controller…
…k on entering 360)
chore: Swap webvr ro webxr polyfill packages
…k on entering 360)
chore: Swap webvr to webxr polyfill packages
Had planned to add thinks like thumb stick controls scrubbing rate maybe and throw a TV not your AR wall - but then those can be a separate issue items This is more a quick fix to get the player playing immersive VR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OMG, thank you so much for this! Been highly requested for sure.
Had a couple of questions.
@@ -1,6 +1,8 @@ | |||
import 'babel-polyfill'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is babel-polyfill needed for here? Since this is a library, we generally prefer not including polyfills, requesting users include it on their page themselves.
Also, should this be using @babel/polyfill
as it's newer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seemed to be a dependency of webxr polyfill - I'll recheck
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it isn't required https://github.com/immersive-web/webxr-polyfill#browser-support
I think we should make a note of it in the README, but otherwise leave it out of the bundle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so I tried pulling it out this results in the WebXR sessions not starting because there is some regenerator code not generated without babel-polyfill
I encourage other to help remove it if its a concern, I'm out of cycles today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
going to "unresolve" this just so that it doesn't get lost, but I don't think it's a blocker.
// Detect WebXR is supported | ||
if (window.navigator.xr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
if (window.navigator.xr) { | ||
this.log('WebXR is supported'); | ||
window.navigator.xr.isSessionSupported('immersive-vr').then((supportsImmersiveVR) => { | ||
if (supportsImmersiveVR) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is false
should we try and fall back to the WebVRPolyfill
? Or does it happen automatically? A bit hard to understand since it's asynchronous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I went through a few permutations of that - after testing it seemed to not really matter
Then again could be getting lucky in timing each time
src/plugin.js
Outdated
} | ||
|
||
// No WebXR support fall back to using WebVR polyfill | ||
if (!hasWebXR) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't this always get set up regardless of whether webXR is available? Since hasWebXR
will always be false
here as it hasn't been set to true
in the promise above.
Co-authored-by: Gary Katsevman <[email protected]>
Co-authored-by: Gary Katsevman <[email protected]>
… out-of-date plugins so embedding base64/png images instead
Description
Fixes #216 (WebXR support)
Specific Changes proposed
For devices that support WebXR use that
Add iOS device orientation permission check on entering 360 (note ESLint workaround as this is iOS Safari specfic)
Fall back to original WebVR polyfill if navigator.xr not available after initialising WebXR polyfill
Requirements Checklist