-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
[Merged by Bors] - Fix wasm examples #4967
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
Changes from all commits
fa996cc
6a6fa72
325ba84
34e80c5
b1b2090
ddc433f
4fa8d87
014e819
3e5e4ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ keywords = ["bevy"] | |
|
||
[features] | ||
trace = [] | ||
webgl = [] | ||
|
||
[dependencies] | ||
# bevy | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,10 @@ use bevy_ecs::system::{Res, ResMut}; | |
use bevy_utils::{default, tracing::error, Entry, HashMap, HashSet}; | ||
use std::{hash::Hash, mem, ops::Deref, sync::Arc}; | ||
use thiserror::Error; | ||
use wgpu::{PipelineLayoutDescriptor, ShaderModule, VertexBufferLayout as RawVertexBufferLayout}; | ||
use wgpu::{ | ||
BufferBindingType, PipelineLayoutDescriptor, ShaderModule, | ||
VertexBufferLayout as RawVertexBufferLayout, | ||
}; | ||
|
||
enum PipelineDescriptor { | ||
RenderPipelineDescriptor(Box<RenderPipelineDescriptor>), | ||
|
@@ -117,9 +120,26 @@ impl ShaderCache { | |
let module = match data.processed_shaders.entry(shader_defs.to_vec()) { | ||
Entry::Occupied(entry) => entry.into_mut(), | ||
Entry::Vacant(entry) => { | ||
let mut shader_defs = shader_defs.to_vec(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious what @superdump thinks about this one. I think I'm on board for having a suite of "low level feature detection shader defs" that exist everywhere, but this is a big enough change to merit a discussion of its own. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah... I fixed a similar issue in #4949 and wanted to try a way that wouldn't need to fix it for other pipelines one by one There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think centralising these kinds of ‘global’ shader defs based on detected features/limits makes a lot of sense. They aren’t different in different renderers or pipelines and they are useful to be able to use without core changes / user code reimplementation, in my opinion. And while specialisation has different roots, the shader cache is centralised. Good idea. This is my initial 0730 after a night out take. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. best kind of takes! |
||
#[cfg(feature = "webgl")] | ||
shader_defs.push(String::from("NO_ARRAY_TEXTURES_SUPPORT")); | ||
|
||
// TODO: 3 is the value from CLUSTERED_FORWARD_STORAGE_BUFFER_COUNT declared in bevy_pbr | ||
// consider exposing this in shaders in a more generally useful way, such as: | ||
// # if AVAILABLE_STORAGE_BUFFER_BINDINGS == 3 | ||
// /* use storage buffers here */ | ||
// # elif | ||
// /* use uniforms here */ | ||
if !matches!( | ||
render_device.get_supported_read_only_binding_type(3), | ||
BufferBindingType::Storage { .. } | ||
) { | ||
shader_defs.push(String::from("NO_STORAGE_BUFFERS_SUPPORT")); | ||
} | ||
|
||
let processed = self.processor.process( | ||
shader, | ||
shader_defs, | ||
&shader_defs, | ||
&self.shaders, | ||
&self.import_path_shaders, | ||
)?; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its worth directly calling out the behavior quirks of WebGL explicitly, to better justify this code to future readers.
On that note, I'm still a little unclear about what the behavior actually is. Is it just "the next renderpass will still have the viewport set, but every pass after that will be fine"?
Should we open a wgpu bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what it looks like without that extra render pass

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried making the comment more explicit. I don't know if it's a wgpu bug or how WebGL2 works, but I found a lot of people trying to reset viewport when using OpenGL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Its a bit unfortunate because this means users developing things like custom post-processing effects will need to manually hard-code the reset if they want to support wasm viewports. But thats not something we can reasonably fix in this pr.