Fix error when using the reflection component on desktop browsers

[Elettrotecnica]

Dear maintainers,

when opening the excellent boilerplate example available at https://aframe-xr-starterkit.glitch.me/ with a desktop browser such as Chrome, upon entering fullscreen mode, the console will complain about:

The code responsible for this error seem to be relevant only to AR mode and to my understanding should not be executed in other contexts. I have simply ensured that the session is not null to fix the problem.

Additionally, I have also changed that the VR environment update performed in stopLightProbe is executed only if a light probe was set up in the first place (e.g. startLightProbe was executed), which again seems to be supposed to happen only in limited circumstances.

All the best

[vincentfretin]

The real issue is why there is a enter-vr event when you click on fullscreen icon I think. Is this expected?

[Elettrotecnica]

@vincentfretin personally I rely on the current behavior of the event being fired when immersive mode is entered (headset) or fullscreen is activated (desktop). I also find this to make things consistent across the differen platforms. AFAIK this is also a very long standing behavior, so I am not sure how tricky it is to change it...

Just my two cents

[dmarcos]

Have you tried 1.5.0 on the XR starter kit and without this PR? The enter VR flow and UI have changed. And yeah that code shouldn't run. Components should not check for the session unless in immersive mode

[dmarcos]

can do an early return if (!self.el.is('ar-mode')) { return; }

[Elettrotecnica]

I have reworked the commit according to your suggestion 👍

[dmarcos]

we still need to move these?

[Elettrotecnica]

I am not sure: on one hand I do not see why the VR environment needs to be updated again if no light probes have been added, on the other hand recalculating should not have any effect if the light conditions have not changed, e.g. these line should be harmless...

However (full disclosure), in a use case of mine this does make a difference, specifically this happens: the scene starts "bright enough", but if I toggle the fullscreen mode, upon exiting fullscreen this becomes suddenly much darker.

I am unfortunately unable to reproduce this behavior on a public domain asset at the moment. I will remove the moving of these lines for the time being and will open a different PR if I manage to collect further understanding of the issue. So far I believe it has to do to light conditions at start of the scene being different from those at a later time (e.g. after a model has loaded).

[Elettrotecnica]

Have you tried 1.5.0 on the XR starter kit and without this PR? The enter VR flow and UI have changed. And yeah that code shouldn't run. Components should not check for the session unless in immersive mode

I have tried cloning https://github.com/AdaRoseCannon/aframe-xr-boilerplate and using https://cdn.jsdelivr.net/npm/aframe@1.5.0/dist/aframe-master.min.js instead of the original https://cdn.jsdelivr.net/npm/aframe@1.4.2/dist/aframe-master.min.js I can still reproduce this particular issue.

Something else seem to be off unrelated to this problem concerning light and model loading, see screenshot:

[dmarcos]

Something else seem to be off unrelated to this problem concerning light and model loading, see screenshot:

What do you mean by off? Could be related to #5383?

[Elettrotecnica]

Something else seem to be off unrelated to this problem concerning light and model loading, see screenshot:

What do you mean by off? Could be related to #5383?

I don't think so: if you look at the ceiling in the screenshot you will see that textures belonging to the window are rendered. It is also very dark compared to the expected behavior, see screenshot from the original page:

[dmarcos]

Something else seem to be off unrelated to this problem concerning light and model loading, see screenshot:

What do you mean by off? Could be related to #5383?

I don't think so: if you look at the ceiling in the screenshot you will see that textures belonging to the window are rendered. It is also very dark compared to the expected behavior, see screenshot from the original page:

I think it could be related to #5383 I mentioned. Can you try?

<a-scene renderer="colorManagement: false">

[Elettrotecnica]

The a-scene definition in https://github.com/AdaRoseCannon/aframe-xr-boilerplate/blob/glitch/index.html is quite involved and e.g. just the renderer definition looks like:

renderer="alpha:true;physicallyCorrectLights:true;colorManagement:true;exposure:2;toneMapping:ACESFilmic;"

hence, color management is true regardless of the A-Frame version.

I have tried setting colorManagement to false, again on 1.5.0, and the result is the same as in the first "dark" screenshot

[vincentfretin]

In case you're not aware in
https://github.com/AdaRoseCannon/aframe-xr-boilerplate/blob/f1bab15eaabab921ea5a81775e768051cd5ce2fe/main.js#L191-L194
we have

 // Once the building has loaded update the reflections
  building.addEventListener('object3dset', function () {
    if (this.components && this.components.reflection) this.components.reflection.needsVREnvironmentUpdate = true;
  }, {once: true});

to update the reflection map after the building model is loaded.

[Elettrotecnica]

Dear @vincentfretin I was not aware of this, it might explain the behavior I encounter!

Still, I am still not sure if the VR environment should be updated again when extit-vr is emitted and no probe has been set. Do you you have an opinion on this?

[vincentfretin]

If you are in AR and then exit, it make sense to update the reflection, yes, the probe is removed so the lighting changed and the map need to be recalculated.

[Elettrotecnica]

If you are in AR and then exit, it make sense to update the reflection, yes, the probe is removed so the lighting changed and the map need to be recalculated.

Indeed and I agree. But what if we were not in AR mode, hence the probe was not set at all? In this case we are computing the new light for nothing imo.

[vincentfretin]

I'm actually using the reflection component on non AR/VR scene, just on desktop. It takes into account the lights in the aframe scene and calculate an envmap.

