Fixed XR Layer rendering race condition with session initialization and RenderState Update

[zjm-meta]

Description:
The aframe comicbook example was broken with exceptions of

TypeError: Failed to execute 'updateRenderState' on 'XRSession': Failed to read the 'layers' property
from 'XRRenderStateInit': Failed to convert value to 'XRLayer'.

and

Uncaught InvalidStateError: Failed to execute 'getSubImage' on 'XRWebGLBinding': Layer must be in the session's RenderState.

The issues are related to the race condition. The!this.el.sceneEl.xrSession check in this line doesn't guarantee the full initialization of the xr session. We should actually use the this.el.sceneEl.renderer.xr.isPresenting value which is set to true at the end of XR session initialization here.

Another race condition is that the this.xrGLFactory.getSubImage call should happen AFTER the layer is added to the sessions layers array (related W3C doc). This happens in the next frame after sceneEl.renderer.xr.addLayer is called. Thus, we shouldn't force a redraw in onEnterVR. Instead, we need to rely on the this.layer.needsRedraw value here to trigger a redraw after the layer is added to the session layers array.

Changes proposed:

• Skipped all layer tick() function if this.el.sceneEl.renderer.xr.isPresenting === false
• Removed the force redraw logic in the 1st frame in onEnterVR .

Testing
Ran npm start and used port forwarding to test in a Quest 3 headset. The comic book example works smoothly.

[mrxz]

Good catch, wondering if it might be better to ensure that we only set sceneEl.xrSession once the XRSession is fully setup. As the layer component isn't the only code assuming the presence of xrSession means it's ready.

Should be a matter of rearranging the code in a-scene.js. Basically moving it into the then callback of setSession.

[dmarcos]

Thanks so much for this.

Yes, we should probably do this in a-scene as suggested.

vrManager.setSession(xrSession).then(function () {
  vrManager.setFoveation(rendererSystem.foveationLevel);
  self.xrSession = xrSession;
});

@zjm-meta What do you think?

[zjm-meta]

Yeah that makes perfect sense to me. I will update the commit to reflect this and do some tests. Thanks for the suggestion.

[zjm-meta]

I did some tests and I found out that we are generally not handling the undefined sceneEl.xrSession gracefully in the enterVR() functions. For example, in the layers.js we would just skip the initialization steps when xrSession is undefined instead of waiting for the promise. This breaks the experience if we move self.xrSession = xrSession;. Do you think we should change the way we implement enterVR?

[dmarcos]

I did some tests and I found out that we are generally not handling the undefined sceneEl.xrSession gracefully in the enterVR() functions. For example, in the layers.js we would just skip the initialization steps when xrSession is undefined instead of waiting for the promise. This breaks the experience if we move self.xrSession = xrSession;. Do you think we should change the way we implement enterVR?

Thanks. Is the change we suggested in #5489 (comment) not enough?

Feel free to suggest alternatives. We should definitively handle sceneEl.xrSession undefined gracefully.

[mrxz]

The enterVRSuccess(resolve); should also be moved. Otherwise we'd indeed have enterVR events before sceneEl.xrSession is set. Basically enterVR should imply sceneEl.xrSession is available (and ready).

[zjm-meta]

Thanks @mrxz ! Moving the position of enterVRSuccess(resolve); did the job. I have updated this PR to reflect this.

[dmarcos]

@zjm-meta thanks so much for the effort. great work. congrats on your first contribution! 👏