From 965d71c3723e74947e2d063372d51d9ae89cc9dd Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 1 Dec 2023 11:11:32 +0100 Subject: [PATCH] make view builder draw take a non mutable reference to self to resolve anther egui callback conundrum. Trickle out resulting mut removal all the way to viewer_context --- crates/re_data_ui/src/image.rs | 10 +-- .../src/allocator/gpu_readback_belt.rs | 4 +- crates/re_renderer/src/context.rs | 5 -- .../re_renderer/src/draw_phases/outlines.rs | 6 +- .../src/draw_phases/picking_layer.rs | 11 +-- .../re_renderer/src/draw_phases/screenshot.rs | 12 +-- crates/re_renderer/src/view_builder.rs | 8 +- crates/re_space_view_spatial/src/ui_2d.rs | 1 - crates/re_space_view_spatial/src/ui_3d.rs | 1 - .../src/space_view_class.rs | 10 +-- crates/re_viewer/src/app_state.rs | 2 +- crates/re_viewer/src/ui/selection_panel.rs | 4 +- .../src/gpu_bridge/colormap.rs | 4 +- .../re_viewer_context/src/gpu_bridge/mod.rs | 3 +- .../src/gpu_bridge/re_renderer_callback.rs | 73 ++----------------- .../re_viewer_context/src/viewer_context.rs | 2 +- 16 files changed, 46 insertions(+), 110 deletions(-) diff --git a/crates/re_data_ui/src/image.rs b/crates/re_data_ui/src/image.rs index aac97e779128..39428804d193 100644 --- a/crates/re_data_ui/src/image.rs +++ b/crates/re_data_ui/src/image.rs @@ -63,7 +63,7 @@ impl EntityDataUi for re_types::components::TensorData { #[allow(clippy::too_many_arguments)] fn tensor_ui( - ctx: &mut ViewerContext<'_>, + ctx: &ViewerContext<'_>, ui: &mut egui::Ui, verbosity: UiVerbosity, entity_path: &re_data_store::EntityPath, @@ -253,7 +253,7 @@ fn texture_size(colormapped_texture: &ColormappedTexture) -> Vec2 { /// Extremely small images will be stretched on their thin axis to make them visible. /// This does not preserve aspect ratio, but we only stretch it to a very thin size, so it is fine. fn show_image_preview( - render_ctx: &mut re_renderer::RenderContext, + render_ctx: &re_renderer::RenderContext, re_ui: &ReUi, ui: &mut egui::Ui, colormapped_texture: ColormappedTexture, @@ -433,7 +433,7 @@ pub fn tensor_summary_ui( #[allow(clippy::too_many_arguments)] fn show_zoomed_image_region_tooltip( - render_ctx: &mut re_renderer::RenderContext, + render_ctx: &re_renderer::RenderContext, parent_ui: &egui::Ui, response: egui::Response, tensor_data_row_id: RowId, @@ -523,7 +523,7 @@ pub fn show_zoomed_image_region_area_outline( /// `meter`: iff this is a depth map, how long is one meter? #[allow(clippy::too_many_arguments)] pub fn show_zoomed_image_region( - render_ctx: &mut re_renderer::RenderContext, + render_ctx: &re_renderer::RenderContext, ui: &mut egui::Ui, tensor_data_row_id: RowId, tensor: &DecodedTensor, @@ -553,7 +553,7 @@ pub fn show_zoomed_image_region( /// `meter`: iff this is a depth map, how long is one meter? #[allow(clippy::too_many_arguments)] fn try_show_zoomed_image_region( - render_ctx: &mut re_renderer::RenderContext, + render_ctx: &re_renderer::RenderContext, ui: &mut egui::Ui, tensor_data_row_id: RowId, tensor: &DecodedTensor, diff --git a/crates/re_renderer/src/allocator/gpu_readback_belt.rs b/crates/re_renderer/src/allocator/gpu_readback_belt.rs index 5a1a3760a096..b314437ada98 100644 --- a/crates/re_renderer/src/allocator/gpu_readback_belt.rs +++ b/crates/re_renderer/src/allocator/gpu_readback_belt.rs @@ -39,7 +39,7 @@ impl GpuReadbackBuffer { /// Does 2D-only entirely for convenience as it greatly simplifies the input parameters. /// Additionally, we assume as tightly as possible packed data as this is by far the most common use. pub fn read_texture2d( - self, + &mut self, encoder: &mut wgpu::CommandEncoder, source: wgpu::ImageCopyTexture<'_>, copy_extents: glam::UVec2, @@ -58,7 +58,7 @@ impl GpuReadbackBuffer { /// This method will add the required padding between the texture copies if necessary. /// Panics if the buffer is too small. pub fn read_multiple_texture2d( - mut self, + &mut self, encoder: &mut wgpu::CommandEncoder, sources_and_extents: &[(wgpu::ImageCopyTexture<'_>, glam::UVec2)], ) -> Result<(), GpuReadbackError> { diff --git a/crates/re_renderer/src/context.rs b/crates/re_renderer/src/context.rs index 456c075775b7..18a33bfdbeb5 100644 --- a/crates/re_renderer/src/context.rs +++ b/crates/re_renderer/src/context.rs @@ -186,7 +186,6 @@ impl RenderContext { let active_frame = ActiveFrameContext { before_view_builder_encoder: Mutex::new(FrameGlobalCommandEncoder::new(&device)), - per_frame_data_helper: TypeMap::new(), pinned_render_pipelines: None, frame_index: 0, }; @@ -312,7 +311,6 @@ impl RenderContext { before_view_builder_encoder: Mutex::new(FrameGlobalCommandEncoder::new(&self.device)), frame_index: self.active_frame.frame_index + 1, pinned_render_pipelines: None, - per_frame_data_helper: TypeMap::new(), }; let frame_index = self.active_frame.frame_index; @@ -462,9 +460,6 @@ pub struct ActiveFrameContext { /// (i.e. typically in [`crate::renderer::DrawData`] creation!) pub before_view_builder_encoder: Mutex, - /// 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. diff --git a/crates/re_renderer/src/draw_phases/outlines.rs b/crates/re_renderer/src/draw_phases/outlines.rs index e7ef1c41c925..615519b7edce 100644 --- a/crates/re_renderer/src/draw_phases/outlines.rs +++ b/crates/re_renderer/src/draw_phases/outlines.rs @@ -371,7 +371,7 @@ impl OutlineMaskProcessor { } pub fn compute_outlines( - self, + &self, pipelines: &GpuRenderPipelinePoolAccessor<'_>, encoder: &mut wgpu::CommandEncoder, ) -> Result<(), PoolError> { @@ -402,7 +402,7 @@ impl OutlineMaskProcessor { // Perform jump flooding. let render_pipeline_step = pipelines.get(self.render_pipeline_jumpflooding_step)?; - for (i, bind_group) in self.bind_group_jumpflooding_steps.into_iter().enumerate() { + for (i, bind_group) in self.bind_group_jumpflooding_steps.iter().enumerate() { let mut jumpflooding_step = encoder.begin_render_pass(&wgpu::RenderPassDescriptor { label: DebugLabel::from(format!("{} - jumpflooding_step {i}", self.label)).get(), color_attachments: &[Some(wgpu::RenderPassColorAttachment { @@ -417,7 +417,7 @@ impl OutlineMaskProcessor { }); jumpflooding_step.set_pipeline(render_pipeline_step); - jumpflooding_step.set_bind_group(0, &bind_group, &[]); + jumpflooding_step.set_bind_group(0, bind_group, &[]); jumpflooding_step.draw(0..3, 0..1); } diff --git a/crates/re_renderer/src/draw_phases/picking_layer.rs b/crates/re_renderer/src/draw_phases/picking_layer.rs index fed640a1e005..52053c9d951d 100644 --- a/crates/re_renderer/src/draw_phases/picking_layer.rs +++ b/crates/re_renderer/src/draw_phases/picking_layer.rs @@ -25,6 +25,7 @@ use crate::{ DebugLabel, GpuReadbackBuffer, GpuReadbackIdentifier, RectInt, RenderContext, }; +use parking_lot::Mutex; use smallvec::smallvec; /// GPU retrieved & processed picking data result. @@ -141,7 +142,7 @@ pub enum PickingLayerError { pub struct PickingLayerProcessor { pub picking_target: GpuTexture, picking_depth_target: GpuTexture, - readback_buffer: GpuReadbackBuffer, + readback_buffer: Mutex, bind_group_0: GpuBindGroup, depth_readback_workaround: Option, @@ -285,7 +286,7 @@ impl PickingLayerProcessor { ); let buffer_size = row_info_id.buffer_size_padded + row_info_depth.buffer_size_padded; - let readback_buffer = ctx.gpu_readback_belt.lock().allocate( + let readback_buffer = Mutex::new(ctx.gpu_readback_belt.lock().allocate( &ctx.device, &ctx.gpu_resources.buffers, buffer_size, @@ -296,7 +297,7 @@ impl PickingLayerProcessor { world_from_cropped_projection: cropped_projection_from_world.inverse(), depth_readback_workaround_in_use: depth_readback_workaround.is_some(), }), - ); + )); PickingLayerProcessor { bind_group_0, @@ -342,7 +343,7 @@ impl PickingLayerProcessor { } pub fn end_render_pass( - self, + &self, encoder: &mut wgpu::CommandEncoder, render_pipelines: &GpuRenderPipelinePoolAccessor<'_>, ) -> Result<(), PickingLayerError> { @@ -362,7 +363,7 @@ impl PickingLayerProcessor { &self.picking_depth_target }; - self.readback_buffer.read_multiple_texture2d( + self.readback_buffer.lock().read_multiple_texture2d( encoder, &[ ( diff --git a/crates/re_renderer/src/draw_phases/screenshot.rs b/crates/re_renderer/src/draw_phases/screenshot.rs index 28a46398f9e9..b954d1be6f72 100644 --- a/crates/re_renderer/src/draw_phases/screenshot.rs +++ b/crates/re_renderer/src/draw_phases/screenshot.rs @@ -10,6 +10,8 @@ //! We could render the same image with subpixel moved camera in order to get super-sampling without hitting texture size limitations. //! Or alternatively try to render the images in several tiles 🤔. In any case this would greatly improve quality! +use parking_lot::Mutex; + use crate::{ allocator::GpuReadbackError, texture_info::Texture2DBufferInfo, @@ -25,7 +27,7 @@ struct ReadbackBeltMetadata { pub struct ScreenshotProcessor { screenshot_texture: GpuTexture, - screenshot_readback_buffer: GpuReadbackBuffer, + screenshot_readback_buffer: Mutex, } impl ScreenshotProcessor { @@ -40,7 +42,7 @@ impl ScreenshotProcessor { readback_user_data: T, ) -> Self { let buffer_info = Texture2DBufferInfo::new(Self::SCREENSHOT_COLOR_FORMAT, resolution); - let screenshot_readback_buffer = ctx.gpu_readback_belt.lock().allocate( + let screenshot_readback_buffer = Mutex::new(ctx.gpu_readback_belt.lock().allocate( &ctx.device, &ctx.gpu_resources.buffers, buffer_info.buffer_size_padded, @@ -49,7 +51,7 @@ impl ScreenshotProcessor { extent: resolution, user_data: readback_user_data, }), - ); + )); let screenshot_texture = ctx.gpu_resources.textures.alloc( &ctx.device, @@ -100,10 +102,10 @@ impl ScreenshotProcessor { } pub fn end_render_pass( - self, + &self, encoder: &mut wgpu::CommandEncoder, ) -> Result<(), GpuReadbackError> { - self.screenshot_readback_buffer.read_texture2d( + self.screenshot_readback_buffer.lock().read_texture2d( encoder, wgpu::ImageCopyTexture { texture: &self.screenshot_texture.texture, diff --git a/crates/re_renderer/src/view_builder.rs b/crates/re_renderer/src/view_builder.rs index 7fbdb0a0464f..907a55679606 100644 --- a/crates/re_renderer/src/view_builder.rs +++ b/crates/re_renderer/src/view_builder.rs @@ -534,7 +534,7 @@ impl ViewBuilder { /// Draws the frame as instructed to a temporary HDR target. pub fn draw( - &mut self, + &self, ctx: &RenderContext, clear_color: Rgba, ) -> Result { @@ -608,7 +608,7 @@ impl ViewBuilder { } } - if let Some(picking_processor) = self.picking_processor.take() { + if let Some(picking_processor) = &self.picking_processor { { let mut pass = picking_processor.begin_render_pass(&setup.name, &mut encoder); // PickingProcessor has as custom frame uniform buffer. @@ -635,7 +635,7 @@ impl ViewBuilder { } } - if let Some(outline_mask_processor) = self.outline_mask_processor.take() { + if let Some(outline_mask_processor) = &self.outline_mask_processor { re_tracing::profile_scope!("outlines"); { re_tracing::profile_scope!("outline mask pass"); @@ -646,7 +646,7 @@ impl ViewBuilder { outline_mask_processor.compute_outlines(&pipelines, &mut encoder)?; } - if let Some(screenshot_processor) = self.screenshot_processor.take() { + if let Some(screenshot_processor) = &self.screenshot_processor { { let mut pass = screenshot_processor.begin_render_pass(&setup.name, &mut encoder); pass.set_bind_group(0, &setup.bind_group_0, &[]); diff --git a/crates/re_space_view_spatial/src/ui_2d.rs b/crates/re_space_view_spatial/src/ui_2d.rs index 7e2fe3602589..3b02f1612489 100644 --- a/crates/re_space_view_spatial/src/ui_2d.rs +++ b/crates/re_space_view_spatial/src/ui_2d.rs @@ -360,7 +360,6 @@ pub fn view_2d( // Draw a re_renderer driven view. // Camera & projection are configured to ingest space coordinates directly. painter.add(gpu_bridge::new_renderer_callback( - ctx.render_ctx, view_builder, painter.clip_rect(), ui.visuals().extreme_bg_color.into(), diff --git a/crates/re_space_view_spatial/src/ui_3d.rs b/crates/re_space_view_spatial/src/ui_3d.rs index 4a210a57ff91..d03f060001b9 100644 --- a/crates/re_space_view_spatial/src/ui_3d.rs +++ b/crates/re_space_view_spatial/src/ui_3d.rs @@ -610,7 +610,6 @@ pub fn view_3d( ctx.render_ctx, )); ui.painter().add(gpu_bridge::new_renderer_callback( - ctx.render_ctx, view_builder, rect, re_renderer::Rgba::TRANSPARENT, diff --git a/crates/re_space_view_tensor/src/space_view_class.rs b/crates/re_space_view_tensor/src/space_view_class.rs index 9773812f13ce..19380f48c5ab 100644 --- a/crates/re_space_view_tensor/src/space_view_class.rs +++ b/crates/re_space_view_tensor/src/space_view_class.rs @@ -87,7 +87,7 @@ impl PerTensorState { &self.color_mapping } - pub fn ui(&mut self, ctx: &mut ViewerContext<'_>, ui: &mut egui::Ui) { + pub fn ui(&mut self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui) { let Some((tensor_data_row_id, tensor)) = &self.tensor else { ui.label("No Tensor shown in this Space View."); return; @@ -234,7 +234,7 @@ impl SpaceViewClass for TensorSpaceView { } fn view_tensor( - ctx: &mut ViewerContext<'_>, + ctx: &ViewerContext<'_>, ui: &mut egui::Ui, state: &mut PerTensorState, tensor_data_row_id: RowId, @@ -292,7 +292,7 @@ fn view_tensor( } fn tensor_slice_ui( - ctx: &mut ViewerContext<'_>, + ctx: &ViewerContext<'_>, ui: &mut egui::Ui, state: &PerTensorState, tensor_data_row_id: RowId, @@ -311,7 +311,7 @@ fn tensor_slice_ui( } fn paint_tensor_slice( - ctx: &mut ViewerContext<'_>, + ctx: &ViewerContext<'_>, ui: &mut egui::Ui, state: &PerTensorState, tensor_data_row_id: RowId, @@ -384,7 +384,7 @@ impl Default for ColorMapping { impl ColorMapping { fn ui( &mut self, - render_ctx: &mut re_renderer::RenderContext, + render_ctx: &re_renderer::RenderContext, re_ui: &re_ui::ReUi, ui: &mut egui::Ui, ) { diff --git a/crates/re_viewer/src/app_state.rs b/crates/re_viewer/src/app_state.rs index 0529564f7801..e2b60349e681 100644 --- a/crates/re_viewer/src/app_state.rs +++ b/crates/re_viewer/src/app_state.rs @@ -83,7 +83,7 @@ impl AppState { &mut self, app_blueprint: &AppBlueprint<'_>, ui: &mut egui::Ui, - render_ctx: &mut re_renderer::RenderContext, + render_ctx: &re_renderer::RenderContext, store_db: &StoreDb, store_context: &StoreContext<'_>, re_ui: &re_ui::ReUi, diff --git a/crates/re_viewer/src/ui/selection_panel.rs b/crates/re_viewer/src/ui/selection_panel.rs index 104b05d55742..c8d373d9d6dd 100644 --- a/crates/re_viewer/src/ui/selection_panel.rs +++ b/crates/re_viewer/src/ui/selection_panel.rs @@ -401,7 +401,7 @@ fn space_view_top_level_properties( fn container_top_level_properties( ui: &mut egui::Ui, - _ctx: &mut ViewerContext<'_>, + _ctx: &ViewerContext<'_>, viewport: &mut ViewportBlueprint<'_>, tile_id: &egui_tiles::TileId, ) { @@ -739,7 +739,7 @@ fn entity_props_ui( } fn colormap_props_ui( - ctx: &mut ViewerContext<'_>, + ctx: &ViewerContext<'_>, ui: &mut egui::Ui, entity_props: &mut EntityProperties, ) { diff --git a/crates/re_viewer_context/src/gpu_bridge/colormap.rs b/crates/re_viewer_context/src/gpu_bridge/colormap.rs index 2714e21d5db6..2d3274d36b0f 100644 --- a/crates/re_viewer_context/src/gpu_bridge/colormap.rs +++ b/crates/re_viewer_context/src/gpu_bridge/colormap.rs @@ -2,7 +2,7 @@ use crate::gpu_bridge::{get_or_create_texture, render_image}; /// Show the given colormap as a horizontal bar. fn colormap_preview_ui( - render_ctx: &mut re_renderer::RenderContext, + render_ctx: &re_renderer::RenderContext, ui: &mut egui::Ui, colormap: re_renderer::Colormap, ) -> anyhow::Result { @@ -60,7 +60,7 @@ fn colormap_preview_ui( } pub fn colormap_dropdown_button_ui( - render_ctx: &mut re_renderer::RenderContext, + render_ctx: &re_renderer::RenderContext, ui: &mut egui::Ui, map: &mut re_renderer::Colormap, ) { diff --git a/crates/re_viewer_context/src/gpu_bridge/mod.rs b/crates/re_viewer_context/src/gpu_bridge/mod.rs index 98597a30ad6f..2c14f20c19b5 100644 --- a/crates/re_viewer_context/src/gpu_bridge/mod.rs +++ b/crates/re_viewer_context/src/gpu_bridge/mod.rs @@ -118,7 +118,7 @@ pub fn get_or_create_texture<'a>( /// Render the given image, respecting the clip rectangle of the given painter. pub fn render_image( - render_ctx: &mut re_renderer::RenderContext, + render_ctx: &re_renderer::RenderContext, egui_painter: &egui::Painter, image_rect_on_screen: egui::Rect, colormapped_texture: ColormappedTexture, @@ -196,7 +196,6 @@ pub fn render_image( )?); egui_painter.add(new_renderer_callback( - render_ctx, view_builder, viewport, re_renderer::Rgba::TRANSPARENT, 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 00259e74b1c1..31fe6179d75b 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 @@ -1,37 +1,21 @@ slotmap::new_key_type! { pub struct ViewBuilderHandle; } -type ViewBuilderMap = slotmap::SlotMap>; - pub fn new_renderer_callback( - render_ctx: &mut re_renderer::RenderContext, view_builder: re_renderer::ViewBuilder, viewport: egui::Rect, clear_color: re_renderer::Rgba, ) -> egui::PaintCallback { - let composition_view_builder_map = render_ctx - .active_frame - .per_frame_data_helper - .entry::() - .or_insert_with(Default::default); - let view_builder_handle = composition_view_builder_map.insert(Some(view_builder)); - egui_wgpu::Callback::new_paint_callback( viewport, ReRendererCallback { - view_builder: view_builder_handle, + view_builder, clear_color, }, ) } struct ReRendererCallback { - // It would be nice to put the ViewBuilder in here directly, but this - // struct is required to be Send/Sync and wgpu resources aren't on wasm. - // Technically, we ignore this restriction by using the `fragile-send-sync-non-atomic-wasm` wgpu feature flag. - // - // However, in addition, we need to make sure that the ViewBuilder outlives the render pass that is used to draw egui. - // (This restriction is likely to be address by Arcanization https://github.com/gfx-rs/wgpu/pull/3626). - view_builder: ViewBuilderHandle, + view_builder: re_renderer::ViewBuilder, clear_color: re_renderer::Rgba, } @@ -46,45 +30,15 @@ impl egui_wgpu::CallbackTrait for ReRendererCallback { _egui_encoder: &mut wgpu::CommandEncoder, paint_callback_resources: &mut egui_wgpu::CallbackResources, ) -> Vec { - let Some(ctx) = paint_callback_resources.get_mut::() else { + let Some(ctx) = paint_callback_resources.get::() else { re_log::error_once!( "Failed to execute egui prepare callback. No render context available." ); return Vec::new(); }; - // Takes the view_builder out of the slotmap, so we don't have a mutable reference to ctx in use. - let Some(mut view_builder) = ctx - .active_frame - .per_frame_data_helper - .get_mut::() - .and_then(|view_builder_map| { - view_builder_map - .get_mut(self.view_builder) - .and_then(|slot| slot.take()) - }) - else { - re_log::error_once!( - "Failed to execute egui prepare callback. View builder with handle {:?} not found.", - self.view_builder - ); - return Vec::new(); - }; - - 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 - .per_frame_data_helper - .get_mut::() - .and_then(|view_builder_map| { - view_builder_map - .get_mut(self.view_builder) - .and_then(|slot| slot.replace(view_builder)) - }); - vec![command_buffer] - } - + match self.view_builder.draw(ctx, self.clear_color) { + Ok(command_buffer) => vec![command_buffer], Err(err) => { re_log::error_once!("Failed to fill view builder: {err}"); // TODO(andreas): It would be nice to paint an error message instead. @@ -142,23 +96,10 @@ impl egui_wgpu::CallbackTrait for ReRendererCallback { return; }; - let Some(Some(view_builder)) = ctx - .active_frame - .per_frame_data_helper - .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 - ); - return; - }; - 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_pipelines, render_pass, screen_position); + self.view_builder + .composite(ctx, render_pipelines, render_pass, screen_position); } } diff --git a/crates/re_viewer_context/src/viewer_context.rs b/crates/re_viewer_context/src/viewer_context.rs index 6d9b9fd9c103..5c893b09bca8 100644 --- a/crates/re_viewer_context/src/viewer_context.rs +++ b/crates/re_viewer_context/src/viewer_context.rs @@ -45,7 +45,7 @@ pub struct ViewerContext<'a> { pub re_ui: &'a re_ui::ReUi, /// The global `re_renderer` context, holds on to all GPU resources. - pub render_ctx: &'a mut re_renderer::RenderContext, + pub render_ctx: &'a re_renderer::RenderContext, /// Interface for sending commands back to the app pub command_sender: &'a CommandSender,