[Elettrotecnica]

I'm actually using the reflection component on non AR/VR scene, just on desktop. It takes into account the lights in the aframe scene and calculate an envmap.

Of course, this is done once at init time (or better, at the first valid tick) regardless of mode and works on every platform.

What I mean is that if one does not enter AR mode, no further updates to the envmap are required: only in AR mode we have the case of the component adding another light to the scene.

[vincentfretin]

This is my understanding of how it works.
The envmap is calculated once in tick, calculated a second time if you set again needsVREnvironmentUpdate = true after your model is loaded. In AR, it adds an additional probe light to the scene at a estimated position and intensity. I'm wondering if we shouldn't do a needsVREnvironmentUpdate = true the first time we have a valid light estimation in AR because of this additional light at a position that didn't necessary match the directional light position in the aframe scene, not sure (1). When you exit AR, needsVREnvironmentUpdate is set to true to recreate the envmap because we removed the probe light. The change of intensity of the light probe during the session doesn't matter, the rendered scene is the combination of the calculated envmap and lights.
If we specified a directionalLight property on the reflection component reflection="directionalLight:#dirlight;", the position and intensity of this light is also updated like the probe light. But when we exit AR the directional light position and intensity are not reset to their original position and intensity (2). The points 1 and 2 may be issues, other than that the current code looks good to me.
Also be aware the envmap is calculated from the hardcoded position "0 1.6 0" because of this.cubeCamera.position.set(0, 1.6, 0). If you are inside a wall at that position, that may be an issue. Also this position is probably not what you want on a non-AR big scene where you may want the envmap to be recalculated at your current position if you move far enough for example, but that's another issue.

[Elettrotecnica]

By looking at the code it looks as the directionalLight property comes into play only if this.xrLightProbe is set

aframe/src/components/scene/reflection.js

Lines 126 to 138 in c9b51a2

	if (frame && this.xrLightProbe) {
	// light estimate may not yet be available, it takes a few frames to start working
	var estimate = frame.getLightEstimate(this.xrLightProbe);
	if (estimate) {
	updateLights(
	estimate,
	this.probeLight.components.light.light,
	this.data.directionalLight && this.data.directionalLight.components.light.light,
	this.data.directionalLight && this.data.directionalLight.object3D.position
	);
	}
	}

The only way this.xrLightProbe can be set, is if setupLightProbe is executed

aframe/src/components/scene/reflection.js

Lines 71 to 109 in c9b51a2

	setupLightProbe: function () {
	var renderer = this.el.renderer;
	var xrSession = renderer.xr.getSession();
	var self = this;
	var gl = renderer.getContext();
	if (!this.probeLight) {
	var probeLight = document.createElement('a-light');
	probeLight.setAttribute('type', 'probe');
	probeLight.setAttribute('intensity', 0);
	this.el.appendChild(probeLight);
	this.probeLight = probeLight;
	}
	// Ensure that we have any extensions needed to use the preferred cube map format.
	switch (xrSession.preferredReflectionFormat) {
	case 'srgba8':
	gl.getExtension('EXT_sRGB');
	break;
	case 'rgba16f':
	gl.getExtension('OES_texture_half_float');
	break;
	}
	this.glBinding = new XRWebGLBinding(xrSession, gl);
	gl.getExtension('EXT_sRGB');
	gl.getExtension('OES_texture_half_float');
	xrSession.requestLightProbe()
	.then(function (lightProbe) {
	self.xrLightProbe = lightProbe;
	lightProbe.addEventListener('reflectionchange', self.updateXRCubeMap.bind(self));
	})
	.catch(function (err) {
	console.warn('Lighting estimation not supported: ' + err.message);
	console.warn('Are you missing: webxr="optionalFeatures: light-estimation;" from <a-scene>?');
	});
	},

which can only happen if this.needsLightProbeUpdate is set to true by the startLightProbe method

aframe/src/components/scene/reflection.js

Lines 148 to 153 in c9b51a2

	if (this.needsLightProbeUpdate && frame) {
	// wait until the XR Session has started before trying to make
	// the light probe
	this.setupLightProbe();
	this.needsLightProbeUpdate = false;
	}

aframe/src/components/scene/reflection.js

Lines 68 to 70 in c9b51a2

	startLightProbe: function () {
	this.needsLightProbeUpdate = true;
	},

My argument is simply that, as startLightProbe will never be executed as long as we do not enter AR mode (as it is the case on desktop), there is not need to recalculate the light upon exiting vr mode in this case.

In practice, if we assume the lights on a scene to be stable, it should not make any visual difference that we recalculate once or a 1000 times the envmap whenever we exit fullscreen, so my argument only concerns a possible performance improvement and not a functional change. In my use case it bites me only because the light conditions at startup (before my model is loaded) are different from those later, but I should address it by fixing my lights on the scene rather than relying on the current behavior.

I am not sure if there is something I don't understand here, but in any case the current changes in this PR are sufficient to address the original error and there is no really need for further changes from my side.

Many thanks for the time and effort reviewing this!

[vincentfretin]

Oh I just understood what you were saying. Yeah, calling stopLightProbe in exit-vr and doing the recalculation is not needed if we were in VR and not AR. We should call stopLightProbe only if were were in AR, but we probably need to store that info in a variable in the component to be able to write a condition here. That can be done in a separate PR.

[dmarcos]

Thanks so much!