Fix not handling empty line strips in the viewer

Cargo.lock

@@ -6265,6 +6265,7 @@ dependencies = [
  "ordered-float",
  "parking_lot",
  "pathdiff",
+ "pollster 0.4.0",
  "profiling",
  "re_arrow2",
  "re_build_tools",

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 {

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,6 +78,40 @@ pub fn setup_logging() {
 
 // ----------------------------------------------------------------------------
 
+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);
+    }
+}
+
 #[cfg(not(target_arch = "wasm32"))]
 fn env_var_bool(name: &str) -> Option<bool> {
     std::env::var(name).ok()
@@ -90,14 +127,19 @@ fn env_var_bool(name: &str) -> Option<bool> {
         })
 }
 
-#[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,
         }
     }

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:

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,72 @@ 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.
+#[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!");
+
+    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.

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<I>(&mut self, create_gpu_work: impl FnOnce(&mut Self) -> I)
+    where
+        I: IntoIterator<Item = wgpu::CommandBuffer>,
+    {
+        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();
+    }
+}

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;

crates/viewer/re_renderer/src/renderer/lines.rs

@@ -343,7 +343,7 @@ impl LineDrawData {
 
         let line_renderer = ctx.renderer::<LineRenderer>();
 
-        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()]
+        });
+    }
+}

crates/viewer/re_viewer_context/src/test_context.rs

@@ -162,67 +162,8 @@ static SHARED_WGPU_RENDERER_SETUP: Lazy<SharedWgpuResources> =
     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) =