Skip to content

Apply a consistent padding in shader uniforms for the WebGL2 target #18863

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Henauxg
Copy link
Contributor

@Henauxg Henauxg commented Apr 16, 2025

Objective

Following the discussions in #18812 (and a few messages on Discord), paddings for the WebGL2 target in the different bevy shader uniforms are found to not always be optimal (or even correct) in size, nor consistent in style.

As an example, in post_processing.wgsl, the following uniform does not have the expected size.

struct PostProcessSettings {
    intensity: f32,
#ifdef SIXTEEN_BYTE_ALIGNMENT
    // WebGL2 structs must be 16 byte aligned.
    _webgl2_padding: vec3<f32>
#endif
}
[...] depending on how the data is prepared, either the vec3 in the shader is pointing to garbage data or the rust serialisation is 
wasting bytes (minor issue if only one value, significant if many values) and is misleading because it says an f32 followed by 12 
empty bytes, followed by the vec3 of padding bytes and then 4 more empty bytes. So it’s 32 bytes just to bind 4 and it’s confusing
because the vec3 doesn’t pack straight after the f32.

Solution

  • Remove usage of vec2 & vec3 as padding fields
  • Changed all shader uniforms to use single scalar u32 fields as padding
  • Changed the naming of padding fields to be consistent (_webgl2_padding_*b)
  • The uniform declaration in line_joints.wgsl seemed not up to date with the rust-side following the gizmos dashed line update. The gap_scale and line_scale fields were missing from the shader-side uniform declaration. They were only declared in LineGizmoUniform in lines.wgsl.
    • Question to reviewers: should/could both shaders share a common LineGizmoUniform declaration in lines.wgsl exported via a #define_import_path bevy_gizmos::lines ?
  • In SkyboxUniforms, the transform and brightness fields were swapped. I may be wrong but I supposed that the mat4x4<f32> being after the f32 could cause padding to be inserted between the brightness and transform fields (to align the mat4x4 on 16 byte).
    • Question to reviewers: I could not get the skybox example to crash on WebGL2, even by removing all the padding fields in this uniform, or by adding just 1 or 2 instead of 3. I don't know why this is the case.

For reference, WGSL alignment and size.

Testing

  • Tested the examples locally for both Native Vulkan & WebGL2
    • 2d_gizmos
    • 3d_gizmos
    • skybox
    • post_processing
    • motion_blur
    • deferred lightning

Copy link
Contributor

@djeedai djeedai left a comment

Choose a reason for hiding this comment

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

Yes, the implementation is the same (more or less) as Hanabi, and u32 was already found to be a good idea there. Feels like we're double-solving problems, but that's a different issue.

On the PR description, I think it should be emphasized that's it's not just "not optimal"; padding with vec3 is just plain wrong and the worst choice of all possible WGSL types because it has a 16-byte alignment requirement itself (despite being 12 bytes long only) so will never pad anything else to 16 bytes.

Also I'd point to the rule of WebGL2 because it feels like there's some 16-byte alignment rules I'm not aware of specifically for WebGL2, in addition (and independently of) any WGSL alignment rules, so linking the latter and not the former in the PR description is confusing. The WGSL alignments are as far as I know all based on per-device requirements you can only query at runtime (like min_storage_buffer_offset_alignment which requires padding your structs if you want to bind a sub-view of an array).

Approving as the fix is correct in any case.

@Henauxg
Copy link
Contributor Author

Henauxg commented Apr 19, 2025

(The WGSL spec link was there only for the vec3 alignment and size references. Here's the equivalent link to the to the OpenGL ES spec from whcih the WebGL2 spec is derived https://registry.khronos.org/OpenGL/specs/es/3.0/es_spec_3.0.pdf#nameddest=subsection.2.12.6)

But, I agree with the missing link to the relevant information about the very specific UBO 16 byte alignment constraint. I could not find the exact information yet in any of the specifications.
What I did find:

@Henauxg Henauxg added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 23, 2025
@Henauxg Henauxg added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants