From 8e4b0d88d9d01df8121a84850b251c1840494695 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Tue, 5 Dec 2023 11:43:50 +0100 Subject: [PATCH] comment fixes --- crates/re_renderer/src/view_builder.rs | 2 +- .../wgpu_resources/static_resource_pool.rs | 37 ++++++++----------- .../src/gpu_bridge/re_renderer_callback.rs | 11 +++--- 3 files changed, 23 insertions(+), 27 deletions(-) diff --git a/crates/re_renderer/src/view_builder.rs b/crates/re_renderer/src/view_builder.rs index 23c06d87025b..a022aa907872 100644 --- a/crates/re_renderer/src/view_builder.rs +++ b/crates/re_renderer/src/view_builder.rs @@ -554,7 +554,7 @@ impl ViewBuilder { // Renderers can't be added anyways at this point (RendererData add their Renderer on creation), // so no point in taking the lock repeatedly. // - // Note that this is a limitation that will be lifted in future versions of wgpu. + // TODO(gfx-rs/wgpu#1453): Note that this is a limitation that will be lifted in future versions of wgpu. // However, having our locking concentrated for the duration of a view draw // is also beneficial since it enforces the model of prepare->draw which avoids a lot of repeated // locking and unlocking. diff --git a/crates/re_renderer/src/wgpu_resources/static_resource_pool.rs b/crates/re_renderer/src/wgpu_resources/static_resource_pool.rs index 04e24c2b7065..6d175040e22c 100644 --- a/crates/re_renderer/src/wgpu_resources/static_resource_pool.rs +++ b/crates/re_renderer/src/wgpu_resources/static_resource_pool.rs @@ -49,30 +49,24 @@ where Desc: std::fmt::Debug + Clone + Eq + Hash, { pub fn get_or_create Res>(&self, desc: &Desc, creation_func: F) -> Handle { - { - // Ensure the lock isn't held in the creation case. - if let Some(handle) = self.lookup.read().get(desc) { - return *handle; - } + // Ensure the lock isn't held in the creation case. + if let Some(handle) = self.lookup.read().get(desc) { + return *handle; } - { - re_tracing::profile_scope!( - "Creating new static resource", - std::any::type_name::() - ); - let resource = creation_func(desc); - let handle = self.resources.write().insert(StoredResource { - resource, - statistics: ResourceStatistics { - frame_created: self.current_frame_index, - last_frame_used: self.current_frame_index.into(), - }, - }); - self.lookup.write().insert(desc.clone(), handle); + re_tracing::profile_scope!("Creating new static resource", std::any::type_name::()); - handle - } + let resource = creation_func(desc); + let handle = self.resources.write().insert(StoredResource { + resource, + statistics: ResourceStatistics { + frame_created: self.current_frame_index, + last_frame_used: self.current_frame_index.into(), + }, + }); + self.lookup.write().insert(desc.clone(), handle); + + handle } pub fn recreate_resources Option>(&mut self, mut recreation_func: F) { @@ -137,6 +131,7 @@ fn to_pool_error(get_result: Option, handle: impl Key) -> Result { fn get(&self, handle: Handle) -> Result<&Res, PoolError>; } diff --git a/crates/re_viewer_context/src/gpu_bridge/re_renderer_callback.rs b/crates/re_viewer_context/src/gpu_bridge/re_renderer_callback.rs index 979049ef357b..00259e74b1c1 100644 --- a/crates/re_viewer_context/src/gpu_bridge/re_renderer_callback.rs +++ b/crates/re_viewer_context/src/gpu_bridge/re_renderer_callback.rs @@ -71,7 +71,7 @@ impl egui_wgpu::CallbackTrait for ReRendererCallback { return Vec::new(); }; - let command_buffer = match view_builder.draw(ctx, self.clear_color) { + match view_builder.draw(ctx, self.clear_color) { Ok(command_buffer) => { // If drawing worked, put the view_builder back in so we can use it during paint. ctx.active_frame @@ -90,9 +90,7 @@ impl egui_wgpu::CallbackTrait for ReRendererCallback { // TODO(andreas): It would be nice to paint an error message instead. Vec::new() } - }; - - command_buffer + } } fn finish_prepare( @@ -114,7 +112,7 @@ impl egui_wgpu::CallbackTrait for ReRendererCallback { // are longer lived than the pass itself. // This is a bit of a conundrum since we can't store a lock guard in the callback resources. // So instead, we work around this by moving the render pipelines out of their lock! - // Future wgpu versions will lift this restriction and will allow us to remove this workaround. + // TODO(gfx-rs/wgpu#1453): Future wgpu versions will lift this restriction and will allow us to remove this workaround. if ctx.active_frame.pinned_render_pipelines.is_none() { let render_pipelines = ctx.gpu_resources.render_pipelines.take_resources(); ctx.active_frame.pinned_render_pipelines = Some(render_pipelines); @@ -130,12 +128,14 @@ impl egui_wgpu::CallbackTrait for ReRendererCallback { paint_callback_resources: &'a egui_wgpu::CallbackResources, ) { let Some(ctx) = paint_callback_resources.get::() else { + // TODO(#4433): Shouldn't show up like this. re_log::error_once!( "Failed to execute egui draw callback. No render context available." ); return; }; let Some(render_pipelines) = ctx.active_frame.pinned_render_pipelines.as_ref() else { + // TODO(#4433): Shouldn't show up like this. re_log::error_once!( "Failed to execute egui draw callback. Render pipelines weren't transferred out of the pool first." ); @@ -148,6 +148,7 @@ impl egui_wgpu::CallbackTrait for ReRendererCallback { .get::() .and_then(|view_builder_map| view_builder_map.get(self.view_builder)) else { + // TODO(#4433): Shouldn't show up like this. re_log::error_once!( "Failed to execute egui draw callback. View builder with handle {:?} not found.", self.view_builder