review fixes

rerun-io · Dec 11, 2023 · 7a79e3f · 7a79e3f
1 parent 550de99
commit 7a79e3f
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 43 deletions.
diff --git a/crates/re_space_view_spatial/src/space_view_2d.rs b/crates/re_space_view_spatial/src/space_view_2d.rs
@@ -151,15 +151,6 @@ impl SpaceViewClass for SpatialSpaceView2D {
             .num_primitives
             .load(std::sync::atomic::Ordering::Relaxed);
 
-        // TODO(andreas): Pass on system_output.
-        crate::ui_2d::view_2d(
-            ctx,
-            ui,
-            state,
-            &system_output.context_systems,
-            &system_output.view_systems,
-            query,
-            system_output.draw_data,
-        )
+        crate::ui_2d::view_2d(ctx, ui, state, query, system_output)
     }
 }
diff --git a/crates/re_space_view_spatial/src/space_view_3d.rs b/crates/re_space_view_spatial/src/space_view_3d.rs
@@ -129,15 +129,6 @@ impl SpaceViewClass for SpatialSpaceView3D {
             .num_primitives
             .load(std::sync::atomic::Ordering::Relaxed);
 
-        // TODO(andreas): Pass on system_output.
-        crate::ui_3d::view_3d(
-            ctx,
-            ui,
-            state,
-            &system_output.context_systems,
-            &system_output.view_systems,
-            query,
-            system_output.draw_data,
-        )
+        crate::ui_3d::view_3d(ctx, ui, state, query, system_output)
     }
 }
