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

Fix the WebGL 2 backend by giving the visibility_ranges array a fixed length. #13210

Merged
merged 6 commits into from
May 8, 2024

Conversation

pcwalton
Copy link
Contributor

@pcwalton pcwalton commented May 3, 2024

WebGL 2 doesn't support variable-length uniform buffer arrays. So we arbitrarily set the length of the visibility ranges field to 64 on that platform.

length.

WebGL 2 doesn't support variable-length uniform buffer arrays. Since we
never loop over the array in the shader, we don't actually care what the
size is, so this is harmless.
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

Added a suggestion with a comment to explain what you said in the description

@@ -38,7 +38,7 @@
#if AVAILABLE_STORAGE_BUFFER_BINDINGS >= 6
@group(0) @binding(12) var<storage> visibility_ranges: array<vec4<f32>>;
#else
@group(0) @binding(12) var<uniform> visibility_ranges: array<vec4<f32>>;
@group(0) @binding(12) var<uniform> visibility_ranges: array<vec4<f32>, 1024u>;
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
@group(0) @binding(12) var<uniform> visibility_ranges: array<vec4<f32>, 1024u>;
// In WebGL2 we never loop over this array so the size number is not important. We only set it to something so it compiles correctly
@group(0) @binding(12) var<uniform> visibility_ranges: array<vec4<f32>, 1024u>;

@alice-i-cecile alice-i-cecile added this to the 0.14 milestone May 3, 2024
@IceSentry IceSentry added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels May 3, 2024
@IceSentry
Copy link
Contributor

IceSentry commented May 3, 2024

I just tested it and it gives me a new error. The error seems to be that arrayLength doesn't work with fixed size uniform buffer array. That's really annoying but a way to avoid this error seems to be to use this ugly hack inside get_visibility_range_dither_level():

    #if AVAILABLE_STORAGE_BUFFER_BINDINGS >= 6
    if (visibility_buffer_index > arrayLength(&visibility_ranges)) {
        return -16;
    }
    #else
    // In WebGL2 we use a uniform buffer array with a fixed size and this doesn't work with arrayLength()
    if (visibility_buffer_index > 1024u) {
        return -16;
    }
    #endif

It gets rid of the error but no meshes are visible. I'm not sure if that's expected.

@kristoff3r
Copy link
Contributor

I just tested it and it gives me a new error. The error seems to be that arrayLength doesn't work with fixed size uniforme buffer array. That's really annoying but a way to avoid this error seems to be to use this ugly hack inside get_visibility_range_dither_level():

    #if AVAILABLE_STORAGE_BUFFER_BINDINGS >= 6
    if (visibility_buffer_index > arrayLength(&visibility_ranges)) {
        return -16;
    }
    #else
    // In WebGL2 we use a uniform buffer array with a fixed size and this doesn't work with arrayLength()
    if (visibility_buffer_index > 1024u) {
        return -16;
    }
    #endif

It gets rid of the error but no meshes are visible. I'm not sure if that's expected.

Weird, on this PR I don't get this error on the 3d_scene example where I encountered the original error, it just seems to work. Which example are you running?

@IceSentry
Copy link
Contributor

Ah, yeah, that's when running the visibility_range example. I should have clarified. It still shows the shadows but not the mesh.

@IceSentry
Copy link
Contributor

I don't get any error in 3d_scene in webgl2 on main personally. So I didn't realize the error was in another example.

Copy link
Contributor

@kristoff3r kristoff3r left a comment

Choose a reason for hiding this comment

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

I don't see the errors that IceSentry is seeing, which is weird, but this definitely fixes it for me.

@pcwalton
Copy link
Contributor Author

pcwalton commented May 4, 2024

OK, I was wrong here: WebGL 2 actually does require the size to be exact if you end up using the UBO. So I set the size arbitrarily to 64. This fixes all the issues for me.

Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

This resolved all the issues for me too.

I'll make a PR to your branch with this suggestion. It's possible to use a const instead of the hard coded value so we don't have to change it in 2 places. It's not a blocker though. I'd rather get this merged quickly to fix the error. We could even use shader_defs for that I think, but this would require a few more changes because that would be the first case of shader_defs that only exist for the mesh_view_bindings. So definitely out of scope here.

@pcwalton
Copy link
Contributor Author

pcwalton commented May 4, 2024

Sure, that sounds good. I merged your PR.

@IceSentry IceSentry added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 4, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 8, 2024
Merged via the queue into bevyengine:main with commit 0dddfa0 May 8, 2024
28 checks passed
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-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants