Skip to content

Fix the extended_material example on WebGL2 #18812

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 2 commits into
base: main
Choose a base branch
from

Conversation

Henauxg
Copy link
Contributor

@Henauxg Henauxg commented Apr 11, 2025

Objective

Solution

Replaced the u32 field in MyExtension by a UVec4 and only use the x coordinate.

Alternatives

1- Separate conditional padding field

We can also add conditional padding to both structs:

// ... In extended_material.rs

struct MyExtension {
  #[uniform(100)]
  quantize_steps: u32,
  // WebGL2 structs must be 16 byte aligned.
  #[cfg(all(feature = "webgl2", target_arch = "wasm32", not(feature = "webgpu")))]
  #[uniform(100)]
   _webgl2_padding: Vec3,
}

// ... In extended_material.wgsl

struct MyExtendedMaterial {
   quantize_steps: u32,
#ifdef SIXTEEN_BYTE_ALIGNMENT
   // WebGL2 structs must be 16 byte aligned.
   _webgl2_padding: vec3,
#endif
}

EDIT: this alternative works fine too

2- Don't fix it, unlist it

While the fix is quite simple, it does muddy the waters a tiny bit due to quantize_steps now being a UVec4 instead of a simple u32. We could simply remove this example from the examples that support WebGL2.

Testing

  • Ran the example locally on WebGL2 (and native Vulkan) successfully

@mockersf mockersf added this to the 0.16 milestone Apr 11, 2025
@JMS55 JMS55 added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Apr 13, 2025
#[uniform(100)]
quantize_steps: u32,
quantize_steps: UVec4,
Copy link
Contributor

Choose a reason for hiding this comment

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

As evidenced by other parts of the Bevy codebase, we prefer to use this type of pattern to address alignment issues:

_padding: bevy_math::Vec3,

@@ -16,7 +16,8 @@
#endif

struct MyExtendedMaterial {
quantize_steps: u32,
// WebGL2 structs must be 16 byte aligned. We only use the `x` field
quantize_steps: vec4<u32>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
quantize_steps: vec4<u32>,
quantize_steps: u32,
#ifdef SIXTEEN_BYTE_ALIGNMENT
_padding: vec3<u32>,
#endif

Copy link
Contributor

Choose a reason for hiding this comment

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

Some maintainers mentioned we should try to stay away from conditional compilation, if this is not the pattern we should be following anymore we should update the other examples to the simpler version too, or create a backlog task in the issues.

@Henauxg
Copy link
Contributor Author

Henauxg commented Apr 14, 2025

I'm fine with both approaches (the proposed one and the first alternative). And even the mix of both (adding a separate padding field with no conditional compilation).
If you look at all the existing the examples, it's mostly conditional padding fields with sometimes a vec3, sometimes 3 f32 fields and other variations.

As far as the examples go, my personal vote would go to a padding field without conditional compilation (in rust and WGSL) + a comment to mention the WebGL2 special requirement.

@mate-h
Copy link
Contributor

mate-h commented Apr 14, 2025

Thats a very good suggestion I like it! Add a separate padding field but no conditional compilation clutter.

@mockersf mockersf modified the milestones: 0.16, 0.16.1 Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extended material example fails in browser
4 participants