From 13e9a39eb7c26017fcc3568df6cdf393bfaba8f2 Mon Sep 17 00:00:00 2001 From: Arman Uguray Date: Tue, 4 Jun 2024 01:07:39 -0700 Subject: [PATCH] [shaders] More graceful error handling (#584) The transition of the vello crate to use the vello_shaders crate regressed developer ergonomics around compilation failures, in which compilation failures always caused a panic and the displayed debug-formatted error message wasn't pretty printed. The panic was particularly bad with hot-reload since it effectively undid its usefulness by crashing the whole process on shader misspellings. This change alleviates those regressions by adding better display formatting to WGSL parse errors returned from the shaders crate and propagating errors up to clients instead of always panicking. The Debug formatting is still a little wonky so call-sites are currently responsible for formatting errors with Display to get a more readable compiler message. This PR only updates `with_winit` with pretty-printing. --- examples/with_winit/src/lib.rs | 15 ++++-- vello/src/lib.rs | 17 ++++-- vello/src/shaders.rs | 10 ++-- vello_shaders/build.rs | 8 ++- vello_shaders/src/compile/mod.rs | 91 +++++++++++++++++++++++++++----- 5 files changed, 114 insertions(+), 27 deletions(-) diff --git a/examples/with_winit/src/lib.rs b/examples/with_winit/src/lib.rs index 7defd3cb7..00085be16 100644 --- a/examples/with_winit/src/lib.rs +++ b/examples/with_winit/src/lib.rs @@ -131,7 +131,12 @@ fn run( num_init_threads: NonZeroUsize::new(1), }, ) - .expect("Could create renderer"); + .map_err(|e| { + // Pretty-print any renderer creation error using Display formatting before unwrapping. + eprintln!("{e}"); + e + }) + .expect("Failed to create renderer"); #[cfg(feature = "wgpu-profiler")] renderer .profiler @@ -571,7 +576,7 @@ fn run( // We know that the only async here (`pop_error_scope`) is actually sync, so blocking is fine match pollster::block_on(result) { Ok(_) => log::info!("Reloading took {:?}", start.elapsed()), - Err(e) => log::warn!("Failed to reload shaders because of {e}"), + Err(e) => log::error!("Failed to reload shaders: {e}"), } } }, @@ -624,7 +629,11 @@ fn run( num_init_threads: NonZeroUsize::new(args.num_init_threads), }, ) - .expect("Could create renderer"); + .map_err(|e| { + // Pretty-print any renderer creation error using Display formatting before unwrapping. + anyhow::format_err!("{e}") + }) + .expect("Failed to create renderer"); log::info!("Creating renderer {id} took {:?}", start.elapsed()); #[cfg(feature = "wgpu-profiler")] renderer diff --git a/vello/src/lib.rs b/vello/src/lib.rs index 43bf435b2..7772a8ef9 100644 --- a/vello/src/lib.rs +++ b/vello/src/lib.rs @@ -209,11 +209,20 @@ pub enum Error { #[error("Failed to async map a buffer")] BufferAsyncError(#[from] wgpu::BufferAsyncError), + #[cfg(feature = "wgpu")] + #[error("wgpu Error from scope")] + WgpuErrorFromScope(#[from] wgpu::Error), + /// Failed to create [`GpuProfiler`]. /// See [`wgpu_profiler::CreationError`] for more information. #[cfg(feature = "wgpu-profiler")] #[error("Couldn't create wgpu profiler")] ProfilerCreationError(#[from] wgpu_profiler::CreationError), + + /// Failed to compile the shaders. + #[cfg(feature = "hot_reload")] + #[error("Failed to compile shaders:\n{0}")] + ShaderCompilation(#[from] vello_shaders::compile::ErrorVec), } #[allow(dead_code)] // this can be unused when wgpu feature is not used @@ -293,7 +302,7 @@ impl Renderer { #[cfg(not(target_arch = "wasm32"))] engine.use_parallel_initialisation(); } - let shaders = shaders::full_shaders(device, &mut engine, &options); + let shaders = shaders::full_shaders(device, &mut engine, &options)?; #[cfg(not(target_arch = "wasm32"))] engine.build_shaders_if_needed(device, options.num_init_threads); let blit = options @@ -435,14 +444,14 @@ impl Renderer { /// Reload the shaders. This should only be used during `vello` development #[cfg(feature = "hot_reload")] - pub async fn reload_shaders(&mut self, device: &Device) -> Result<(), wgpu::Error> { + pub async fn reload_shaders(&mut self, device: &Device) -> Result<(), Error> { device.push_error_scope(wgpu::ErrorFilter::Validation); let mut engine = WgpuEngine::new(self.options.use_cpu); // We choose not to initialise these shaders in parallel, to ensure the error scope works correctly - let shaders = shaders::full_shaders(device, &mut engine, &self.options); + let shaders = shaders::full_shaders(device, &mut engine, &self.options)?; let error = device.pop_error_scope().await; if let Some(error) = error { - return Err(error); + return Err(error.into()); } self.engine = engine; self.shaders = shaders; diff --git a/vello/src/shaders.rs b/vello/src/shaders.rs index abba984bc..06944f7f6 100644 --- a/vello/src/shaders.rs +++ b/vello/src/shaders.rs @@ -12,7 +12,7 @@ use crate::ShaderId; use crate::{ recording::{BindType, ImageFormat}, wgpu_engine::WgpuEngine, - RendererOptions, + Error, RendererOptions, }; // Shaders for the full pipeline @@ -49,7 +49,7 @@ pub fn full_shaders( device: &Device, engine: &mut WgpuEngine, options: &RendererOptions, -) -> FullShaders { +) -> Result { use crate::wgpu_engine::CpuShaderType; use BindType::*; @@ -60,7 +60,7 @@ pub fn full_shaders( //let force_gpu_from = Some("binning"); #[cfg(feature = "hot_reload")] - let mut shaders = vello_shaders::compile::ShaderInfo::from_default(); + let mut shaders = vello_shaders::compile::ShaderInfo::from_default()?; #[cfg(not(feature = "hot_reload"))] let shaders = vello_shaders::SHADERS; @@ -247,7 +247,7 @@ pub fn full_shaders( None }; - FullShaders { + Ok(FullShaders { pathtag_reduce, pathtag_reduce2, pathtag_scan, @@ -271,5 +271,5 @@ pub fn full_shaders( fine_msaa8, fine_msaa16, pathtag_is_cpu: options.use_cpu, - } + }) } diff --git a/vello_shaders/build.rs b/vello_shaders/build.rs index 4334aef77..6c762b86a 100644 --- a/vello_shaders/build.rs +++ b/vello_shaders/build.rs @@ -18,7 +18,13 @@ fn main() { println!("cargo:rerun-if-changed={}", compile::shader_dir().display()); - let mut shaders = compile::ShaderInfo::from_default(); + let mut shaders = match compile::ShaderInfo::from_default() { + Ok(s) => s, + Err(err) => { + eprintln!("{err}"); + return; + } + }; // Drop the HashMap and sort by name so that we get deterministic order. let mut shaders = shaders.drain().collect::>(); diff --git a/vello_shaders/src/compile/mod.rs b/vello_shaders/src/compile/mod.rs index ab0d9c9e3..ab11729b3 100644 --- a/vello_shaders/src/compile/mod.rs +++ b/vello_shaders/src/compile/mod.rs @@ -5,6 +5,7 @@ use naga::front::wgsl; use naga::valid::{Capabilities, ModuleInfo, ValidationError, ValidationFlags}; use naga::{AddressSpace, ArraySize, ImageClass, Module, StorageAccess, WithSpan}; use std::collections::{HashMap, HashSet}; +use std::fmt; use std::path::{Path, PathBuf}; use std::sync::OnceLock; use thiserror::Error; @@ -17,18 +18,62 @@ pub mod msl; use crate::types::{BindType, BindingInfo, WorkgroupBufferInfo}; +pub type Result = std::result::Result; +pub type CoalescedResult = std::result::Result; + +#[derive(Error, Debug)] +pub struct ErrorVec(Vec); + +#[derive(Error, Debug)] +#[error("{source} ({name}) {msg}")] +pub struct Error { + name: String, + msg: String, + source: InnerError, +} + #[derive(Error, Debug)] -pub enum Error { - #[error("failed to parse shader: {0}")] +enum InnerError { + #[error("failed to parse shader")] Parse(#[from] wgsl::ParseError), - #[error("failed to validate shader: {0}")] + #[error("failed to validate shader")] Validate(#[from] WithSpan), #[error("missing entry point function")] EntryPointNotFound, } +impl fmt::Display for ErrorVec { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + for e in self.0.iter() { + write!(f, "{e}")?; + } + Ok(()) + } +} + +impl Error { + fn new(wgsl: &str, name: &str, error: impl Into) -> Error { + let source = error.into(); + Error { + name: name.to_owned(), + msg: source.emit_msg(wgsl, &format!("({name} preprocessed)")), + source, + } + } +} + +impl InnerError { + fn emit_msg(&self, wgsl: &str, name: &str) -> String { + match self { + Self::Parse(e) => e.emit_to_string_with_path(wgsl, name), + Self::Validate(e) => e.emit_to_string_with_path(wgsl, name), + _ => String::default(), + } + } +} + #[derive(Debug)] pub struct ShaderInfo { pub source: String, @@ -40,16 +85,17 @@ pub struct ShaderInfo { } impl ShaderInfo { - pub fn new(source: String, entry_point: &str) -> Result { - let module = wgsl::parse_str(&source)?; + pub fn new(name: &str, source: String, entry_point: &str) -> Result { + let module = wgsl::parse_str(&source).map_err(|error| Error::new(&source, name, error))?; let module_info = naga::valid::Validator::new(ValidationFlags::all(), Capabilities::all()) - .validate(&module)?; + .validate(&module) + .map_err(|error| Error::new(&source, name, error))?; let (entry_index, entry) = module .entry_points .iter() .enumerate() .find(|(_, entry)| entry.name.as_str() == entry_point) - .ok_or(Error::EntryPointNotFound)?; + .ok_or(Error::new(&source, name, InnerError::EntryPointNotFound))?; let mut bindings = vec![]; let mut workgroup_buffers = vec![]; let mut wg_buffer_idx = 0; @@ -131,11 +177,11 @@ impl ShaderInfo { } /// Same as [`ShaderInfo::from_dir`] but uses the default shader directory provided by [`shader_dir`]. - pub fn from_default() -> HashMap { + pub fn from_default() -> CoalescedResult> { ShaderInfo::from_dir(shader_dir()) } - pub fn from_dir(shader_dir: impl AsRef) -> HashMap { + pub fn from_dir(shader_dir: impl AsRef) -> CoalescedResult> { use std::fs; let shader_dir = shader_dir.as_ref(); let permutation_map = if let Ok(permutations_source) = @@ -147,6 +193,7 @@ impl ShaderInfo { }; //println!("{permutation_map:?}"); let imports = preprocess::get_imports(shader_dir); + let mut errors = vec![]; let mut info = HashMap::default(); let defines: HashSet<_> = if cfg!(feature = "full") { vec!["full".to_string()] @@ -174,18 +221,34 @@ impl ShaderInfo { let mut defines = defines.clone(); defines.extend(permutation.defines.iter().cloned()); let source = preprocess::preprocess(&contents, &defines, &imports); - let shader_info = Self::new(source.clone(), "main").unwrap(); - info.insert(permutation.name.clone(), shader_info); + match Self::new(&permutation.name, source, "main") { + Ok(shader_info) => { + info.insert(permutation.name.clone(), shader_info); + } + Err(e) => { + errors.push(e); + } + } } } else { let source = preprocess::preprocess(&contents, &defines, &imports); - let shader_info = Self::new(source.clone(), "main").unwrap(); - info.insert(shader_name.to_string(), shader_info); + match Self::new(shader_name, source, "main") { + Ok(shader_info) => { + info.insert(shader_name.to_string(), shader_info); + } + Err(e) => { + errors.push(e); + } + } } } } } - info + if !errors.is_empty() { + Err(ErrorVec(errors)) + } else { + Ok(info) + } } }