From fdac6baa9a52a72cf3cf178aba6d5b7b62976d23 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Tue, 7 Jan 2025 14:42:43 +0100 Subject: [PATCH 1/9] make viewer test context tests only --- crates/viewer/re_viewer_context/Cargo.toml | 4 ++++ crates/viewer/re_viewer_context/src/lib.rs | 4 +++- crates/viewer/re_viewport_blueprint/Cargo.toml | 3 +++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/crates/viewer/re_viewer_context/Cargo.toml b/crates/viewer/re_viewer_context/Cargo.toml index 9c3d6f233d77..3655632d67b8 100644 --- a/crates/viewer/re_viewer_context/Cargo.toml +++ b/crates/viewer/re_viewer_context/Cargo.toml @@ -18,6 +18,10 @@ workspace = true [package.metadata.docs.rs] all-features = true +[features] +## Enable for testing utilities. +testing = [] + [dependencies] re_capabilities.workspace = true re_chunk_store.workspace = true diff --git a/crates/viewer/re_viewer_context/src/lib.rs b/crates/viewer/re_viewer_context/src/lib.rs index 6a8c002443dc..d28b232d390f 100644 --- a/crates/viewer/re_viewer_context/src/lib.rs +++ b/crates/viewer/re_viewer_context/src/lib.rs @@ -24,7 +24,6 @@ mod selection_state; mod store_context; pub mod store_hub; mod tensor; -pub mod test_context; //TODO(ab): this should be behind #[cfg(test)], but then ` cargo clippy --all-targets` fails mod time_control; mod time_drag_value; mod typed_entity_collections; @@ -33,6 +32,9 @@ mod utils; mod view; mod viewer_context; +#[cfg(feature = "testing")] +pub mod test_context; + // TODO(andreas): Move to its own crate? pub mod gpu_bridge; diff --git a/crates/viewer/re_viewport_blueprint/Cargo.toml b/crates/viewer/re_viewport_blueprint/Cargo.toml index de84226d4558..8b324a16620f 100644 --- a/crates/viewer/re_viewport_blueprint/Cargo.toml +++ b/crates/viewer/re_viewport_blueprint/Cargo.toml @@ -41,3 +41,6 @@ parking_lot.workspace = true slotmap.workspace = true smallvec.workspace = true thiserror.workspace = true + +[dev-dependencies] +re_viewer_context = { workspace = true, features = ["testing"] } From bbd8b90978328433c43a389153082bcc8dd4b1ea Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Tue, 7 Jan 2025 18:34:07 +0100 Subject: [PATCH 2/9] enable re_renderer in test context tests --- Cargo.lock | 3 + crates/viewer/re_component_ui/Cargo.toml | 2 + .../Colormap_placeholder.png | 4 +- .../Colormap_placeholder.png | 4 +- .../tests/test_all_components_ui.rs | 3 +- crates/viewer/re_renderer/src/config.rs | 13 ++ .../re_time_panel/tests/time_panel_tests.rs | 3 +- crates/viewer/re_view_graph/tests/basic.rs | 3 +- crates/viewer/re_viewer/src/app.rs | 2 - crates/viewer/re_viewer/src/lib.rs | 4 +- crates/viewer/re_viewer_context/Cargo.toml | 7 +- .../re_viewer_context/src/test_context.rs | 125 +++++++++++++++++- 12 files changed, 160 insertions(+), 13 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index beb1006fb8ad..01d9a8304dcd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6917,8 +6917,10 @@ dependencies = [ "directories", "egui", "egui-wgpu", + "egui_kittest", "egui_tiles", "emath", + "env_logger", "glam", "half", "home", @@ -6930,6 +6932,7 @@ dependencies = [ "nohash-hasher", "once_cell", "parking_lot", + "pollster 0.4.0", "re_capabilities", "re_chunk", "re_chunk_store", diff --git a/crates/viewer/re_component_ui/Cargo.toml b/crates/viewer/re_component_ui/Cargo.toml index bef6b1d32796..2c849f389994 100644 --- a/crates/viewer/re_component_ui/Cargo.toml +++ b/crates/viewer/re_component_ui/Cargo.toml @@ -39,6 +39,8 @@ egui.workspace = true [dev-dependencies] +re_viewer_context = { workspace = true, features = ["testing"] } + egui_kittest.workspace = true itertools.workspace = true nohash-hasher.workspace = true diff --git a/crates/viewer/re_component_ui/tests/snapshots/all_components_list_item_narrow/Colormap_placeholder.png b/crates/viewer/re_component_ui/tests/snapshots/all_components_list_item_narrow/Colormap_placeholder.png index d11afc2fbfdb..a0a2955ab84a 100644 --- a/crates/viewer/re_component_ui/tests/snapshots/all_components_list_item_narrow/Colormap_placeholder.png +++ b/crates/viewer/re_component_ui/tests/snapshots/all_components_list_item_narrow/Colormap_placeholder.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:18b8ada626cbb59ec85db0ba8d167deeef516d18279d5b85ad3dd43bef7b8113 -size 3326 +oid sha256:addb336fa14268b87e22974902909b3a3e5629fbe483f7d98fe9ea04f2836e47 +size 2986 diff --git a/crates/viewer/re_component_ui/tests/snapshots/all_components_list_item_wide/Colormap_placeholder.png b/crates/viewer/re_component_ui/tests/snapshots/all_components_list_item_wide/Colormap_placeholder.png index d2de7193371d..692dc938a304 100644 --- a/crates/viewer/re_component_ui/tests/snapshots/all_components_list_item_wide/Colormap_placeholder.png +++ b/crates/viewer/re_component_ui/tests/snapshots/all_components_list_item_wide/Colormap_placeholder.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:b15bc121e4102dbc979e92f5564c36ad7f085a3c9465518f3d90427d0fbe3f38 -size 3637 +oid sha256:64a67110962fafff3f8bef5f849b5401a6d7f2adebf3a3ac82d286ef70eea7ed +size 4208 diff --git a/crates/viewer/re_component_ui/tests/test_all_components_ui.rs b/crates/viewer/re_component_ui/tests/test_all_components_ui.rs index 37ae2bb022a6..0500ec9ca87a 100644 --- a/crates/viewer/re_component_ui/tests/test_all_components_ui.rs +++ b/crates/viewer/re_component_ui/tests/test_all_components_ui.rs @@ -213,7 +213,8 @@ fn test_single_component_ui_as_list_item( ); }; - let mut harness = egui_kittest::Harness::builder() + let mut harness = test_context + .setup_kittest_for_rendering() .with_size(Vec2::new(ui_width, 40.0)) .build_ui(|ui| { test_context.run(&ui.ctx().clone(), |ctx| { diff --git a/crates/viewer/re_renderer/src/config.rs b/crates/viewer/re_renderer/src/config.rs index c2b8c4aa8071..2bbffa958ade 100644 --- a/crates/viewer/re_renderer/src/config.rs +++ b/crates/viewer/re_renderer/src/config.rs @@ -296,6 +296,19 @@ impl DeviceCaps { } } +pub fn instance_descriptor(supported_backends: wgpu::Backends) -> wgpu::InstanceDescriptor { + wgpu::InstanceDescriptor { + backends: supported_backends, + // TODO(#8466): Experiment with `ALLOW_UNDERLYING_NONCOMPLIANT_ADAPTER` + flags: wgpu::InstanceFlags::from_build_config().with_env(), + + gles_minor_version: wgpu::Gles3MinorVersion::Automatic, + + // Fxc is slow and has many issues but shipping with DXC is more complex. + dx12_shader_compiler: wgpu::Dx12Compiler::Fxc, + } +} + /// 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_time_panel/tests/time_panel_tests.rs b/crates/viewer/re_time_panel/tests/time_panel_tests.rs index 8feeeeb2ad1a..ae93eac6531e 100644 --- a/crates/viewer/re_time_panel/tests/time_panel_tests.rs +++ b/crates/viewer/re_time_panel/tests/time_panel_tests.rs @@ -76,7 +76,8 @@ fn run_time_panel_and_save_snapshot(mut test_context: TestContext, _snapshot_nam let mut panel = TimePanel::default(); //TODO(ab): this contains a lot of boilerplate which should be provided by helpers - let mut harness = egui_kittest::Harness::builder() + let mut harness = test_context + .setup_kittest_for_rendering() .with_size(Vec2::new(700.0, 300.0)) .build_ui(|ui| { test_context.run(&ui.ctx().clone(), |viewer_ctx| { diff --git a/crates/viewer/re_view_graph/tests/basic.rs b/crates/viewer/re_view_graph/tests/basic.rs index 44338ac3ecc3..6dfd59f0c807 100644 --- a/crates/viewer/re_view_graph/tests/basic.rs +++ b/crates/viewer/re_view_graph/tests/basic.rs @@ -215,7 +215,8 @@ fn run_graph_view_and_save_snapshot( .collect(); //TODO(ab): this contains a lot of boilerplate which should be provided by helpers - let mut harness = egui_kittest::Harness::builder() + let mut harness = test_context + .setup_kittest_for_rendering() .with_size(size) .build_ui(|ui| { test_context.run(&ui.ctx().clone(), |viewer_ctx| { diff --git a/crates/viewer/re_viewer/src/app.rs b/crates/viewer/re_viewer/src/app.rs index 9f9a7eb67ff3..818dca4a912a 100644 --- a/crates/viewer/re_viewer/src/app.rs +++ b/crates/viewer/re_viewer/src/app.rs @@ -1810,7 +1810,6 @@ impl eframe::App for App { } // NOTE: GPU resource stats are cheap to compute so we always do. - // TODO(andreas): store the re_renderer somewhere else. let gpu_resource_stats = { re_tracing::profile_scope!("gpu_resource_stats"); @@ -1840,7 +1839,6 @@ impl eframe::App for App { self.purge_memory_if_needed(&mut store_hub); { - // TODO(andreas): store the re_renderer somewhere else. let egui_renderer = { let render_state = frame.wgpu_render_state().unwrap(); &mut render_state.renderer.read() diff --git a/crates/viewer/re_viewer/src/lib.rs b/crates/viewer/re_viewer/src/lib.rs index 70875688acd8..a8fe078e0b18 100644 --- a/crates/viewer/re_viewer/src/lib.rs +++ b/crates/viewer/re_viewer/src/lib.rs @@ -224,15 +224,15 @@ pub fn customize_eframe_and_setup_renderer( if let Some(render_state) = &cc.wgpu_render_state { use re_renderer::RenderContext; + // Put the renderer into paint callback resources, so we have access to the renderer + // when we need to process egui draw callbacks. let paint_callback_resources = &mut render_state.renderer.write().callback_resources; - let render_ctx = RenderContext::new( &render_state.adapter, render_state.device.clone(), render_state.queue.clone(), render_state.target_format, )?; - paint_callback_resources.insert(render_ctx); } diff --git a/crates/viewer/re_viewer_context/Cargo.toml b/crates/viewer/re_viewer_context/Cargo.toml index 3655632d67b8..fd2d7fd5e460 100644 --- a/crates/viewer/re_viewer_context/Cargo.toml +++ b/crates/viewer/re_viewer_context/Cargo.toml @@ -20,7 +20,7 @@ all-features = true [features] ## Enable for testing utilities. -testing = [] +testing = ["dep:pollster", "dep:egui_kittest", "dep:env_logger"] [dependencies] re_capabilities.workspace = true @@ -73,6 +73,11 @@ thiserror.workspace = true uuid = { workspace = true, features = ["serde", "v4", "js"] } wgpu.workspace = true +# Optional dependencies: +egui_kittest = { workspace = true, features = ["wgpu"], optional = true } +env_logger = { workspace = true, optional = true } +pollster = { workspace = true, optional = true } + # Native dependencies: [target.'cfg(not(target_arch = "wasm32"))'.dependencies] arboard = { workspace = true, default-features = false, features = [ diff --git a/crates/viewer/re_viewer_context/src/test_context.rs b/crates/viewer/re_viewer_context/src/test_context.rs index 3e014354bc54..6ba298173304 100644 --- a/crates/viewer/re_viewer_context/src/test_context.rs +++ b/crates/viewer/re_viewer_context/src/test_context.rs @@ -1,6 +1,9 @@ use std::sync::Arc; use ahash::HashMap; +use once_cell::sync::Lazy; +use parking_lot::Mutex; + use re_chunk_store::LatestAtQuery; use re_entity_db::EntityDb; use re_log_types::{StoreId, StoreKind}; @@ -40,10 +43,15 @@ pub struct TestContext { command_sender: CommandSender, command_receiver: CommandReceiver, + egui_render_state: Mutex>, } impl Default for TestContext { fn default() -> Self { + // 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 recording_store = EntityDb::new(StoreId::random(StoreKind::Recording)); let blueprint_store = EntityDb::new(StoreId::random(StoreKind::Blueprint)); @@ -72,11 +80,113 @@ impl Default for TestContext { reflection, command_sender, command_receiver, + + // Created lazily since each egui_kittest harness needs a new one. + egui_render_state: Mutex::new(None), } } } +fn create_egui_renderstate() -> egui_wgpu::RenderState { + re_tracing::profile_function!(); + + let config = egui_wgpu::WgpuConfiguration { + wgpu_setup: egui_wgpu::WgpuSetup::Existing { + instance: SHARED_WGPU_RENDERER_SETUP.instance.clone(), + adapter: SHARED_WGPU_RENDERER_SETUP.adapter.clone(), + device: SHARED_WGPU_RENDERER_SETUP.device.clone(), + queue: SHARED_WGPU_RENDERER_SETUP.queue.clone(), + }, + + // None of these matter for tests as we're not going to draw to a surfaces. + present_mode: wgpu::PresentMode::Immediate, + desired_maximum_frame_latency: None, + on_surface_error: Arc::new(|_| { + unreachable!("tests aren't expected to draw to surfaces"); + }), + }; + + let compatible_surface = None; + let depth_format = None; + let msaa_samples = 1; + let dithering = false; + let render_state = pollster::block_on(egui_wgpu::RenderState::create( + &config, + &SHARED_WGPU_RENDERER_SETUP.instance, + compatible_surface, + depth_format, + msaa_samples, + dithering, + )) + .expect("Failed to set up egui_wgpu::RenderState"); + + // Put re_renderer::RenderContext into the callback resources so that render callbacks can access it. + render_state.renderer.write().callback_resources.insert( + re_renderer::RenderContext::new( + &SHARED_WGPU_RENDERER_SETUP.adapter, + SHARED_WGPU_RENDERER_SETUP.device.clone(), + SHARED_WGPU_RENDERER_SETUP.queue.clone(), + wgpu::TextureFormat::Rgba8Unorm, + ) + .expect("Failed to initialize re_renderer"), + ); + render_state +} + +/// Instance & adapter +struct SharedWgpuResources { + instance: Arc, + adapter: Arc, + device: Arc, + + // Sharing the queue accross parallel running tests should work fine in theory - it's obviously threadsafe. + // Note though that this becomes an odd sync point that is shared with all tests that put in work here. + queue: Arc, +} + +static SHARED_WGPU_RENDERER_SETUP: Lazy = Lazy::new(|| { + // 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 + // For the moment we just use wgpu defaults. + + let instance = wgpu::Instance::default(); + let adapter = pollster::block_on(instance.request_adapter(&wgpu::RequestAdapterOptions { + power_preference: wgpu::PowerPreference::default(), + force_fallback_adapter: false, + compatible_surface: None, + })) + .expect("Failed to request adapter"); + + let device_caps = re_renderer::config::DeviceCaps::from_adapter(&adapter) + .expect("Insufficient device capabilities."); + let (device, queue) = + pollster::block_on(adapter.request_device(&device_caps.device_descriptor(), None)) + .expect("Failed to request device."); + + SharedWgpuResources { + instance: Arc::new(instance), + adapter: Arc::new(adapter), + device: Arc::new(device), + queue: Arc::new(queue), + } +}); + impl TestContext { + pub fn setup_kittest_for_rendering(&self) -> egui_kittest::HarnessBuilder<()> { + let new_render_state = create_egui_renderstate(); + let renderer = egui_kittest::Harness::builder().renderer( + // Note that render state clone is mostly cloning of inner `Arc`. + // This does _not_ duplicate re_renderer's context. + egui_kittest::wgpu::WgpuTestRenderer::from_render_state(new_render_state.clone()), + ); + + // Egui kittests insists on having a fresh render state for each test. + self.egui_render_state.lock().replace(new_render_state); + renderer + } + /// Timeline the recording config is using by default. pub fn active_timeline(&self) -> re_chunk::Timeline { *self.recording_config.time_ctrl.read().timeline() @@ -108,6 +218,17 @@ impl TestContext { let drag_and_drop_manager = crate::DragAndDropManager::new(ItemCollection::default()); + let mut context_render_state = self.egui_render_state.lock(); + let mut egui_renderer = context_render_state + .get_or_insert_with(create_egui_renderstate) + .renderer + .write(); + let render_ctx = egui_renderer + .callback_resources + .get_mut::() + .expect("No re_renderer::RenderContext in egui_render_state"); + render_ctx.begin_frame(); + let ctx = ViewerContext { app_options: &Default::default(), cache: &Default::default(), @@ -123,13 +244,15 @@ impl TestContext { selection_state: &self.selection_state, blueprint_query: &self.blueprint_query, egui_ctx, - render_ctx: None, + render_ctx: Some(render_ctx), command_sender: &self.command_sender, focused_item: &None, drag_and_drop_manager: &drag_and_drop_manager, }; func(&ctx); + + render_ctx.before_submit(); } /// Run the given function with a [`ViewerContext`] produced by the [`Self`], in the context of From 8ae9f1dc97ee856ba2c462d17644242e0ad8e209 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Tue, 7 Jan 2025 19:04:37 +0100 Subject: [PATCH 3/9] cargo deny & typo fixes --- .../re_viewer_context/src/test_context.rs | 4 ++-- deny.toml | 21 ++++++++++--------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/crates/viewer/re_viewer_context/src/test_context.rs b/crates/viewer/re_viewer_context/src/test_context.rs index 6ba298173304..cc985b80145c 100644 --- a/crates/viewer/re_viewer_context/src/test_context.rs +++ b/crates/viewer/re_viewer_context/src/test_context.rs @@ -139,13 +139,13 @@ struct SharedWgpuResources { adapter: Arc, device: Arc, - // Sharing the queue accross parallel running tests should work fine in theory - it's obviously threadsafe. + // Sharing the queue across parallel running tests should work fine in theory - it's obviously threadsafe. // Note though that this becomes an odd sync point that is shared with all tests that put in work here. queue: Arc, } static SHARED_WGPU_RENDERER_SETUP: Lazy = Lazy::new(|| { - // TODO(andreas, emilk/egui#5506): Use centralized wgpu setup logic that... + // 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 diff --git a/deny.toml b/deny.toml index 91c5b9afdd06..7f00aa30cbe5 100644 --- a/deny.toml +++ b/deny.toml @@ -45,16 +45,17 @@ deny = [ { name = "openssl" }, # We prefer rustls ] skip = [ - { name = "ahash" }, # Popular crate + fast release schedule = lots of crates still using old versions - { name = "base64" }, # Too popular - { name = "cargo_metadata" }, # Older version used by ply-rs. It's small, and it's build-time only! - { name = "cfg_aliases" }, # Tiny macro-only crate. wgpu/naga is using an old version - { name = "hashbrown" }, # Old version used by polar-rs - { name = "memoffset" }, # Small crate - { name = "prettyplease" }, # Old version being used by prost - { name = "pulldown-cmark" }, # Build-dependency via `ply-rs` (!). TODO(emilk): use a better crate for .ply parsing - { name = "redox_syscall" }, # Plenty of versions in the wild - { name = "pollster" }, # rfd is still on 0.3 + { name = "accesskit_consumer"}, # Duplicate as when used as dev dependency - eframe & egui_kittest are on different versions. + { name = "ahash" }, # Popular crate + fast release schedule = lots of crates still using old versions + { name = "base64" }, # Too popular + { name = "cargo_metadata" }, # Older version used by ply-rs. It's small, and it's build-time only! + { name = "cfg_aliases" }, # Tiny macro-only crate. wgpu/naga is using an old version + { name = "hashbrown" }, # Old version used by polar-rs + { name = "memoffset" }, # Small crate + { name = "pollster" }, # rfd is still on 0.3 + { name = "prettyplease" }, # Old version being used by prost + { name = "pulldown-cmark" }, # Build-dependency via `ply-rs` (!). TODO(emilk): use a better crate for .ply parsing + { name = "redox_syscall" }, # Plenty of versions in the wild ] skip-tree = [ { name = "cargo-run-wasm" }, # Dev-tool From b078f9bae839a0dfcc5de8b52b11c1d71bba373f Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Tue, 7 Jan 2025 23:13:50 +0100 Subject: [PATCH 4/9] toml fmt --- deny.toml | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/deny.toml b/deny.toml index 7f00aa30cbe5..30765b149088 100644 --- a/deny.toml +++ b/deny.toml @@ -45,17 +45,17 @@ deny = [ { name = "openssl" }, # We prefer rustls ] skip = [ - { name = "accesskit_consumer"}, # Duplicate as when used as dev dependency - eframe & egui_kittest are on different versions. - { name = "ahash" }, # Popular crate + fast release schedule = lots of crates still using old versions - { name = "base64" }, # Too popular - { name = "cargo_metadata" }, # Older version used by ply-rs. It's small, and it's build-time only! - { name = "cfg_aliases" }, # Tiny macro-only crate. wgpu/naga is using an old version - { name = "hashbrown" }, # Old version used by polar-rs - { name = "memoffset" }, # Small crate - { name = "pollster" }, # rfd is still on 0.3 - { name = "prettyplease" }, # Old version being used by prost - { name = "pulldown-cmark" }, # Build-dependency via `ply-rs` (!). TODO(emilk): use a better crate for .ply parsing - { name = "redox_syscall" }, # Plenty of versions in the wild + { name = "accesskit_consumer" }, # Duplicate as when used as dev dependency - eframe & egui_kittest are on different versions. + { name = "ahash" }, # Popular crate + fast release schedule = lots of crates still using old versions + { name = "base64" }, # Too popular + { name = "cargo_metadata" }, # Older version used by ply-rs. It's small, and it's build-time only! + { name = "cfg_aliases" }, # Tiny macro-only crate. wgpu/naga is using an old version + { name = "hashbrown" }, # Old version used by polar-rs + { name = "memoffset" }, # Small crate + { name = "pollster" }, # rfd is still on 0.3 + { name = "prettyplease" }, # Old version being used by prost + { name = "pulldown-cmark" }, # Build-dependency via `ply-rs` (!). TODO(emilk): use a better crate for .ply parsing + { name = "redox_syscall" }, # Plenty of versions in the wild ] skip-tree = [ { name = "cargo-run-wasm" }, # Dev-tool From 34cbdaeeb216947aed98a7391a049c8b3669edf4 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 8 Jan 2025 12:03:07 +0100 Subject: [PATCH 5/9] make render adapter creation optional for the moment --- .../re_viewer_context/src/test_context.rs | 95 +++++++++++-------- 1 file changed, 55 insertions(+), 40 deletions(-) diff --git a/crates/viewer/re_viewer_context/src/test_context.rs b/crates/viewer/re_viewer_context/src/test_context.rs index cc985b80145c..468b8febe056 100644 --- a/crates/viewer/re_viewer_context/src/test_context.rs +++ b/crates/viewer/re_viewer_context/src/test_context.rs @@ -6,6 +6,7 @@ use parking_lot::Mutex; use re_chunk_store::LatestAtQuery; use re_entity_db::EntityDb; +use re_log::ResultExt as _; use re_log_types::{StoreId, StoreKind}; use re_types_core::reflection::Reflection; @@ -87,15 +88,17 @@ impl Default for TestContext { } } -fn create_egui_renderstate() -> egui_wgpu::RenderState { +fn create_egui_renderstate() -> Option { re_tracing::profile_function!(); + let shared_wgpu_setup = (*SHARED_WGPU_RENDERER_SETUP).as_ref()?; + let config = egui_wgpu::WgpuConfiguration { wgpu_setup: egui_wgpu::WgpuSetup::Existing { - instance: SHARED_WGPU_RENDERER_SETUP.instance.clone(), - adapter: SHARED_WGPU_RENDERER_SETUP.adapter.clone(), - device: SHARED_WGPU_RENDERER_SETUP.device.clone(), - queue: SHARED_WGPU_RENDERER_SETUP.queue.clone(), + instance: shared_wgpu_setup.instance.clone(), + adapter: shared_wgpu_setup.adapter.clone(), + device: shared_wgpu_setup.device.clone(), + queue: shared_wgpu_setup.queue.clone(), }, // None of these matter for tests as we're not going to draw to a surfaces. @@ -112,7 +115,7 @@ fn create_egui_renderstate() -> egui_wgpu::RenderState { let dithering = false; let render_state = pollster::block_on(egui_wgpu::RenderState::create( &config, - &SHARED_WGPU_RENDERER_SETUP.instance, + &shared_wgpu_setup.instance, compatible_surface, depth_format, msaa_samples, @@ -123,14 +126,14 @@ fn create_egui_renderstate() -> egui_wgpu::RenderState { // Put re_renderer::RenderContext into the callback resources so that render callbacks can access it. render_state.renderer.write().callback_resources.insert( re_renderer::RenderContext::new( - &SHARED_WGPU_RENDERER_SETUP.adapter, - SHARED_WGPU_RENDERER_SETUP.device.clone(), - SHARED_WGPU_RENDERER_SETUP.queue.clone(), + &shared_wgpu_setup.adapter, + shared_wgpu_setup.device.clone(), + shared_wgpu_setup.queue.clone(), wgpu::TextureFormat::Rgba8Unorm, ) .expect("Failed to initialize re_renderer"), ); - render_state + Some(render_state) } /// Instance & adapter @@ -144,47 +147,54 @@ struct SharedWgpuResources { queue: Arc, } -static SHARED_WGPU_RENDERER_SETUP: Lazy = Lazy::new(|| { +static SHARED_WGPU_RENDERER_SETUP: Lazy> = + Lazy::new(|| try_init_shared_renderer_setup()); + +fn try_init_shared_renderer_setup() -> Option { // 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 // For the moment we just use wgpu defaults. + // TODO(#8245): Should we require this to succeed? + let instance = wgpu::Instance::default(); let adapter = pollster::block_on(instance.request_adapter(&wgpu::RequestAdapterOptions { power_preference: wgpu::PowerPreference::default(), force_fallback_adapter: false, compatible_surface: None, - })) - .expect("Failed to request adapter"); + }))?; let device_caps = re_renderer::config::DeviceCaps::from_adapter(&adapter) - .expect("Insufficient device capabilities."); + .warn_on_err_once("Failed to determine device capabilities")?; let (device, queue) = pollster::block_on(adapter.request_device(&device_caps.device_descriptor(), None)) - .expect("Failed to request device."); + .warn_on_err_once("Failed to request device.")?; - SharedWgpuResources { + Some(SharedWgpuResources { instance: Arc::new(instance), adapter: Arc::new(adapter), device: Arc::new(device), queue: Arc::new(queue), - } -}); + }) +} impl TestContext { pub fn setup_kittest_for_rendering(&self) -> egui_kittest::HarnessBuilder<()> { - let new_render_state = create_egui_renderstate(); - let renderer = egui_kittest::Harness::builder().renderer( - // Note that render state clone is mostly cloning of inner `Arc`. - // This does _not_ duplicate re_renderer's context. - egui_kittest::wgpu::WgpuTestRenderer::from_render_state(new_render_state.clone()), - ); - - // Egui kittests insists on having a fresh render state for each test. - self.egui_render_state.lock().replace(new_render_state); - renderer + if let Some(new_render_state) = create_egui_renderstate() { + let builder = egui_kittest::Harness::builder().renderer( + // Note that render state clone is mostly cloning of inner `Arc`. + // This does _not_ duplicate re_renderer's context. + egui_kittest::wgpu::WgpuTestRenderer::from_render_state(new_render_state.clone()), + ); + + // Egui kittests insists on having a fresh render state for each test. + self.egui_render_state.lock().replace(new_render_state); + builder + } else { + egui_kittest::Harness::builder() + } } /// Timeline the recording config is using by default. @@ -218,16 +228,19 @@ impl TestContext { let drag_and_drop_manager = crate::DragAndDropManager::new(ItemCollection::default()); - let mut context_render_state = self.egui_render_state.lock(); - let mut egui_renderer = context_render_state - .get_or_insert_with(create_egui_renderstate) - .renderer - .write(); - let render_ctx = egui_renderer - .callback_resources - .get_mut::() - .expect("No re_renderer::RenderContext in egui_render_state"); - render_ctx.begin_frame(); + let context_render_state = self.egui_render_state.lock(); + let mut renderer; + let render_ctx = if let Some(render_state) = context_render_state.as_ref() { + renderer = render_state.renderer.write(); + let render_ctx = renderer + .callback_resources + .get_mut::() + .expect("No re_renderer::RenderContext in egui_render_state"); + render_ctx.begin_frame(); + Some(render_ctx) + } else { + None + }; let ctx = ViewerContext { app_options: &Default::default(), @@ -244,7 +257,7 @@ impl TestContext { selection_state: &self.selection_state, blueprint_query: &self.blueprint_query, egui_ctx, - render_ctx: Some(render_ctx), + render_ctx: render_ctx.as_deref(), command_sender: &self.command_sender, focused_item: &None, drag_and_drop_manager: &drag_and_drop_manager, @@ -252,7 +265,9 @@ impl TestContext { func(&ctx); - render_ctx.before_submit(); + if let Some(render_ctx) = render_ctx { + render_ctx.before_submit(); + } } /// Run the given function with a [`ViewerContext`] produced by the [`Self`], in the context of From ce464f831180f678084c8992352f02df211a2ff4 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 8 Jan 2025 12:15:30 +0100 Subject: [PATCH 6/9] clippy fix --- crates/viewer/re_viewer_context/src/test_context.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/viewer/re_viewer_context/src/test_context.rs b/crates/viewer/re_viewer_context/src/test_context.rs index 468b8febe056..433b90067fb7 100644 --- a/crates/viewer/re_viewer_context/src/test_context.rs +++ b/crates/viewer/re_viewer_context/src/test_context.rs @@ -148,7 +148,7 @@ struct SharedWgpuResources { } static SHARED_WGPU_RENDERER_SETUP: Lazy> = - Lazy::new(|| try_init_shared_renderer_setup()); + Lazy::new(try_init_shared_renderer_setup); fn try_init_shared_renderer_setup() -> Option { // TODO(andreas, emilk/egui#5506): Use centralized wgpu setup logic that… From cc7df5226d8f746799e51e145d41be0334a549fa Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Wed, 8 Jan 2025 14:23:40 +0100 Subject: [PATCH 7/9] remove renderer config method that doesn't belong here --- crates/viewer/re_renderer/src/config.rs | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/crates/viewer/re_renderer/src/config.rs b/crates/viewer/re_renderer/src/config.rs index 2bbffa958ade..c2b8c4aa8071 100644 --- a/crates/viewer/re_renderer/src/config.rs +++ b/crates/viewer/re_renderer/src/config.rs @@ -296,19 +296,6 @@ impl DeviceCaps { } } -pub fn instance_descriptor(supported_backends: wgpu::Backends) -> wgpu::InstanceDescriptor { - wgpu::InstanceDescriptor { - backends: supported_backends, - // TODO(#8466): Experiment with `ALLOW_UNDERLYING_NONCOMPLIANT_ADAPTER` - flags: wgpu::InstanceFlags::from_build_config().with_env(), - - gles_minor_version: wgpu::Gles3MinorVersion::Automatic, - - // Fxc is slow and has many issues but shipping with DXC is more complex. - dx12_shader_compiler: wgpu::Dx12Compiler::Fxc, - } -} - /// Backends that are officially supported by `re_renderer`. /// /// Other backend might work as well, but lack of support isn't regarded as a bug. From b22cd8fe1da281347dbc8c9c31afa4e68f46a9d2 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 9 Jan 2025 17:42:03 +0100 Subject: [PATCH 8/9] comment improvements --- .../viewer/re_viewer_context/src/test_context.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/crates/viewer/re_viewer_context/src/test_context.rs b/crates/viewer/re_viewer_context/src/test_context.rs index 433b90067fb7..049df8d5cb39 100644 --- a/crates/viewer/re_viewer_context/src/test_context.rs +++ b/crates/viewer/re_viewer_context/src/test_context.rs @@ -88,18 +88,22 @@ impl Default for TestContext { } } +/// Create an egui_wgpu::RenderState for tests. +/// +/// May be `None` if we failed to initialize the wgpu renderer setup. fn create_egui_renderstate() -> Option { re_tracing::profile_function!(); let shared_wgpu_setup = (*SHARED_WGPU_RENDERER_SETUP).as_ref()?; let config = egui_wgpu::WgpuConfiguration { - wgpu_setup: egui_wgpu::WgpuSetup::Existing { + wgpu_setup: egui_wgpu::WgpuSetupExisting { instance: shared_wgpu_setup.instance.clone(), adapter: shared_wgpu_setup.adapter.clone(), device: shared_wgpu_setup.device.clone(), queue: shared_wgpu_setup.queue.clone(), - }, + } + .into(), // None of these matter for tests as we're not going to draw to a surfaces. present_mode: wgpu::PresentMode::Immediate, @@ -110,9 +114,14 @@ fn create_egui_renderstate() -> Option { }; let compatible_surface = None; - let depth_format = None; + // `re_renderer`'s individual views (managed each by a `ViewBuilder`) have MSAA, + // but egui's final target doesn't - re_renderer resolves and copies into egui in `ViewBuilder::composite`. let msaa_samples = 1; + // Similarly, depth is handled by re_renderer. + let depth_format = None; + // Disable dithering in order to not unnecessarily add a source of noise & variance between renderers. let dithering = false; + let render_state = pollster::block_on(egui_wgpu::RenderState::create( &config, &shared_wgpu_setup.instance, From 10f9533f0a1dca7461f4a66edd7b9eb60b93b8d2 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 9 Jan 2025 18:26:50 +0100 Subject: [PATCH 9/9] fix doc comment issue --- crates/viewer/re_viewer_context/src/test_context.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/viewer/re_viewer_context/src/test_context.rs b/crates/viewer/re_viewer_context/src/test_context.rs index 049df8d5cb39..cd9354ab1adb 100644 --- a/crates/viewer/re_viewer_context/src/test_context.rs +++ b/crates/viewer/re_viewer_context/src/test_context.rs @@ -88,7 +88,7 @@ impl Default for TestContext { } } -/// Create an egui_wgpu::RenderState for tests. +/// Create an `egui_wgpu::RenderState` for tests. /// /// May be `None` if we failed to initialize the wgpu renderer setup. fn create_egui_renderstate() -> Option {