From c5fba2b66da54ec99b549e44917f9dedcd0f2dc8 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 17 May 2023 18:55:43 +0200 Subject: [PATCH 1/4] Revert "Fix depth precision issues on WebGL due to different NDC space (#2123)" This reverts commit 4f60fd73e80f1a8475ff00985f41d0fa5b6b7223. --- crates/re_renderer/src/view_builder.rs | 59 ++++++++------------------ 1 file changed, 17 insertions(+), 42 deletions(-) diff --git a/crates/re_renderer/src/view_builder.rs b/crates/re_renderer/src/view_builder.rs index 1cc0a5b9e0d2..7b84d8c0ef32 100644 --- a/crates/re_renderer/src/view_builder.rs +++ b/crates/re_renderer/src/view_builder.rs @@ -4,7 +4,6 @@ use std::sync::Arc; use crate::{ allocator::{create_and_fill_uniform_buffer, GpuReadbackIdentifier}, - config::HardwareTier, context::RenderContext, draw_phases::{ DrawPhase, OutlineConfig, OutlineMaskProcessor, PickingLayerError, PickingLayerProcessor, @@ -331,30 +330,11 @@ impl ViewBuilder { // We use infinite reverse-z projection matrix // * great precision both with floating point and integer: https://developer.nvidia.com/content/depth-precision-visualized // * no need to worry about far plane - // - // There's a pretty big catch though: - // When we're on GLES, the NDC coordinates go from -1 to 1 and are then mapped to 0 to 1 as part of the viewport transformation. - // This means, that if we don't care about this, we'll not only throw away "half" of the space, - // we also have the most precise region around 0 in the wrong spot initially 😱 - // Therefore, we pretty much *have* to use a different projection in this case. - // Modern OpenGL has a way to change this to the more common 0 to 1 z-range for NDC - // which is the standard in Metal/Vulkan/DirectX/WebGPU, but WebGL is lacking this. - // see https://registry.khronos.org/OpenGL-Refpages/gl4/html/glClipControl.xhtml - let projection_from_view = - if ctx.shared_renderer_data.config.hardware_tier == HardwareTier::Gles { - glam::Mat4::perspective_rh_gl( - vertical_fov, - aspect_ratio, - near_plane_distance, - 100000.0, - ) - } else { - glam::Mat4::perspective_infinite_reverse_rh( - vertical_fov, - aspect_ratio, - near_plane_distance, - ) - }; + let projection_from_view = glam::Mat4::perspective_infinite_reverse_rh( + vertical_fov, + aspect_ratio, + near_plane_distance, + ); // Calculate ratio between screen size and screen distance. // Great for getting directions from normalized device coordinates. @@ -391,8 +371,8 @@ impl ViewBuilder { config.resolution_in_pixel[0] as f32 / config.resolution_in_pixel[1] as f32; let horizontal_world_size = vertical_world_size * aspect_ratio; // Note that we inverse z (by swapping near and far plane) to be consistent with our perspective projection. - let (left, right, bottom, top, near, far) = match camera_mode { - OrthographicCameraMode::NearPlaneCenter => ( + let projection_from_view = match camera_mode { + OrthographicCameraMode::NearPlaneCenter => glam::Mat4::orthographic_rh( -0.5 * horizontal_world_size, 0.5 * horizontal_world_size, -0.5 * vertical_world_size, @@ -400,22 +380,17 @@ impl ViewBuilder { far_plane_distance, 0.0, ), - OrthographicCameraMode::TopLeftCornerAndExtendZ => ( - 0.0, - horizontal_world_size, - vertical_world_size, - 0.0, - far_plane_distance, - -far_plane_distance, - ), + OrthographicCameraMode::TopLeftCornerAndExtendZ => { + glam::Mat4::orthographic_rh( + 0.0, + horizontal_world_size, + vertical_world_size, + 0.0, + far_plane_distance, + -far_plane_distance, + ) + } }; - let projection_from_view = - if ctx.shared_renderer_data.config.hardware_tier == HardwareTier::Gles { - // See comment on perspective projection for why we need to care about Gles vs non-Gles. - glam::Mat4::orthographic_rh_gl(left, right, bottom, top, near, far) - } else { - glam::Mat4::orthographic_rh(left, right, bottom, top, near, far) - }; let tan_half_fov = glam::vec2(f32::MAX, f32::MAX); let pixel_world_size_from_camera_distance = vertical_world_size From 017796ec3cae9251635ead7d67d5e369b127e6aa Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 17 May 2023 19:01:47 +0200 Subject: [PATCH 2/4] fudge depth offsets on Webgl --- crates/re_renderer/shader/device_info.wgsl | 3 +++ crates/re_renderer/shader/utils/depth_offset.wgsl | 13 ++++++++++++- crates/re_renderer/src/context.rs | 5 +++++ crates/re_renderer/src/workspace_shaders.rs | 6 ++++++ 4 files changed, 26 insertions(+), 1 deletion(-) create mode 100644 crates/re_renderer/shader/device_info.wgsl diff --git a/crates/re_renderer/shader/device_info.wgsl b/crates/re_renderer/shader/device_info.wgsl new file mode 100644 index 000000000000..c5948f4b8101 --- /dev/null +++ b/crates/re_renderer/shader/device_info.wgsl @@ -0,0 +1,3 @@ +// True if we're running on Gles/WebGL. +// (patched accordingly if necessary) +const GLES = false; diff --git a/crates/re_renderer/shader/utils/depth_offset.wgsl b/crates/re_renderer/shader/utils/depth_offset.wgsl index 815c36eef023..66bd5f819ad3 100644 --- a/crates/re_renderer/shader/utils/depth_offset.wgsl +++ b/crates/re_renderer/shader/utils/depth_offset.wgsl @@ -1,5 +1,6 @@ #import <../global_bindings.wgsl> #import <../types.wgsl> +#import <../device_info.wgsl> fn apply_depth_offset(position: Vec4, offset: f32) -> Vec4 { // We're using inverse z, i.e. 0.0 is far, 1.0 is near. @@ -16,7 +17,17 @@ fn apply_depth_offset(position: Vec4, offset: f32) -> Vec4 { // 1.0 * eps _should_ be enough, but in practice it causes Z-fighting for unknown reasons. // Maybe because of GPU interpolation of vertex coordinates? - let eps = 5.0 * f32eps; + var eps = 5.0 * f32eps; + + if GLES { + // On GLES/WebGL, the NDC clipspace range for depth is from -1 to 1 and y is flipped. + // wgpu/Naga counteracts this by patching all vertex shader with: + // "gl_Position.yz = vec2(-gl_Position.y, gl_Position.z * 2.0 - gl_Position.w);", + // This is great, since it means that we can pretend depth is 0 to 1 as specified by WebGPU. + // But it completely messes up depth precision, in particular since we use + // an inverse depth projection that tries to make use of the high float precision closer to zero. + eps *= 1000.0; + } return Vec4( position.xy, diff --git a/crates/re_renderer/src/context.rs b/crates/re_renderer/src/context.rs index a2c8d032b819..1f57bf4afadd 100644 --- a/crates/re_renderer/src/context.rs +++ b/crates/re_renderer/src/context.rs @@ -201,6 +201,11 @@ impl RenderContext { "@invariant @builtin(position)".to_owned(), "@builtin(position)".to_owned(), )); + } else if adapter.get_info().backend == wgpu::Backend::Gl { + gpu_resources + .shader_modules + .shader_text_workaround_replacements + .push(("GLES = false".to_owned(), "GLES = true".to_owned())); } RenderContext { diff --git a/crates/re_renderer/src/workspace_shaders.rs b/crates/re_renderer/src/workspace_shaders.rs index 7d999da73645..50abc69da36c 100644 --- a/crates/re_renderer/src/workspace_shaders.rs +++ b/crates/re_renderer/src/workspace_shaders.rs @@ -43,6 +43,12 @@ pub fn init() { fs.create_file(virtpath, content).unwrap(); } + { + let virtpath = Path::new("shader/device_info.wgsl"); + let content = include_str!("../shader/device_info.wgsl").into(); + fs.create_file(virtpath, content).unwrap(); + } + { let virtpath = Path::new("shader/generic_skybox.wgsl"); let content = include_str!("../shader/generic_skybox.wgsl").into(); From 7fdca02fc96f0462e01e479a99d864297b3ecd92 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Mon, 22 May 2023 19:33:03 +0200 Subject: [PATCH 3/4] hardware_tier global constant, better depth handling for gles --- crates/re_renderer/shader/device_info.wgsl | 3 -- .../re_renderer/shader/global_bindings.wgsl | 7 ++++ .../shader/utils/depth_offset.wgsl | 38 ++++++++----------- crates/re_renderer/src/config.rs | 6 ++- crates/re_renderer/src/context.rs | 5 --- crates/re_renderer/src/global_bindings.rs | 4 +- crates/re_renderer/src/view_builder.rs | 2 +- crates/re_renderer/src/workspace_shaders.rs | 6 --- 8 files changed, 29 insertions(+), 42 deletions(-) delete mode 100644 crates/re_renderer/shader/device_info.wgsl diff --git a/crates/re_renderer/shader/device_info.wgsl b/crates/re_renderer/shader/device_info.wgsl deleted file mode 100644 index c5948f4b8101..000000000000 --- a/crates/re_renderer/shader/device_info.wgsl +++ /dev/null @@ -1,3 +0,0 @@ -// True if we're running on Gles/WebGL. -// (patched accordingly if necessary) -const GLES = false; diff --git a/crates/re_renderer/shader/global_bindings.wgsl b/crates/re_renderer/shader/global_bindings.wgsl index f50f4923e698..0568f2b4c49d 100644 --- a/crates/re_renderer/shader/global_bindings.wgsl +++ b/crates/re_renderer/shader/global_bindings.wgsl @@ -27,6 +27,9 @@ struct FrameUniformBuffer { // Size used for all line radii given with Size::AUTO. auto_size_lines: f32, + + /// re_renderer defined hardware tier. + hardware_tier: u32, }; @group(0) @binding(0) var frame: FrameUniformBuffer; @@ -36,3 +39,7 @@ var nearest_sampler: sampler; @group(0) @binding(2) var trilinear_sampler: sampler; + +// See config.rs#HardwareTier +const HARDWARE_TIER_GLES = 0u; +const HARDWARE_TIER_WEBGPU = 1u; diff --git a/crates/re_renderer/shader/utils/depth_offset.wgsl b/crates/re_renderer/shader/utils/depth_offset.wgsl index 66bd5f819ad3..fbd23ac66e12 100644 --- a/crates/re_renderer/shader/utils/depth_offset.wgsl +++ b/crates/re_renderer/shader/utils/depth_offset.wgsl @@ -1,6 +1,5 @@ #import <../global_bindings.wgsl> #import <../types.wgsl> -#import <../device_info.wgsl> fn apply_depth_offset(position: Vec4, offset: f32) -> Vec4 { // We're using inverse z, i.e. 0.0 is far, 1.0 is near. @@ -11,35 +10,28 @@ fn apply_depth_offset(position: Vec4, offset: f32) -> Vec4 { // Since we're actually supposed to have an *infinite* far plane this should never happen! // Therefore we simply dictacte a minimum z value. // This ofc wrecks the depth offset and may cause z fighting with all very far away objects, but it's better than having things disappear! - - if true { - // This path assumes a `f32` depth buffer! - - // 1.0 * eps _should_ be enough, but in practice it causes Z-fighting for unknown reasons. - // Maybe because of GPU interpolation of vertex coordinates? - var eps = 5.0 * f32eps; - - if GLES { - // On GLES/WebGL, the NDC clipspace range for depth is from -1 to 1 and y is flipped. - // wgpu/Naga counteracts this by patching all vertex shader with: - // "gl_Position.yz = vec2(-gl_Position.y, gl_Position.z * 2.0 - gl_Position.w);", - // This is great, since it means that we can pretend depth is 0 to 1 as specified by WebGPU. - // But it completely messes up depth precision, in particular since we use - // an inverse depth projection that tries to make use of the high float precision closer to zero. - eps *= 1000.0; - } - + if frame.hardware_tier == HARDWARE_TIER_GLES { + // On GLES/WebGL, the NDC clipspace range for depth is from -1 to 1 and y is flipped. + // wgpu/Naga counteracts this by patching all vertex shaders with: + // "gl_Position.yz = vec2(-gl_Position.y, gl_Position.z * 2.0 - gl_Position.w);", + // This is great, since it means that we can pretend depth is 0 to 1 as specified by WebGPU. + // But it completely messes up depth precision, in particular since we use + // an inverse depth projection that tries to make use of the high float precision closer to zero. + let eps = f32eps; return Vec4( position.xy, - max(position.z * (1.0 + eps * offset), f32eps), + max(position.z + eps * offset * position.w, f32eps), position.w ); } else { - // Causes Z-collision at far distances - let eps = f32eps; + // This path assumes a `f32` depth buffer! + + // 1.0 * eps _should_ be enough, but in practice it causes Z-fighting for unknown reasons. + // Maybe because of GPU interpolation of vertex coordinates? + let eps = 5.0 * f32eps; return Vec4( position.xy, - max(position.z + eps * offset * position.w, f32eps), + max(position.z * (1.0 + eps * offset), f32eps), position.w ); } diff --git a/crates/re_renderer/src/config.rs b/crates/re_renderer/src/config.rs index 7b85283b505f..afa4340bd0f6 100644 --- a/crates/re_renderer/src/config.rs +++ b/crates/re_renderer/src/config.rs @@ -7,16 +7,18 @@ /// but choosing lower tiers is always possible. /// Tiers may loosely relate to quality settings, but their primary function is an easier way to /// do bundle feature *support* checks. +/// +/// See also `global_bindings.wgsl` #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum HardwareTier { /// Limited feature support as provided by WebGL and native GLES2/OpenGL3(ish). - Gles, + Gles = 0, /// Full support of WebGPU spec without additional feature requirements. /// /// Expecting to run either in a stable WebGPU implementation. /// I.e. either natively with Vulkan/Metal or in a browser with WebGPU support. - FullWebGpuSupport, + FullWebGpuSupport = 1, // Run natively with Vulkan/Metal and require additional features. //HighEnd } diff --git a/crates/re_renderer/src/context.rs b/crates/re_renderer/src/context.rs index 1f57bf4afadd..a2c8d032b819 100644 --- a/crates/re_renderer/src/context.rs +++ b/crates/re_renderer/src/context.rs @@ -201,11 +201,6 @@ impl RenderContext { "@invariant @builtin(position)".to_owned(), "@builtin(position)".to_owned(), )); - } else if adapter.get_info().backend == wgpu::Backend::Gl { - gpu_resources - .shader_modules - .shader_text_workaround_replacements - .push(("GLES = false".to_owned(), "GLES = true".to_owned())); } RenderContext { diff --git a/crates/re_renderer/src/global_bindings.rs b/crates/re_renderer/src/global_bindings.rs index c00ad0315c67..7c651cd19f42 100644 --- a/crates/re_renderer/src/global_bindings.rs +++ b/crates/re_renderer/src/global_bindings.rs @@ -45,8 +45,8 @@ pub struct FrameUniformBuffer { // Size used for all line radii given with Size::AUTO. pub auto_size_lines: f32, - /// Factor used to compute depth offsets, see `depth_offset.wgsl`. - pub end_padding: wgpu_buffer_types::PaddingRow, + /// re_renderer defined hardware tier. + pub hardware_tier: wgpu_buffer_types::U32RowPadded, } pub(crate) struct GlobalBindings { diff --git a/crates/re_renderer/src/view_builder.rs b/crates/re_renderer/src/view_builder.rs index 7b84d8c0ef32..fc987ece35f7 100644 --- a/crates/re_renderer/src/view_builder.rs +++ b/crates/re_renderer/src/view_builder.rs @@ -457,7 +457,7 @@ impl ViewBuilder { auto_size_points: auto_size_points.0, auto_size_lines: auto_size_lines.0, - end_padding: Default::default(), + hardware_tier: (ctx.shared_renderer_data.config.hardware_tier as u32).into(), }; let frame_uniform_buffer = create_and_fill_uniform_buffer( ctx, diff --git a/crates/re_renderer/src/workspace_shaders.rs b/crates/re_renderer/src/workspace_shaders.rs index 50abc69da36c..7d999da73645 100644 --- a/crates/re_renderer/src/workspace_shaders.rs +++ b/crates/re_renderer/src/workspace_shaders.rs @@ -43,12 +43,6 @@ pub fn init() { fs.create_file(virtpath, content).unwrap(); } - { - let virtpath = Path::new("shader/device_info.wgsl"); - let content = include_str!("../shader/device_info.wgsl").into(); - fs.create_file(virtpath, content).unwrap(); - } - { let virtpath = Path::new("shader/generic_skybox.wgsl"); let content = include_str!("../shader/generic_skybox.wgsl").into(); From 9721d4f8d354fdb036635af1da1edc03064cc7b8 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 22 May 2023 20:34:10 +0200 Subject: [PATCH 4/4] Improve the depth offset for all platforms --- .../re_renderer/shader/global_bindings.wgsl | 1 + .../shader/utils/depth_offset.wgsl | 87 ++++++++++++------- 2 files changed, 55 insertions(+), 33 deletions(-) diff --git a/crates/re_renderer/shader/global_bindings.wgsl b/crates/re_renderer/shader/global_bindings.wgsl index 0568f2b4c49d..a3ee3e981fc7 100644 --- a/crates/re_renderer/shader/global_bindings.wgsl +++ b/crates/re_renderer/shader/global_bindings.wgsl @@ -31,6 +31,7 @@ struct FrameUniformBuffer { /// re_renderer defined hardware tier. hardware_tier: u32, }; + @group(0) @binding(0) var frame: FrameUniformBuffer; diff --git a/crates/re_renderer/shader/utils/depth_offset.wgsl b/crates/re_renderer/shader/utils/depth_offset.wgsl index fbd23ac66e12..54dba8113ef8 100644 --- a/crates/re_renderer/shader/utils/depth_offset.wgsl +++ b/crates/re_renderer/shader/utils/depth_offset.wgsl @@ -1,38 +1,59 @@ #import <../global_bindings.wgsl> #import <../types.wgsl> +/* +We use reverse infinite depth, as promoted by https://developer.nvidia.com/content/depth-precision-visualized + +The projection matrix (from `glam::Mat4::perspective_infinite_reverse_rh`) looks like this: + +f / aspect_ratio 0 0 0 +0 f 0 0 +0 0 0 z_near +0 0 -1 0 + +This means after multiplication with xyzw (with w=1) we end up with: + + x_proj: x * f / aspect_ratio, + y_proj: y * f, + z_proj: w * z_near, + w_proj: -z + +This is then projected by dividing with w, giving: + + x_ndc: x_proj / w_proj + y_ndc: y_proj / w_proj + z_ndc: z_proj / w_proj + +Without z offset, we get this: + + x_ndc: x * f / aspect_ratio / -z + y_ndc: y * f / -z + z_ndc: w * z_near / -z + +The negative -z axis is away from the camera, so with w=1 we get +z_near mapping to z_ndc=1, and infinity mapping to z_ndc=0. + +The code below act on the *_proj values by adding a scale multiplier on `w_proj` resulting in: + x_ndc: x_proj / (-z * w_scale) + y_ndc: y_proj / (-z * w_scale) + z_ndc: z_proj / (-z * w_scale) +*/ + fn apply_depth_offset(position: Vec4, offset: f32) -> Vec4 { - // We're using inverse z, i.e. 0.0 is far, 1.0 is near. - // We want a positive offset to move towards the viewer, so offset needs to be added. - // - // With this in place we still may cross over to 0.0 (the far plane) too early, - // making objects disappear into the far when they'd be otherwise still rendered. - // Since we're actually supposed to have an *infinite* far plane this should never happen! - // Therefore we simply dictacte a minimum z value. - // This ofc wrecks the depth offset and may cause z fighting with all very far away objects, but it's better than having things disappear! - if frame.hardware_tier == HARDWARE_TIER_GLES { - // On GLES/WebGL, the NDC clipspace range for depth is from -1 to 1 and y is flipped. - // wgpu/Naga counteracts this by patching all vertex shaders with: - // "gl_Position.yz = vec2(-gl_Position.y, gl_Position.z * 2.0 - gl_Position.w);", - // This is great, since it means that we can pretend depth is 0 to 1 as specified by WebGPU. - // But it completely messes up depth precision, in particular since we use - // an inverse depth projection that tries to make use of the high float precision closer to zero. - let eps = f32eps; - return Vec4( - position.xy, - max(position.z + eps * offset * position.w, f32eps), - position.w - ); - } else { - // This path assumes a `f32` depth buffer! - - // 1.0 * eps _should_ be enough, but in practice it causes Z-fighting for unknown reasons. - // Maybe because of GPU interpolation of vertex coordinates? - let eps = 5.0 * f32eps; - return Vec4( - position.xy, - max(position.z * (1.0 + eps * offset), f32eps), - position.w - ); - } + // On GLES/WebGL, the NDC clipspace range for depth is from -1 to 1 and y is flipped. + // wgpu/Naga counteracts this by patching all vertex shaders with: + // "gl_Position.yz = vec2(-gl_Position.y, gl_Position.z * 2.0 - gl_Position.w);", + // This doesn't matter for us though. + + // This path assumes a `f32` depth buffer. + + // We set up the depth comparison to Greater, so that large z means closer (overdraw). + // We want a greater offset to win over a smaller offset, + // so a great depth offset should result in a large z_ndc. + // How do we get there? We let large depth offset lead to a smaller divisor (w_proj): + + return Vec4( + position.xyz, + position.w * (1.0 - f32eps * offset), + ); }