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.

Sign up for GitHub

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

Partially fix panics when setting WGPU_SETTINGS_PRIO=webgl2 #18113

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+32 −6
Open

Partially fix panics when setting WGPU_SETTINGS_PRIO=webgl2 #18113

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e4382fc
Added limit checks.
greeble-dev Mar 2, 2025
d89d39c
Clarified logic.
greeble-dev Mar 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/bevy_core_pipeline/src/experimental/mip_generation/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,7 @@ fn create_downsample_depth_pipelines(
.get_downlevel_capabilities()
.flags
.contains(DownlevelFlags::COMPUTE_SHADERS)
|| (render_device.limits().max_compute_workgroup_storage_size == 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is a little awkward - there's no straightforward "is compute available" on wgpu::Limits.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is bizarre. The default for WebGPU is fairly high https://gpuweb.github.io/gpuweb/#dom-supported-limits-maxcomputeworkgroupstoragesize. I didn't even know some platforms support compute shaders, but not workgroup-shared data.

@cwfitzgerald, can we get some input from wgpu/gpuweb people? Is this valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should have added a comment to clarify. I don't think this can ever happen on a real GPU. But the downlevel_webgl2_defaults() override simulates a lack of compute by setting all max_compute_* limits to zero. I arbitrarily picked one limit as a canary.

I'll see if I can make this cleaner or come up with an alternative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarified in d89d39c. Still kinda icky though.

I also considered working out some real limits for the mip generation, e.g. a minimum for max_compute_workgroup_size_x/y/z. But I'm not confident I can get that right.

Copy link

@cwfitzgerald cwfitzgerald Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't even know some platforms support compute shaders, but not workgroup-shared data.

You don't have to worry about this, compute shaders imply some amount of workgroup shared data.

downlevel_webgl2_defaults()

You should only need to check the COMPUTE_SHADERS downlevel flag to know if compute shaders are supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, checking COMPUTE_SHADERS on the adapter is not sufficient when the device limits have been overridden via DeviceDescriptor::limits (with the intent of simulating webgl2 limits on another platform).

If I rely only on COMPUTE_SHADERS:

$env:WGPU_SETTINGS_PRIO = "webgl2"
cargo run --example occlusion_culling

  In Device::create_bind_group_layout, label = 'downsample depth bind group layout'
    Too many bindings of type StorageTextures in Stage ShaderStages(COMPUTE), limit is 0, count was 12. Check the limit `max_storage_textures_per_shader_stage` passed to `Adapter::request_device`

I guess there's a deeper question on whether setting device limits is the right way to simulate another platform's limits?

{
return;
}
Expand Down
14 changes: 14 additions & 0 deletions crates/bevy_core_pipeline/src/oit/resolve/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ pub const OIT_RESOLVE_SHADER_HANDLE: Handle<Shader> =
/// Contains the render node used to run the resolve pass.
pub mod node;

/// Minimum required value of `wgpu::Limits::max_storage_buffers_per_shader_stage`.
pub const OIT_REQUIRED_STORAGE_BUFFERS: u32 = 2;

/// Plugin needed to resolve the Order Independent Transparency (OIT) buffer to the screen.
pub struct OitResolvePlugin;
impl Plugin for OitResolvePlugin {
Expand Down Expand Up @@ -61,6 +64,17 @@ impl Plugin for OitResolvePlugin {
return;
}

if render_app
.world()
.resource::<RenderDevice>()
.limits()
.max_storage_buffers_per_shader_stage
< OIT_REQUIRED_STORAGE_BUFFERS
{
warn!("OrderIndependentTransparencyPlugin not loaded. RenderDevice lacks support: max_storage_buffers_per_shader_stage < OIT_REQUIRED_STORAGE_BUFFERS.");
return;
}

render_app
.add_systems(
Render,
Expand Down
6 changes: 5 additions & 1 deletion crates/bevy_pbr/src/render/mesh_view_bindings.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
use alloc::sync::Arc;
use bevy_core_pipeline::{
core_3d::ViewTransmissionTexture,
oit::{OitBuffers, OrderIndependentTransparencySettings},
oit::{
resolve::OIT_REQUIRED_STORAGE_BUFFERS, OitBuffers, OrderIndependentTransparencySettings,
},
prepass::ViewPrepassTextures,
tonemapping::{
get_lut_bind_group_layout_entries, get_lut_bindings, Tonemapping, TonemappingLuts,
Expand Down Expand Up @@ -388,6 +390,8 @@ fn layout_entries(
.get_downlevel_capabilities()
.flags
.contains(DownlevelFlags::FRAGMENT_WRITABLE_STORAGE)
&& (render_device.limits().max_storage_buffers_per_shader_stage
>= OIT_REQUIRED_STORAGE_BUFFERS)
{
entries = entries.extend_with_indices((
// oit_layers
Expand Down