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

Work around naga/wgpu WGSL instance_index -> GLSL gl_InstanceID bug on WebGL2 #9383

Merged
merged 3 commits into from
Aug 9, 2023

Conversation

superdump
Copy link
Contributor

@superdump superdump commented Aug 8, 2023

naga and wgpu should polyfill WGSL instance_index functionality where it is not available in GLSL. Until that is done, we can work around it in bevy using a push constant which is converted to a uniform by naga and wgpu.

Objective

Solution

  • Use a push constant to pass in the base instance to the shader on WebGL2 so that base instance + gl_InstanceID is used to correctly represent the instance index.

TODO

  • Benchmark vs per-object dynamic offset MeshUniform as this will now push a uniform value per-draw as well as update the dynamic offset per-batch.
  • Test on DX12 AMD/NVIDIA to check that this PR does not regress any problems that were observed there. (@Elabajaba @robtfm were testing that last time - help appreciated. <3 )

Changelog

  • Added: bevy_render::instance_index shader import which includes a workaround for the lack of a WGSL instance_index polyfill for WebGL2 in naga and wgpu for the time being. It uses a push_constant which gets converted to a plain uniform by naga and wgpu.

Migration Guide

Shader code before:

struct Vertex {
    @builtin(instance_index) instance_index: u32,
...
}

@vertex
fn vertex(vertex_no_morph: Vertex) -> VertexOutput {
...

    var model = mesh[vertex_no_morph.instance_index].model;

After:

#import bevy_render::instance_index

struct Vertex {
    @builtin(instance_index) instance_index: u32,
...
}

@vertex
fn vertex(vertex_no_morph: Vertex) -> VertexOutput {
...

    var model = mesh[bevy_render::instance_index::get_instance_index(vertex_no_morph.instance_index)].model;

…n WebGL2

naga and wgpu should polyfill WGSL instance_index functionality where it is not
available in GLSL. Until that is done, we can work around it in bevy using a
push constant which is converted to a uniform by naga and wgpu.
@superdump superdump requested a review from robtfm August 9, 2023 10:47
Copy link
Contributor

@robtfm robtfm left a comment

Choose a reason for hiding this comment

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

code looks good to me.

works on win/nvidia with vulkan and dx12. i always forget how to best test wasm...

Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

OK for me on metal, webgl2 and webgpu

@superdump
Copy link
Contributor Author

@robtfm I use the wasm-server-runner approach from https://bevy-cheatbook.github.io/platforms/wasm.html to test wasm builds

@superdump superdump added this pull request to the merge queue Aug 9, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 9, 2023
@superdump superdump added this pull request to the merge queue Aug 9, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 9, 2023
@mockersf
Copy link
Member

mockersf commented Aug 9, 2023

it has this error with webgl2 in CI:

ERROR    Handling wgpu errors as fatal by default log.target = "wgpu::backend::direct";
log.module_path = "wgpu::backend::direct";
log.file = "/home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wgpu-0.16.3/src/backend/direct.rs";
log.line = 3018;
panicked at 'wgpu error: Validation Error

Caused by:
    In a RenderPass
      note: encoder = `<CommandBuffer-(0, 1, Gl)>`
    In a set_push_constant command
    Provided push constant is for stage(s) ShaderStages(VERTEX), however the pipeline layout has no push constant range for the stage(s) ShaderStages(VERTEX)

@superdump
Copy link
Contributor Author

it has this error with webgl2 in CI:

ERROR    Handling wgpu errors as fatal by default log.target = "wgpu::backend::direct";
log.module_path = "wgpu::backend::direct";
log.file = "/home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wgpu-0.16.3/src/backend/direct.rs";
log.line = 3018;
panicked at 'wgpu error: Validation Error

Caused by:
    In a RenderPass
      note: encoder = `<CommandBuffer-(0, 1, Gl)>`
    In a set_push_constant command
    Provided push constant is for stage(s) ShaderStages(VERTEX), however the pipeline layout has no push constant range for the stage(s) ShaderStages(VERTEX)

I can't reproduce. Looks like wasm32 + webgl and both the pipeline descriptor and draw parts are gated by those cfg. I did find that prepass was missing the change.

@superdump superdump added this pull request to the merge queue Aug 9, 2023
Merged via the queue into bevyengine:main with commit c1a5428 Aug 9, 2023
@IceSentry IceSentry added the A-Rendering Drawing game state to the screen label Oct 29, 2023
github-merge-queue bot pushed a commit that referenced this pull request Dec 13, 2023
# Objective

A workaround for a webgl issue was introduced in #9383 but one function
for mesh2d was missed.

## Solution

Applied the migration guide from #9383 in
`mesh2d_normal_local_to_world()

Note: I'm not using normals so I have not tested the bug & fix
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WebGL2 rendering broken: correct transform are not used
4 participants