[Merged by Bors] - make pbr shader std140 compatible

[mockersf]

In shaders, vec3 should be avoided for std140 layout, as they take the size of a vec4 and won't support manual padding by adding an additional float.

This change is needed for 3D to work in WebGL2. With it, I get PBR to render

Without it, nothing renders... @cart Could this be considered for 0.5 release?

Also, I learned shaders 2 days ago, so don't hesitate to correct any issue or misunderstanding I may have

bevy_webgl2 PR in progress for Bevy 0.5 is here if you want to test: https://github.com/rparrett/bevy_webgl2/pull/1

[mrk-its]

It seems this change may be reverted (memory layout is the same). I've tested that in bevy_webgl2 and it seems to work.

[mockersf]

yes, in std140 memory layout is the same between vec3 and vec4. That's why, from what I read, vec3 should be avoided to not make mistake when reading the code to think that the layout is different

[mrk-its]

It seems adding std140 only is sufficient:

layout(std140, set = 0, binding = 1) uniform CameraPosition {
    vec3 CameraPos;
}

[mrk-its]

the same

[cart]

I think im in agreement with the idea to just avoid vec3 in this context. Better to use the least surprising type.

[cart]

(we've hit this exact issue in the past)

[cart]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-android
• build (nightly, ubuntu-latest)
• build (stable, macos-latest)
• build (stable, ubuntu-latest)
• build (stable, windows-latest)
• build-wasm (nightly, ubuntu-latest)
• build-wasm (stable, ubuntu-latest)
• markdownlint

[mtsr]

Avoiding vec3 is simply not necessary. Using std140 correctly on both sides, GPU and CPU, solves this problem and allows Vec3's to just be vec3's, etc.

[cart]

Hmm yeah is this a fair assessment of the situation?

It seems like we have three options:

1. Use vec4 in place of all vec3s without including std140 (in the interest of avoiding special annotations)
2. Use vec3s, but always include std140 so we don't need to care on the cpu side?
3. Use vec3s, don't include std140, and account for the mismatch on the cpu side with padding?

None of these seem fun 😄

[mtsr]

I have been working on a crevice PR in #1931.

The biggest problem I ran into is the orphan rule and having to wrap any external types that need crevice traits (or doing PRs, but that's not going to fly for many things).

I also spent a few hours yesterday looking into the possibilities of using serde for Std140 because of:

• everybody already implements Serialize for their types
• serde attributes are a pretty powerful way of customizing
• it's zero-copy

On the other hand, it's not a dedicated solution and alignment of arrays and structs in particular is a bit of a challenge with how serde passes struct fields to the serializer in order, whereas the first element (of an array or struct) needs to be aligned according to the contents of the array or struct. In the case of arrays we have the information we need when the first element is passed, but for structs we might need to make a copy, because we need the largest alignment of any member.

[mtsr]

I agree that this needed merging, to fix the current issue. But if we're going to do manual padding anyway, it doesn't matter at all whether we avoid vec3 or not, as long as the padding is correct and the uniforms are marked std140 (which they should anyway, because otherwise the API is free to reorder fields).

Instead of arbitrarily avoiding vec3, and there's actually a few more rules, we should implement a general solution that does not require users to be aware of these details.

In my ideal world, I think we track uniform types for all compiled shaders, so that we can:

• Generate buffers matching the uniform;
• Signaling when uniforms with the same name in different shaders don't match;
• Figure out a way of correct serialization of structs to the created buffers.

Since we really need shader includes anyway, as our shaders grow, it actually makes sense to make this part of the solution. Instead of specifying uniforms once in rust and once in glsl, we should really have a single definition that is reusable on the other side. Whether that definition is rust and glsl is generated or the other way around.

[cart]

as long as the padding is correct and the uniforms are marked std140 (which they should anyway, because otherwise the API is free to reorder fields).

I really don't like the extra boilerplate of requiring std140 everywhere. Its one more thing to remember (and users likely won't include it). Could you elaborate a bit on (or link to) the implications of the "the api is free to reorder fields"?

If we really need to include std140 everywhere to ensure predictable behavior, we should look into ways to make this automatic and implicit (ex: flip a bit in the shader compiler or add a post-processing step). Alternatively, we could look into the WGSL memory layout situation and consider moving to that if it is more reasonable.

Since we really need shader includes anyway, as our shaders grow, it actually makes sense to make this part of the solution. Instead of specifying uniforms once in rust and once in glsl, we should really have a single definition that is reusable on the other side. Whether that definition is rust and glsl is generated or the other way around.

I'm not a huge fan of codegen here as it complicates the build process and introduces its own complexity (in terms of importing the generated items). I would personally prefer a macro that handles proper memory layout and solid validation layers that yell when something doesn't line up.

However there is some prior art here. Rafx has optional codegen: https://github.com/aclysma/rafx/blob/master/docs/shaders/shader_processor.md.

[mtsr]

I really don't like the extra boilerplate of requiring std140 everywhere. Its one more thing to remember (and users likely won't include it). Could you elaborate a bit on (or link to) the implications of the "the api is free to reorder fields"?

This is because, technically, you have to query the API for the field offsets of the uniform. By specifying std140 you force it to adhere to a single layout. This is also likely why issues showed up for webgl2 and DX12 that didn't for vulkan. Read https://www.khronos.org/opengl/wiki/Interface_Block_(GLSL)#Memory_layout and weep.

If we really need to include std140 everywhere to ensure predictable behavior, we should look into ways to make this automatic and implicit (ex: flip a bit in the shader compiler or add a post-processing step). Alternatively, we could look into the WGSL memory layout situation and consider moving to that if it is more reasonable.

As of last June, there wasn't agreement on a single layout, see https://github.com/gfx-rs/wgpu-rs/issues/349#issuecomment-638873319. Maybe this has changed, but I can't find reference to it, so we should probably ask Groves or kvark.

I'm not a huge fan of codegen here as it complicates the build process and introduces its own complexity (in terms of importing the generated items). I would personally prefer a macro that handles proper memory layout and solid validation layers that yell when something doesn't line up.

That works, I mean, one can actually query the offsets from the API.

But what I mostly mean is we need to make this easier on users. Because there's going to be way more issues once we start having multiple shaders specifying the same uniforms. Because, as we've seen, there's 0 useful errors from them being wrong. Which is why something like filament uses includes (of generated uniforms, as well, but that's not needed) to be sure of uniformity (pun intended).