From bb0ce8311c931e80d5dbc54a8864b35d1242f78c Mon Sep 17 00:00:00 2001 From: Artavazd Balaian Date: Sun, 29 Jan 2023 23:41:43 +0800 Subject: [PATCH 1/4] fix: Write `gl_PointSize = 1.0;` in case of vertex shader and set bit `FORCE_POINT_SIZE` According to https://registry.khronos.org/OpenGL/specs/es/3.2/GLSL_ES_Specification_3.20.html#built-in-language-variables ``` The variable gl_PointSize is intended for a shader to write the size of the point to be rasterized. It is measured in pixels. If gl_PointSize is not written to, its value is undefined in subsequent pipe stages. ``` --- src/back/glsl/mod.rs | 51 ++++++++++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 16 deletions(-) diff --git a/src/back/glsl/mod.rs b/src/back/glsl/mod.rs index 6c90fcb26d..58655f36c7 100644 --- a/src/back/glsl/mod.rs +++ b/src/back/glsl/mod.rs @@ -218,6 +218,13 @@ bitflags::bitflags! { /// global variables that are not used in the specified entrypoint (including indirect use), /// all constant declarations, and functions that use excluded global variables. const INCLUDE_UNUSED_ITEMS = 0x4; + /// Emit `PointSize` output builtin to vertex shaders, which is + /// required for drawing with `PointList` topology. + /// + /// https://registry.khronos.org/OpenGL/specs/es/3.2/GLSL_ES_Specification_3.20.html#built-in-language-variables + /// The variable gl_PointSize is intended for a shader to write the size of the point to be rasterized. It is measured in pixels. + /// If gl_PointSize is not written to, its value is undefined in subsequent pipe stages. + const FORCE_POINT_SIZE = 0x10; } } @@ -1978,6 +1985,7 @@ impl<'a, W: Write> Writer<'a, W> { writeln!(self.out, ";")?; } back::FunctionType::EntryPoint(ep_index) => { + let mut has_point_size = false; let ep = &self.module.entry_points[ep_index as usize]; if let Some(ref result) = ep.function.result { let value = value.unwrap(); @@ -2005,10 +2013,11 @@ impl<'a, W: Write> Writer<'a, W> { if let Some(crate::Binding::BuiltIn(builtin)) = member.binding { + has_point_size |= builtin == crate::BuiltIn::PointSize; + match builtin { crate::BuiltIn::ClipDistance - | crate::BuiltIn::CullDistance - | crate::BuiltIn::PointSize => { + | crate::BuiltIn::CullDistance => { if self.options.version.is_es() { continue; } @@ -2056,20 +2065,30 @@ impl<'a, W: Write> Writer<'a, W> { } } - if let back::FunctionType::EntryPoint(ep_index) = ctx.ty { - if self.module.entry_points[ep_index as usize].stage - == crate::ShaderStage::Vertex - && self - .options - .writer_flags - .contains(WriterFlags::ADJUST_COORDINATE_SPACE) - { - writeln!( - self.out, - "gl_Position.yz = vec2(-gl_Position.y, gl_Position.z * 2.0 - gl_Position.w);", - )?; - write!(self.out, "{level}")?; - } + let is_vertex_stage = self.module.entry_points[ep_index as usize].stage + == ShaderStage::Vertex; + if is_vertex_stage + && self + .options + .writer_flags + .contains(WriterFlags::ADJUST_COORDINATE_SPACE) + { + writeln!( + self.out, + "gl_Position.yz = vec2(-gl_Position.y, gl_Position.z * 2.0 - gl_Position.w);", + )?; + write!(self.out, "{level}")?; + } + + if is_vertex_stage + && self + .options + .writer_flags + .contains(WriterFlags::FORCE_POINT_SIZE) + && !has_point_size + { + writeln!(self.out, "gl_PointSize = 1.0;",)?; + write!(self.out, "{level}")?; } writeln!(self.out, "return;")?; } From cf30dd632f372ea434f56a0748f642986ee21b86 Mon Sep 17 00:00:00 2001 From: Artavazd Balaian Date: Wed, 1 Feb 2023 19:46:46 +0800 Subject: [PATCH 2/4] feat: Add test - Write warn message if `ClipDistance` and `CullDistance` are used on unsupported version --- src/back/glsl/mod.rs | 17 ++++++++++++++++- ...rce_point_size_vertex_shader_webgl.param.ron | 11 +++++++++++ .../force_point_size_vertex_shader_webgl.wgsl | 14 ++++++++++++++ ...ze_vertex_shader_webgl.fs_main.Fragment.glsl | 12 ++++++++++++ ...size_vertex_shader_webgl.vs_main.Vertex.glsl | 15 +++++++++++++++ tests/snapshots.rs | 1 + 6 files changed, 69 insertions(+), 1 deletion(-) create mode 100644 tests/in/force_point_size_vertex_shader_webgl.param.ron create mode 100644 tests/in/force_point_size_vertex_shader_webgl.wgsl create mode 100644 tests/out/glsl/force_point_size_vertex_shader_webgl.fs_main.Fragment.glsl create mode 100644 tests/out/glsl/force_point_size_vertex_shader_webgl.vs_main.Vertex.glsl diff --git a/src/back/glsl/mod.rs b/src/back/glsl/mod.rs index 58655f36c7..8d234ff60c 100644 --- a/src/back/glsl/mod.rs +++ b/src/back/glsl/mod.rs @@ -2019,6 +2019,21 @@ impl<'a, W: Write> Writer<'a, W> { crate::BuiltIn::ClipDistance | crate::BuiltIn::CullDistance => { if self.options.version.is_es() { + // According to https://github.com/KhronosGroup/GLSL/issues/132 + /* > We agreed that gl_ClipDistance and gl_CullDistance, and related language, + should not appear in the ESSL spec since it's not part of OpenGL ES 3.2. + */ + if let Version::Embedded { + version, + is_webgl: _, + } = self.options.version + { + if version <= 320 { + log::warn!( + "{:?} is not part of OpenGL ES <= 3.2", + builtin); + } + } continue; } } @@ -2087,7 +2102,7 @@ impl<'a, W: Write> Writer<'a, W> { .contains(WriterFlags::FORCE_POINT_SIZE) && !has_point_size { - writeln!(self.out, "gl_PointSize = 1.0;",)?; + writeln!(self.out, "gl_PointSize = 1.0;")?; write!(self.out, "{level}")?; } writeln!(self.out, "return;")?; diff --git a/tests/in/force_point_size_vertex_shader_webgl.param.ron b/tests/in/force_point_size_vertex_shader_webgl.param.ron new file mode 100644 index 0000000000..9499c6b990 --- /dev/null +++ b/tests/in/force_point_size_vertex_shader_webgl.param.ron @@ -0,0 +1,11 @@ +( + glsl: ( + version: Embedded ( + version: 300, + is_webgl: true + ), + writer_flags: (bits: 16), + binding_map: {}, + zero_initialize_workgroup_memory: true, + ), +) diff --git a/tests/in/force_point_size_vertex_shader_webgl.wgsl b/tests/in/force_point_size_vertex_shader_webgl.wgsl new file mode 100644 index 0000000000..001468bd50 --- /dev/null +++ b/tests/in/force_point_size_vertex_shader_webgl.wgsl @@ -0,0 +1,14 @@ +// AUTHOR: REASY +// ISSUE: https://github.com/gfx-rs/wgpu/issues/3179 +// FIX: https://github.com/gfx-rs/wgpu/pull/3440 +@vertex +fn vs_main(@builtin(vertex_index) in_vertex_index: u32) -> @builtin(position) vec4 { + let x = f32(i32(in_vertex_index) - 1); + let y = f32(i32(in_vertex_index & 1u) * 2 - 1); + return vec4(x, y, 0.0, 1.0); +} + +@fragment +fn fs_main() -> @location(0) vec4 { + return vec4(1.0, 0.0, 0.0, 1.0); +} diff --git a/tests/out/glsl/force_point_size_vertex_shader_webgl.fs_main.Fragment.glsl b/tests/out/glsl/force_point_size_vertex_shader_webgl.fs_main.Fragment.glsl new file mode 100644 index 0000000000..5d0f7a8072 --- /dev/null +++ b/tests/out/glsl/force_point_size_vertex_shader_webgl.fs_main.Fragment.glsl @@ -0,0 +1,12 @@ +#version 300 es + +precision highp float; +precision highp int; + +layout(location = 0) out vec4 _fs2p_location0; + +void main() { + _fs2p_location0 = vec4(1.0, 0.0, 0.0, 1.0); + return; +} + diff --git a/tests/out/glsl/force_point_size_vertex_shader_webgl.vs_main.Vertex.glsl b/tests/out/glsl/force_point_size_vertex_shader_webgl.vs_main.Vertex.glsl new file mode 100644 index 0000000000..069595bc81 --- /dev/null +++ b/tests/out/glsl/force_point_size_vertex_shader_webgl.vs_main.Vertex.glsl @@ -0,0 +1,15 @@ +#version 300 es + +precision highp float; +precision highp int; + + +void main() { + uint in_vertex_index = uint(gl_VertexID); + float x = float((int(in_vertex_index) - 1)); + float y = float(((int((in_vertex_index & 1u)) * 2) - 1)); + gl_Position = vec4(x, y, 0.0, 1.0); + gl_PointSize = 1.0; + return; +} + diff --git a/tests/snapshots.rs b/tests/snapshots.rs index 25f307c110..cc2b300558 100644 --- a/tests/snapshots.rs +++ b/tests/snapshots.rs @@ -557,6 +557,7 @@ fn convert_wgsl() { Targets::WGSL | Targets::GLSL | Targets::SPIRV | Targets::HLSL | Targets::METAL, ), ("sprite", Targets::SPIRV), + ("force_point_size_vertex_shader_webgl", Targets::GLSL), ]; for &(name, targets) in inputs.iter() { From 426d4b6883722b8d20927648fede8a8c4f42ebb5 Mon Sep 17 00:00:00 2001 From: Artavazd Balaian Date: Thu, 2 Feb 2023 06:27:46 +0800 Subject: [PATCH 3/4] fix: Log regardless of version --- src/back/glsl/mod.rs | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/back/glsl/mod.rs b/src/back/glsl/mod.rs index 8d234ff60c..813dc655ab 100644 --- a/src/back/glsl/mod.rs +++ b/src/back/glsl/mod.rs @@ -2023,17 +2023,10 @@ impl<'a, W: Write> Writer<'a, W> { /* > We agreed that gl_ClipDistance and gl_CullDistance, and related language, should not appear in the ESSL spec since it's not part of OpenGL ES 3.2. */ - if let Version::Embedded { - version, - is_webgl: _, - } = self.options.version - { - if version <= 320 { - log::warn!( + log::warn!( "{:?} is not part of OpenGL ES <= 3.2", - builtin); - } - } + builtin + ); continue; } } From 8423c5397103f525b044279305354757de23981a Mon Sep 17 00:00:00 2001 From: Teodor Tanasoaia <28601907+teoxoy@users.noreply.github.com> Date: Thu, 2 Feb 2023 12:35:57 +0100 Subject: [PATCH 4/4] Update comment --- src/back/glsl/mod.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/back/glsl/mod.rs b/src/back/glsl/mod.rs index 813dc655ab..838b323441 100644 --- a/src/back/glsl/mod.rs +++ b/src/back/glsl/mod.rs @@ -2019,12 +2019,10 @@ impl<'a, W: Write> Writer<'a, W> { crate::BuiltIn::ClipDistance | crate::BuiltIn::CullDistance => { if self.options.version.is_es() { - // According to https://github.com/KhronosGroup/GLSL/issues/132 - /* > We agreed that gl_ClipDistance and gl_CullDistance, and related language, - should not appear in the ESSL spec since it's not part of OpenGL ES 3.2. - */ + // Note that gl_ClipDistance and gl_CullDistance are listed in the GLSL ES 3.2 spec but shouldn't + // See https://github.com/KhronosGroup/GLSL/issues/132#issuecomment-685818465 log::warn!( - "{:?} is not part of OpenGL ES <= 3.2", + "{:?} is not part of GLSL ES", builtin ); continue;