Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[glsl] Inject default gl_PointSize = 1.0 in vertex shaders if FORCE_POINT_SIZE option was set #2223

Merged
merged 4 commits into from
Feb 2, 2023

Conversation

REASY
Copy link
Contributor

@REASY REASY commented Jan 26, 2023

Write gl_PointSize = 1.0; in case of vertex shader and set writer flag 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.

This is the first part of the changes to address gfx-rs/wgpu#3179

@REASY REASY force-pushed the fix/wgpu-3179-set-point-size branch 3 times, most recently from 2118afb to 4df4161 Compare January 26, 2023 13:58
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be implemented in the same way it is for the spv backend.

naga/src/back/spv/writer.rs

Lines 524 to 526 in c7d0215

if self.flags.contains(WriterFlags::FORCE_POINT_SIZE)
&& iface.stage == crate::ShaderStage::Vertex
&& !has_point_size

  • rename allow_point_size to force_point_size
  • check if we already have (and wrote/will write) the BuiltIn::PointSize

@JCapucho
Copy link
Collaborator

It would also help to include a comment in the code explaining this for future reference with a mention of the section of where it happens in the specification (for glsl ES 3.1 this is in section 7.1.1)

@REASY REASY force-pushed the fix/wgpu-3179-set-point-size branch from 4df4161 to c3e5073 Compare January 31, 2023 12:34
@REASY REASY force-pushed the fix/wgpu-3179-set-point-size branch from c3e5073 to f8aff4d Compare January 31, 2023 12:44
src/back/glsl/mod.rs Outdated Show resolved Hide resolved
… `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.
```
- Write warn message if `ClipDistance` and `CullDistance` are used on unsupported version
@REASY REASY force-pushed the fix/wgpu-3179-set-point-size branch from f8aff4d to cf30dd6 Compare February 1, 2023 12:19
@REASY REASY requested a review from teoxoy February 1, 2023 12:24
src/back/glsl/mod.rs Outdated Show resolved Hide resolved
@REASY REASY requested a review from teoxoy February 1, 2023 22:54
src/back/glsl/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@teoxoy teoxoy changed the title [glsl] fix: Write gl_PointSize = 1.0; in case of vertex shader and allow_point_size == true [glsl] Inject default gl_PointSize = 1.0 in vertex shaders if FORCE_POINT_SIZE option was set Feb 2, 2023
@teoxoy teoxoy merged commit fe851fb into gfx-rs:master Feb 2, 2023
REASY added a commit to REASY/wgpu that referenced this pull request Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants