From c5c8387cd1080fd1368e94d78d525d87efb4c5c5 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 30 Nov 2023 23:58:20 +0100 Subject: [PATCH 1/7] Make static resource pool interior mutable with some compromises --- crates/re_renderer/examples/framework.rs | 4 + crates/re_renderer/src/context.rs | 19 +- .../re_renderer/src/draw_phases/outlines.rs | 14 +- .../src/draw_phases/picking_layer.rs | 27 +-- crates/re_renderer/src/queuable_draw_data.rs | 11 +- crates/re_renderer/src/renderer/compositor.rs | 8 +- .../re_renderer/src/renderer/debug_overlay.rs | 8 +- .../re_renderer/src/renderer/depth_cloud.rs | 7 +- .../src/renderer/generic_skybox.rs | 7 +- crates/re_renderer/src/renderer/lines.rs | 7 +- .../re_renderer/src/renderer/mesh_renderer.rs | 7 +- crates/re_renderer/src/renderer/mod.rs | 4 +- .../re_renderer/src/renderer/point_cloud.rs | 8 +- crates/re_renderer/src/renderer/rectangles.rs | 7 +- .../re_renderer/src/renderer/test_triangle.rs | 4 +- crates/re_renderer/src/view_builder.rs | 67 +++++-- .../wgpu_resources/bind_group_layout_pool.rs | 15 +- .../src/wgpu_resources/bind_group_pool.rs | 14 +- crates/re_renderer/src/wgpu_resources/mod.rs | 16 +- .../wgpu_resources/pipeline_layout_pool.rs | 31 +-- .../wgpu_resources/render_pipeline_pool.rs | 46 ++++- .../src/wgpu_resources/sampler_pool.rs | 14 +- .../src/wgpu_resources/shader_module_pool.rs | 19 +- .../wgpu_resources/static_resource_pool.rs | 189 +++++++++++++----- .../src/gpu_bridge/re_renderer_callback.rs | 42 +++- 25 files changed, 418 insertions(+), 177 deletions(-) diff --git a/crates/re_renderer/examples/framework.rs b/crates/re_renderer/examples/framework.rs index 6ee3231e4ee3..37f635bbbf07 100644 --- a/crates/re_renderer/examples/framework.rs +++ b/crates/re_renderer/examples/framework.rs @@ -276,6 +276,9 @@ impl Application { ); { + // Lock render pipelines for the lifetime of the composite pass. + let render_pipelines = self.re_ctx.gpu_resources.render_pipelines.resources(); + let mut composite_pass = composite_cmd_encoder.begin_render_pass(&wgpu::RenderPassDescriptor { label: None, @@ -295,6 +298,7 @@ impl Application { for draw_result in &draw_results { draw_result.view_builder.composite( &self.re_ctx, + &render_pipelines, &mut composite_pass, draw_result.target_location, ); diff --git a/crates/re_renderer/src/context.rs b/crates/re_renderer/src/context.rs index 6f0c888d023a..59f60f89bdcb 100644 --- a/crates/re_renderer/src/context.rs +++ b/crates/re_renderer/src/context.rs @@ -9,7 +9,7 @@ use crate::{ global_bindings::GlobalBindings, renderer::Renderer, resource_managers::{MeshManager, TextureManager2D}, - wgpu_resources::WgpuResourcePools, + wgpu_resources::{GpuRenderPipelinePoolMemMoveAccessor, WgpuResourcePools}, FileResolver, FileServer, FileSystem, RecommendedFileResolver, }; @@ -187,6 +187,7 @@ impl RenderContext { let active_frame = ActiveFrameContext { before_view_builder_encoder: Mutex::new(FrameGlobalCommandEncoder::new(&device)), per_frame_data_helper: TypeMap::new(), + moved_render_pipelines: None, frame_index: 0, }; @@ -299,9 +300,18 @@ impl RenderContext { // Map all read staging buffers. self.gpu_readback_belt.get_mut().after_queue_submit(); + // Give back moved render pipelines to the pool if any were moved out. + if let Some(moved_render_pipelines) = self.active_frame.moved_render_pipelines.take() { + self.gpu_resources + .render_pipelines + .return_resources(moved_render_pipelines); + } + + // New active frame! self.active_frame = ActiveFrameContext { before_view_builder_encoder: Mutex::new(FrameGlobalCommandEncoder::new(&self.device)), frame_index: self.active_frame.frame_index + 1, + moved_render_pipelines: None, per_frame_data_helper: TypeMap::new(), }; let frame_index = self.active_frame.frame_index; @@ -431,6 +441,13 @@ pub struct ActiveFrameContext { /// Utility type map that will be cleared every frame. pub per_frame_data_helper: TypeMap, + /// Render pipelines that were moved out of the resource pool. + /// + /// Will be moved back to the resource pool at the start of the frame. + /// This is needed for accessing the render pipelines without keeping a reference + /// to the resource pool lock during the lifetime of a render pass. + pub moved_render_pipelines: Option, + /// Index of this frame. Is incremented for every render frame. frame_index: u64, } diff --git a/crates/re_renderer/src/draw_phases/outlines.rs b/crates/re_renderer/src/draw_phases/outlines.rs index e57e35511791..e2cac2b179e6 100644 --- a/crates/re_renderer/src/draw_phases/outlines.rs +++ b/crates/re_renderer/src/draw_phases/outlines.rs @@ -51,8 +51,8 @@ use crate::{ view_builder::ViewBuilder, wgpu_resources::{ BindGroupDesc, BindGroupEntry, BindGroupLayoutDesc, GpuBindGroup, GpuBindGroupLayoutHandle, - GpuRenderPipelineHandle, GpuTexture, PipelineLayoutDesc, PoolError, RenderPipelineDesc, - SamplerDesc, WgpuResourcePools, + GpuRenderPipelineHandle, GpuRenderPipelinePoolAccessor, GpuTexture, PipelineLayoutDesc, + PoolError, RenderPipelineDesc, SamplerDesc, }, DebugLabel, RenderContext, }; @@ -372,11 +372,9 @@ impl OutlineMaskProcessor { pub fn compute_outlines( self, - pools: &WgpuResourcePools, + pipelines: &GpuRenderPipelinePoolAccessor<'_>, encoder: &mut wgpu::CommandEncoder, ) -> Result<(), PoolError> { - let pipelines = &pools.render_pipelines; - let ops = wgpu::Operations { load: wgpu::LoadOp::Clear(wgpu::Color::TRANSPARENT), // Clear is the closest to "don't care" store: wgpu::StoreOp::Store, @@ -396,16 +394,14 @@ impl OutlineMaskProcessor { occlusion_query_set: None, }); - let render_pipeline_init = - pipelines.get_resource(self.render_pipeline_jumpflooding_init)?; + let render_pipeline_init = pipelines.get(self.render_pipeline_jumpflooding_init)?; jumpflooding_init.set_bind_group(0, &self.bind_group_jumpflooding_init, &[]); jumpflooding_init.set_pipeline(render_pipeline_init); jumpflooding_init.draw(0..3, 0..1); } // Perform jump flooding. - let render_pipeline_step = - pipelines.get_resource(self.render_pipeline_jumpflooding_step)?; + let render_pipeline_step = pipelines.get(self.render_pipeline_jumpflooding_step)?; for (i, bind_group) in self.bind_group_jumpflooding_steps.into_iter().enumerate() { let mut jumpflooding_step = encoder.begin_render_pass(&wgpu::RenderPassDescriptor { label: DebugLabel::from(format!("{} - jumpflooding_step {i}", self.label)).get(), diff --git a/crates/re_renderer/src/draw_phases/picking_layer.rs b/crates/re_renderer/src/draw_phases/picking_layer.rs index c11ca73f4271..2bfe998f1b8b 100644 --- a/crates/re_renderer/src/draw_phases/picking_layer.rs +++ b/crates/re_renderer/src/draw_phases/picking_layer.rs @@ -19,8 +19,8 @@ use crate::{ view_builder::ViewBuilder, wgpu_resources::{ BindGroupDesc, BindGroupEntry, BindGroupLayoutDesc, GpuBindGroup, GpuRenderPipelineHandle, - GpuTexture, GpuTextureHandle, PipelineLayoutDesc, PoolError, RenderPipelineDesc, - TextureDesc, WgpuResourcePools, + GpuRenderPipelinePoolAccessor, GpuTexture, GpuTextureHandle, PipelineLayoutDesc, PoolError, + RenderPipelineDesc, TextureDesc, }, DebugLabel, GpuReadbackBuffer, GpuReadbackIdentifier, RectInt, RenderContext, }; @@ -344,20 +344,23 @@ impl PickingLayerProcessor { pub fn end_render_pass( self, encoder: &mut wgpu::CommandEncoder, - pools: &WgpuResourcePools, + render_pipelines: &GpuRenderPipelinePoolAccessor<'_>, ) -> Result<(), PickingLayerError> { let extent = glam::uvec2( self.picking_target.texture.width(), self.picking_target.texture.height(), ); - let readable_depth_texture = if let Some(depth_copy_workaround) = - self.depth_readback_workaround.as_ref() - { - depth_copy_workaround.copy_to_readable_texture(encoder, pools, &self.bind_group_0)? - } else { - &self.picking_depth_target - }; + let readable_depth_texture = + if let Some(depth_copy_workaround) = self.depth_readback_workaround.as_ref() { + depth_copy_workaround.copy_to_readable_texture( + encoder, + render_pipelines, + &self.bind_group_0, + )? + } else { + &self.picking_depth_target + }; self.readback_buffer.read_multiple_texture2d( encoder, @@ -581,7 +584,7 @@ impl DepthReadbackWorkaround { fn copy_to_readable_texture( &self, encoder: &mut wgpu::CommandEncoder, - pools: &WgpuResourcePools, + render_pipelines: &GpuRenderPipelinePoolAccessor<'_>, global_binding_bind_group: &GpuBindGroup, ) -> Result<&GpuTexture, PoolError> { // Copy depth texture to a readable (color) texture with a screen filling triangle. @@ -600,7 +603,7 @@ impl DepthReadbackWorkaround { occlusion_query_set: None, }); - let pipeline = pools.render_pipelines.get_resource(self.render_pipeline)?; + let pipeline = render_pipelines.get(self.render_pipeline)?; pass.set_pipeline(pipeline); pass.set_bind_group(0, global_binding_bind_group, &[]); pass.set_bind_group(1, &self.bind_group, &[]); diff --git a/crates/re_renderer/src/queuable_draw_data.rs b/crates/re_renderer/src/queuable_draw_data.rs index b4abbba6c993..de13479b9dc0 100644 --- a/crates/re_renderer/src/queuable_draw_data.rs +++ b/crates/re_renderer/src/queuable_draw_data.rs @@ -1,7 +1,8 @@ use crate::{ + context::Renderers, draw_phases::DrawPhase, renderer::{DrawData, DrawError, Renderer}, - RenderContext, + wgpu_resources::GpuRenderPipelinePoolAccessor, }; #[derive(thiserror::Error, Debug)] @@ -17,7 +18,8 @@ pub enum QueueableDrawDataError { } type DrawFn = dyn for<'a, 'b> Fn( - &'b RenderContext, + &Renderers, + &'b GpuRenderPipelinePoolAccessor<'b>, DrawPhase, &'a mut wgpu::RenderPass<'b>, &'b dyn std::any::Any, @@ -36,8 +38,7 @@ pub struct QueueableDrawData { impl From for QueueableDrawData { fn from(draw_data: D) -> Self { QueueableDrawData { - draw_func: Box::new(move |ctx, phase, pass, draw_data| { - let renderers = ctx.renderers.read(); + draw_func: Box::new(move |renderers, gpu_resources, phase, pass, draw_data| { let renderer = renderers.get::().ok_or( QueueableDrawDataError::FailedToRetrieveRenderer(std::any::type_name::< D::Renderer, @@ -47,7 +48,7 @@ impl From for QueueableDrawData { QueueableDrawDataError::UnexpectedDrawDataType(std::any::type_name::()), )?; renderer - .draw(&ctx.gpu_resources, phase, pass, draw_data) + .draw(gpu_resources, phase, pass, draw_data) .map_err(QueueableDrawDataError::from) }), draw_data: Box::new(draw_data), diff --git a/crates/re_renderer/src/renderer/compositor.rs b/crates/re_renderer/src/renderer/compositor.rs index f3ee7fbfbb2e..0ce1b58c0f17 100644 --- a/crates/re_renderer/src/renderer/compositor.rs +++ b/crates/re_renderer/src/renderer/compositor.rs @@ -6,8 +6,8 @@ use crate::{ view_builder::ViewBuilder, wgpu_resources::{ BindGroupDesc, BindGroupEntry, BindGroupLayoutDesc, GpuBindGroup, GpuBindGroupLayoutHandle, - GpuRenderPipelineHandle, GpuTexture, PipelineLayoutDesc, RenderPipelineDesc, - WgpuResourcePools, + GpuRenderPipelineHandle, GpuRenderPipelinePoolAccessor, GpuTexture, PipelineLayoutDesc, + RenderPipelineDesc, WgpuResourcePools, }, OutlineConfig, Rgba, }; @@ -206,7 +206,7 @@ impl Renderer for Compositor { fn draw<'a>( &self, - pools: &'a WgpuResourcePools, + render_pipelines: &'a GpuRenderPipelinePoolAccessor<'a>, phase: DrawPhase, pass: &mut wgpu::RenderPass<'a>, draw_data: &'a CompositorDrawData, @@ -216,7 +216,7 @@ impl Renderer for Compositor { DrawPhase::CompositingScreenshot => self.render_pipeline_screenshot, _ => unreachable!("We were called on a phase we weren't subscribed to: {phase:?}"), }; - let pipeline = pools.render_pipelines.get_resource(pipeline_handle)?; + let pipeline = render_pipelines.get(pipeline_handle)?; pass.set_pipeline(pipeline); pass.set_bind_group(1, &draw_data.bind_group, &[]); diff --git a/crates/re_renderer/src/renderer/debug_overlay.rs b/crates/re_renderer/src/renderer/debug_overlay.rs index 7f7eff15370b..4fc601833216 100644 --- a/crates/re_renderer/src/renderer/debug_overlay.rs +++ b/crates/re_renderer/src/renderer/debug_overlay.rs @@ -5,8 +5,8 @@ use crate::{ include_shader_module, wgpu_resources::{ BindGroupDesc, BindGroupEntry, BindGroupLayoutDesc, GpuBindGroup, GpuBindGroupLayoutHandle, - GpuRenderPipelineHandle, GpuTexture, PipelineLayoutDesc, RenderPipelineDesc, - WgpuResourcePools, + GpuRenderPipelineHandle, GpuRenderPipelinePoolAccessor, GpuTexture, PipelineLayoutDesc, + RenderPipelineDesc, WgpuResourcePools, }, RectInt, }; @@ -239,12 +239,12 @@ impl Renderer for DebugOverlayRenderer { fn draw<'a>( &self, - pools: &'a WgpuResourcePools, + render_pipelines: &'a GpuRenderPipelinePoolAccessor<'a>, _phase: DrawPhase, pass: &mut wgpu::RenderPass<'a>, draw_data: &'a DebugOverlayDrawData, ) -> Result<(), DrawError> { - let pipeline = pools.render_pipelines.get_resource(self.render_pipeline)?; + let pipeline = render_pipelines.get(self.render_pipeline)?; pass.set_pipeline(pipeline); pass.set_bind_group(1, &draw_data.bind_group, &[]); diff --git a/crates/re_renderer/src/renderer/depth_cloud.rs b/crates/re_renderer/src/renderer/depth_cloud.rs index a9250b69fece..a2c5e5201950 100644 --- a/crates/re_renderer/src/renderer/depth_cloud.rs +++ b/crates/re_renderer/src/renderer/depth_cloud.rs @@ -22,7 +22,8 @@ use crate::{ view_builder::ViewBuilder, wgpu_resources::{ BindGroupDesc, BindGroupEntry, BindGroupLayoutDesc, GpuBindGroup, GpuBindGroupLayoutHandle, - GpuRenderPipelineHandle, PipelineLayoutDesc, RenderPipelineDesc, + GpuRenderPipelineHandle, GpuRenderPipelinePoolAccessor, PipelineLayoutDesc, + RenderPipelineDesc, }, Colormap, OutlineMaskPreference, PickingLayerObjectId, PickingLayerProcessor, }; @@ -500,7 +501,7 @@ impl Renderer for DepthCloudRenderer { fn draw<'a>( &self, - pools: &'a WgpuResourcePools, + render_pipelines: &'a GpuRenderPipelinePoolAccessor<'a>, phase: DrawPhase, pass: &mut wgpu::RenderPass<'a>, draw_data: &'a Self::RendererDrawData, @@ -516,7 +517,7 @@ impl Renderer for DepthCloudRenderer { DrawPhase::OutlineMask => self.render_pipeline_outline_mask, _ => unreachable!("We were called on a phase we weren't subscribed to: {phase:?}"), }; - let pipeline = pools.render_pipelines.get_resource(pipeline_handle)?; + let pipeline = render_pipelines.get(pipeline_handle)?; pass.set_pipeline(pipeline); diff --git a/crates/re_renderer/src/renderer/generic_skybox.rs b/crates/re_renderer/src/renderer/generic_skybox.rs index 732e51ac8e21..55892af7fee3 100644 --- a/crates/re_renderer/src/renderer/generic_skybox.rs +++ b/crates/re_renderer/src/renderer/generic_skybox.rs @@ -7,7 +7,8 @@ use crate::{ renderer::screen_triangle_vertex_shader, view_builder::ViewBuilder, wgpu_resources::{ - GpuRenderPipelineHandle, PipelineLayoutDesc, RenderPipelineDesc, WgpuResourcePools, + GpuRenderPipelineHandle, GpuRenderPipelinePoolAccessor, PipelineLayoutDesc, + RenderPipelineDesc, WgpuResourcePools, }, }; @@ -96,14 +97,14 @@ impl Renderer for GenericSkybox { fn draw<'a>( &self, - pools: &'a WgpuResourcePools, + render_pipelines: &'a GpuRenderPipelinePoolAccessor<'a>, _phase: DrawPhase, pass: &mut wgpu::RenderPass<'a>, _draw_data: &GenericSkyboxDrawData, ) -> Result<(), DrawError> { re_tracing::profile_function!(); - let pipeline = pools.render_pipelines.get_resource(self.render_pipeline)?; + let pipeline = render_pipelines.get(self.render_pipeline)?; pass.set_pipeline(pipeline); pass.draw(0..3, 0..1); diff --git a/crates/re_renderer/src/renderer/lines.rs b/crates/re_renderer/src/renderer/lines.rs index 3d335bd8439b..02948522ead9 100644 --- a/crates/re_renderer/src/renderer/lines.rs +++ b/crates/re_renderer/src/renderer/lines.rs @@ -119,7 +119,8 @@ use crate::{ view_builder::ViewBuilder, wgpu_resources::{ BindGroupDesc, BindGroupEntry, BindGroupLayoutDesc, GpuBindGroup, GpuBindGroupLayoutHandle, - GpuRenderPipelineHandle, PipelineLayoutDesc, PoolError, RenderPipelineDesc, TextureDesc, + GpuRenderPipelineHandle, GpuRenderPipelinePoolAccessor, PipelineLayoutDesc, PoolError, + RenderPipelineDesc, TextureDesc, }, Color32, DebugLabel, DepthOffset, LineStripSeriesBuilder, OutlineMaskPreference, PickingLayerObjectId, PickingLayerProcessor, @@ -961,7 +962,7 @@ impl Renderer for LineRenderer { fn draw<'a>( &self, - pools: &'a WgpuResourcePools, + render_pipelines: &'a GpuRenderPipelinePoolAccessor<'a>, phase: DrawPhase, pass: &mut wgpu::RenderPass<'a>, draw_data: &'a Self::RendererDrawData, @@ -982,7 +983,7 @@ impl Renderer for LineRenderer { return Ok(()); // No lines submitted. }; - let pipeline = pools.render_pipelines.get_resource(pipeline_handle)?; + let pipeline = render_pipelines.get(pipeline_handle)?; pass.set_pipeline(pipeline); pass.set_bind_group(1, bind_group_all_lines, &[]); diff --git a/crates/re_renderer/src/renderer/mesh_renderer.rs b/crates/re_renderer/src/renderer/mesh_renderer.rs index e813f30c016c..f1a09a87c409 100644 --- a/crates/re_renderer/src/renderer/mesh_renderer.rs +++ b/crates/re_renderer/src/renderer/mesh_renderer.rs @@ -16,7 +16,8 @@ use crate::{ view_builder::ViewBuilder, wgpu_resources::{ BindGroupLayoutDesc, BufferDesc, GpuBindGroupLayoutHandle, GpuBuffer, - GpuRenderPipelineHandle, PipelineLayoutDesc, RenderPipelineDesc, + GpuRenderPipelineHandle, GpuRenderPipelinePoolAccessor, PipelineLayoutDesc, + RenderPipelineDesc, }, Color32, OutlineMaskPreference, PickingLayerId, PickingLayerProcessor, }; @@ -416,7 +417,7 @@ impl Renderer for MeshRenderer { fn draw<'a>( &self, - pools: &'a WgpuResourcePools, + render_pipelines: &'a GpuRenderPipelinePoolAccessor<'a>, phase: DrawPhase, pass: &mut wgpu::RenderPass<'a>, draw_data: &'a Self::RendererDrawData, @@ -433,7 +434,7 @@ impl Renderer for MeshRenderer { DrawPhase::PickingLayer => self.render_pipeline_picking_layer, _ => unreachable!("We were called on a phase we weren't subscribed to: {phase:?}"), }; - let pipeline = pools.render_pipelines.get_resource(pipeline_handle)?; + let pipeline = render_pipelines.get(pipeline_handle)?; pass.set_pipeline(pipeline); diff --git a/crates/re_renderer/src/renderer/mod.rs b/crates/re_renderer/src/renderer/mod.rs index d86a59d90ffe..66ed89e3a576 100644 --- a/crates/re_renderer/src/renderer/mod.rs +++ b/crates/re_renderer/src/renderer/mod.rs @@ -39,7 +39,7 @@ use crate::{ context::{RenderContext, SharedRendererData}, draw_phases::DrawPhase, include_shader_module, - wgpu_resources::{PoolError, WgpuResourcePools}, + wgpu_resources::{GpuRenderPipelinePoolAccessor, PoolError, WgpuResourcePools}, FileResolver, FileSystem, }; @@ -76,7 +76,7 @@ pub trait Renderer { /// Called once per phase given by [`Renderer::participated_phases`]. fn draw<'a>( &self, - pools: &'a WgpuResourcePools, + render_pipelines: &'a GpuRenderPipelinePoolAccessor<'a>, phase: DrawPhase, pass: &mut wgpu::RenderPass<'a>, draw_data: &'a Self::RendererDrawData, diff --git a/crates/re_renderer/src/renderer/point_cloud.rs b/crates/re_renderer/src/renderer/point_cloud.rs index df4515ec85de..ad8c8ec77f30 100644 --- a/crates/re_renderer/src/renderer/point_cloud.rs +++ b/crates/re_renderer/src/renderer/point_cloud.rs @@ -18,7 +18,9 @@ use std::{num::NonZeroU64, ops::Range}; use crate::{ allocator::create_and_fill_uniform_buffer_batch, draw_phases::{DrawPhase, OutlineMaskProcessor, PickingLayerObjectId, PickingLayerProcessor}, - include_shader_module, DebugLabel, DepthOffset, OutlineMaskPreference, PointCloudBuilder, + include_shader_module, + wgpu_resources::GpuRenderPipelinePoolAccessor, + DebugLabel, DepthOffset, OutlineMaskPreference, PointCloudBuilder, }; use bitflags::bitflags; use bytemuck::Zeroable as _; @@ -711,7 +713,7 @@ impl Renderer for PointCloudRenderer { fn draw<'a>( &self, - pools: &'a WgpuResourcePools, + render_pipelines: &'a GpuRenderPipelinePoolAccessor<'a>, phase: DrawPhase, pass: &mut wgpu::RenderPass<'a>, draw_data: &'a Self::RendererDrawData, @@ -731,7 +733,7 @@ impl Renderer for PointCloudRenderer { let Some(bind_group_all_points) = bind_group_all_points else { return Ok(()); // No points submitted. }; - let pipeline = pools.render_pipelines.get_resource(pipeline_handle)?; + let pipeline = render_pipelines.get(pipeline_handle)?; pass.set_pipeline(pipeline); pass.set_bind_group(1, bind_group_all_points, &[]); diff --git a/crates/re_renderer/src/renderer/rectangles.rs b/crates/re_renderer/src/renderer/rectangles.rs index ce8e52ea9d0b..5c6caa1e1111 100644 --- a/crates/re_renderer/src/renderer/rectangles.rs +++ b/crates/re_renderer/src/renderer/rectangles.rs @@ -22,7 +22,8 @@ use crate::{ view_builder::ViewBuilder, wgpu_resources::{ BindGroupDesc, BindGroupEntry, BindGroupLayoutDesc, GpuBindGroup, GpuBindGroupLayoutHandle, - GpuRenderPipelineHandle, PipelineLayoutDesc, RenderPipelineDesc, + GpuRenderPipelineHandle, GpuRenderPipelinePoolAccessor, PipelineLayoutDesc, + RenderPipelineDesc, }, Colormap, OutlineMaskPreference, PickingLayerProcessor, Rgba, }; @@ -667,7 +668,7 @@ impl Renderer for RectangleRenderer { fn draw<'a>( &self, - pools: &'a WgpuResourcePools, + render_pipelines: &'a GpuRenderPipelinePoolAccessor<'a>, phase: DrawPhase, pass: &mut wgpu::RenderPass<'a>, draw_data: &'a Self::RendererDrawData, @@ -683,7 +684,7 @@ impl Renderer for RectangleRenderer { DrawPhase::OutlineMask => self.render_pipeline_outline_mask, _ => unreachable!("We were called on a phase we weren't subscribed to: {phase:?}"), }; - let pipeline = pools.render_pipelines.get_resource(pipeline_handle)?; + let pipeline = render_pipelines.get(pipeline_handle)?; pass.set_pipeline(pipeline); diff --git a/crates/re_renderer/src/renderer/test_triangle.rs b/crates/re_renderer/src/renderer/test_triangle.rs index 49d110608b25..2288d47d9ff7 100644 --- a/crates/re_renderer/src/renderer/test_triangle.rs +++ b/crates/re_renderer/src/renderer/test_triangle.rs @@ -87,12 +87,12 @@ impl Renderer for TestTriangle { fn draw<'a>( &self, - pools: &'a WgpuResourcePools, + render_pipelines: &'a GpuRenderPipelinePoolAccessor<'a>, _phase: DrawPhase, pass: &mut wgpu::RenderPass<'a>, _draw_data: &TestTriangleDrawData, ) -> Result<(), DrawError> { - let pipeline = pools.render_pipelines.get_resource(self.render_pipeline)?; + let pipeline = render_pipelines.get(self.render_pipeline)?; pass.set_pipeline(pipeline); pass.draw(0..3, 0..1); Ok(()) diff --git a/crates/re_renderer/src/view_builder.rs b/crates/re_renderer/src/view_builder.rs index 07dc9e4d6b33..23c06d87025b 100644 --- a/crates/re_renderer/src/view_builder.rs +++ b/crates/re_renderer/src/view_builder.rs @@ -4,7 +4,7 @@ use std::sync::Arc; use crate::{ allocator::{create_and_fill_uniform_buffer, GpuReadbackIdentifier}, - context::RenderContext, + context::{RenderContext, Renderers}, draw_phases::{ DrawPhase, OutlineConfig, OutlineMaskProcessor, PickingLayerError, PickingLayerProcessor, ScreenshotProcessor, @@ -13,7 +13,9 @@ use crate::{ queuable_draw_data::QueueableDrawData, renderer::{CompositorDrawData, DebugOverlayDrawData}, transform::RectTransform, - wgpu_resources::{GpuBindGroup, GpuTexture, PoolError, TextureDesc}, + wgpu_resources::{ + GpuBindGroup, GpuRenderPipelinePoolAccessor, GpuTexture, PoolError, TextureDesc, + }, DebugLabel, RectInt, Rgba, Size, }; @@ -500,7 +502,8 @@ impl ViewBuilder { fn draw_phase<'a>( &'a self, - ctx: &'a RenderContext, + renderers: &Renderers, + render_pipelines: &'a GpuRenderPipelinePoolAccessor<'a>, phase: DrawPhase, pass: &mut wgpu::RenderPass<'a>, ) { @@ -508,8 +511,14 @@ impl ViewBuilder { for queued_draw in &self.queued_draws { if queued_draw.participated_phases.contains(&phase) { - let res = (queued_draw.draw_func)(ctx, phase, pass, queued_draw.draw_data.as_ref()) - .with_context(|| format!("draw call during phase {phase:?}")); + let res = (queued_draw.draw_func)( + renderers, + render_pipelines, + phase, + pass, + queued_draw.draw_data.as_ref(), + ) + .with_context(|| format!("draw call during phase {phase:?}")); if let Err(err) = res { re_log::error!(renderer=%queued_draw.renderer_name, %err, "renderer failed to draw"); @@ -531,6 +540,27 @@ impl ViewBuilder { ) -> Result { re_tracing::profile_function!(); + // Renderers and render pipelines are locked for the entirety of this method: + // This means it's *not* possible to add renderers or pipelines while drawing is in progress! + // + // This is primarily due to the lifetime association render passes have all passed in resources: + // For dynamic resources like bind groups/textures/buffers we use handles that *store* an arc + // to the wgpu resources to solve this ownership problem. + // But for render pipelines, which we want to be able the resource under a handle via reload, + // so we always have to do some kind of lookup prior to or during rendering. + // Therefore, we just lock the pool for the entirety of the draw which ensures + // that the lock outlives the pass. + // + // 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. + // 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. + let renderers = ctx.renderers.read(); + let pipelines = ctx.gpu_resources.render_pipelines.resources(); + let setup = &self.setup; let mut encoder = ctx @@ -574,7 +604,7 @@ impl ViewBuilder { pass.set_bind_group(0, &setup.bind_group_0, &[]); for phase in [DrawPhase::Opaque, DrawPhase::Background] { - self.draw_phase(ctx, phase, &mut pass); + self.draw_phase(&renderers, &pipelines, phase, &mut pass); } } @@ -592,9 +622,9 @@ impl ViewBuilder { // 3: Draw call in renderer. // //pass.set_bind_group(0, &setup.bind_group_0, &[]); - self.draw_phase(ctx, DrawPhase::PickingLayer, &mut pass); + self.draw_phase(&renderers, &pipelines, DrawPhase::PickingLayer, &mut pass); } - match picking_processor.end_render_pass(&mut encoder, &ctx.gpu_resources) { + match picking_processor.end_render_pass(&mut encoder, &pipelines) { Err(PickingLayerError::ResourcePoolError(err)) => { return Err(err); } @@ -611,16 +641,21 @@ impl ViewBuilder { re_tracing::profile_scope!("outline mask pass"); let mut pass = outline_mask_processor.start_mask_render_pass(&mut encoder); pass.set_bind_group(0, &setup.bind_group_0, &[]); - self.draw_phase(ctx, DrawPhase::OutlineMask, &mut pass); + self.draw_phase(&renderers, &pipelines, DrawPhase::OutlineMask, &mut pass); } - outline_mask_processor.compute_outlines(&ctx.gpu_resources, &mut encoder)?; + outline_mask_processor.compute_outlines(&pipelines, &mut encoder)?; } if let Some(screenshot_processor) = self.screenshot_processor.take() { { let mut pass = screenshot_processor.begin_render_pass(&setup.name, &mut encoder); pass.set_bind_group(0, &setup.bind_group_0, &[]); - self.draw_phase(ctx, DrawPhase::CompositingScreenshot, &mut pass); + self.draw_phase( + &renderers, + &pipelines, + DrawPhase::CompositingScreenshot, + &mut pass, + ); } match screenshot_processor.end_render_pass(&mut encoder) { Ok(()) => {} @@ -751,7 +786,8 @@ impl ViewBuilder { /// `screen_position` specifies where on the output pass the view is placed. pub fn composite<'a>( &'a self, - ctx: &'a RenderContext, + ctx: &RenderContext, + render_pipelines: &'a GpuRenderPipelinePoolAccessor<'a>, pass: &mut wgpu::RenderPass<'a>, screen_position: glam::Vec2, ) { @@ -767,6 +803,11 @@ impl ViewBuilder { ); pass.set_bind_group(0, &self.setup.bind_group_0, &[]); - self.draw_phase(ctx, DrawPhase::Compositing, pass); + self.draw_phase( + &ctx.renderers.read(), + render_pipelines, + DrawPhase::Compositing, + pass, + ); } } diff --git a/crates/re_renderer/src/wgpu_resources/bind_group_layout_pool.rs b/crates/re_renderer/src/wgpu_resources/bind_group_layout_pool.rs index 0ef9e6d46bff..3639776bcd79 100644 --- a/crates/re_renderer/src/wgpu_resources/bind_group_layout_pool.rs +++ b/crates/re_renderer/src/wgpu_resources/bind_group_layout_pool.rs @@ -1,6 +1,6 @@ use crate::debug_label::DebugLabel; -use super::{resource::PoolError, static_resource_pool::StaticResourcePool}; +use super::static_resource_pool::{StaticResourcePool, StaticResourcePoolReadLockAccessor}; slotmap::new_key_type! { pub struct GpuBindGroupLayoutHandle; } @@ -18,7 +18,7 @@ pub struct GpuBindGroupLayoutPool { impl GpuBindGroupLayoutPool { pub fn get_or_create( - &mut self, + &self, device: &wgpu::Device, desc: &BindGroupLayoutDesc, ) -> GpuBindGroupLayoutHandle { @@ -30,11 +30,14 @@ impl GpuBindGroupLayoutPool { }) } - pub fn get_resource( + /// Locks the resource pool for resolving handles. + /// + /// While it is locked, no new resources can be added. + pub fn resources( &self, - handle: GpuBindGroupLayoutHandle, - ) -> Result<&wgpu::BindGroupLayout, PoolError> { - self.pool.get_resource(handle) + ) -> StaticResourcePoolReadLockAccessor<'_, GpuBindGroupLayoutHandle, wgpu::BindGroupLayout> + { + self.pool.resources() } pub fn begin_frame(&mut self, frame_index: u64) { diff --git a/crates/re_renderer/src/wgpu_resources/bind_group_pool.rs b/crates/re_renderer/src/wgpu_resources/bind_group_pool.rs index 5c23bfc5f552..aace9984c1d2 100644 --- a/crates/re_renderer/src/wgpu_resources/bind_group_pool.rs +++ b/crates/re_renderer/src/wgpu_resources/bind_group_pool.rs @@ -9,6 +9,7 @@ use super::{ buffer_pool::{GpuBuffer, GpuBufferHandle, GpuBufferPool}, dynamic_resource_pool::{DynamicResource, DynamicResourcePool, DynamicResourcesDesc}, sampler_pool::{GpuSamplerHandle, GpuSamplerPool}, + static_resource_pool::StaticResourcePoolAccessor as _, texture_pool::{GpuTexture, GpuTextureHandle, GpuTexturePool}, WgpuResourcePools, }; @@ -150,6 +151,8 @@ impl GpuBindGroupPool { let mut buffer_index = 0; let mut texture_index = 0; + let samplers = pools.samplers.resources(); + device.create_bind_group(&wgpu::BindGroupDescriptor { label: desc.label.get(), entries: &desc @@ -180,15 +183,18 @@ impl GpuBindGroupPool { res } BindGroupEntry::Sampler(handle) => wgpu::BindingResource::Sampler( - pools - .samplers - .get_resource(*handle) + samplers + .get(*handle) .expect("BindGroupDesc had an sampler handle"), ), }, }) .collect::>(), - layout: pools.bind_group_layouts.get_resource(desc.layout).unwrap(), + layout: pools + .bind_group_layouts + .resources() + .get(desc.layout) + .unwrap(), }) }); diff --git a/crates/re_renderer/src/wgpu_resources/mod.rs b/crates/re_renderer/src/wgpu_resources/mod.rs index e5e3a389f731..ba66fd46125a 100644 --- a/crates/re_renderer/src/wgpu_resources/mod.rs +++ b/crates/re_renderer/src/wgpu_resources/mod.rs @@ -27,7 +27,8 @@ pub use pipeline_layout_pool::{ mod render_pipeline_pool; pub use render_pipeline_pool::{ - GpuRenderPipelineHandle, GpuRenderPipelinePool, RenderPipelineDesc, VertexBufferLayout, + GpuRenderPipelineHandle, GpuRenderPipelinePool, GpuRenderPipelinePoolAccessor, + GpuRenderPipelinePoolMemMoveAccessor, RenderPipelineDesc, VertexBufferLayout, }; mod sampler_pool; @@ -46,6 +47,7 @@ pub use resource::PoolError; mod dynamic_resource_pool; mod static_resource_pool; +pub use static_resource_pool::StaticResourcePoolAccessor; /// Collection of all wgpu resource pools. /// @@ -55,13 +57,13 @@ mod static_resource_pool; /// for details check their respective allocation/creation functions! #[derive(Default)] pub struct WgpuResourcePools { - pub(crate) bind_group_layouts: GpuBindGroupLayoutPool, - pub(crate) pipeline_layouts: GpuPipelineLayoutPool, - pub(crate) render_pipelines: GpuRenderPipelinePool, - pub(crate) samplers: GpuSamplerPool, - pub(crate) shader_modules: GpuShaderModulePool, + pub bind_group_layouts: GpuBindGroupLayoutPool, + pub pipeline_layouts: GpuPipelineLayoutPool, + pub render_pipelines: GpuRenderPipelinePool, + pub samplers: GpuSamplerPool, + pub shader_modules: GpuShaderModulePool, - pub(crate) bind_groups: GpuBindGroupPool, + pub bind_groups: GpuBindGroupPool, pub buffers: GpuBufferPool, pub textures: GpuTexturePool, diff --git a/crates/re_renderer/src/wgpu_resources/pipeline_layout_pool.rs b/crates/re_renderer/src/wgpu_resources/pipeline_layout_pool.rs index c4b6b4f14872..640774f17d70 100644 --- a/crates/re_renderer/src/wgpu_resources/pipeline_layout_pool.rs +++ b/crates/re_renderer/src/wgpu_resources/pipeline_layout_pool.rs @@ -2,16 +2,13 @@ use crate::debug_label::DebugLabel; use super::{ bind_group_layout_pool::{GpuBindGroupLayoutHandle, GpuBindGroupLayoutPool}, - resource::PoolError, - static_resource_pool::StaticResourcePool, + static_resource_pool::{ + StaticResourcePool, StaticResourcePoolAccessor as _, StaticResourcePoolReadLockAccessor, + }, }; slotmap::new_key_type! { pub struct GpuPipelineLayoutHandle; } -pub struct GpuPipelineLayout { - pub layout: wgpu::PipelineLayout, -} - #[derive(Debug, Clone, Hash, PartialEq, Eq)] pub struct PipelineLayoutDesc { /// Debug label of the pipeline layout. This will show up in graphics debuggers for easy identification. @@ -22,7 +19,7 @@ pub struct PipelineLayoutDesc { #[derive(Default)] pub struct GpuPipelineLayoutPool { - pool: StaticResourcePool, + pool: StaticResourcePool, } impl GpuPipelineLayoutPool { @@ -34,24 +31,28 @@ impl GpuPipelineLayoutPool { ) -> GpuPipelineLayoutHandle { self.pool.get_or_create(desc, |desc| { // TODO(andreas): error handling - let layout = device.create_pipeline_layout(&wgpu::PipelineLayoutDescriptor { + + let bind_groups = bind_group_layout_pool.resources(); + + device.create_pipeline_layout(&wgpu::PipelineLayoutDescriptor { label: desc.label.get(), bind_group_layouts: &desc .entries .iter() - .map(|handle| bind_group_layout_pool.get_resource(*handle).unwrap()) + .map(|handle| bind_groups.get(*handle).unwrap()) .collect::>(), push_constant_ranges: &[], // Sadly not widely supported - }); - GpuPipelineLayout { layout } + }) }) } - pub fn get_resource( + /// Locks the resource pool for resolving handles. + /// + /// While it is locked, no new resources can be added. + pub fn resources( &self, - handle: GpuPipelineLayoutHandle, - ) -> Result<&GpuPipelineLayout, PoolError> { - self.pool.get_resource(handle) + ) -> StaticResourcePoolReadLockAccessor<'_, GpuPipelineLayoutHandle, wgpu::PipelineLayout> { + self.pool.resources() } pub fn begin_frame(&mut self, frame_index: u64) { diff --git a/crates/re_renderer/src/wgpu_resources/render_pipeline_pool.rs b/crates/re_renderer/src/wgpu_resources/render_pipeline_pool.rs index c1584468940a..b950dba727cf 100644 --- a/crates/re_renderer/src/wgpu_resources/render_pipeline_pool.rs +++ b/crates/re_renderer/src/wgpu_resources/render_pipeline_pool.rs @@ -6,7 +6,10 @@ use super::{ pipeline_layout_pool::{GpuPipelineLayoutHandle, GpuPipelineLayoutPool}, resource::PoolError, shader_module_pool::{GpuShaderModuleHandle, GpuShaderModulePool}, - static_resource_pool::StaticResourcePool, + static_resource_pool::{ + StaticResourcePool, StaticResourcePoolAccessor, StaticResourcePoolMemMoveAccessor, + StaticResourcePoolReadLockAccessor, + }, }; slotmap::new_key_type! { pub struct GpuRenderPipelineHandle; } @@ -120,10 +123,12 @@ impl RenderPipelineDesc { pipeline_layouts: &GpuPipelineLayoutPool, shader_modules: &GpuShaderModulePool, ) -> Result { + let pipeline_layouts = pipeline_layouts.resources(); let pipeline_layout = pipeline_layouts - .get_resource(self.pipeline_layout) + .get(self.pipeline_layout) .map_err(RenderPipelineCreationError::PipelineLayout)?; + let shader_modules = shader_modules.resources(); let vertex_shader_module = shader_modules .get(self.vertex_handle) .map_err(RenderPipelineCreationError::VertexShaderNotFound)?; @@ -141,7 +146,7 @@ impl RenderPipelineDesc { Ok( device.create_render_pipeline(&wgpu::RenderPipelineDescriptor { label: self.label.get(), - layout: Some(&pipeline_layout.layout), + layout: Some(pipeline_layout), vertex: wgpu::VertexState { module: vertex_shader_module, entry_point: &self.vertex_entrypoint, @@ -162,6 +167,12 @@ impl RenderPipelineDesc { } } +pub type GpuRenderPipelinePoolAccessor<'a> = + dyn StaticResourcePoolAccessor + 'a; + +pub type GpuRenderPipelinePoolMemMoveAccessor = + StaticResourcePoolMemMoveAccessor; + #[derive(Default)] pub struct GpuRenderPipelinePool { pool: StaticResourcePool, @@ -169,7 +180,7 @@ pub struct GpuRenderPipelinePool { impl GpuRenderPipelinePool { pub fn get_or_create( - &mut self, + &self, device: &wgpu::Device, desc: &RenderPipelineDesc, pipeline_layout_pool: &GpuPipelineLayoutPool, @@ -197,6 +208,7 @@ impl GpuRenderPipelinePool { // Recompile render pipelines referencing shader modules that have been recompiled this frame. self.pool.recreate_resources(|desc| { let frame_created = { + let shader_modules = shader_modules.resources(); let vertex_created = shader_modules .get_statistics(desc.vertex_handle) .map(|sm| sm.frame_created) @@ -230,11 +242,29 @@ impl GpuRenderPipelinePool { }); } - pub fn get_resource( + /// Locks the resource pool for resolving handles. + /// + /// While it is locked, no new resources can be added. + pub fn resources( &self, - handle: GpuRenderPipelineHandle, - ) -> Result<&wgpu::RenderPipeline, PoolError> { - self.pool.get_resource(handle) + ) -> StaticResourcePoolReadLockAccessor<'_, GpuRenderPipelineHandle, wgpu::RenderPipeline> { + self.pool.resources() + } + + /// Takes out all resources from the pool. + /// + /// This is useful when the existing resources need to be accessed without + /// taking a lock on the pool. + /// Resource can be put with `return_resources`. + pub fn take_resources(&mut self) -> GpuRenderPipelinePoolMemMoveAccessor { + self.pool.take_resources() + } + + /// Counterpart to `take_resources`. + /// + /// Logs an error if resources were added to the pool since `take_resources` was called. + pub fn return_resources(&mut self, resources: GpuRenderPipelinePoolMemMoveAccessor) { + self.pool.return_resources(resources); } pub fn num_resources(&self) -> usize { diff --git a/crates/re_renderer/src/wgpu_resources/sampler_pool.rs b/crates/re_renderer/src/wgpu_resources/sampler_pool.rs index 2c03824cf1c9..92398c26ea95 100644 --- a/crates/re_renderer/src/wgpu_resources/sampler_pool.rs +++ b/crates/re_renderer/src/wgpu_resources/sampler_pool.rs @@ -1,6 +1,6 @@ use std::hash::Hash; -use super::{resource::PoolError, static_resource_pool::StaticResourcePool}; +use super::static_resource_pool::{StaticResourcePool, StaticResourcePoolReadLockAccessor}; use crate::debug_label::DebugLabel; slotmap::new_key_type! { pub struct GpuSamplerHandle; } @@ -41,7 +41,7 @@ pub struct GpuSamplerPool { } impl GpuSamplerPool { - pub fn get_or_create(&mut self, device: &wgpu::Device, desc: &SamplerDesc) -> GpuSamplerHandle { + pub fn get_or_create(&self, device: &wgpu::Device, desc: &SamplerDesc) -> GpuSamplerHandle { self.pool.get_or_create(desc, |desc| { device.create_sampler(&wgpu::SamplerDescriptor { label: desc.label.get(), @@ -61,9 +61,13 @@ impl GpuSamplerPool { }) }) } - - pub fn get_resource(&self, handle: GpuSamplerHandle) -> Result<&wgpu::Sampler, PoolError> { - self.pool.get_resource(handle) + /// Locks the resource pool for resolving handles. + /// + /// While it is locked, no new resources can be added. + pub fn resources( + &self, + ) -> StaticResourcePoolReadLockAccessor<'_, GpuSamplerHandle, wgpu::Sampler> { + self.pool.resources() } pub fn begin_frame(&mut self, frame_index: u64) { diff --git a/crates/re_renderer/src/wgpu_resources/shader_module_pool.rs b/crates/re_renderer/src/wgpu_resources/shader_module_pool.rs index e68efa94c2f1..cd2ff264d493 100644 --- a/crates/re_renderer/src/wgpu_resources/shader_module_pool.rs +++ b/crates/re_renderer/src/wgpu_resources/shader_module_pool.rs @@ -5,10 +5,7 @@ use anyhow::Context as _; use crate::{debug_label::DebugLabel, FileResolver, FileSystem}; -use super::{ - resource::{PoolError, ResourceStatistics}, - static_resource_pool::StaticResourcePool, -}; +use super::static_resource_pool::{StaticResourcePool, StaticResourcePoolReadLockAccessor}; // --- @@ -162,15 +159,13 @@ impl GpuShaderModulePool { }); } - pub fn get(&self, handle: GpuShaderModuleHandle) -> Result<&wgpu::ShaderModule, PoolError> { - self.pool.get_resource(handle) - } - - pub fn get_statistics( + /// Locks the resource pool for resolving handles. + /// + /// While it is locked, no new resources can be added. + pub fn resources( &self, - handle: GpuShaderModuleHandle, - ) -> Result<&ResourceStatistics, PoolError> { - self.pool.get_statistics(handle) + ) -> StaticResourcePoolReadLockAccessor<'_, GpuShaderModuleHandle, wgpu::ShaderModule> { + self.pool.resources() } pub fn num_resources(&self) -> usize { 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 66e7f4976748..04e24c2b7065 100644 --- a/crates/re_renderer/src/wgpu_resources/static_resource_pool.rs +++ b/crates/re_renderer/src/wgpu_resources/static_resource_pool.rs @@ -1,21 +1,34 @@ -use std::{collections::HashMap, hash::Hash, sync::atomic::Ordering}; +use std::{collections::HashMap, hash::Hash, ops::Deref}; +use parking_lot::{RwLock, RwLockReadGuard}; use slotmap::{Key, SlotMap}; use super::resource::{PoolError, ResourceStatistics}; -struct StoredResource { +pub struct StoredResource { resource: Res, statistics: ResourceStatistics, } -/// Generic resource pool for all resources that are fully described upon creation, i.e. never have any variable content. +impl Deref for StoredResource { + type Target = Res; + + fn deref(&self) -> &Self::Target { + &self.resource + } +} + +/// Generic resource pool for all resources that are fully described upon creation. /// /// This implies, a resource is uniquely defined by its description. -/// We call these resources "static" because they never change their content over their lifetime. +/// We call these resources "static" because they never change their content during rendering. +/// However, the description may be respect to indirect changes which may to recreation of a resource. +/// The prime example of this is shader reloading: +/// * The resource is semantically the exact same despite having a different wgpu resource. +/// * We do **not** want its handle to change. pub(super) struct StaticResourcePool { - resources: SlotMap>, - lookup: HashMap, + resources: RwLock>>, + lookup: RwLock>, pub current_frame_index: u64, } @@ -35,74 +48,153 @@ where Handle: Key, Desc: std::fmt::Debug + Clone + Eq + Hash, { - fn to_pool_error(get_result: Option, handle: Handle) -> Result { - get_result.ok_or_else(|| { - if handle.is_null() { - PoolError::NullHandle - } else { - PoolError::ResourceNotAvailable + 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; } - }) - } - - pub fn get_or_create Res>( - &mut self, - desc: &Desc, - creation_func: F, - ) -> Handle { - *self.lookup.entry(desc.clone()).or_insert_with(|| { + } + { re_tracing::profile_scope!( "Creating new static resource", std::any::type_name::() ); - re_log::trace!(?desc, "Created new resource"); + let resource = creation_func(desc); - self.resources.insert(StoredResource { + 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) { re_tracing::profile_function!(); - for (desc, handle) in &self.lookup { + for (desc, handle) in self.lookup.get_mut() { if let Some(new_resource) = recreation_func(desc) { - let resource = self.resources.get_mut(*handle).unwrap(); + let resource = self.resources.get_mut().get_mut(*handle).unwrap(); resource.statistics.frame_created = self.current_frame_index; resource.resource = new_resource; } } } - pub fn get_resource(&self, handle: Handle) -> Result<&Res, PoolError> { - Self::to_pool_error( + /// Locks the resource pool for resolving handles. + /// + /// While it is locked, no new resources can be added. + pub fn resources(&self) -> StaticResourcePoolReadLockAccessor<'_, Handle, Res> { + StaticResourcePoolReadLockAccessor { + resources: self.resources.read(), + current_frame_index: self.current_frame_index, + } + } + + /// Takes out all resources from the pool. + /// + /// This is useful when the existing resources need to be accessed without + /// taking a lock on the pool. + pub fn take_resources(&mut self) -> StaticResourcePoolMemMoveAccessor { + StaticResourcePoolMemMoveAccessor { + resources: std::mem::take(self.resources.get_mut()), + current_frame_index: self.current_frame_index, + } + } + + /// Counter part to `take_resources`. + /// + /// Logs an error if resources were added to the pool since `take_resources` was called. + pub fn return_resources(&mut self, resources: StaticResourcePoolMemMoveAccessor) { + let resources_added_meanwhile = + std::mem::replace(self.resources.get_mut(), resources.resources); + if !resources_added_meanwhile.is_empty() { + re_log::error!( + "{} resources were discarded because they were added to the resource pool while the resources were taken out.", + resources_added_meanwhile.len() + ); + } + } + + pub fn num_resources(&self) -> usize { + self.resources.read().len() + } +} + +fn to_pool_error(get_result: Option, handle: impl Key) -> Result { + get_result.ok_or_else(|| { + if handle.is_null() { + PoolError::NullHandle + } else { + PoolError::ResourceNotAvailable + } + }) +} + +pub trait StaticResourcePoolAccessor { + fn get(&self, handle: Handle) -> Result<&Res, PoolError>; +} + +/// Accessor to the resource pool by taking a read lock. +pub struct StaticResourcePoolReadLockAccessor<'a, Handle: Key, Res> { + resources: RwLockReadGuard<'a, SlotMap>>, + current_frame_index: u64, +} + +impl<'a, Handle: Key, Res> StaticResourcePoolAccessor + for StaticResourcePoolReadLockAccessor<'a, Handle, Res> +{ + fn get(&self, handle: Handle) -> Result<&Res, PoolError> { + to_pool_error( self.resources.get(handle).map(|resource| { - resource - .statistics - .last_frame_used - .store(self.current_frame_index, Ordering::Relaxed); + resource.statistics.last_frame_used.store( + self.current_frame_index, + std::sync::atomic::Ordering::Relaxed, + ); &resource.resource }), handle, ) } +} +impl<'a, Handle: Key, Res> StaticResourcePoolReadLockAccessor<'a, Handle, Res> { pub fn get_statistics(&self, handle: Handle) -> Result<&ResourceStatistics, PoolError> { - Self::to_pool_error( + to_pool_error( self.resources .get(handle) .map(|resource| &resource.statistics), handle, ) } +} - pub fn num_resources(&self) -> usize { - self.resources.len() +/// Accessor to the resource pool by moving out the resources. +pub struct StaticResourcePoolMemMoveAccessor { + resources: SlotMap>, + current_frame_index: u64, +} + +impl StaticResourcePoolAccessor + for StaticResourcePoolMemMoveAccessor +{ + fn get(&self, handle: Handle) -> Result<&Res, PoolError> { + to_pool_error( + self.resources.get(handle).map(|resource| { + resource.statistics.last_frame_used.store( + self.current_frame_index, + std::sync::atomic::Ordering::Relaxed, + ); + &resource.resource + }), + handle, + ) } } @@ -113,7 +205,9 @@ mod tests { use slotmap::Key; use super::StaticResourcePool; - use crate::wgpu_resources::resource::PoolError; + use crate::wgpu_resources::{ + resource::PoolError, static_resource_pool::StaticResourcePoolAccessor as _, + }; slotmap::new_key_type! { pub struct ConcreteHandle; } @@ -127,7 +221,7 @@ mod tests { #[test] fn resource_reuse() { - let mut pool = Pool::default(); + let pool = Pool::default(); // New resource let res0 = { @@ -154,24 +248,23 @@ mod tests { #[test] fn get_resource() { - let mut pool = Pool::default(); + let pool = Pool::default(); + let handle = pool.get_or_create(&ConcreteResourceDesc(0), |d| ConcreteResource(d.0)); // Query with valid handle - let handle = pool.get_or_create(&ConcreteResourceDesc(0), |d| ConcreteResource(d.0)); - assert!(pool.get_resource(handle).is_ok()); - assert_eq!(*pool.get_resource(handle).unwrap(), ConcreteResource(0)); + let resources = pool.resources(); + assert!(resources.get(handle).is_ok()); + assert_eq!(*resources.get(handle).unwrap(), ConcreteResource(0)); // Query with null handle assert_eq!( - pool.get_resource(ConcreteHandle::null()), + resources.get(ConcreteHandle::null()), Err(PoolError::NullHandle) ); // Query with invalid handle - pool = Pool::default(); - assert_eq!( - pool.get_resource(handle), - Err(PoolError::ResourceNotAvailable) - ); + let pool = Pool::default(); + let resources = pool.resources(); + assert_eq!(resources.get(handle), Err(PoolError::ResourceNotAvailable)); } } 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 cdc9a6420ea3..509fd1e0b86d 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 @@ -36,6 +36,9 @@ struct ReRendererCallback { } impl egui_wgpu::CallbackTrait for ReRendererCallback { + // TODO(andreas): Prepare callbacks should run in parallel. + // Command buffer recording may be fairly expensive in the future! + // Sticking to egui's current model, each prepare callback could fork of a task and in finish_prepare we wait for them. fn prepare( &self, _device: &wgpu::Device, @@ -68,7 +71,7 @@ impl egui_wgpu::CallbackTrait for ReRendererCallback { return Vec::new(); }; - match view_builder.draw(ctx, self.clear_color) { + let command_buffer = 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 @@ -87,7 +90,37 @@ 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( + &self, + _device: &wgpu::Device, + _queue: &wgpu::Queue, + _egui_encoder: &mut wgpu::CommandEncoder, + callback_resources: &mut egui_wgpu::CallbackResources, + ) -> Vec { + let Some(ctx) = callback_resources.get_mut::() else { + re_log::error_once!( + "Failed to execute egui prepare callback. No render context available." + ); + return Vec::new(); + }; + + // We don't own the render pass that renders the egui ui. + // But we *still* need to somehow ensure that all resources used in callbacks drawing to it, + // 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. + if ctx.active_frame.moved_render_pipelines.is_none() { + let render_pipelines = ctx.gpu_resources.render_pipelines.take_resources(); + ctx.active_frame.moved_render_pipelines = Some(render_pipelines); } + + Vec::new() } fn paint<'a>( @@ -119,6 +152,11 @@ impl egui_wgpu::CallbackTrait for ReRendererCallback { let screen_position = (info.viewport.min.to_vec2() * info.pixels_per_point).round(); let screen_position = glam::vec2(screen_position.x, screen_position.y); - view_builder.composite(ctx, render_pass, screen_position); + view_builder.composite( + ctx, + ctx.active_frame.moved_render_pipelines.as_ref().unwrap(), // TODO: + render_pass, + screen_position, + ); } } From ae3d01e4a3bfa1195d87904a89727353e40695b1 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 1 Dec 2023 00:07:42 +0100 Subject: [PATCH 2/7] remove unnecessary mutable references --- crates/re_renderer/src/context.rs | 2 +- crates/re_renderer/src/draw_phases/outlines.rs | 4 ++-- crates/re_renderer/src/global_bindings.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/re_renderer/src/context.rs b/crates/re_renderer/src/context.rs index 59f60f89bdcb..922458f5dbb0 100644 --- a/crates/re_renderer/src/context.rs +++ b/crates/re_renderer/src/context.rs @@ -116,7 +116,7 @@ impl RenderContext { log_adapter_info(&adapter.get_info()); let mut gpu_resources = WgpuResourcePools::default(); - let global_bindings = GlobalBindings::new(&mut gpu_resources, &device); + let global_bindings = GlobalBindings::new(&gpu_resources, &device); // Validate capabilities of the device. assert!( diff --git a/crates/re_renderer/src/draw_phases/outlines.rs b/crates/re_renderer/src/draw_phases/outlines.rs index e2cac2b179e6..b438214d1c26 100644 --- a/crates/re_renderer/src/draw_phases/outlines.rs +++ b/crates/re_renderer/src/draw_phases/outlines.rs @@ -425,7 +425,7 @@ impl OutlineMaskProcessor { } fn create_bind_group_jumpflooding_init( - ctx: &mut RenderContext, + ctx: &RenderContext, instance_label: &DebugLabel, mask_texture: &GpuTexture, ) -> (GpuBindGroup, GpuBindGroupLayoutHandle) { @@ -462,7 +462,7 @@ impl OutlineMaskProcessor { fn create_bind_groups_for_jumpflooding_steps( config: &OutlineConfig, - ctx: &mut RenderContext, + ctx: &RenderContext, instance_label: &DebugLabel, voronoi_textures: &[GpuTexture; 2], ) -> (Vec, GpuBindGroupLayoutHandle) { diff --git a/crates/re_renderer/src/global_bindings.rs b/crates/re_renderer/src/global_bindings.rs index 1ad7f9ae6c8a..6404da52b024 100644 --- a/crates/re_renderer/src/global_bindings.rs +++ b/crates/re_renderer/src/global_bindings.rs @@ -56,7 +56,7 @@ pub(crate) struct GlobalBindings { } impl GlobalBindings { - pub fn new(pools: &mut WgpuResourcePools, device: &wgpu::Device) -> Self { + pub fn new(pools: &WgpuResourcePools, device: &wgpu::Device) -> Self { Self { layout: pools.bind_group_layouts.get_or_create( device, From 821afd6ccece4124cd2b5fc769bebb49a0340555 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 1 Dec 2023 00:10:32 +0100 Subject: [PATCH 3/7] replace unwrap with early out + error --- .../src/gpu_bridge/re_renderer_callback.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) 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 509fd1e0b86d..00eb48ee8f27 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 @@ -135,6 +135,12 @@ impl egui_wgpu::CallbackTrait for ReRendererCallback { ); return; }; + let Some(render_pipelines) = ctx.active_frame.moved_render_pipelines.as_ref() else { + re_log::error_once!( + "Failed to execute egui draw callback. Render pipelines weren't transferred out of the pool first." + ); + return; + }; let Some(Some(view_builder)) = ctx .active_frame @@ -152,11 +158,6 @@ impl egui_wgpu::CallbackTrait for ReRendererCallback { let screen_position = (info.viewport.min.to_vec2() * info.pixels_per_point).round(); let screen_position = glam::vec2(screen_position.x, screen_position.y); - view_builder.composite( - ctx, - ctx.active_frame.moved_render_pipelines.as_ref().unwrap(), // TODO: - render_pass, - screen_position, - ); + view_builder.composite(ctx, render_pipelines, render_pass, screen_position); } } From c9a080cb8b5bd157dd275fbe9f20a007d80a395a Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Mon, 4 Dec 2023 11:28:09 +0100 Subject: [PATCH 4/7] GpuRenderPipelinePoolMemMoveAccessor -> GpuRenderPipelinePoolMoveAccessor, moved_render_pipelines -> pinned_render_pipelines --- crates/re_renderer/src/context.rs | 10 +++++----- crates/re_renderer/src/wgpu_resources/mod.rs | 2 +- .../src/wgpu_resources/render_pipeline_pool.rs | 6 +++--- .../src/gpu_bridge/re_renderer_callback.rs | 6 +++--- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/crates/re_renderer/src/context.rs b/crates/re_renderer/src/context.rs index 922458f5dbb0..0ab14afc562e 100644 --- a/crates/re_renderer/src/context.rs +++ b/crates/re_renderer/src/context.rs @@ -9,7 +9,7 @@ use crate::{ global_bindings::GlobalBindings, renderer::Renderer, resource_managers::{MeshManager, TextureManager2D}, - wgpu_resources::{GpuRenderPipelinePoolMemMoveAccessor, WgpuResourcePools}, + wgpu_resources::{GpuRenderPipelinePoolMoveAccessor, WgpuResourcePools}, FileResolver, FileServer, FileSystem, RecommendedFileResolver, }; @@ -187,7 +187,7 @@ impl RenderContext { let active_frame = ActiveFrameContext { before_view_builder_encoder: Mutex::new(FrameGlobalCommandEncoder::new(&device)), per_frame_data_helper: TypeMap::new(), - moved_render_pipelines: None, + pinned_render_pipelines: None, frame_index: 0, }; @@ -301,7 +301,7 @@ impl RenderContext { self.gpu_readback_belt.get_mut().after_queue_submit(); // Give back moved render pipelines to the pool if any were moved out. - if let Some(moved_render_pipelines) = self.active_frame.moved_render_pipelines.take() { + if let Some(moved_render_pipelines) = self.active_frame.pinned_render_pipelines.take() { self.gpu_resources .render_pipelines .return_resources(moved_render_pipelines); @@ -311,7 +311,7 @@ impl RenderContext { self.active_frame = ActiveFrameContext { before_view_builder_encoder: Mutex::new(FrameGlobalCommandEncoder::new(&self.device)), frame_index: self.active_frame.frame_index + 1, - moved_render_pipelines: None, + pinned_render_pipelines: None, per_frame_data_helper: TypeMap::new(), }; let frame_index = self.active_frame.frame_index; @@ -446,7 +446,7 @@ pub struct ActiveFrameContext { /// Will be moved back to the resource pool at the start of the frame. /// This is needed for accessing the render pipelines without keeping a reference /// to the resource pool lock during the lifetime of a render pass. - pub moved_render_pipelines: Option, + pub pinned_render_pipelines: Option, /// Index of this frame. Is incremented for every render frame. frame_index: u64, diff --git a/crates/re_renderer/src/wgpu_resources/mod.rs b/crates/re_renderer/src/wgpu_resources/mod.rs index ba66fd46125a..9aecb6ee5148 100644 --- a/crates/re_renderer/src/wgpu_resources/mod.rs +++ b/crates/re_renderer/src/wgpu_resources/mod.rs @@ -28,7 +28,7 @@ pub use pipeline_layout_pool::{ mod render_pipeline_pool; pub use render_pipeline_pool::{ GpuRenderPipelineHandle, GpuRenderPipelinePool, GpuRenderPipelinePoolAccessor, - GpuRenderPipelinePoolMemMoveAccessor, RenderPipelineDesc, VertexBufferLayout, + GpuRenderPipelinePoolMoveAccessor, RenderPipelineDesc, VertexBufferLayout, }; mod sampler_pool; diff --git a/crates/re_renderer/src/wgpu_resources/render_pipeline_pool.rs b/crates/re_renderer/src/wgpu_resources/render_pipeline_pool.rs index b950dba727cf..a3727298975a 100644 --- a/crates/re_renderer/src/wgpu_resources/render_pipeline_pool.rs +++ b/crates/re_renderer/src/wgpu_resources/render_pipeline_pool.rs @@ -170,7 +170,7 @@ impl RenderPipelineDesc { pub type GpuRenderPipelinePoolAccessor<'a> = dyn StaticResourcePoolAccessor + 'a; -pub type GpuRenderPipelinePoolMemMoveAccessor = +pub type GpuRenderPipelinePoolMoveAccessor = StaticResourcePoolMemMoveAccessor; #[derive(Default)] @@ -256,14 +256,14 @@ impl GpuRenderPipelinePool { /// This is useful when the existing resources need to be accessed without /// taking a lock on the pool. /// Resource can be put with `return_resources`. - pub fn take_resources(&mut self) -> GpuRenderPipelinePoolMemMoveAccessor { + pub fn take_resources(&mut self) -> GpuRenderPipelinePoolMoveAccessor { self.pool.take_resources() } /// Counterpart to `take_resources`. /// /// Logs an error if resources were added to the pool since `take_resources` was called. - pub fn return_resources(&mut self, resources: GpuRenderPipelinePoolMemMoveAccessor) { + pub fn return_resources(&mut self, resources: GpuRenderPipelinePoolMoveAccessor) { self.pool.return_resources(resources); } 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 00eb48ee8f27..979049ef357b 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 @@ -115,9 +115,9 @@ impl egui_wgpu::CallbackTrait for ReRendererCallback { // 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. - if ctx.active_frame.moved_render_pipelines.is_none() { + if ctx.active_frame.pinned_render_pipelines.is_none() { let render_pipelines = ctx.gpu_resources.render_pipelines.take_resources(); - ctx.active_frame.moved_render_pipelines = Some(render_pipelines); + ctx.active_frame.pinned_render_pipelines = Some(render_pipelines); } Vec::new() @@ -135,7 +135,7 @@ impl egui_wgpu::CallbackTrait for ReRendererCallback { ); return; }; - let Some(render_pipelines) = ctx.active_frame.moved_render_pipelines.as_ref() else { + let Some(render_pipelines) = ctx.active_frame.pinned_render_pipelines.as_ref() else { re_log::error_once!( "Failed to execute egui draw callback. Render pipelines weren't transferred out of the pool first." ); From 2e754ba35644ef03697310633a2eb11c799be0c7 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Mon, 4 Dec 2023 11:58:57 +0100 Subject: [PATCH 5/7] lint fix: missing new-line --- crates/re_renderer/src/wgpu_resources/sampler_pool.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/re_renderer/src/wgpu_resources/sampler_pool.rs b/crates/re_renderer/src/wgpu_resources/sampler_pool.rs index 92398c26ea95..4d111d9be819 100644 --- a/crates/re_renderer/src/wgpu_resources/sampler_pool.rs +++ b/crates/re_renderer/src/wgpu_resources/sampler_pool.rs @@ -61,6 +61,7 @@ impl GpuSamplerPool { }) }) } + /// Locks the resource pool for resolving handles. /// /// While it is locked, no new resources can be added. From 549cbd35e8a3f64f5ef04dba0fe69fad2598ced5 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Mon, 4 Dec 2023 21:49:47 +0100 Subject: [PATCH 6/7] rust formatting --- crates/re_renderer/examples/framework.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/re_renderer/examples/framework.rs b/crates/re_renderer/examples/framework.rs index 37f635bbbf07..9df43edd5e0b 100644 --- a/crates/re_renderer/examples/framework.rs +++ b/crates/re_renderer/examples/framework.rs @@ -277,7 +277,8 @@ impl Application { { // Lock render pipelines for the lifetime of the composite pass. - let render_pipelines = self.re_ctx.gpu_resources.render_pipelines.resources(); + let render_pipelines = + self.re_ctx.gpu_resources.render_pipelines.resources(); let mut composite_pass = composite_cmd_encoder.begin_render_pass(&wgpu::RenderPassDescriptor { From 8e4b0d88d9d01df8121a84850b251c1840494695 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Tue, 5 Dec 2023 11:43:50 +0100 Subject: [PATCH 7/7] 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