From e043e90027a5921e81bdf4d3a64c6d5b888f9086 Mon Sep 17 00:00:00 2001 From: Artavazd Balaian Date: Thu, 26 Jan 2023 19:44:10 +0800 Subject: [PATCH 1/4] fix: Set `FORCE_POINT_SIZE` if it is vertex shader with mesh consist of point list --- wgpu-hal/src/gles/device.rs | 43 ++++++++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/wgpu-hal/src/gles/device.rs b/wgpu-hal/src/gles/device.rs index f6921d2b69..cc057a551b 100644 --- a/wgpu-hal/src/gles/device.rs +++ b/wgpu-hal/src/gles/device.rs @@ -192,8 +192,10 @@ impl super::Device { naga_stage: naga::ShaderStage, stage: &crate::ProgrammableStage, context: CompilationContext, + primitive_topology: Option, ) -> Result { use naga::back::glsl; + let pipeline_options = glsl::PipelineOptions { shader_stage: naga_stage, entry_point: stage.entry_point.to_string(), @@ -225,12 +227,31 @@ impl super::Device { binding_array: BoundsCheckPolicy::Unchecked, }; + let update_naga_options = primitive_topology == Some(wgt::PrimitiveTopology::PointList) + && naga_stage == naga::ShaderStage::Vertex; + // Update naga options if mesh consist of point list and it is vertex shader + let naga_options = if update_naga_options { + let mut wrt_flags = context.layout.naga_options.writer_flags; + wrt_flags.set(glsl::WriterFlags::FORCE_POINT_SIZE, true); + glsl::Options { + version: context.layout.naga_options.version, + writer_flags: wrt_flags, + binding_map: context.layout.naga_options.binding_map.clone(), + zero_initialize_workgroup_memory: context + .layout + .naga_options + .zero_initialize_workgroup_memory, + } + } else { + context.layout.naga_options.clone() + }; + let mut output = String::new(); let mut writer = glsl::Writer::new( &mut output, &shader.module, &shader.info, - &context.layout.naga_options, + &naga_options, &pipeline_options, policies, ) @@ -262,6 +283,7 @@ impl super::Device { layout: &super::PipelineLayout, #[cfg_attr(target_arch = "wasm32", allow(unused))] label: Option<&str>, multiview: Option, + primitive_topology: Option, ) -> Result, crate::PipelineError> { let mut program_stages = ArrayVec::new(); let mut group_to_binding_to_slot = Vec::with_capacity(layout.group_infos.len()); @@ -300,6 +322,7 @@ impl super::Device { multiview, glsl_version, self.shared.private_caps, + primitive_topology, ) }) .to_owned()?; @@ -316,6 +339,7 @@ impl super::Device { multiview: Option, glsl_version: u16, private_caps: super::PrivateCapabilities, + primitive_topology: Option, ) -> Result, crate::PipelineError> { let program = unsafe { gl.create_program() }.unwrap(); #[cfg(not(target_arch = "wasm32"))] @@ -340,7 +364,7 @@ impl super::Device { multiview, }; - let shader = Self::create_shader(gl, naga_stage, stage, context)?; + let shader = Self::create_shader(gl, naga_stage, stage, context, primitive_topology)?; shaders_to_delete.push(shader); } @@ -1097,8 +1121,16 @@ impl crate::Device for super::Device { if let Some(ref fs) = desc.fragment_stage { shaders.push((naga::ShaderStage::Fragment, fs)); } - let inner = - unsafe { self.create_pipeline(gl, shaders, desc.layout, desc.label, desc.multiview) }?; + let inner = unsafe { + self.create_pipeline( + gl, + shaders, + desc.layout, + desc.label, + desc.multiview, + Some(desc.primitive.topology), + ) + }?; let (vertex_buffers, vertex_attributes) = { let mut buffers = Vec::new(); @@ -1179,7 +1211,8 @@ impl crate::Device for super::Device { let gl = &self.shared.context.lock(); let mut shaders = ArrayVec::new(); shaders.push((naga::ShaderStage::Compute, &desc.stage)); - let inner = unsafe { self.create_pipeline(gl, shaders, desc.layout, desc.label, None) }?; + let inner = + unsafe { self.create_pipeline(gl, shaders, desc.layout, desc.label, None, None) }?; Ok(super::ComputePipeline { inner }) } From 0314f88d7ba4902260dc4d4fdd185a076e37ec3d Mon Sep 17 00:00:00 2001 From: Artavazd Balaian Date: Fri, 3 Feb 2023 20:02:42 +0800 Subject: [PATCH 2/4] chore: Add details into CHANGELOG.md as bugfix for GLES --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 00bb864aa6..c4770223f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,10 @@ Bottom level categories: - Fix DXC validation issues when using a custom `dxil_path`. By @Elabajaba in [#3434](https://github.com/gfx-rs/wgpu/pull/3434) +#### GLES + +- [gles] fix: Set FORCE_POINT_SIZE if it is vertex shader with mesh consist of point list. By @REASY in [3440](https://github.com/gfx-rs/wgpu/pull/3440) + #### General - `copyTextureToTexture` src/dst aspects must both refer to all aspects of src/dst format. By @teoxoy in [#3431](https://github.com/gfx-rs/wgpu/pull/3431) From d5566177aa17aaa307c6a74b095396f956a4a492 Mon Sep 17 00:00:00 2001 From: Artavazd Balaian Date: Sun, 12 Feb 2023 12:40:58 +0800 Subject: [PATCH 3/4] fix: Allow too many args --- wgpu-hal/src/gles/device.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/wgpu-hal/src/gles/device.rs b/wgpu-hal/src/gles/device.rs index cc057a551b..ba1659ff0d 100644 --- a/wgpu-hal/src/gles/device.rs +++ b/wgpu-hal/src/gles/device.rs @@ -331,6 +331,7 @@ impl super::Device { Ok(program) } + #[allow(clippy::too_many_arguments)] unsafe fn create_program<'a>( gl: &glow::Context, shaders: ArrayVec, 3>, From 0669fe0f543e395193efb033b2c5b9908b089f8e Mon Sep 17 00:00:00 2001 From: Artavazd Balaian Date: Wed, 15 Feb 2023 08:16:38 +0800 Subject: [PATCH 4/4] feat: Always set `glsl::WriterFlags::FORCE_POINT_SIZE` to true --- wgpu-hal/src/gles/device.rs | 47 +++++++------------------------------ 1 file changed, 8 insertions(+), 39 deletions(-) diff --git a/wgpu-hal/src/gles/device.rs b/wgpu-hal/src/gles/device.rs index ba1659ff0d..6279e2ddae 100644 --- a/wgpu-hal/src/gles/device.rs +++ b/wgpu-hal/src/gles/device.rs @@ -192,10 +192,8 @@ impl super::Device { naga_stage: naga::ShaderStage, stage: &crate::ProgrammableStage, context: CompilationContext, - primitive_topology: Option, ) -> Result { use naga::back::glsl; - let pipeline_options = glsl::PipelineOptions { shader_stage: naga_stage, entry_point: stage.entry_point.to_string(), @@ -227,31 +225,12 @@ impl super::Device { binding_array: BoundsCheckPolicy::Unchecked, }; - let update_naga_options = primitive_topology == Some(wgt::PrimitiveTopology::PointList) - && naga_stage == naga::ShaderStage::Vertex; - // Update naga options if mesh consist of point list and it is vertex shader - let naga_options = if update_naga_options { - let mut wrt_flags = context.layout.naga_options.writer_flags; - wrt_flags.set(glsl::WriterFlags::FORCE_POINT_SIZE, true); - glsl::Options { - version: context.layout.naga_options.version, - writer_flags: wrt_flags, - binding_map: context.layout.naga_options.binding_map.clone(), - zero_initialize_workgroup_memory: context - .layout - .naga_options - .zero_initialize_workgroup_memory, - } - } else { - context.layout.naga_options.clone() - }; - let mut output = String::new(); let mut writer = glsl::Writer::new( &mut output, &shader.module, &shader.info, - &naga_options, + &context.layout.naga_options, &pipeline_options, policies, ) @@ -283,7 +262,6 @@ impl super::Device { layout: &super::PipelineLayout, #[cfg_attr(target_arch = "wasm32", allow(unused))] label: Option<&str>, multiview: Option, - primitive_topology: Option, ) -> Result, crate::PipelineError> { let mut program_stages = ArrayVec::new(); let mut group_to_binding_to_slot = Vec::with_capacity(layout.group_infos.len()); @@ -322,7 +300,6 @@ impl super::Device { multiview, glsl_version, self.shared.private_caps, - primitive_topology, ) }) .to_owned()?; @@ -331,7 +308,6 @@ impl super::Device { Ok(program) } - #[allow(clippy::too_many_arguments)] unsafe fn create_program<'a>( gl: &glow::Context, shaders: ArrayVec, 3>, @@ -340,7 +316,6 @@ impl super::Device { multiview: Option, glsl_version: u16, private_caps: super::PrivateCapabilities, - primitive_topology: Option, ) -> Result, crate::PipelineError> { let program = unsafe { gl.create_program() }.unwrap(); #[cfg(not(target_arch = "wasm32"))] @@ -365,7 +340,7 @@ impl super::Device { multiview, }; - let shader = Self::create_shader(gl, naga_stage, stage, context, primitive_topology)?; + let shader = Self::create_shader(gl, naga_stage, stage, context)?; shaders_to_delete.push(shader); } @@ -977,6 +952,9 @@ impl crate::Device for super::Device { .private_caps .contains(super::PrivateCapabilities::SHADER_TEXTURE_SHADOW_LOD), ); + // We always force point size to be written and it will be ignored by the driver if it's not a point list primitive. + // https://github.com/gfx-rs/wgpu/pull/3440/files#r1095726950 + writer_flags.set(glsl::WriterFlags::FORCE_POINT_SIZE, true); let mut binding_map = glsl::BindingMap::default(); for (group_index, bg_layout) in desc.bind_group_layouts.iter().enumerate() { @@ -1122,16 +1100,8 @@ impl crate::Device for super::Device { if let Some(ref fs) = desc.fragment_stage { shaders.push((naga::ShaderStage::Fragment, fs)); } - let inner = unsafe { - self.create_pipeline( - gl, - shaders, - desc.layout, - desc.label, - desc.multiview, - Some(desc.primitive.topology), - ) - }?; + let inner = + unsafe { self.create_pipeline(gl, shaders, desc.layout, desc.label, desc.multiview) }?; let (vertex_buffers, vertex_attributes) = { let mut buffers = Vec::new(); @@ -1212,8 +1182,7 @@ impl crate::Device for super::Device { let gl = &self.shared.context.lock(); let mut shaders = ArrayVec::new(); shaders.push((naga::ShaderStage::Compute, &desc.stage)); - let inner = - unsafe { self.create_pipeline(gl, shaders, desc.layout, desc.label, None, None) }?; + let inner = unsafe { self.create_pipeline(gl, shaders, desc.layout, desc.label, None) }?; Ok(super::ComputePipeline { inner }) }