From 672715c6685e6745f7f2e889d05a1beccf3dc4c2 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 10 Jan 2025 17:08:10 +0100 Subject: [PATCH 1/4] New test utlity for failing in warn & error log messages --- crates/utils/re_log/src/lib.rs | 2 +- crates/utils/re_log/src/setup.rs | 57 +++++++++++++++++++++++++++----- 2 files changed, 50 insertions(+), 9 deletions(-) diff --git a/crates/utils/re_log/src/lib.rs b/crates/utils/re_log/src/lib.rs index debe25a13802..c110ca1477d3 100644 --- a/crates/utils/re_log/src/lib.rs +++ b/crates/utils/re_log/src/lib.rs @@ -42,7 +42,7 @@ pub use channel_logger::*; pub use multi_logger::{add_boxed_logger, add_logger, MultiLoggerNotSetupError}; #[cfg(feature = "setup")] -pub use setup::setup_logging; +pub use setup::{setup_logging, PanicOnWarnScope}; /// Re-exports of other crates. pub mod external { diff --git a/crates/utils/re_log/src/setup.rs b/crates/utils/re_log/src/setup.rs index 59b7e44e4a14..58665fbb74da 100644 --- a/crates/utils/re_log/src/setup.rs +++ b/crates/utils/re_log/src/setup.rs @@ -1,5 +1,7 @@ //! Function to setup logging in binaries and web apps. +use std::sync::atomic::AtomicIsize; + /// Automatically does the right thing depending on target environment (native vs. web). /// /// Directs [`log`] calls to stderr on native. @@ -50,11 +52,12 @@ pub fn setup_logging() { stderr_logger.parse_filters(&log_filter); crate::add_boxed_logger(Box::new(stderr_logger.build())).expect("Failed to install logger"); + crate::add_logger(&PANIC_ON_WARN_LOGGER).expect("Failed to install panic-on-warn logger"); if env_var_bool("RERUN_PANIC_ON_WARN") == Some(true) { - crate::add_boxed_logger(Box::new(PanicOnWarn {})) - .expect("Failed to enable RERUN_PANIC_ON_WARN"); - crate::info!("RERUN_PANIC_ON_WARN: any warning or error will cause Rerun to panic."); + PANIC_ON_WARN_LOGGER + .enabled + .store(1, std::sync::atomic::Ordering::Relaxed); } } @@ -75,7 +78,40 @@ pub fn setup_logging() { // ---------------------------------------------------------------------------- -#[cfg(not(target_arch = "wasm32"))] +static PANIC_ON_WARN_LOGGER: PanicOnWarn = PanicOnWarn { + enabled: AtomicIsize::new(0), +}; + +/// Scope for enabling panic on warn/error log messages temporariliy. +/// +/// Use this in tests to ensure that there's no errors & warnings. +pub struct PanicOnWarnScope { + _need_to_use_new: (), +} + +impl PanicOnWarnScope { + /// Enable panic on warn & error log messages for as long as this scope is alive. + /// + /// For multiple threads it's unspecified when other threads will see the change in setting. + #[expect(clippy::new_without_default)] + pub fn new() -> Self { + PANIC_ON_WARN_LOGGER + .enabled + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); + Self { + _need_to_use_new: (), + } + } +} + +impl Drop for PanicOnWarnScope { + fn drop(&mut self) { + PANIC_ON_WARN_LOGGER + .enabled + .fetch_sub(1, std::sync::atomic::Ordering::Relaxed); + } +} + fn env_var_bool(name: &str) -> Option { std::env::var(name).ok() .and_then(|s| match s.to_lowercase().as_str() { @@ -90,14 +126,19 @@ fn env_var_bool(name: &str) -> Option { }) } -#[cfg(not(target_arch = "wasm32"))] -struct PanicOnWarn {} +struct PanicOnWarn { + /// The number of times `enabled` has been set to true. + /// + /// This is so to make `PanicOnWarnScope` trivial to implement. + enabled: AtomicIsize, +} -#[cfg(not(target_arch = "wasm32"))] impl log::Log for PanicOnWarn { fn enabled(&self, metadata: &log::Metadata<'_>) -> bool { match metadata.level() { - log::Level::Error | log::Level::Warn => true, + log::Level::Error | log::Level::Warn => { + self.enabled.load(std::sync::atomic::Ordering::Relaxed) > 0 + } log::Level::Info | log::Level::Debug | log::Level::Trace => false, } } From f0622cd4a698d652b50a7467f00a6a836a648f25 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 10 Jan 2025 17:10:04 +0100 Subject: [PATCH 2/4] re_renderer test utilities --- Cargo.lock | 1 + crates/viewer/re_renderer/Cargo.toml | 2 + crates/viewer/re_renderer/src/config.rs | 68 +++++++++++++++++++ crates/viewer/re_renderer/src/context_test.rs | 46 +++++++++++++ crates/viewer/re_renderer/src/lib.rs | 3 + .../re_viewer_context/src/test_context.rs | 63 +---------------- 6 files changed, 122 insertions(+), 61 deletions(-) create mode 100644 crates/viewer/re_renderer/src/context_test.rs diff --git a/Cargo.lock b/Cargo.lock index e946a50a1935..509b8a8c159d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6265,6 +6265,7 @@ dependencies = [ "ordered-float", "parking_lot", "pathdiff", + "pollster 0.4.0", "profiling", "re_arrow2", "re_build_tools", diff --git a/crates/viewer/re_renderer/Cargo.toml b/crates/viewer/re_renderer/Cargo.toml index da706aa443e7..7f13833c7c1b 100644 --- a/crates/viewer/re_renderer/Cargo.toml +++ b/crates/viewer/re_renderer/Cargo.toml @@ -106,6 +106,8 @@ wasm-bindgen.workspace = true [dev-dependencies] +re_log = { workspace = true, features = ["setup"] } +pollster.workspace = true unindent.workspace = true # For build.rs: diff --git a/crates/viewer/re_renderer/src/config.rs b/crates/viewer/re_renderer/src/config.rs index 93a5b24f9d4f..bfac64245ac0 100644 --- a/crates/viewer/re_renderer/src/config.rs +++ b/crates/viewer/re_renderer/src/config.rs @@ -325,6 +325,9 @@ impl DeviceCaps { } } +/// Returns an instance descriptor with settings preferred by `re_renderer`. +/// +/// `re_renderer` should work fine with any instance descriptor, but those are the settings we generally assume. pub fn instance_descriptor(force_backend: Option<&str>) -> wgpu::InstanceDescriptor { let backends = if let Some(force_backend) = force_backend { if let Some(backend) = parse_graphics_backend(force_backend) { @@ -366,6 +369,71 @@ pub fn instance_descriptor(force_backend: Option<&str>) -> wgpu::InstanceDescrip } } +/// Returns an instance descriptor that is suitable for testing. +pub fn testing_instance_descriptor() -> wgpu::InstanceDescriptor { + // We don't test on GL & DX12 right now (and don't want to do so by mistake!). + // Several reasons for this: + // * our CI is setup to draw with native Mac & lavapipe + // * we generally prefer Vulkan over DX12 on Windows since it reduces the + // number of backends and wgpu's DX12 backend isn't as far along as of writing. + // * we don't want to use the GL backend here since we regard it as a fallback only + // (TODO(andreas): Ideally we'd test that as well to check it is well-behaved, + // but for now we only want to have a look at the happy path) + let backends = wgpu::Backends::VULKAN | wgpu::Backends::METAL; + + let flags = ( + wgpu::InstanceFlags::ALLOW_UNDERLYING_NONCOMPLIANT_ADAPTER | wgpu::InstanceFlags::VALIDATION + // TODO(andreas): GPU based validation layer sounds like a great idea, + // but as of writing this makes tests crash on my Windows machine! + // It looks like the crash is in the Vulkan/Nvidia driver, but further investigation is needed. + // | wgpu::InstanceFlags::GPU_BASED_VALIDATION + ) + .with_env(); // Allow overwriting flags via env vars. + + wgpu::InstanceDescriptor { + backends, + flags, + ..instance_descriptor(None) + } +} + +/// Selects an adapter for testing, preferring software rendering if available. +/// +/// Panics if no adapter was found. +pub fn select_testing_adapter(instance: &wgpu::Instance) -> wgpu::Adapter { + let mut adapters = instance.enumerate_adapters(wgpu::Backends::all()); + assert!(!adapters.is_empty(), "No graphics adapter found!"); + + re_log::debug!("Found the following adapters:"); + for adapter in &adapters { + re_log::debug!("* {}", crate::adapter_info_summary(&adapter.get_info())); + } + + // Adapters are already sorted by preferred backend by wgpu, but let's be explicit. + adapters.sort_by_key(|a| match a.get_info().backend { + wgpu::Backend::Metal => 0, + wgpu::Backend::Vulkan => 1, + wgpu::Backend::Dx12 => 2, + wgpu::Backend::Gl => 4, + wgpu::Backend::BrowserWebGpu => 6, + wgpu::Backend::Empty => 7, + }); + + // Prefer CPU adapters, otherwise if we can't, prefer discrete GPU over integrated GPU. + adapters.sort_by_key(|a| match a.get_info().device_type { + wgpu::DeviceType::Cpu => 0, // CPU is the best for our purposes! + wgpu::DeviceType::DiscreteGpu => 1, + wgpu::DeviceType::Other + | wgpu::DeviceType::IntegratedGpu + | wgpu::DeviceType::VirtualGpu => 2, + }); + + let adapter = adapters.remove(0); + re_log::info!("Picked adapter: {:?}", adapter.get_info()); + + adapter +} + /// Backends that are officially supported by `re_renderer`. /// /// Other backend might work as well, but lack of support isn't regarded as a bug. diff --git a/crates/viewer/re_renderer/src/context_test.rs b/crates/viewer/re_renderer/src/context_test.rs new file mode 100644 index 000000000000..3805072a66d9 --- /dev/null +++ b/crates/viewer/re_renderer/src/context_test.rs @@ -0,0 +1,46 @@ +//! Extensions for the [`RenderContext`] for testing. + +use std::sync::Arc; + +use crate::{config, RenderContext}; + +impl RenderContext { + /// Creates a new [`RenderContext`] for testing. + pub fn new_test() -> Self { + let instance = wgpu::Instance::new(config::testing_instance_descriptor()); + let adapter = config::select_testing_adapter(&instance); + let device_caps = config::DeviceCaps::from_adapter(&adapter) + .expect("Failed to determine device capabilities"); + let (device, queue) = + pollster::block_on(adapter.request_device(&device_caps.device_descriptor(), None)) + .expect("Failed to request device."); + + Self::new( + &adapter, + Arc::new(device), + Arc::new(queue), + wgpu::TextureFormat::Rgba8Unorm, + ) + .expect("Failed to create RenderContext") + } + + /// Executes a test frame. + /// + /// Note that this "executes" a frame in thus far that it doesn't necessarily draw anything, + /// depending on what the callback does. + pub fn execute_test_frame(&mut self, create_gpu_work: impl FnOnce(&mut Self) -> I) + where + I: IntoIterator, + { + self.begin_frame(); + let command_buffers = create_gpu_work(self); + self.before_submit(); + self.queue.submit(command_buffers); + + // Wait for all GPU work to finish. + self.device.poll(wgpu::Maintain::Wait); + + // Start a new frame in order to handle the previous' frame errors. + self.begin_frame(); + } +} diff --git a/crates/viewer/re_renderer/src/lib.rs b/crates/viewer/re_renderer/src/lib.rs index 72fdade72dd1..3864ff138a0c 100644 --- a/crates/viewer/re_renderer/src/lib.rs +++ b/crates/viewer/re_renderer/src/lib.rs @@ -40,6 +40,9 @@ mod transform; mod wgpu_buffer_types; mod wgpu_resources; +#[cfg(test)] +mod context_test; + #[cfg(not(load_shaders_from_disk))] #[rustfmt::skip] // it's auto-generated mod workspace_shaders; diff --git a/crates/viewer/re_viewer_context/src/test_context.rs b/crates/viewer/re_viewer_context/src/test_context.rs index 427e84733659..a33f319dbd5c 100644 --- a/crates/viewer/re_viewer_context/src/test_context.rs +++ b/crates/viewer/re_viewer_context/src/test_context.rs @@ -162,67 +162,8 @@ static SHARED_WGPU_RENDERER_SETUP: Lazy = Lazy::new(init_shared_renderer_setup); fn init_shared_renderer_setup() -> SharedWgpuResources { - // TODO(andreas, emilk/egui#5506): Use centralized wgpu setup logic that… - // * lives mostly in re_renderer and is shared with viewer & renderer examples - // * can be told to prefer software rendering - // * can be told to match a specific device tier - - // We rely a lot on logging in the viewer to identify issues. - // Make sure logging is set up if it hasn't been done yet. - //let _ = env_logger::builder().is_test(true).try_init(); - let _ = env_logger::builder() - .filter_level(re_log::external::log::LevelFilter::Trace) - .is_test(false) - .try_init(); - - // We don't test on GL & DX12 right now (and don't want to do so by mistake!). - // Several reasons for this: - // * our CI is setup to draw with native Mac & lavapipe - // * we generally prefer Vulkan over DX12 on Windows since it reduces the - // number of backends and wgpu's DX12 backend isn't as far along as of writing. - // * we don't want to use the GL backend here since we regard it as a fallback only - // (TODO(andreas): Ideally we'd test that as well to check it is well-behaved, - // but for now we only want to have a look at the happy path) - let backends = wgpu::Backends::VULKAN | wgpu::Backends::METAL; - let flags = (wgpu::InstanceFlags::ALLOW_UNDERLYING_NONCOMPLIANT_ADAPTER - | wgpu::InstanceFlags::VALIDATION - | wgpu::InstanceFlags::GPU_BASED_VALIDATION) - .with_env(); // Allow overwriting flags via env vars. - let instance = wgpu::Instance::new(wgpu::InstanceDescriptor { - backends, - flags, - ..Default::default() - }); - - let mut adapters = instance.enumerate_adapters(backends); - assert!(!adapters.is_empty(), "No graphics adapter found!"); - re_log::info!("Found the following adapters:"); - for adapter in &adapters { - re_log::info!("* {}", egui_wgpu::adapter_info_summary(&adapter.get_info())); - } - - // Adapters are already sorted by preferred backend by wgpu, but let's be explicit. - adapters.sort_by_key(|a| match a.get_info().backend { - wgpu::Backend::Metal => 0, - wgpu::Backend::Vulkan => 1, - wgpu::Backend::Dx12 => 2, - wgpu::Backend::Gl => 4, - wgpu::Backend::BrowserWebGpu => 6, - wgpu::Backend::Empty => 7, - }); - - // Prefer CPU adapters, otherwise if we can't, prefer discrete GPU over integrated GPU. - adapters.sort_by_key(|a| match a.get_info().device_type { - wgpu::DeviceType::Cpu => 0, // CPU is the best for our purposes! - wgpu::DeviceType::DiscreteGpu => 1, - wgpu::DeviceType::Other - | wgpu::DeviceType::IntegratedGpu - | wgpu::DeviceType::VirtualGpu => 2, - }); - - let adapter = adapters.remove(0); - re_log::info!("Picked adapter: {:?}", adapter.get_info()); - + let instance = wgpu::Instance::new(re_renderer::config::testing_instance_descriptor()); + let adapter = re_renderer::config::select_testing_adapter(&instance); let device_caps = re_renderer::config::DeviceCaps::from_adapter(&adapter) .expect("Failed to determine device capabilities"); let (device, queue) = From 99526be413976e277d971f407dbb7968d447a19d Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 10 Jan 2025 17:10:27 +0100 Subject: [PATCH 3/4] fix lines not handling empty batches correctly, add regression test --- .../viewer/re_renderer/src/renderer/lines.rs | 44 ++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/crates/viewer/re_renderer/src/renderer/lines.rs b/crates/viewer/re_renderer/src/renderer/lines.rs index c10fec15aa32..e2bb04e71b86 100644 --- a/crates/viewer/re_renderer/src/renderer/lines.rs +++ b/crates/viewer/re_renderer/src/renderer/lines.rs @@ -343,7 +343,7 @@ impl LineDrawData { let line_renderer = ctx.renderer::(); - if strips_buffer.is_empty() { + if strips_buffer.is_empty() || vertices_buffer.is_empty() { return Ok(Self { bind_group_all_lines: None, bind_group_all_lines_outline_mask: None, @@ -762,3 +762,45 @@ impl Renderer for LineRenderer { Ok(()) } } + +#[cfg(test)] +mod tests { + use crate::{view_builder::TargetConfiguration, Rgba}; + + use super::*; + + // Regression test for https://github.com/rerun-io/rerun/issues/8639 + #[test] + fn empty_strips() { + re_log::setup_logging(); + re_log::PanicOnWarnScope::new(); + + RenderContext::new_test().execute_test_frame(|ctx| { + let mut view = ViewBuilder::new(&ctx, TargetConfiguration::default()); + + let empty = LineDrawableBuilder::new(&ctx); + view.queue_draw(empty.into_draw_data().unwrap()); + + // This is the case that triggered + // https://github.com/rerun-io/rerun/issues/8639 + // The others are here for completeness. + let mut empty_batch = LineDrawableBuilder::new(&ctx); + empty_batch.batch("empty batch").add_strip([].into_iter()); + view.queue_draw(empty_batch.into_draw_data().unwrap()); + + let mut empty_batch_between_non_empty = LineDrawableBuilder::new(&ctx); + empty_batch_between_non_empty + .batch("non-empty batch") + .add_strip([glam::Vec3::ZERO, glam::Vec3::ZERO].into_iter()); + empty_batch_between_non_empty + .batch("empty batch") + .add_strip([].into_iter()); + empty_batch_between_non_empty + .batch("non-empty batch") + .add_strip([glam::Vec3::ZERO, glam::Vec3::ZERO].into_iter()); + view.queue_draw(empty_batch_between_non_empty.into_draw_data().unwrap()); + + [view.draw(&ctx, Rgba::BLACK).unwrap()] + }); + } +} From 18fe4cbaf378bdc88ef1cc1f76af0956839258d7 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 10 Jan 2025 17:33:26 +0100 Subject: [PATCH 4/4] fix wasm compilation --- crates/utils/re_log/src/setup.rs | 1 + crates/viewer/re_renderer/src/config.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/crates/utils/re_log/src/setup.rs b/crates/utils/re_log/src/setup.rs index 58665fbb74da..e4fc77bb2b38 100644 --- a/crates/utils/re_log/src/setup.rs +++ b/crates/utils/re_log/src/setup.rs @@ -112,6 +112,7 @@ impl Drop for PanicOnWarnScope { } } +#[cfg(not(target_arch = "wasm32"))] fn env_var_bool(name: &str) -> Option { std::env::var(name).ok() .and_then(|s| match s.to_lowercase().as_str() { diff --git a/crates/viewer/re_renderer/src/config.rs b/crates/viewer/re_renderer/src/config.rs index bfac64245ac0..57ece87f6269 100644 --- a/crates/viewer/re_renderer/src/config.rs +++ b/crates/viewer/re_renderer/src/config.rs @@ -400,6 +400,7 @@ pub fn testing_instance_descriptor() -> wgpu::InstanceDescriptor { /// Selects an adapter for testing, preferring software rendering if available. /// /// Panics if no adapter was found. +#[cfg(native)] pub fn select_testing_adapter(instance: &wgpu::Instance) -> wgpu::Adapter { let mut adapters = instance.enumerate_adapters(wgpu::Backends::all()); assert!(!adapters.is_empty(), "No graphics adapter found!");