Objects in custom layer flicker when using atmosphere sky layer

[nagix]

When I add a custom layer to show 3D models using three.js and a sky layer with 'sky-type': 'atmosphere' to the map, 3D objects flicker when calling setPaintProperty('sky', 'sky-atmosphere-sun', ...). This only happens when the horizon is invisible.

See my CodePen below to reproduce the problem.

mapbox-gl-js version: v2.1.1
three.js version: any
browser: Chrome, Safari, Firefox and Edge

Link to Demonstration

https://codepen.io/nagixx/pen/yLVJEqr

Expected Behavior

3D models don't flicker.

[rreusser]

I'm still working on exactly why, but adding gl.cullFace(gl.BACK) into the render loop removes the flickering.

[rreusser]

The fundamental problem is that OpenGL/WebGL is extremely stateful, and each library manages state in its own way, so that getting different WebGL libraries to play well together in the same context is a somewhat common problem.

I'm not certain what renderer.state.reset() did in r117, but r124 – specifically mrdoob/three.js#20859 – introduces resetState() for just this purpose. It's a bit heavy-handed as it flushes Three.js's state cache and causes it to reset each piece of state, ensuring that none of Mapbox's state pollutes Three.js's assumptions about that state.

(Additionally, mrdoob/three.js#21281 and mrdoob/three.js#21295 squashed a few more unaccounted for pieces of state, so that it looks like r126 is the first version of Three.js for which resetState fixes your problem.)

A fixed example: https://codepen.io/rsreusser/pen/ExNooXE

Thus, there are two solutions to this problem, I think:

• Apply one-off state changes (e.g. the gl.cullFace fix above) which fixes specific problems. This is lower cost and may work, but it may fail to prevent other faulty assumptions about state.
• Call renderer.resetState(), which is slightly higher cost but hopefully ensures correct state.

(different layer orders might be able to work around the problem, but that might be unnecessarily fragile.)

[astojilj]

@rreusser Nice find.

The place we set front face:

mapbox-gl-js/src/render/draw_sky.js

Line 115 in 63f39e5

program.draw(context, gl.TRIANGLES, DepthMode.disabled, StencilMode.disabled, ColorMode.unblended, CullFaceMode.frontCW,

Code resetting state before custom layer rendering

mapbox-gl-js/src/render/painter.js

Line 758 in 63f39e5

this.context.cullFace.setDefault();

seems to be resulting with gl.disable(gl.CULL_FACE).

mapbox-gl-js/src/gl/value.js

Line 292 in 63f39e5

gl.disable(gl.CULL_FACE);

and your analysis shows that there it is needed to call setDefault also for Context.cullFaceSide and Context.frontFace (in addition to disabling cull face).

mapbox-gl-js/src/gl/context.js

Lines 247 to 255 in 63f39e5

	setCullFace(cullFaceMode: $ReadOnly<CullFaceMode>) {
	if (cullFaceMode.enable === false) {
	this.cullFace.set(false);
	} else {
	this.cullFace.set(true);
	this.cullFaceSide.set(cullFaceMode.mode);
	this.frontFace.set(cullFaceMode.frontFace);
	}
	}

Didn't run the code to verify it.

[rreusser]

Thanks for the additional sleuthing @astojilj! It sounds like it would benefit from a small change on the Mapbox side, though does that still leave room for problems? Here is what Three.js is doing to manage culling:

https://github.com/mrdoob/three.js/blob/8514f004072d5e33a964f347d17c3d33cb3cd5a7/src/renderers/webgl/WebGLState.js#L669-L701

My concern is that setting a more common default may reduce the likelihood of problems in common scenarios, but that if you rendered a custom layer with front face culling twice in Three.js, setting this default before a custom layer would confuse Three.js as it would still expect front face culling to be active on the second call. (unless resetState() is called in Three.js)

It seems like adding setDefault for culling before custom layers is probably appropriate so that custom layers are predictable, but that three.resetState() might be the only way to really ensure that no incorrect assumptions are made about state.

[astojilj]

It sounds like it would benefit from a small change on the Mapbox side, though does that still leave room for problems?

The idea is to reset GL state to default before passing control over to custom layer, regardless the engine used by custom layer. Note that Three.js is just one of the approaches that could be used to render custom layer: similar code we have in gl-native that can use e.g. plain OpenGL, Metal, SceneKit to render custom layer. We cannot make any assumptions on state of custom layer code but to set GL state to default: gl.CWW is default for gl.frontFace, gl.BACK is default for gl.cullFace.

[rreusser]

Got it, thanks. That makes sense. I submitted #10412 which addresses this. I did some equivocating in that ticket since if external libraries maintain their own concept of state, they may or may not expect things to be in a default state, but if the guiding principle here is that Mapbox ought to hand off in a default state, then that seems very clear and still beneficial for consistency.