Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable re_renderer for all tests using the TestContext if graphics adapter is present #8606

Merged
merged 10 commits into from
Jan 9, 2025
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -6917,8 +6917,10 @@ dependencies = [
"directories",
"egui",
"egui-wgpu",
"egui_kittest",
"egui_tiles",
"emath",
"env_logger",
"glam",
"half",
"home",
Expand All @@ -6930,6 +6932,7 @@ dependencies = [
"nohash-hasher",
"once_cell",
"parking_lot",
"pollster 0.4.0",
"re_capabilities",
"re_chunk",
"re_chunk_store",
Expand Down
2 changes: 2 additions & 0 deletions crates/viewer/re_component_ui/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random thought: currently we silently opt-out when ViewerContext::render_ctx is None.
Maybe we should panic instead in debug builds?

That way we can run tests that don't require the render_ctx by setting it to None, but will not accidentally forget to set it for tests that do need it (like the colormap). i.e., forgetting to call setup_kittest_for_rendering will be a hard error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're soooo close to having this be a thing of the past 😄
let's hold on for a bit

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -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| {
Expand Down
3 changes: 2 additions & 1 deletion crates/viewer/re_time_panel/tests/time_panel_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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| {
Expand Down
3 changes: 2 additions & 1 deletion crates/viewer/re_view_graph/tests/basic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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| {
Expand Down
2 changes: 0 additions & 2 deletions crates/viewer/re_viewer/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand Down Expand Up @@ -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()
Expand Down
4 changes: 2 additions & 2 deletions crates/viewer/re_viewer/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
9 changes: 9 additions & 0 deletions crates/viewer/re_viewer_context/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ workspace = true
[package.metadata.docs.rs]
all-features = true

[features]
## Enable for testing utilities.
testing = ["dep:pollster", "dep:egui_kittest", "dep:env_logger"]

[dependencies]
re_capabilities.workspace = true
re_chunk_store.workspace = true
Expand Down Expand Up @@ -69,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 = [
Expand Down
4 changes: 3 additions & 1 deletion crates/viewer/re_viewer_context/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down
140 changes: 139 additions & 1 deletion crates/viewer/re_viewer_context/src/test_context.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
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::ResultExt as _;
use re_log_types::{StoreId, StoreKind};
use re_types_core::reflection::Reflection;

Expand Down Expand Up @@ -40,10 +44,15 @@ pub struct TestContext {

command_sender: CommandSender,
command_receiver: CommandReceiver,
egui_render_state: Mutex<Option<egui_wgpu::RenderState>>,
}

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));

Expand Down Expand Up @@ -72,11 +81,122 @@ 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() -> Option<egui_wgpu::RenderState> {
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_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.
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_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_setup.adapter,
shared_wgpu_setup.device.clone(),
shared_wgpu_setup.queue.clone(),
wgpu::TextureFormat::Rgba8Unorm,
)
.expect("Failed to initialize re_renderer"),
);
Some(render_state)
}

/// Instance & adapter
struct SharedWgpuResources {
instance: Arc<wgpu::Instance>,
adapter: Arc<wgpu::Adapter>,
device: Arc<wgpu::Device>,

// 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<wgpu::Queue>,
}

static SHARED_WGPU_RENDERER_SETUP: Lazy<Option<SharedWgpuResources>> =
Lazy::new(try_init_shared_renderer_setup);

fn try_init_shared_renderer_setup() -> Option<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
// 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,
}))?;

let device_caps = re_renderer::config::DeviceCaps::from_adapter(&adapter)
.warn_on_err_once("Failed to determine device capabilities")?;
let (device, queue) =
pollster::block_on(adapter.request_device(&device_caps.device_descriptor(), None))
.warn_on_err_once("Failed to request device.")?;

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<()> {
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.
pub fn active_timeline(&self) -> re_chunk::Timeline {
*self.recording_config.time_ctrl.read().timeline()
Expand Down Expand Up @@ -108,6 +228,20 @@ impl TestContext {

let drag_and_drop_manager = crate::DragAndDropManager::new(ItemCollection::default());

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::<re_renderer::RenderContext>()
.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(),
cache: &Default::default(),
Expand All @@ -123,13 +257,17 @@ impl TestContext {
selection_state: &self.selection_state,
blueprint_query: &self.blueprint_query,
egui_ctx,
render_ctx: None,
render_ctx: render_ctx.as_deref(),
command_sender: &self.command_sender,
focused_item: &None,
drag_and_drop_manager: &drag_and_drop_manager,
};

func(&ctx);

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
Expand Down
3 changes: 3 additions & 0 deletions crates/viewer/re_viewport_blueprint/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"] }
21 changes: 11 additions & 10 deletions deny.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line is new, nothing else changed

{ 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
Expand Down
Loading