Skip to content

Conversation

@mrxz
Copy link
Contributor

@mrxz mrxz commented Sep 7, 2022

Description:
These changes make foveationLevel property have an effect on WebXR by setting the foveation property through Three.js. In contrast to the foveationLevel of WebVR, this can be changed on the fly.

One thing I noticed is that the async WebXRManager.setSession is not awaited. Setting the foveation becomes a no-op unless the base layer is initialized, which happens during setSession. I now simply chain the setting of the foveation to it, but it feels like the promise returned by enterVR should not resolve before setSession is done in the first place. But I might be missing something and didn't want to change more than is needed.

Changes proposed:

  • Let foveationLevel set the foveation of WebXR (and no longer for WebVR)
  • Changes default value for foveationLevel to 1 (which it effectively already was in case of WebXR)

@dmarcos
Copy link
Member

dmarcos commented Sep 8, 2022

Thank you! This is awesome. Congrats on your first contribution 👏

@dmarcos dmarcos merged commit 34665ed into aframevr:master Sep 8, 2022
vrManager.layersEnabled = xrInit.requiredFeatures.indexOf('layers') !== -1;
vrManager.setSession(xrSession);
vrManager.setSession(xrSession).then(function () {
vrManager.setFoveation(rendererSystem.foveationLevel);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This broke Firefox, we need to check if vrManager.setFoveation is defined before calling it. On Firefox vrManager is WebVRManager that we still maintain in the super-three fork, not WebXRManager.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding a condition in aframe code, we could maybe add a dummy setFoveation function in WebVRManager to fix that @dmarcos?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's this invocation that causes issues, since it's inside the this.hasWebXR clause. It's probably the one added in renderer.js#update, as that one is called unconditionally. Good catch, in Firefox I always have dom.vr.webxr.enabled set to true, so it didn't fail for me during testing.

In any case, I also think a (dummy) setFoveation in WebVRManager is the cleanest solution. The less difference there is between the WebVRManager and WebXRManager in terms of interface, the less likely bugs like these are.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. We need to do some other fixes in WebVRManager, see #5102 (comment) so I guess it's one more. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants