-
Notifications
You must be signed in to change notification settings - Fork 147
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.
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.
FWIW, when I test this on oculus browser 36 / chromium 130, this works well. I would remove the polyfill here and leave it up to consumers if they want to support older browsers?
// 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
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.
I've started looking at this and noticed this PR has been open for a little while, but has seen some recent activity. Is the MetaCDN team still planning on taking this forward? I've tried this out for a little bit and does seem to behave pretty well.
* rollback THREE.js as this introduced a noticable gamma/brightness increase, adding gyro support, moved | ||
* immersive controller images into base64 |
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.
FWIW, testing locally, an upgrade to 170 seems OK when setting the color spaces manually. This was an explicit change in r152: https://discourse.threejs.org/t/updates-to-color-management-in-three-js-r152/50791
I think these should be all:
// Line 742
this.videoTexture.colorSpace = THREE.SRGBColorSpace;
// Line 769
this.renderer.outputColorSpace = THREE.SRGBColorSpace;
That doesn't have to happen as part of this PR though.
MathUtils, | ||
Quaternion, | ||
Vector3 | ||
} from '../../node_modules/three/build/three.module.js'; |
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.
I think you can just import from 'three'
?
I'm looking at making threejs a peer dependency and that should help a good deal with three shaking?
window.navigator.xr.isSessionSupported('immersive-vr').then((supportsImmersiveVR) => { | ||
if (supportsImmersiveVR) { | ||
// We support WebXR show the enter VRButton | ||
this.vrButton = VRButton.createButton(this.renderer); |
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.
thought: createButton
takes in an optional sessionInit
object that's spread into the requestSession parameters so that users can request additional features. That may be a neat thing to add in. Doesn't have to happen here though.
import {XRControllerModelFactory} from '../node_modules/three/examples/jsm/webxr/XRControllerModelFactory'; | ||
import {BoxLineGeometry} from '../node_modules/three/examples/jsm/geometries/BoxLineGeometry'; |
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.
These can be imported from three directly I think?
import {XRControllerModelFactory} from '../node_modules/three/examples/jsm/webxr/XRControllerModelFactory'; | |
import {BoxLineGeometry} from '../node_modules/three/examples/jsm/geometries/BoxLineGeometry'; | |
import {XRControllerModelFactory} from 'three/examples/jsm/webxr/XRControllerModelFactory'; | |
import {BoxLineGeometry} from 'three/examples/jsm/geometries/BoxLineGeometry'; |
@@ -761,15 +899,350 @@ void main() { | |||
window.addEventListener('resize', this.handleResize_, true); | |||
window.addEventListener('vrdisplayactivate', this.handleVrDisplayActivate_, true); | |||
window.addEventListener('vrdisplaydeactivate', this.handleVrDisplayDeactivate_, true); | |||
window.addEventListener('pointerdown', this.handleUserActive_, true); | |||
|
|||
// For iOS we need permission for the device orientation data, this will pop up an 'Allow' |
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.
Safari/iOS is only allowing DeviceMotionEvent.requestPermission
during a user event and not on page load.
That said, onDeviceOrientationChange
doesn't seem to do anything with the event data, is it needed?
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