From 682565be01877a6f7dac771e87bdd1d2c21bdd4b Mon Sep 17 00:00:00 2001 From: Jakob Hellermann Date: Fri, 10 Dec 2021 16:33:07 +0100 Subject: [PATCH 1/4] validate shaders early before passing to wgpu --- crates/bevy_render/src/render_resource/shader.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/crates/bevy_render/src/render_resource/shader.rs b/crates/bevy_render/src/render_resource/shader.rs index 2e6f921e36f69..1c31ad5f7776e 100644 --- a/crates/bevy_render/src/render_resource/shader.rs +++ b/crates/bevy_render/src/render_resource/shader.rs @@ -156,7 +156,15 @@ impl ProcessedShader { Ok(ShaderModuleDescriptor { label: None, source: match self { - ProcessedShader::Wgsl(source) => ShaderSource::Wgsl(source.clone()), + ProcessedShader::Wgsl(source) => { + #[cfg(debug_assertions)] + // This isn't neccessary, but catches errors early during hot reloading of invalid wgsl shaders. + // Eventually, wgpu will have features that will make this unneccessary like compilation info + // or error scopes, but until then parsing the shader twice during development the easiest solution. + let _ = self.reflect()?; + + ShaderSource::Wgsl(source.clone()) + } ProcessedShader::Glsl(_source, _stage) => { let reflection = self.reflect()?; // TODO: it probably makes more sense to convert this to spirv, but as of writing From 05572c330ae964dd0065b0887b21f82b4bd627ea Mon Sep 17 00:00:00 2001 From: Jakob Hellermann Date: Fri, 10 Dec 2021 17:12:42 +0100 Subject: [PATCH 2/4] prettier messages for shader processing errors --- crates/bevy_render/Cargo.toml | 1 + .../src/render_resource/pipeline_cache.rs | 73 +++++++++++++++++-- 2 files changed, 66 insertions(+), 8 deletions(-) diff --git a/crates/bevy_render/Cargo.toml b/crates/bevy_render/Cargo.toml index 9b30e7c1abcdb..7ff840b52d814 100644 --- a/crates/bevy_render/Cargo.toml +++ b/crates/bevy_render/Cargo.toml @@ -37,6 +37,7 @@ image = { version = "0.23.12", default-features = false } # misc wgpu = { version = "0.12.0", features = ["spirv"] } +codespan-reporting = "0.11.0" naga = { version = "0.8.0", features = ["glsl-in", "spv-in", "spv-out", "wgsl-in", "wgsl-out"] } serde = { version = "1", features = ["derive"] } bitflags = "1.2.1" diff --git a/crates/bevy_render/src/render_resource/pipeline_cache.rs b/crates/bevy_render/src/render_resource/pipeline_cache.rs index c904ff74715d8..77f892b9aa902 100644 --- a/crates/bevy_render/src/render_resource/pipeline_cache.rs +++ b/crates/bevy_render/src/render_resource/pipeline_cache.rs @@ -2,7 +2,7 @@ use crate::{ render_resource::{ AsModuleDescriptorError, BindGroupLayout, BindGroupLayoutId, ProcessShaderError, RawFragmentState, RawRenderPipelineDescriptor, RawVertexState, RenderPipeline, - RenderPipelineDescriptor, Shader, ShaderImport, ShaderProcessor, + RenderPipelineDescriptor, Shader, ShaderImport, ShaderProcessor, ShaderReflectError, }, renderer::RenderDevice, RenderWorld, @@ -10,11 +10,13 @@ use crate::{ use bevy_app::EventReader; use bevy_asset::{AssetEvent, Assets, Handle}; use bevy_ecs::system::{Res, ResMut}; -use bevy_utils::{HashMap, HashSet}; +use bevy_utils::{tracing::error, HashMap, HashSet}; use std::{collections::hash_map::Entry, hash::Hash, ops::Deref, sync::Arc}; use thiserror::Error; use wgpu::{PipelineLayoutDescriptor, ShaderModule, VertexBufferLayout}; +use super::ProcessedShader; + #[derive(Default)] pub struct ShaderData { pipelines: HashSet, @@ -68,7 +70,12 @@ impl ShaderCache { &self.shaders, &self.import_path_shaders, )?; - let module_descriptor = processed.get_module_descriptor()?; + let module_descriptor = match processed.get_module_descriptor() { + Ok(module_descriptor) => module_descriptor, + Err(err) => { + return Err(RenderPipelineError::AsModuleDescriptorError(err, processed)); + } + }; entry.insert(Arc::new( render_device.create_shader_module(&module_descriptor), )) @@ -206,8 +213,8 @@ pub enum RenderPipelineError { ShaderNotLoaded(Handle), #[error(transparent)] ProcessShaderError(#[from] ProcessShaderError), - #[error(transparent)] - AsModuleDescriptorError(#[from] AsModuleDescriptorError), + #[error("{0}")] + AsModuleDescriptorError(AsModuleDescriptorError, ProcessedShader), #[error("Shader import not yet available.")] ShaderImportNotYetAvailable, } @@ -274,9 +281,13 @@ impl RenderPipelineCache { match err { RenderPipelineError::ShaderNotLoaded(_) | RenderPipelineError::ShaderImportNotYetAvailable => { /* retry */ } - RenderPipelineError::ProcessShaderError(_) - | RenderPipelineError::AsModuleDescriptorError(_) => { - // shader could not be processed ... retrying won't help + // shader could not be processed ... retrying won't help + RenderPipelineError::ProcessShaderError(err) => { + error!("failed to process shader: {}", err); + continue; + } + RenderPipelineError::AsModuleDescriptorError(err, source) => { + log_shader_error(source, err); continue; } } @@ -386,3 +397,49 @@ impl RenderPipelineCache { } } } + +fn log_shader_error(source: &ProcessedShader, err: &AsModuleDescriptorError) { + use codespan_reporting::{ + diagnostic::{Diagnostic, Label}, + files::SimpleFile, + term, + }; + + match source { + ProcessedShader::Wgsl(source) => match err { + AsModuleDescriptorError::ShaderReflectError(err) => match err { + ShaderReflectError::WgslParse(parse) => { + let msg = parse.emit_to_string(source); + error!("failed to process shader:\n{}", msg); + } + ShaderReflectError::Validation(error) => { + let files = SimpleFile::new("wgsl", &source); + let config = term::Config::default(); + let mut writer = term::termcolor::Ansi::new(Vec::new()); + + let diagnostic = Diagnostic::error().with_labels( + error + .spans() + .map(|(span, desc)| { + Label::primary((), span.to_range().unwrap()) + .with_message(desc.to_owned()) + }) + .collect(), + ); + + term::emit(&mut writer, &config, &files, &diagnostic) + .expect("cannot write error"); + + let msg = writer.into_inner(); + let msg = String::from_utf8_lossy(&msg); + + error!("failed to process shader: \n{}", msg); + } + ShaderReflectError::GlslParse(_) => {} + ShaderReflectError::SpirVParse(_) => {} + }, + _ => (), + }, + _ => error!("failed to process shader: {}", err), + } +} From 690271a3fd7bac10e6066e25a37656e6f332acbb Mon Sep 17 00:00:00 2001 From: Jakob Hellermann Date: Fri, 10 Dec 2021 17:45:10 +0100 Subject: [PATCH 3/4] show error on unresolved custom shader import --- .../bevy_render/src/render_resource/pipeline_cache.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/crates/bevy_render/src/render_resource/pipeline_cache.rs b/crates/bevy_render/src/render_resource/pipeline_cache.rs index 77f892b9aa902..4b3868da0f29c 100644 --- a/crates/bevy_render/src/render_resource/pipeline_cache.rs +++ b/crates/bevy_render/src/render_resource/pipeline_cache.rs @@ -54,7 +54,16 @@ impl ShaderCache { .get(handle) .ok_or_else(|| RenderPipelineError::ShaderNotLoaded(handle.clone_weak()))?; let data = self.data.entry(handle.clone_weak()).or_default(); - if shader.imports().len() != data.resolved_imports.len() { + let n_asset_imports = shader + .imports() + .filter(|import| matches!(import, ShaderImport::AssetPath(_))) + .count(); + let n_resolved_asset_imports = data + .resolved_imports + .keys() + .filter(|import| matches!(import, ShaderImport::AssetPath(_))) + .count(); + if n_asset_imports != n_resolved_asset_imports { return Err(RenderPipelineError::ShaderImportNotYetAvailable); } From 305ca70e4c89883c6f3fd8fe676b2017e23838ab Mon Sep 17 00:00:00 2001 From: Jakob Hellermann Date: Wed, 22 Dec 2021 22:13:24 +0100 Subject: [PATCH 4/4] fix clippy --- .../src/render_resource/pipeline_cache.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/crates/bevy_render/src/render_resource/pipeline_cache.rs b/crates/bevy_render/src/render_resource/pipeline_cache.rs index 4b3868da0f29c..a2690eacee3f1 100644 --- a/crates/bevy_render/src/render_resource/pipeline_cache.rs +++ b/crates/bevy_render/src/render_resource/pipeline_cache.rs @@ -414,9 +414,9 @@ fn log_shader_error(source: &ProcessedShader, err: &AsModuleDescriptorError) { term, }; - match source { - ProcessedShader::Wgsl(source) => match err { - AsModuleDescriptorError::ShaderReflectError(err) => match err { + if let ProcessedShader::Wgsl(source) = source { + if let AsModuleDescriptorError::ShaderReflectError(err) = err { + match err { ShaderReflectError::WgslParse(parse) => { let msg = parse.emit_to_string(source); error!("failed to process shader:\n{}", msg); @@ -446,9 +446,7 @@ fn log_shader_error(source: &ProcessedShader, err: &AsModuleDescriptorError) { } ShaderReflectError::GlslParse(_) => {} ShaderReflectError::SpirVParse(_) => {} - }, - _ => (), - }, - _ => error!("failed to process shader: {}", err), + } + } } }