pex-gl / pex-context

Modern WebGL state wrapper for PEX: allocate GPU resources (textures, buffers), setup state pipelines and passes, and combine them into commands.
http://pex-gl.github.io/pex-context/
MIT License
160 stars 12 forks source link

Default DEPTH_COMPONENT to 24bits #127

Closed vorg closed 1 year ago

vorg commented 1 year ago

Given the recent constant problems with depth buffer precision i think it's because WebGL 1 implementation were defaulting to 24 bits on desktop and our new implementation forces 16bits

https://registry.khronos.org/OpenGL/extensions/OES/OES_depth_texture.txt

s per the OpenGL ES spec, there is no guarantee that the OpenGL ES implementation will use the to determine how to store the depth texture internally.
It may choose to downsample the 32-bit depth values to 16-bit or even 24-bit. There is currently no way for the application to know or find out how the depth texture (or any texture) will be stored internally by the OpenGL ES implementation.

https://github.com/pex-gl/pex-context/blob/master/index.js#L262

TextureFormat.DEPTH_COMPONENT = [gl.DEPTH_COMPONENT, DataType.Uint16]

vorg commented 1 year ago

@dmnsgn or we leave it as a penalty for choosing implementation specific precision and move to PixelFormat.DEPTH_COMPONENT24 ASAP (so not even Depth24).

dmnsgn commented 1 year ago

So the proposed change is actually:

https://github.com/pex-gl/pex-context/blob/40747cc8e32de98edab23a28681bc444cc859d3d/index.js#L262

- TextureFormat.DEPTH_COMPONENT = [gl.DEPTH_COMPONENT, DataType.Uint16] 
+ TextureFormat.DEPTH_COMPONENT = [gl.DEPTH_COMPONENT, DataType.Uint32]

or
+ TextureFormat.DEPTH_COMPONENT = [
+  gl.DEPTH_COMPONENT,
+  gl.getExtension('WEBGL_depth_texture').UNSIGNED_INT_24_8_WEBGL
+]

and will only affect WebGL1 as In WebGL2 DEPTH_COMPONENT is not a valid internal format.

From now on, following WebGL2 specs and to be clear, we should only use:

vorg commented 1 year ago

I would say a) because i didn't ask for Stencil so why enable on (even though regl did enable stencil by default from what i remember). As on mobile you might still get 16 bits only (old phones). Then we default to lowest common denominator and try using DEPTH_COMPONENT24 for new projects and pex-renderer.

dmnsgn commented 1 year ago

@dmnsgn or we leave it as a penalty for choosing implementation specific precision and move to PixelFormat.DEPTH_COMPONENT24 ASAP (so not even Depth24).

Yes, we should move to ctx.PixelFormat.DEPTH_COMPONENT24. I marked old DepthX as legacy here to make that clearer: https://github.com/pex-gl/pex-context/blob/40747cc8e32de98edab23a28681bc444cc859d3d/index.js#L275-L278

vorg commented 1 year ago

Switched to ctx.PixelFormat.DEPTH_COMPONENT24 in https://github.com/pex-gl/pex-renderer/commit/b1fe5514a7c579f2318e9266f4f33de3bbd2a396

vorg commented 1 year ago

For history here is broken precision in chrome with camera near:0.01, far: 1000 (see edge of the red sphere where it touches the floor)

Screenshot 2022-10-26 at 03 22 22

that is not broken in safari? as it forces 24bit no matter what? Screenshot 2022-10-26 at 03 22 30