diff --git a/crates/re_space_view_spatial/src/ui_2d.rs b/crates/re_space_view_spatial/src/ui_2d.rs
@@ -9,8 +9,8 @@ use re_renderer::view_builder::{TargetConfiguration, ViewBuilder};
 use re_space_view::controls::{DRAG_PAN2D_BUTTON, RESET_VIEW_BUTTON_TEXT, ZOOM_SCROLL_MODIFIER};
 use re_types::{archetypes::Pinhole, components::ViewCoordinates};
 use re_viewer_context::{
-    gpu_bridge, HoveredSpace, SpaceViewSystemExecutionError, ViewContextCollection,
-    ViewPartCollection, ViewQuery, ViewerContext,
+    gpu_bridge, HoveredSpace, SpaceViewSystemExecutionError, SystemExecutionOutput, ViewQuery,
+    ViewerContext,
 };
 
 use super::{
@@ -228,13 +228,17 @@ pub fn view_2d(
     ctx: &ViewerContext<'_>,
     ui: &mut egui::Ui,
     state: &mut SpatialSpaceViewState,
-    view_ctx: &ViewContextCollection,
-    parts: &ViewPartCollection,
     query: &ViewQuery<'_>,
-    mut draw_data: Vec<re_renderer::QueueableDrawData>,
+    system_output: re_viewer_context::SystemExecutionOutput,
 ) -> Result<(), SpaceViewSystemExecutionError> {
     re_tracing::profile_function!();
 
+    let SystemExecutionOutput {
+        view_systems: parts,
+        context_systems: view_ctx,
+        draw_data,
+    } = system_output;
+
     // Save off the available_size since this is used for some of the layout updates later
     let available_size = ui.available_size();
     let store = ctx.store_db.store();
@@ -312,7 +316,7 @@ pub fn view_2d(
 
         // Create labels now since their shapes participate are added to scene.ui for picking.
         let (label_shapes, ui_rects) = create_labels(
-            &collect_ui_labels(parts),
+            &collect_ui_labels(&parts),
             ui_from_canvas,
             &eye,
             ui,
@@ -330,15 +334,15 @@ pub fn view_2d(
                 eye,
                 &mut view_builder,
                 state,
-                view_ctx,
-                parts,
+                &view_ctx,
+                &parts,
                 &ui_rects,
                 query,
                 SpatialSpaceViewKind::TwoD,
             )?;
         }
 
-        for draw_data in draw_data.drain(..) {
+        for draw_data in draw_data {
             view_builder.queue_draw(draw_data);
         }
         if let Ok(shared_render_builders) = view_ctx.get::<SharedRenderBuilders>() {

diff --git a/crates/re_space_view_spatial/src/ui_3d.rs b/crates/re_space_view_spatial/src/ui_3d.rs
@@ -13,8 +13,8 @@ use re_space_view::controls::{
 };
 use re_types::components::ViewCoordinates;
 use re_viewer_context::{
-    gpu_bridge, HoveredSpace, Item, SpaceViewSystemExecutionError, ViewContextCollection,
-    ViewPartCollection, ViewQuery, ViewerContext,
+    gpu_bridge, HoveredSpace, Item, SpaceViewSystemExecutionError, SystemExecutionOutput,
+    ViewQuery, ViewerContext,
 };
 
 use crate::{
@@ -307,18 +307,21 @@ pub fn help_text(re_ui: &re_ui::ReUi) -> egui::WidgetText {
     layout.layout_job.into()
 }
 
-/// TODO(andreas): Split into smaller parts, more re-use with `ui_2d`
 pub fn view_3d(
     ctx: &ViewerContext<'_>,
     ui: &mut egui::Ui,
     state: &mut SpatialSpaceViewState,
-    view_ctx: &ViewContextCollection,
-    parts: &ViewPartCollection,
     query: &ViewQuery<'_>,
-    mut draw_data: Vec<re_renderer::QueueableDrawData>,
+    system_output: re_viewer_context::SystemExecutionOutput,
 ) -> Result<(), SpaceViewSystemExecutionError> {
     re_tracing::profile_function!();
 
+    let SystemExecutionOutput {
+        view_systems: parts,
+        context_systems: view_ctx,
+        draw_data,
+    } = system_output;
+
     let highlights = &query.highlights;
     let space_cameras = &parts.get::<CamerasPart>()?.space_cameras;
     let view_coordinates = ctx
@@ -412,7 +415,7 @@ pub fn view_3d(
 
     // Create labels now since their shapes participate are added to scene.ui for picking.
     let (label_shapes, ui_rects) = create_labels(
-        &collect_ui_labels(parts),
+        &collect_ui_labels(&parts),
         RectTransform::from_to(rect, rect),
         &eye,
         ui,
@@ -430,8 +433,8 @@ pub fn view_3d(
             eye,
             &mut view_builder,
             state,
-            view_ctx,
-            parts,
+            &view_ctx,
+            &parts,
             &ui_rects,
             query,
             SpatialSpaceViewKind::ThreeD,
@@ -586,7 +589,7 @@ pub fn view_3d(
         }
     }
 
-    for draw_data in draw_data.drain(..) {
+    for draw_data in draw_data {
         view_builder.queue_draw(draw_data);
     }
     if let Ok(shared_render_builders) = view_ctx.get::<SharedRenderBuilders>() {

diff --git a/crates/re_viewer_context/src/space_view/system_execution_output.rs b/crates/re_viewer_context/src/space_view/system_execution_output.rs
@@ -2,7 +2,7 @@ use crate::{ViewContextCollection, ViewPartCollection};
 
 /// Output of space view system execution.
 pub struct SystemExecutionOutput {
-    /// Executed view system    s, may hold state that the ui method needs.
+    /// Executed view systems, may hold state that the ui method needs.
     pub view_systems: ViewPartCollection,
 
     /// Executed context systems, may hold state that the ui method needs.
@@ -11,8 +11,8 @@ pub struct SystemExecutionOutput {
     /// Draw data gathered during execution of the view part systems.
     ///
     /// Ui methods are supposed to use this to create [`re_renderer::ViewBuilder`]s.
-    /// TODO(andreas): In the future view builder execution should be outside of the space view ui method.
-    ///                This would allow to run the wgpu command buffer buildup in parallel.
-    ///                (This implies that we'd pass out the readily built command buffer here instead of drawables.)
+    // _TODO(andreas)_: In the future view builder execution should be outside of the space view ui method.
+    //                This would allow to run the wgpu command buffer buildup in parallel.
+    //                (This implies that we'd pass out the readily built command buffer here instead of drawables.)
     pub draw_data: Vec<re_renderer::QueueableDrawData>,
 }