From bbdfa2e21785799d735343cff2574c1253e68c5c Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Wed, 13 Dec 2023 20:32:55 +0100 Subject: [PATCH 01/26] Clear blueprint via command-sender --- crates/re_viewer/src/app_state.rs | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/crates/re_viewer/src/app_state.rs b/crates/re_viewer/src/app_state.rs index 1d5fbaf08672..16fa62591b0a 100644 --- a/crates/re_viewer/src/app_state.rs +++ b/crates/re_viewer/src/app_state.rs @@ -6,7 +6,7 @@ use re_smart_channel::ReceiveSet; use re_space_view::DataQuery as _; use re_viewer_context::{ AppOptions, Caches, CommandSender, ComponentUiRegistry, PlayState, RecordingConfig, - SelectionState, SpaceViewClassRegistry, StoreContext, ViewerContext, + SelectionState, SpaceViewClassRegistry, StoreContext, SystemCommandSender as _, ViewerContext, }; use re_viewport::{ identify_entities_per_system_per_class, SpaceInfoCollection, Viewport, ViewportState, @@ -104,7 +104,20 @@ impl AppState { viewport_state, } = self; - let mut viewport = Viewport::from_db(store_context.blueprint, viewport_state); + let viewport = Viewport::from_db(store_context.blueprint, viewport_state); + + // If the blueprint is invalid, reset it. + if viewport.blueprint.is_invalid() { + re_log::warn!("Incompatible blueprint detected. Resetting to default."); + command_sender.send_system(re_viewer_context::SystemCommand::ResetBlueprint); + + // The blueprint isn't valid so nothing past this is going to work properly. + // we might as well return and it will get fixed on the next frame. + + // TODO(jleibs): If we move viewport loading up to a context where the StoreDb is mutable + // we can run the clear and re-load. + return; + } recording_config_entry(recording_configs, store_db.store_id().clone(), store_db) .selection_state @@ -125,7 +138,7 @@ impl AppState { viewport .blueprint .space_views - .values_mut() + .values() .flat_map(|space_view| { space_view.queries.iter().map(|query| { let resolver = @@ -163,12 +176,6 @@ impl AppState { // have the latest information. let spaces_info = SpaceInfoCollection::new(ctx.store_db); - // If the blueprint is invalid, reset it. - if viewport.blueprint.is_invalid() { - re_log::warn!("Incompatible blueprint detected. Resetting to default."); - viewport.blueprint.reset(&ctx, &spaces_info); - } - viewport.on_frame_start(&ctx, &spaces_info); // TODO(jleibs): Running the queries a second time is annoying, but we need From 7632b761a0c3eb23e91442ef3aba5f674b1e3327 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 14 Dec 2023 02:27:07 +0100 Subject: [PATCH 02/26] First pass tearing out the old viewport sync logic --- Cargo.lock | 1 + crates/re_viewer/src/app_state.rs | 23 +- crates/re_viewer/src/ui/blueprint_panel.rs | 2 +- .../re_viewer/src/ui/selection_history_ui.rs | 12 +- crates/re_viewer/src/ui/selection_panel.rs | 80 +++-- crates/re_viewport/Cargo.toml | 1 + crates/re_viewport/src/space_view.rs | 93 ++++-- .../src/space_view_entity_picker.rs | 12 +- crates/re_viewport/src/viewport.rs | 146 +++++---- crates/re_viewport/src/viewport_blueprint.rs | 299 ++++++------------ .../re_viewport/src/viewport_blueprint_ui.rs | 162 ++++++---- 11 files changed, 433 insertions(+), 398 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e5c320456326..3cff1a46550e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5267,6 +5267,7 @@ dependencies = [ "itertools 0.11.0", "nohash-hasher", "once_cell", + "parking_lot 0.12.1", "rayon", "re_arrow_store", "re_data_store", diff --git a/crates/re_viewer/src/app_state.rs b/crates/re_viewer/src/app_state.rs index 16fa62591b0a..968db6425d8a 100644 --- a/crates/re_viewer/src/app_state.rs +++ b/crates/re_viewer/src/app_state.rs @@ -141,8 +141,9 @@ impl AppState { .values() .flat_map(|space_view| { space_view.queries.iter().map(|query| { - let resolver = - query.build_resolver(space_view.id, &space_view.auto_properties); + let state = viewport.state.read(); + let props = state.space_view_props(space_view.id); + let resolver = query.build_resolver(space_view.id, props); ( query.id, query.execute_query( @@ -186,11 +187,12 @@ impl AppState { viewport .blueprint .space_views - .values_mut() + .values() .flat_map(|space_view| { space_view.queries.iter().map(|query| { - let resolver = - query.build_resolver(space_view.id, &space_view.auto_properties); + let state = viewport.state.read(); + let props = state.space_view_props(space_view.id); + let resolver = query.build_resolver(space_view.id, props); ( query.id, query.execute_query( @@ -206,12 +208,7 @@ impl AppState { ctx.query_results = &updated_query_results; time_panel.show_panel(&ctx, ui, app_blueprint.time_panel_expanded); - selection_panel.show_panel( - &ctx, - ui, - &mut viewport, - app_blueprint.selection_panel_expanded, - ); + selection_panel.show_panel(&ctx, ui, &viewport, app_blueprint.selection_panel_expanded); let central_panel_frame = egui::Frame { fill: ui.style().visuals.panel_fill, @@ -250,7 +247,7 @@ impl AppState { ui.add_space(4.0); } - blueprint_panel_ui(&mut viewport.blueprint, &ctx, ui, &spaces_info); + blueprint_panel_ui(&viewport.blueprint, &ctx, ui, &spaces_info); }, ); @@ -273,8 +270,6 @@ impl AppState { }); }); - viewport.sync_blueprint_changes(command_sender); - { // We move the time at the very end of the frame, // so we have one frame to see the first data before we move the time. diff --git a/crates/re_viewer/src/ui/blueprint_panel.rs b/crates/re_viewer/src/ui/blueprint_panel.rs index b7fae9404540..f946d3a0e32c 100644 --- a/crates/re_viewer/src/ui/blueprint_panel.rs +++ b/crates/re_viewer/src/ui/blueprint_panel.rs @@ -3,7 +3,7 @@ use re_viewport::{SpaceInfoCollection, ViewportBlueprint}; /// Show the Blueprint section of the left panel based on the current [`ViewportBlueprint`] pub fn blueprint_panel_ui( - blueprint: &mut ViewportBlueprint<'_>, + blueprint: &ViewportBlueprint, ctx: &ViewerContext<'_>, ui: &mut egui::Ui, spaces_info: &SpaceInfoCollection, diff --git a/crates/re_viewer/src/ui/selection_history_ui.rs b/crates/re_viewer/src/ui/selection_history_ui.rs index b7e00769bc18..76845c17fe19 100644 --- a/crates/re_viewer/src/ui/selection_history_ui.rs +++ b/crates/re_viewer/src/ui/selection_history_ui.rs @@ -15,7 +15,7 @@ impl SelectionHistoryUi { &mut self, re_ui: &re_ui::ReUi, ui: &mut egui::Ui, - blueprint: &ViewportBlueprint<'_>, + blueprint: &ViewportBlueprint, history: &mut SelectionHistory, ) -> Option { let next = self.next_button_ui(re_ui, ui, blueprint, history); @@ -27,7 +27,7 @@ impl SelectionHistoryUi { &mut self, re_ui: &re_ui::ReUi, ui: &mut egui::Ui, - blueprint: &ViewportBlueprint<'_>, + blueprint: &ViewportBlueprint, history: &mut SelectionHistory, ) -> Option { // undo selection @@ -77,7 +77,7 @@ impl SelectionHistoryUi { &mut self, re_ui: &re_ui::ReUi, ui: &mut egui::Ui, - blueprint: &ViewportBlueprint<'_>, + blueprint: &ViewportBlueprint, history: &mut SelectionHistory, ) -> Option { // redo selection @@ -126,7 +126,7 @@ impl SelectionHistoryUi { #[allow(clippy::unused_self)] fn history_item_ui( &mut self, - blueprint: &ViewportBlueprint<'_>, + blueprint: &ViewportBlueprint, ui: &mut egui::Ui, index: usize, history: &mut SelectionHistory, @@ -157,7 +157,7 @@ fn item_kind_ui(ui: &mut egui::Ui, sel: &Item) { ui.weak(RichText::new(format!("({})", sel.kind()))); } -fn item_collection_to_string(blueprint: &ViewportBlueprint<'_>, items: &ItemCollection) -> String { +fn item_collection_to_string(blueprint: &ViewportBlueprint, items: &ItemCollection) -> String { assert!(!items.is_empty()); // history never contains empty selections. if items.len() == 1 { item_to_string(blueprint, items.iter().next().unwrap()) @@ -168,7 +168,7 @@ fn item_collection_to_string(blueprint: &ViewportBlueprint<'_>, items: &ItemColl } } -fn item_to_string(blueprint: &ViewportBlueprint<'_>, item: &Item) -> String { +fn item_to_string(blueprint: &ViewportBlueprint, item: &Item) -> String { match item { Item::SpaceView(sid) => { if let Some(space_view) = blueprint.space_view(sid) { diff --git a/crates/re_viewer/src/ui/selection_panel.rs b/crates/re_viewer/src/ui/selection_panel.rs index 276373f2f6ba..a3e9ecd6b594 100644 --- a/crates/re_viewer/src/ui/selection_panel.rs +++ b/crates/re_viewer/src/ui/selection_panel.rs @@ -39,7 +39,7 @@ impl SelectionPanel { &mut self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui, - viewport: &mut Viewport<'_, '_>, + viewport: &Viewport<'_>, expanded: bool, ) { let screen_width = ui.ctx().screen_rect().width(); @@ -97,12 +97,7 @@ impl SelectionPanel { } #[allow(clippy::unused_self)] - fn contents( - &mut self, - ctx: &ViewerContext<'_>, - ui: &mut egui::Ui, - viewport: &mut Viewport<'_, '_>, - ) { + fn contents(&mut self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui, viewport: &Viewport<'_>) { re_tracing::profile_function!(); let query = ctx.current_query(); @@ -122,18 +117,18 @@ impl SelectionPanel { }; for (i, item) in selection.iter().enumerate() { ui.push_id(i, |ui| { - what_is_selected_ui(ui, ctx, &mut viewport.blueprint, item); + what_is_selected_ui(ui, ctx, &viewport.blueprint, item); match item { Item::Container(tile_id) => { - container_top_level_properties(ui, ctx, &mut viewport.blueprint, tile_id); + container_top_level_properties(ui, ctx, &viewport.blueprint, tile_id); } Item::SpaceView(space_view_id) => { space_view_top_level_properties( ui, ctx, - &mut viewport.blueprint, + &viewport.blueprint, space_view_id, ); } @@ -197,7 +192,7 @@ fn space_view_button( fn what_is_selected_ui( ui: &mut egui::Ui, ctx: &ViewerContext<'_>, - viewport: &mut ViewportBlueprint<'_>, + viewport: &ViewportBlueprint, item: &Item, ) { match item { @@ -236,7 +231,7 @@ fn what_is_selected_ui( list_existing_data_blueprints(ui, ctx, entity_path, viewport); } Item::SpaceView(space_view_id) => { - if let Some(space_view) = viewport.space_view_mut(space_view_id) { + if let Some(space_view) = viewport.space_view(space_view_id) { let space_view_class = space_view.class(ctx.space_view_class_registry); item_title_ui( ctx.re_ui, @@ -259,7 +254,7 @@ fn what_is_selected_ui( }; if let Some(space_view_id) = space_view_id { - if let Some(space_view) = viewport.space_view_mut(space_view_id) { + if let Some(space_view) = viewport.space_view(space_view_id) { item_title_ui( ctx.re_ui, ui, @@ -334,7 +329,7 @@ fn list_existing_data_blueprints( ui: &mut egui::Ui, ctx: &ViewerContext<'_>, entity_path: &EntityPath, - blueprint: &ViewportBlueprint<'_>, + blueprint: &ViewportBlueprint, ) { let space_views_with_path = blueprint.space_views_containing_entity_path(ctx, entity_path); @@ -367,20 +362,23 @@ fn list_existing_data_blueprints( fn space_view_top_level_properties( ui: &mut egui::Ui, ctx: &ViewerContext<'_>, - viewport: &mut ViewportBlueprint<'_>, + viewport: &ViewportBlueprint, space_view_id: &SpaceViewId, ) { - if let Some(space_view) = viewport.space_view_mut(space_view_id) { + if let Some(space_view) = viewport.space_view(space_view_id) { egui::Grid::new("space_view_top_level_properties") .num_columns(2) .show(ui, |ui| { + let mut name = space_view.display_name.clone(); ui.label("Name").on_hover_text( "The name of the Space View used for display purposes. This can be any text \ string.", ); - ui.text_edit_singleline(&mut space_view.display_name); + ui.text_edit_singleline(&mut name); ui.end_row(); + space_view.set_display_name(name, ctx); + ui.label("Space origin").on_hover_text( "The origin Entity for this Space View. For spatial Space Views, the Space \ View's origin is the same as this Entity's origin and all transforms are \ @@ -409,10 +407,11 @@ fn space_view_top_level_properties( fn container_top_level_properties( ui: &mut egui::Ui, _ctx: &ViewerContext<'_>, - viewport: &mut ViewportBlueprint<'_>, + viewport: &ViewportBlueprint, tile_id: &egui_tiles::TileId, ) { - if let Some(Tile::Container(container)) = viewport.tree.tiles.get_mut(*tile_id) { + // TODO(jleibs): fix container-editing + if let Some(Tile::Container(container)) = viewport.tree.tiles.get(*tile_id) { egui::Grid::new("container_top_level_properties") .num_columns(2) .show(ui, |ui| { @@ -447,7 +446,9 @@ fn container_top_level_properties( ); }); - container.set_kind(container_kind); + // TODO(jleibs): fix container-editing + re_log::warn!("TODO(jleibs): Fix container editing"); + //container.set_kind(container_kind); ui.end_row(); @@ -467,19 +468,30 @@ fn container_top_level_properties( ui.style_mut().wrap = Some(false); ui.set_min_width(64.0); + // TODO(jleibs): Fix container-editing + ui.label(format!("Layout: {}", grid_layout_to_string(&grid.layout))); + /* ui.selectable_value( &mut grid.layout, GridLayout::Auto, grid_layout_to_string(&GridLayout::Auto), ); + */ ui.separator(); for columns in 1..=grid.num_children() { + // TODO(jleibs): fix container-editing + ui.label(format!( + "Layout: {}", + grid_layout_to_string(&GridLayout::Columns(columns)) + )); + /* ui.selectable_value( &mut grid.layout, GridLayout::Columns(columns), grid_layout_to_string(&GridLayout::Columns(columns)), ); + */ } }); @@ -498,12 +510,7 @@ fn has_blueprint_section(item: &Item) -> bool { } /// What is the blueprint stuff for this item? -fn blueprint_ui( - ui: &mut egui::Ui, - ctx: &ViewerContext<'_>, - viewport: &mut Viewport<'_, '_>, - item: &Item, -) { +fn blueprint_ui(ui: &mut egui::Ui, ctx: &ViewerContext<'_>, viewport: &Viewport<'_>, item: &Item) { match item { Item::SpaceView(space_view_id) => { ui.horizontal(|ui| { @@ -524,13 +531,13 @@ fn blueprint_ui( if let Some(space_view) = viewport.blueprint.space_view(space_view_id) { let mut new_space_view = space_view.clone(); new_space_view.id = SpaceViewId::random(); - viewport.blueprint.add_space_view(new_space_view); - viewport.blueprint.mark_user_interaction(); + viewport.blueprint.add_space_view(new_space_view, ctx); + viewport.blueprint.mark_user_interaction(ctx); } } }); - if let Some(space_view) = viewport.blueprint.space_view_mut(space_view_id) { + if let Some(space_view) = viewport.blueprint.space_view(space_view_id) { if let Some(query) = space_view.queries.first() { let inclusions = query.expressions.inclusions.join("\n"); let mut edited_inclusions = inclusions.clone(); @@ -578,16 +585,19 @@ fn blueprint_ui( vec![row], )); - space_view.entities_determined_by_user = true; + space_view.set_entity_determined_by_user(ctx); } } } ui.add_space(ui.spacing().item_spacing.y); - if let Some(space_view) = viewport.blueprint.space_view_mut(space_view_id) { + if let Some(space_view) = viewport.blueprint.space_view(space_view_id) { let space_view_class = *space_view.class_identifier(); - let space_view_state = viewport.state.space_view_state_mut( + + let mut state = viewport.state.write(); + + let space_view_state = state.space_view_state_mut( ctx.space_view_class_registry, space_view.id, space_view.class_identifier(), @@ -615,7 +625,7 @@ fn blueprint_ui( .selection_ui( ctx, ui, - space_view_state, + space_view_state.space_view_state.as_mut(), &space_view.space_origin, space_view.id, &mut props, @@ -643,7 +653,7 @@ fn blueprint_ui( Item::InstancePath(space_view_id, instance_path) => { if let Some(space_view_id) = space_view_id { - if let Some(space_view) = viewport.blueprint.space_view_mut(space_view_id) { + if let Some(space_view) = viewport.blueprint.space_view(space_view_id) { if instance_path.instance_key.is_specific() { ui.horizontal(|ui| { ui.label("Part of"); @@ -687,7 +697,7 @@ fn blueprint_ui( } Item::DataBlueprintGroup(space_view_id, query_id, group_path) => { - if let Some(space_view) = viewport.blueprint.space_view_mut(space_view_id) { + if let Some(space_view) = viewport.blueprint.space_view(space_view_id) { let as_group = true; let query_result = ctx.lookup_query_result(*query_id); diff --git a/crates/re_viewport/Cargo.toml b/crates/re_viewport/Cargo.toml index 53c5385a7e98..d873246e2216 100644 --- a/crates/re_viewport/Cargo.toml +++ b/crates/re_viewport/Cargo.toml @@ -45,6 +45,7 @@ image = { workspace = true, default-features = false, features = ["png"] } itertools.workspace = true nohash-hasher.workspace = true once_cell.workspace = true +parking_lot.workspace = true rayon.workspace = true rmp-serde.workspace = true tinyvec.workspace = true diff --git a/crates/re_viewport/src/space_view.rs b/crates/re_viewport/src/space_view.rs index 40bfe88d078e..9eb2e2e59143 100644 --- a/crates/re_viewport/src/space_view.rs +++ b/crates/re_viewport/src/space_view.rs @@ -3,18 +3,21 @@ use re_arrow_store::LatestAtQuery; use re_data_store::{EntityPath, EntityProperties, StoreDb, TimeInt, VisibleHistory}; use re_data_store::{EntityPropertiesComponent, EntityPropertyMap}; -use re_log_types::{EntityPathExpr, Timeline}; +use re_log_types::{DataRow, EntityPathExpr, RowId, TimePoint, Timeline}; use re_query::query_archetype; use re_renderer::ScreenshotProcessor; use re_space_view::{DataQueryBlueprint, ScreenshotMode}; use re_space_view_time_series::TimeSeriesSpaceView; +use re_types::blueprint::components::{EntitiesDeterminedByUser, Name, SpaceViewOrigin}; use re_viewer_context::{ DataQueryId, DataResult, DynSpaceViewClass, PerSystemDataResults, PerSystemEntities, SpaceViewClass, SpaceViewClassIdentifier, SpaceViewHighlights, SpaceViewId, SpaceViewState, - SpaceViewSystemRegistry, StoreContext, SystemExecutionOutput, ViewQuery, ViewerContext, + SpaceViewSystemRegistry, StoreContext, SystemCommand, SystemCommandSender as _, + SystemExecutionOutput, ViewQuery, ViewerContext, }; use crate::system_execution::create_and_run_space_view_systems; +use crate::viewport_blueprint::{add_delta_from_single_component, save_single_component}; // ---------------------------------------------------------------------------- @@ -36,10 +39,6 @@ pub struct SpaceViewBlueprint { /// True if the user is expected to add entities themselves. False otherwise. pub entities_determined_by_user: bool, - - /// Auto Properties - // TODO(jleibs): This needs to be per-query - pub auto_properties: EntityPropertyMap, } /// Determine whether this `SpaceViewBlueprint` has user-edits relative to another `SpaceViewBlueprint` @@ -52,7 +51,6 @@ impl SpaceViewBlueprint { space_origin, queries, entities_determined_by_user, - auto_properties: _, } = self; id != &other.id @@ -92,7 +90,6 @@ impl SpaceViewBlueprint { space_origin: space_path.clone(), queries: vec![query], entities_determined_by_user: false, - auto_properties: Default::default(), } } @@ -160,10 +157,63 @@ impl SpaceViewBlueprint { space_origin, queries, entities_determined_by_user, - auto_properties: Default::default(), }) } + pub fn save_full(&self, ctx: &ViewerContext<'_>) { + let timepoint = TimePoint::timeless(); + + let arch = re_types::blueprint::archetypes::SpaceViewBlueprint::new( + self.class_identifier().as_str(), + ) + .with_display_name(self.display_name.clone()) + .with_space_origin(&self.space_origin) + .with_entities_determined_by_user(self.entities_determined_by_user) + .with_contents(self.queries.iter().map(|q| q.id)); + + let mut deltas = vec![]; + + if let Ok(row) = + DataRow::from_archetype(RowId::new(), timepoint.clone(), self.entity_path(), &arch) + { + deltas.push(row); + } + + for query in &self.queries { + add_delta_from_single_component( + &mut deltas, + &query.id.as_entity_path(), + &timepoint, + query.expressions.clone(), + ); + } + + ctx.command_sender + .send_system(SystemCommand::UpdateBlueprint( + ctx.store_context.blueprint.store_id().clone(), + deltas, + )); + } + + pub fn set_entity_determined_by_user(&self, ctx: &ViewerContext<'_>) { + let component = EntitiesDeterminedByUser(true); + save_single_component(&self.entity_path(), component, ctx); + } + + pub fn set_display_name(&self, name: String, ctx: &ViewerContext<'_>) { + if name != self.display_name { + let component = Name(name.into()); + save_single_component(&self.entity_path(), component, ctx); + } + } + + pub fn set_origin(&self, origin: &EntityPath, ctx: &ViewerContext<'_>) { + if origin != &self.space_origin { + let component = SpaceViewOrigin(origin.into()); + save_single_component(&self.entity_path(), component, ctx); + } + } + pub fn class_identifier(&self) -> &SpaceViewClassIdentifier { &self.class_identifier } @@ -182,7 +232,12 @@ impl SpaceViewBlueprint { space_view_class_registry.get_system_registry_or_log_error(&self.class_identifier) } - pub fn on_frame_start(&mut self, ctx: &ViewerContext<'_>, view_state: &mut dyn SpaceViewState) { + pub fn on_frame_start( + &self, + ctx: &ViewerContext<'_>, + view_state: &mut dyn SpaceViewState, + view_props: &mut EntityPropertyMap, + ) { while ScreenshotProcessor::next_readback_result( ctx.render_ctx, self.id.gpu_readback_id(), @@ -214,7 +269,7 @@ impl SpaceViewBlueprint { ctx, view_state, &per_system_entities, - &mut self.auto_properties, + view_props, ); } @@ -371,26 +426,26 @@ impl SpaceViewBlueprint { } // TODO(jleibs): Get rid of mut by sending blueprint update - pub fn add_entity_exclusion(&mut self, ctx: &ViewerContext<'_>, expr: EntityPathExpr) { + pub fn add_entity_exclusion(&self, ctx: &ViewerContext<'_>, expr: EntityPathExpr) { if let Some(query) = self.queries.first() { query.add_entity_exclusion(ctx, expr); } - self.entities_determined_by_user = true; + self.set_entity_determined_by_user(ctx); } // TODO(jleibs): Get rid of mut by sending blueprint update - pub fn add_entity_inclusion(&mut self, ctx: &ViewerContext<'_>, expr: EntityPathExpr) { + pub fn add_entity_inclusion(&self, ctx: &ViewerContext<'_>, expr: EntityPathExpr) { if let Some(query) = self.queries.first() { query.add_entity_inclusion(ctx, expr); } - self.entities_determined_by_user = true; + self.set_entity_determined_by_user(ctx); } - pub fn clear_entity_expression(&mut self, ctx: &ViewerContext<'_>, expr: &EntityPathExpr) { + pub fn clear_entity_expression(&self, ctx: &ViewerContext<'_>, expr: &EntityPathExpr) { if let Some(query) = self.queries.first() { query.clear_entity_expression(ctx, expr); } - self.entities_determined_by_user = true; + self.set_entity_determined_by_user(ctx); } pub fn exclusions(&self) -> impl Iterator + '_ { @@ -458,6 +513,8 @@ mod tests { ), ); + let auto_properties = Default::default(); + let mut entities_per_system_per_class = EntitiesPerSystemPerClass::default(); entities_per_system_per_class .entry("3D".into()) @@ -474,7 +531,7 @@ mod tests { let query = space_view.queries.first().unwrap(); - let resolver = query.build_resolver(space_view.id, &space_view.auto_properties); + let resolver = query.build_resolver(space_view.id, &auto_properties); // No overrides set. Everybody has default values. { diff --git a/crates/re_viewport/src/space_view_entity_picker.rs b/crates/re_viewport/src/space_view_entity_picker.rs index 4df0f1d8d285..a2dfe566e0ef 100644 --- a/crates/re_viewport/src/space_view_entity_picker.rs +++ b/crates/re_viewport/src/space_view_entity_picker.rs @@ -27,7 +27,7 @@ impl SpaceViewEntityPicker { &mut self, ctx: &ViewerContext<'_>, ui: &egui::Ui, - space_view: &mut SpaceViewBlueprint, + space_view: &SpaceViewBlueprint, ) -> bool { // This function fakes a modal window, since egui doesn't have them yet: https://github.com/emilk/egui/issues/686 @@ -83,11 +83,7 @@ impl SpaceViewEntityPicker { } } -fn add_entities_ui( - ctx: &ViewerContext<'_>, - ui: &mut egui::Ui, - space_view: &mut SpaceViewBlueprint, -) { +fn add_entities_ui(ctx: &ViewerContext<'_>, ui: &mut egui::Ui, space_view: &SpaceViewBlueprint) { let spaces_info = SpaceInfoCollection::new(ctx.store_db); let tree = &ctx.store_db.tree(); let heuristic_context_per_entity = compute_heuristic_context_for_entities(ctx.store_db); @@ -123,7 +119,7 @@ fn add_entities_tree_ui( ui: &mut egui::Ui, name: &str, tree: &EntityTree, - space_view: &mut SpaceViewBlueprint, + space_view: &SpaceViewBlueprint, query_result: &DataQueryResult, inclusions: &HashSet, exclusions: &HashSet, @@ -192,7 +188,7 @@ fn add_entities_line_ui( ui: &mut egui::Ui, name: &str, entity_tree: &EntityTree, - space_view: &mut SpaceViewBlueprint, + space_view: &SpaceViewBlueprint, query_result: &DataQueryResult, inclusions: &HashSet, exclusions: &HashSet, diff --git a/crates/re_viewport/src/viewport.rs b/crates/re_viewport/src/viewport.rs index fa95c6ce8ce0..fb5daec05b1e 100644 --- a/crates/re_viewport/src/viewport.rs +++ b/crates/re_viewport/src/viewport.rs @@ -7,12 +7,15 @@ use std::collections::BTreeMap; use ahash::HashMap; use egui_tiles::Behavior as _; +use once_cell::sync::Lazy; +use parking_lot::RwLock; +use re_data_store::EntityPropertyMap; use re_data_ui::item_ui; use re_ui::{Icon, ReUi}; use re_viewer_context::{ - CommandSender, Item, SpaceViewClassIdentifier, SpaceViewClassRegistry, SpaceViewId, - SpaceViewState, SystemExecutionOutput, ViewQuery, ViewerContext, + Item, SpaceViewClassIdentifier, SpaceViewClassRegistry, SpaceViewId, SpaceViewState, + SystemExecutionOutput, ViewQuery, ViewerContext, }; use crate::{ @@ -23,13 +26,20 @@ use crate::{ SpaceInfoCollection, SpaceViewBlueprint, ViewportBlueprint, }; +// State for each `SpaceView` including both the auto properties and +// the internal state of the space view itself. +pub struct PerSpaceViewState { + pub auto_properties: EntityPropertyMap, + pub space_view_state: Box, +} + // ---------------------------------------------------------------------------- /// State for the [`Viewport`] that persists across frames but otherwise /// is not saved. #[derive(Default)] pub struct ViewportState { pub(crate) space_view_entity_window: Option, - space_view_states: HashMap>, + space_view_states: HashMap, /// List of all space views that were visible *on screen* (excluding e.g. unselected tabs) the last frame. /// @@ -37,75 +47,65 @@ pub struct ViewportState { space_views_displayed_last_frame: Vec, } +static DEFAULT_PROPS: Lazy = Lazy::::new(Default::default); + impl ViewportState { pub fn space_view_state_mut( &mut self, space_view_class_registry: &SpaceViewClassRegistry, space_view_id: SpaceViewId, space_view_class: &SpaceViewClassIdentifier, - ) -> &mut dyn SpaceViewState { + ) -> &mut PerSpaceViewState { self.space_view_states .entry(space_view_id) - .or_insert_with(|| { - space_view_class_registry + .or_insert_with(|| PerSpaceViewState { + auto_properties: Default::default(), + space_view_state: space_view_class_registry .get_class_or_log_error(space_view_class) - .new_state() + .new_state(), }) - .as_mut() + } + + pub fn space_view_props(&self, space_view_id: SpaceViewId) -> &EntityPropertyMap { + self.space_view_states + .get(&space_view_id) + .map_or(&DEFAULT_PROPS, |state| &state.auto_properties) } } // ---------------------------------------------------------------------------- /// Defines the layout of the Viewport -pub struct Viewport<'a, 'b> { - /// The initial state of the Viewport read from the blueprint store on this frame. - /// - /// This is used to compare to the possibly mutated blueprint to - /// determine whether or not we need to save changes back - /// to the store as part of `sync_blueprint_changes`. - start_of_frame_snapshot: ViewportBlueprint<'a>, - +pub struct Viewport<'a> { // This is what me mutate during the frame. - pub blueprint: ViewportBlueprint<'a>, + pub blueprint: ViewportBlueprint, - pub state: &'b mut ViewportState, + pub state: RwLock<&'a mut ViewportState>, } -impl<'a, 'b> Viewport<'a, 'b> { - pub fn from_db(blueprint_db: &'a re_data_store::StoreDb, state: &'b mut ViewportState) -> Self { +impl<'a> Viewport<'a> { + pub fn from_db(blueprint_db: &re_data_store::StoreDb, state: &'a mut ViewportState) -> Self { re_tracing::profile_function!(); let blueprint = load_viewport_blueprint(blueprint_db); - let start_of_frame_snapshot = blueprint.clone(); - Self { - start_of_frame_snapshot, blueprint, - state, + state: RwLock::new(state), } } - pub fn sync_blueprint_changes(&self, command_sender: &CommandSender) { - ViewportBlueprint::sync_viewport_blueprint( - &self.start_of_frame_snapshot, - &self.blueprint, - command_sender, - ); + pub fn show_add_remove_entities_window(&self, space_view_id: SpaceViewId) { + self.state.write().space_view_entity_window = Some(SpaceViewEntityPicker { space_view_id }); } - pub fn show_add_remove_entities_window(&mut self, space_view_id: SpaceViewId) { - self.state.space_view_entity_window = Some(SpaceViewEntityPicker { space_view_id }); - } + pub fn viewport_ui(&self, ui: &mut egui::Ui, ctx: &'a ViewerContext<'_>) { + let Viewport { blueprint, state } = self; - pub fn viewport_ui(&mut self, ui: &mut egui::Ui, ctx: &'a ViewerContext<'_>) { - let Viewport { - blueprint, state, .. - } = self; + let mut state = state.write(); if let Some(window) = &mut state.space_view_entity_window { - if let Some(space_view) = blueprint.space_views.get_mut(&window.space_view_id) { + if let Some(space_view) = blueprint.space_views.get(&window.space_view_id) { if !window.ui(ctx, ui, space_view) { state.space_view_entity_window = None; } @@ -120,33 +120,31 @@ impl<'a, 'b> Viewport<'a, 'b> { return; } + let mut maximized = blueprint.maximized; + if let Some(space_view_id) = blueprint.maximized { if !blueprint.space_views.contains_key(&space_view_id) { - blueprint.maximized = None; // protect against bad deserialized data + maximized = None; } else if let Some(tile_id) = blueprint.tree.tiles.find_pane(&space_view_id) { if !blueprint.tree.tiles.is_visible(tile_id) { - blueprint.maximized = None; // Automatically de-maximize views that aren't visible anymore. + maximized = None; } } } - let mut maximized_tree; - - let tree = if let Some(space_view_id) = blueprint.maximized { + // TODO(jleibs): This tree won't have the edits from `viewport_blueprint_ui`. + // Maybe we should route that all the way through to here and only save it once. + let mut tree = if let Some(space_view_id) = blueprint.maximized { let mut tiles = egui_tiles::Tiles::default(); let root = tiles.insert_pane(space_view_id); - maximized_tree = egui_tiles::Tree::new("viewport_tree", root, tiles); - &mut maximized_tree + egui_tiles::Tree::new("viewport_tree", root, tiles) } else { - if blueprint.tree.is_empty() { - blueprint.tree = super::auto_layout::tree_from_space_views( - ctx.space_view_class_registry, - &blueprint.space_views, - ); - } - &mut blueprint.tree + blueprint.tree.clone() }; + // Snapshot the tree so we can save it if it changes + let tree_snapshot = tree.clone(); + let executed_systems_per_space_view = execute_systems_for_space_views( ctx, std::mem::take(&mut state.space_views_displayed_last_frame), @@ -159,10 +157,10 @@ impl<'a, 'b> Viewport<'a, 'b> { re_tracing::profile_scope!("tree.ui"); let mut tab_viewer = TabViewer { - viewport_state: state, + viewport_state: &mut state, ctx, space_views: &blueprint.space_views, - maximized: &mut blueprint.maximized, + maximized: &mut maximized, edited: false, space_views_displayed_current_frame: Vec::new(), executed_systems_per_space_view, @@ -180,24 +178,39 @@ impl<'a, 'b> Viewport<'a, 'b> { ); } - blueprint.auto_layout = false; + ViewportBlueprint::set_auto_layout(false, ctx); } state.space_views_displayed_last_frame = tab_viewer.space_views_displayed_current_frame; }); + + // TODO(jleibs): These edits happen independently of those made by `viewport_blueprint_ui`. + // If both edit there will be a conflict and some will get missed. + if tree != tree_snapshot && blueprint.maximized.is_none() { + ViewportBlueprint::set_tree(tree, ctx); + } + + if maximized != blueprint.maximized { + ViewportBlueprint::set_maximized(maximized, ctx); + } } - pub fn on_frame_start(&mut self, ctx: &ViewerContext<'_>, spaces_info: &SpaceInfoCollection) { + pub fn on_frame_start(&self, ctx: &ViewerContext<'_>, spaces_info: &SpaceInfoCollection) { re_tracing::profile_function!(); - for space_view in self.blueprint.space_views.values_mut() { - let space_view_state = self.state.space_view_state_mut( + let mut state = self.state.write(); + + for space_view in self.blueprint.space_views.values() { + let PerSpaceViewState { + auto_properties, + space_view_state, + } = state.space_view_state_mut( ctx.space_view_class_registry, space_view.id, space_view.class_identifier(), ); - space_view.on_frame_start(ctx, space_view_state); + space_view.on_frame_start(ctx, space_view_state.as_mut(), auto_properties); } if self.blueprint.auto_space_views { @@ -205,7 +218,7 @@ impl<'a, 'b> Viewport<'a, 'b> { default_created_space_views(ctx, spaces_info, ctx.entities_per_system_per_class) { if self.should_auto_add_space_view(&space_view_candidate) { - self.blueprint.add_space_view(space_view_candidate); + self.blueprint.add_space_view(space_view_candidate, ctx); } } } @@ -306,7 +319,10 @@ impl<'a, 'b> egui_tiles::Behavior for TabViewer<'a, 'b> { space_view_blueprint.execute_systems(self.ctx, latest_at, highlights) }; - let space_view_state = self.viewport_state.space_view_state_mut( + let PerSpaceViewState { + auto_properties: _, + space_view_state, + } = self.viewport_state.space_view_state_mut( self.ctx.space_view_class_registry, space_view_blueprint.id, space_view_blueprint.class_identifier(), @@ -315,7 +331,13 @@ impl<'a, 'b> egui_tiles::Behavior for TabViewer<'a, 'b> { self.space_views_displayed_current_frame .push(space_view_blueprint.id); - space_view_blueprint.scene_ui(space_view_state, self.ctx, ui, &query, system_output); + space_view_blueprint.scene_ui( + space_view_state.as_mut(), + self.ctx, + ui, + &query, + system_output, + ); Default::default() } diff --git a/crates/re_viewport/src/viewport_blueprint.rs b/crates/re_viewport/src/viewport_blueprint.rs index 16847cdcff89..59f80f24c566 100644 --- a/crates/re_viewport/src/viewport_blueprint.rs +++ b/crates/re_viewport/src/viewport_blueprint.rs @@ -1,22 +1,20 @@ use std::collections::BTreeMap; +use parking_lot::Mutex; use re_arrow_store::LatestAtQuery; -use re_data_store::{EntityPath, StoreDb}; +use re_data_store::EntityPath; use re_log_types::{DataRow, RowId, TimePoint, Timeline}; use re_query::query_archetype; use re_types_core::{archetypes::Clear, AsComponents as _}; use re_viewer_context::{ - CommandSender, Item, SpaceViewClassIdentifier, SpaceViewId, SystemCommand, SystemCommandSender, - ViewerContext, + Item, SpaceViewClassIdentifier, SpaceViewId, SystemCommand, SystemCommandSender, ViewerContext, }; use crate::{ blueprint::components::{ AutoLayout, AutoSpaceViews, IncludedSpaceViews, SpaceViewMaximized, ViewportLayout, }, - space_info::SpaceInfoCollection, space_view::SpaceViewBlueprint, - space_view_heuristics::default_created_space_views, VIEWPORT_PATH, }; @@ -26,16 +24,14 @@ use crate::{ // so that we don't iterate over something while modifying it. #[derive(Clone, Default)] pub(crate) struct TreeActions { + pub reset: bool, + pub create: Option, pub focus_tab: Option, pub remove: Vec, } /// Describes the layout and contents of the Viewport Panel. -#[derive(Clone)] -pub struct ViewportBlueprint<'a> { - /// The StoreDb used to instantiate this blueprint - blueprint_db: &'a StoreDb, - +pub struct ViewportBlueprint { /// Where the space views are stored. /// /// Not a hashmap in order to preserve the order of the space views. @@ -59,10 +55,11 @@ pub struct ViewportBlueprint<'a> { /// /// We delay any modifications to the tree until the end of the frame, /// so that we don't mutate something while inspecitng it. - pub(crate) deferred_tree_actions: TreeActions, + //TODO(jleibs): Can we use the SystemCommandSender for this, too? + pub(crate) deferred_tree_actions: Mutex, } -impl<'a> ViewportBlueprint<'a> { +impl ViewportBlueprint { /// Determine whether all views in a blueprint are invalid. /// /// This most commonly happens due to a change in struct definition that @@ -80,41 +77,6 @@ impl<'a> ViewportBlueprint<'a> { .all(|sv| sv.class_identifier() == &SpaceViewClassIdentifier::invalid()) } - /// Reset the blueprint to a default state using some heuristics. - pub fn reset(&mut self, ctx: &ViewerContext<'_>, spaces_info: &SpaceInfoCollection) { - // TODO(jleibs): When using blueprint API, "reset" should go back to the initially transmitted - // blueprint, not the default blueprint. - re_tracing::profile_function!(); - - let ViewportBlueprint { - blueprint_db: _, - space_views, - tree, - maximized, - auto_layout, - auto_space_views, - deferred_tree_actions: tree_actions, - } = self; - - // Note, it's important that these values match the behavior in `load_viewport_blueprint` below. - *space_views = Default::default(); - *tree = egui_tiles::Tree::empty("viewport_tree"); - *maximized = None; - *auto_layout = true; - // Only enable auto-space-views if this is the app-default blueprint - *auto_space_views = self - .blueprint_db - .store_info() - .map_or(false, |ri| ri.is_app_default_blueprint()); - *tree_actions = Default::default(); - - for space_view in - default_created_space_views(ctx, spaces_info, ctx.entities_per_system_per_class) - { - self.add_space_view(space_view); - } - } - pub fn space_view_ids(&self) -> impl Iterator + '_ { self.space_views.keys() } @@ -130,28 +92,38 @@ impl<'a> ViewportBlueprint<'a> { self.space_views.get_mut(space_view_id) } - pub(crate) fn remove(&mut self, space_view_id: &SpaceViewId) -> Option { - self.mark_user_interaction(); - - let Self { - blueprint_db: _, - space_views, - tree, - maximized, - auto_layout: _, - auto_space_views: _, - deferred_tree_actions: _, - } = self; - - if *maximized == Some(*space_view_id) { - *maximized = None; - } + pub(crate) fn remove_space_view(&self, space_view_id: &SpaceViewId, ctx: &ViewerContext<'_>) { + self.mark_user_interaction(ctx); - if let Some(tile_id) = tree.tiles.find_pane(space_view_id) { - tree.tiles.remove(tile_id); + let timepoint = TimePoint::timeless(); + let mut deltas = vec![]; + + if self.maximized == Some(*space_view_id) { + let component = SpaceViewMaximized(None); + add_delta_from_single_component( + &mut deltas, + &VIEWPORT_PATH.into(), + &timepoint, + component, + ); } - space_views.remove(space_view_id) + let component = IncludedSpaceViews( + self.space_views + .keys() + .filter(|id| id != &space_view_id) + .map(|id| (*id).into()) + .collect(), + ); + add_delta_from_single_component(&mut deltas, &VIEWPORT_PATH.into(), &timepoint, component); + + clear_space_view(&mut deltas, space_view_id); + + ctx.command_sender + .send_system(SystemCommand::UpdateBlueprint( + ctx.store_context.blueprint.store_id().clone(), + deltas, + )); } /// If `false`, the item is referring to data that is not present in this blueprint. @@ -184,16 +156,23 @@ impl<'a> ViewportBlueprint<'a> { } } - pub fn mark_user_interaction(&mut self) { + pub fn mark_user_interaction(&self, ctx: &ViewerContext<'_>) { if self.auto_layout { re_log::trace!("User edits - will no longer auto-layout"); } - self.auto_layout = false; - self.auto_space_views = false; + let component = AutoLayout(false); + save_single_component(&VIEWPORT_PATH.into(), component, ctx); + + let component = AutoSpaceViews(false); + save_single_component(&VIEWPORT_PATH.into(), component, ctx); } - pub fn add_space_view(&mut self, mut space_view: SpaceViewBlueprint) -> SpaceViewId { + pub fn add_space_view( + &self, + mut space_view: SpaceViewBlueprint, + ctx: &ViewerContext<'_>, + ) -> SpaceViewId { let space_view_id = space_view.id; // Find a unique name for the space view @@ -213,31 +192,20 @@ impl<'a> ViewportBlueprint<'a> { space_view.display_name = unique_name; - self.space_views.insert(space_view_id, space_view); + // Save the space view to the store + space_view.save_full(ctx); - if self.auto_layout { - // Re-run the auto-layout next frame: - re_log::trace!("Added a space view with no user edits yet - will re-run auto-layout"); - self.tree = egui_tiles::Tree::empty("viewport_tree"); - } else { - // Try to insert it in the tree, in the top level: - if let Some(root_id) = self.tree.root { - let tile_id = self.tree.tiles.insert_pane(space_view_id); - if let Some(egui_tiles::Tile::Container(container)) = - self.tree.tiles.get_mut(root_id) - { - re_log::trace!("Inserting new space view into root container"); - container.add_child(tile_id); - } else { - re_log::trace!("Root was not a container - will re-run auto-layout"); - self.tree = egui_tiles::Tree::empty("viewport_tree"); - } - } else { - re_log::trace!("No root found - will re-run auto-layout"); - } - } + // Update the space-view ids: + let component = IncludedSpaceViews( + self.space_views + .keys() + .map(|id| (*id).into()) + .chain(std::iter::once(space_view_id.into())) + .collect(), + ); + save_single_component(&VIEWPORT_PATH.into(), component, ctx); - self.deferred_tree_actions.focus_tab = Some(space_view_id); + self.deferred_tree_actions.lock().create = Some(space_view_id); space_view_id } @@ -265,79 +233,26 @@ impl<'a> ViewportBlueprint<'a> { .collect() } - /// Compares the before and after snapshots and sends any necessary deltas to the store. - pub fn sync_viewport_blueprint( - before: &ViewportBlueprint<'_>, - after: &ViewportBlueprint<'_>, - command_sender: &CommandSender, - ) { - let mut deltas = vec![]; - - let entity_path = EntityPath::from(VIEWPORT_PATH); - - // TODO(jleibs): Seq instead of timeless? - let timepoint = TimePoint::timeless(); - - if after.space_views.len() != before.space_views.len() - || after - .space_views - .keys() - .zip(before.space_views.keys()) - .any(|(a, b)| a != b) - { - let component = - IncludedSpaceViews(after.space_views.keys().map(|id| (*id).into()).collect()); - add_delta_from_single_component(&mut deltas, &entity_path, &timepoint, component); - } - - if after.auto_layout != before.auto_layout { - let component = AutoLayout(after.auto_layout); - add_delta_from_single_component(&mut deltas, &entity_path, &timepoint, component); - } - - if after.auto_space_views != before.auto_space_views { - let component = AutoSpaceViews(after.auto_space_views); - add_delta_from_single_component(&mut deltas, &entity_path, &timepoint, component); - } - - if after.maximized != before.maximized { - let component = SpaceViewMaximized(after.maximized.map(|id| id.into())); - add_delta_from_single_component(&mut deltas, &entity_path, &timepoint, component); - } - - if after.tree != before.tree { - re_log::trace!("Syncing tree"); - - let component = ViewportLayout(after.tree.clone()); - - add_delta_from_single_component(&mut deltas, &entity_path, &timepoint, component); - } - - // Add any new or modified space views - for id in after.space_view_ids() { - if let Some(space_view) = after.space_view(id) { - sync_space_view(&mut deltas, space_view, before.space_view(id)); - } - } + pub fn set_auto_layout(value: bool, ctx: &ViewerContext<'_>) { + let component = AutoLayout(value); + save_single_component(&VIEWPORT_PATH.into(), component, ctx); + } - // Remove any deleted space views - for space_view_id in before.space_view_ids() { - if after.space_view(space_view_id).is_none() { - clear_space_view(&mut deltas, space_view_id); - } - } + pub fn set_maximized(space_view_id: Option, ctx: &ViewerContext<'_>) { + let component = SpaceViewMaximized(space_view_id.map(|id| id.into())); + save_single_component(&VIEWPORT_PATH.into(), component, ctx); + } - command_sender.send_system(SystemCommand::UpdateBlueprint( - after.blueprint_db.store_id().clone(), - deltas, - )); + pub fn set_tree(tree: egui_tiles::Tree, ctx: &ViewerContext<'_>) { + let component = ViewportLayout(tree); + save_single_component(&VIEWPORT_PATH.into(), component, ctx); } } // ---------------------------------------------------------------------------- // TODO(jleibs): Move this helper to a better location -fn add_delta_from_single_component<'a, C>( +pub fn add_delta_from_single_component<'a, C>( deltas: &mut Vec, entity_path: &EntityPath, timepoint: &TimePoint, @@ -358,9 +273,33 @@ fn add_delta_from_single_component<'a, C>( deltas.push(row); } +// TODO(jleibs): Move this helper to a better location +pub fn save_single_component<'a, C>(entity_path: &EntityPath, component: C, ctx: &ViewerContext<'_>) +where + C: re_types::Component + Clone + 'a, + std::borrow::Cow<'a, C>: std::convert::From, +{ + let timepoint = TimePoint::timeless(); + + let row = DataRow::from_cells1_sized( + RowId::new(), + entity_path.clone(), + timepoint.clone(), + 1, + [component], + ) + .unwrap(); // TODO(emilk): statically check that the component is a mono-component - then this cannot fail! + + ctx.command_sender + .send_system(SystemCommand::UpdateBlueprint( + ctx.store_context.blueprint.store_id().clone(), + vec![row], + )); +} + // ---------------------------------------------------------------------------- -pub fn load_viewport_blueprint(blueprint_db: &re_data_store::StoreDb) -> ViewportBlueprint<'_> { +pub fn load_viewport_blueprint(blueprint_db: &re_data_store::StoreDb) -> ViewportBlueprint { re_tracing::profile_function!(); let query = LatestAtQuery::latest(Timeline::default()); @@ -420,7 +359,6 @@ pub fn load_viewport_blueprint(blueprint_db: &re_data_store::StoreDb) -> Viewpor .0; ViewportBlueprint { - blueprint_db, space_views, tree, maximized, @@ -453,47 +391,6 @@ pub fn load_viewport_blueprint(blueprint_db: &re_data_store::StoreDb) -> Viewpor // ---------------------------------------------------------------------------- -pub fn sync_space_view( - deltas: &mut Vec, - space_view: &SpaceViewBlueprint, - snapshot: Option<&SpaceViewBlueprint>, -) { - if snapshot.map_or(true, |snapshot| space_view.has_edits(snapshot)) { - // TODO(jleibs): Seq instead of timeless? - let timepoint = TimePoint::timeless(); - - let arch = re_types::blueprint::archetypes::SpaceViewBlueprint::new( - space_view.class_identifier().as_str(), - ) - .with_display_name(space_view.display_name.clone()) - .with_space_origin(&space_view.space_origin) - .with_entities_determined_by_user(space_view.entities_determined_by_user) - .with_contents(space_view.queries.iter().map(|q| q.id)); - - if let Ok(row) = DataRow::from_archetype( - RowId::new(), - timepoint.clone(), - space_view.entity_path(), - &arch, - ) { - deltas.push(row); - } - - // The only time we need to create a query is if this is a new space-view. All other edits - // happen directly via `UpdateBlueprint` commands. - if snapshot.is_none() { - for query in &space_view.queries { - add_delta_from_single_component( - deltas, - &query.id.as_entity_path(), - &timepoint, - query.expressions.clone(), - ); - } - } - } -} - pub fn clear_space_view(deltas: &mut Vec, space_view_id: &SpaceViewId) { // TODO(jleibs): Seq instead of timeless? let timepoint = TimePoint::timeless(); diff --git a/crates/re_viewport/src/viewport_blueprint_ui.rs b/crates/re_viewport/src/viewport_blueprint_ui.rs index 98e8a46ddbcf..dad9aadfff05 100644 --- a/crates/re_viewport/src/viewport_blueprint_ui.rs +++ b/crates/re_viewport/src/viewport_blueprint_ui.rs @@ -13,30 +13,63 @@ use re_viewer_context::{ }; use crate::{ - space_view_heuristics::all_possible_space_views, viewport_blueprint::TreeActions, - SpaceInfoCollection, SpaceViewBlueprint, ViewportBlueprint, + space_view_heuristics::all_possible_space_views, SpaceInfoCollection, SpaceViewBlueprint, + ViewportBlueprint, }; -impl ViewportBlueprint<'_> { +impl ViewportBlueprint { /// Show the blueprint panel tree view. - pub fn tree_ui(&mut self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui) { + pub fn tree_ui(&self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui) { re_tracing::profile_function!(); + let mut tree = self.tree.clone(); + egui::ScrollArea::both() .id_source("blueprint_tree_scroll_area") .auto_shrink([true, false]) .show(ui, |ui| { ctx.re_ui.panel_content(ui, |_, ui| { if let Some(root) = self.tree.root() { - self.tile_ui(ctx, ui, root); + self.tile_ui(&mut tree, ctx, ui, root); } }); }); - let TreeActions { focus_tab, remove } = std::mem::take(&mut self.deferred_tree_actions); + // At the end of the Tree-UI, we can safely apply deferred actions. + + let mut deferred = self.deferred_tree_actions.lock(); + + let mut reset = std::mem::take(&mut deferred.reset); + let create = std::mem::take(&mut deferred.create); + let mut focus_tab = std::mem::take(&mut deferred.focus_tab); + let remove = std::mem::take(&mut deferred.remove); + + if let Some(create) = &create { + if self.auto_layout { + // Re-run the auto-layout next frame: + re_log::trace!( + "Added a space view with no user edits yet - will re-run auto-layout" + ); + + reset = true; + } else if let Some(root_id) = tree.root { + let tile_id = tree.tiles.insert_pane(*create); + if let Some(egui_tiles::Tile::Container(container)) = tree.tiles.get_mut(root_id) { + re_log::trace!("Inserting new space view into root container"); + container.add_child(tile_id); + } else { + re_log::trace!("Root was not a container - will re-run auto-layout"); + reset = true; + } + } else { + re_log::trace!("No root found - will re-run auto-layout"); + } + + focus_tab = Some(*create); + } if let Some(focus_tab) = &focus_tab { - let found = self.tree.make_active(|tile| match tile { + let found = tree.make_active(|tile| match tile { egui_tiles::Tile::Pane(space_view_id) => space_view_id == focus_tab, egui_tiles::Tile::Container(_) => false, }); @@ -44,16 +77,28 @@ impl ViewportBlueprint<'_> { } for tile_id in remove { - for tile in self.tree.tiles.remove_recursively(tile_id) { + for tile in tree.tiles.remove_recursively(tile_id) { if let egui_tiles::Tile::Pane(space_view_id) = tile { - self.remove(&space_view_id); + tree.tiles.remove(tile_id); + self.remove_space_view(&space_view_id, ctx); } } if Some(tile_id) == self.tree.root { - self.tree.root = None; + tree.root = None; } } + + if reset { + re_log::trace!("Resetting viewport tree"); + tree = super::auto_layout::tree_from_space_views( + ctx.space_view_class_registry, + &self.space_views, + ); + Self::set_tree(tree, ctx); + } else if tree != self.tree { + Self::set_tree(tree, ctx); + } } /// If a group or spaceview has a total of this number of elements, show its subtree by default? @@ -62,27 +107,34 @@ impl ViewportBlueprint<'_> { 2 <= num_children && num_children <= 3 } - fn tile_ui(&mut self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui, tile_id: egui_tiles::TileId) { + fn tile_ui( + &self, + tree: &mut egui_tiles::Tree, + ctx: &ViewerContext<'_>, + ui: &mut egui::Ui, + tile_id: egui_tiles::TileId, + ) { // Temporarily remove the tile so we don't get borrow-checker fights: - let Some(mut tile) = self.tree.tiles.remove(tile_id) else { + let Some(mut tile) = tree.tiles.remove(tile_id) else { return; }; match &mut tile { egui_tiles::Tile::Container(container) => { - self.container_tree_ui(ctx, ui, tile_id, container); + self.container_tree_ui(tree, ctx, ui, tile_id, container); } egui_tiles::Tile::Pane(space_view_id) => { // A space view - self.space_view_entry_ui(ctx, ui, tile_id, space_view_id); + self.space_view_entry_ui(tree, ctx, ui, tile_id, space_view_id); } }; - self.tree.tiles.insert(tile_id, tile); + tree.tiles.insert(tile_id, tile); } fn container_tree_ui( - &mut self, + &self, + tree: &mut egui_tiles::Tree, ctx: &ViewerContext<'_>, ui: &mut egui::Ui, tile_id: egui_tiles::TileId, @@ -93,8 +145,8 @@ impl ViewportBlueprint<'_> { // This means we won't be showing the visibility button of the parent container, // so if the child is made invisible, we should do the same for the parent. let child_is_visible = self.tree.is_visible(child_id); - self.tree.set_visible(tile_id, child_is_visible); - return self.tile_ui(ctx, ui, child_id); + tree.set_visible(tile_id, child_is_visible); + return self.tile_ui(tree, ctx, ui, child_id); } let item = Item::Container(tile_id); @@ -119,7 +171,7 @@ impl ViewportBlueprint<'_> { }) .show_collapsing(ui, ui.id().with(tile_id), default_open, |_, ui| { for &child in container.children() { - self.tile_ui(ctx, ui, child); + self.tile_ui(tree, ctx, ui, child); } }) .item_response; @@ -127,8 +179,8 @@ impl ViewportBlueprint<'_> { item_ui::select_hovered_on_click(ctx, &response, &[item]); if remove { - self.mark_user_interaction(); - self.deferred_tree_actions.remove.push(tile_id); + self.mark_user_interaction(ctx); + self.deferred_tree_actions.lock().remove.push(tile_id); } if visibility_changed { @@ -136,21 +188,24 @@ impl ViewportBlueprint<'_> { re_log::trace!("Container visibility changed - will no longer auto-layout"); } - self.auto_layout = false; // Keep `auto_space_views` enabled. - self.tree.set_visible(tile_id, visible); + // Keep `auto_space_views` enabled. + Self::set_auto_layout(false, ctx); + + tree.set_visible(tile_id, visible); } } fn space_view_entry_ui( - &mut self, + &self, + tree: &mut egui_tiles::Tree, ctx: &ViewerContext<'_>, ui: &mut egui::Ui, tile_id: egui_tiles::TileId, space_view_id: &SpaceViewId, ) { - let Some(space_view) = self.space_views.get_mut(space_view_id) else { + let Some(space_view) = self.space_views.get(space_view_id) else { re_log::warn_once!("Bug: asked to show a ui for a Space View that doesn't exist"); - self.deferred_tree_actions.remove.push(tile_id); + self.deferred_tree_actions.lock().remove.push(tile_id); return; }; debug_assert_eq!(space_view.id, *space_view_id); @@ -185,7 +240,7 @@ impl ViewportBlueprint<'_> { let response = remove_button_ui(re_ui, ui, "Remove Space View from the Viewport"); if response.clicked() { - self.deferred_tree_actions.remove.push(tile_id); + self.deferred_tree_actions.lock().remove.push(tile_id); } response | vis_response @@ -211,7 +266,7 @@ impl ViewportBlueprint<'_> { .on_hover_text("Space View"); if response.clicked() { - self.deferred_tree_actions.focus_tab = Some(space_view.id); + self.deferred_tree_actions.lock().focus_tab = Some(space_view.id); } item_ui::select_hovered_on_click(ctx, &response, &[item]); @@ -221,8 +276,10 @@ impl ViewportBlueprint<'_> { re_log::trace!("Space view visibility changed - will no longer auto-layout"); } - self.auto_layout = false; // Keep `auto_space_views` enabled. - self.tree.set_visible(tile_id, visible); + // Keep `auto_space_views` enabled. + Self::set_auto_layout(false, ctx); + + tree.set_visible(tile_id, visible); } } @@ -231,7 +288,7 @@ impl ViewportBlueprint<'_> { ui: &mut egui::Ui, query_result: &DataQueryResult, result_handle: DataResultHandle, - space_view: &mut SpaceViewBlueprint, + space_view: &SpaceViewBlueprint, space_view_visible: bool, ) { let Some(top_node) = query_result.tree.lookup_node(result_handle) else { @@ -391,7 +448,7 @@ impl ViewportBlueprint<'_> { } pub fn add_new_spaceview_button_ui( - &mut self, + &self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui, spaces_info: &SpaceInfoCollection, @@ -403,27 +460,26 @@ impl ViewportBlueprint<'_> { |ui| { ui.style_mut().wrap = Some(false); - let mut add_space_view_item = - |ui: &mut egui::Ui, space_view: SpaceViewBlueprint| { - if ctx - .re_ui - .selectable_label_with_icon( - ui, - space_view.class(ctx.space_view_class_registry).icon(), - if space_view.space_origin.is_root() { - space_view.display_name.clone() - } else { - space_view.space_origin.to_string() - }, - false, - ) - .clicked() - { - ui.close_menu(); - let new_space_view_id = self.add_space_view(space_view); - ctx.set_single_selection(&Item::SpaceView(new_space_view_id)); - } - }; + let add_space_view_item = |ui: &mut egui::Ui, space_view: SpaceViewBlueprint| { + if ctx + .re_ui + .selectable_label_with_icon( + ui, + space_view.class(ctx.space_view_class_registry).icon(), + if space_view.space_origin.is_root() { + space_view.display_name.clone() + } else { + space_view.space_origin.to_string() + }, + false, + ) + .clicked() + { + ui.close_menu(); + let new_space_view_id = self.add_space_view(space_view, ctx); + ctx.set_single_selection(&Item::SpaceView(new_space_view_id)); + } + }; // Space view options proposed by heuristics let mut possible_space_views = From 7f55c5cef0218db79f90e933cff2ef866b8b9d5a Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 14 Dec 2023 02:55:53 +0100 Subject: [PATCH 03/26] Fix multi-space-view additions --- crates/re_viewer/src/ui/selection_panel.rs | 4 +- crates/re_viewport/src/viewport.rs | 20 +++++++-- crates/re_viewport/src/viewport_blueprint.rs | 44 ++++++++++++++++++- .../re_viewport/src/viewport_blueprint_ui.rs | 11 +++-- 4 files changed, 69 insertions(+), 10 deletions(-) diff --git a/crates/re_viewer/src/ui/selection_panel.rs b/crates/re_viewer/src/ui/selection_panel.rs index a3e9ecd6b594..5b4ed7529248 100644 --- a/crates/re_viewer/src/ui/selection_panel.rs +++ b/crates/re_viewer/src/ui/selection_panel.rs @@ -447,7 +447,9 @@ fn container_top_level_properties( }); // TODO(jleibs): fix container-editing - re_log::warn!("TODO(jleibs): Fix container editing"); + if container_kind != container.kind() { + re_log::warn!("TODO(jleibs): Fix container editing"); + } //container.set_kind(container_kind); ui.end_row(); diff --git a/crates/re_viewport/src/viewport.rs b/crates/re_viewport/src/viewport.rs index fb5daec05b1e..1210cad01ec5 100644 --- a/crates/re_viewport/src/viewport.rs +++ b/crates/re_viewport/src/viewport.rs @@ -214,20 +214,32 @@ impl<'a> Viewport<'a> { } if self.blueprint.auto_space_views { + let mut new_space_views = vec![]; for space_view_candidate in default_created_space_views(ctx, spaces_info, ctx.entities_per_system_per_class) { - if self.should_auto_add_space_view(&space_view_candidate) { - self.blueprint.add_space_view(space_view_candidate, ctx); + if self.should_auto_add_space_view(&new_space_views, &space_view_candidate) { + new_space_views.push(space_view_candidate); } } + self.blueprint + .add_multi_space_view(new_space_views.into_iter(), ctx); } } - fn should_auto_add_space_view(&self, space_view_candidate: &SpaceViewBlueprint) -> bool { + fn should_auto_add_space_view( + &self, + already_added: &[SpaceViewBlueprint], + space_view_candidate: &SpaceViewBlueprint, + ) -> bool { re_tracing::profile_function!(); - for existing_view in self.blueprint.space_views.values() { + for existing_view in self + .blueprint + .space_views + .values() + .chain(already_added.iter()) + { if existing_view.space_origin == space_view_candidate.space_origin { if existing_view.entities_determined_by_user { // Since the user edited a space view with the same space path, we can't be sure our new one isn't redundant. diff --git a/crates/re_viewport/src/viewport_blueprint.rs b/crates/re_viewport/src/viewport_blueprint.rs index 59f80f24c566..5cbef21012b7 100644 --- a/crates/re_viewport/src/viewport_blueprint.rs +++ b/crates/re_viewport/src/viewport_blueprint.rs @@ -25,7 +25,7 @@ use crate::{ #[derive(Clone, Default)] pub(crate) struct TreeActions { pub reset: bool, - pub create: Option, + pub create: Vec, pub focus_tab: Option, pub remove: Vec, } @@ -205,11 +205,51 @@ impl ViewportBlueprint { ); save_single_component(&VIEWPORT_PATH.into(), component, ctx); - self.deferred_tree_actions.lock().create = Some(space_view_id); + self.deferred_tree_actions.lock().create.push(space_view_id); space_view_id } + pub fn add_multi_space_view( + &self, + space_views: impl Iterator, + ctx: &ViewerContext<'_>, + ) { + let mut new_ids: Vec<_> = self.space_views.keys().cloned().collect(); + + for mut space_view in space_views { + let space_view_id = space_view.id; + + // Find a unique name for the space view + let mut candidate_name = space_view.display_name.clone(); + let mut append_count = 1; + let unique_name = 'outer: loop { + for view in &self.space_views { + if candidate_name == view.1.display_name { + append_count += 1; + candidate_name = format!("{} ({})", space_view.display_name, append_count); + + continue 'outer; + } + } + break candidate_name; + }; + + space_view.display_name = unique_name; + + // Save the space view to the store + space_view.save_full(ctx); + new_ids.push(space_view_id); + + // Update the space-view ids: + + self.deferred_tree_actions.lock().create.push(space_view_id); + } + + let component = IncludedSpaceViews(new_ids.into_iter().map(|id| id.into()).collect()); + save_single_component(&VIEWPORT_PATH.into(), component, ctx); + } + #[allow(clippy::unused_self)] pub fn space_views_containing_entity_path( &self, diff --git a/crates/re_viewport/src/viewport_blueprint_ui.rs b/crates/re_viewport/src/viewport_blueprint_ui.rs index dad9aadfff05..b58c8ce8ab46 100644 --- a/crates/re_viewport/src/viewport_blueprint_ui.rs +++ b/crates/re_viewport/src/viewport_blueprint_ui.rs @@ -44,7 +44,7 @@ impl ViewportBlueprint { let mut focus_tab = std::mem::take(&mut deferred.focus_tab); let remove = std::mem::take(&mut deferred.remove); - if let Some(create) = &create { + for space_view in &create { if self.auto_layout { // Re-run the auto-layout next frame: re_log::trace!( @@ -53,7 +53,7 @@ impl ViewportBlueprint { reset = true; } else if let Some(root_id) = tree.root { - let tile_id = tree.tiles.insert_pane(*create); + let tile_id = tree.tiles.insert_pane(*space_view); if let Some(egui_tiles::Tile::Container(container)) = tree.tiles.get_mut(root_id) { re_log::trace!("Inserting new space view into root container"); container.add_child(tile_id); @@ -65,7 +65,7 @@ impl ViewportBlueprint { re_log::trace!("No root found - will re-run auto-layout"); } - focus_tab = Some(*create); + focus_tab = Some(*space_view); } if let Some(focus_tab) = &focus_tab { @@ -89,6 +89,11 @@ impl ViewportBlueprint { } } + if tree.is_empty() && !self.space_views.is_empty() { + re_log::trace!("Tree is empty - will re-run auto-layout"); + reset = true; + } + if reset { re_log::trace!("Resetting viewport tree"); tree = super::auto_layout::tree_from_space_views( From 0be8bea2690e085e65bcc368b5721b37b2ba6b72 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 14 Dec 2023 18:13:29 +0100 Subject: [PATCH 04/26] Load blueprint separately and only hold immutable ref --- crates/re_viewer/src/app_state.rs | 21 ++- crates/re_viewer/src/ui/selection_panel.rs | 33 ++-- crates/re_viewport/src/viewport.rs | 39 ++-- crates/re_viewport/src/viewport_blueprint.rs | 182 +++++++++---------- 4 files changed, 135 insertions(+), 140 deletions(-) diff --git a/crates/re_viewer/src/app_state.rs b/crates/re_viewer/src/app_state.rs index 968db6425d8a..c262e37034dc 100644 --- a/crates/re_viewer/src/app_state.rs +++ b/crates/re_viewer/src/app_state.rs @@ -9,7 +9,8 @@ use re_viewer_context::{ SelectionState, SpaceViewClassRegistry, StoreContext, SystemCommandSender as _, ViewerContext, }; use re_viewport::{ - identify_entities_per_system_per_class, SpaceInfoCollection, Viewport, ViewportState, + identify_entities_per_system_per_class, SpaceInfoCollection, Viewport, ViewportBlueprint, + ViewportState, }; use crate::ui::recordings_panel_ui; @@ -104,7 +105,8 @@ impl AppState { viewport_state, } = self; - let viewport = Viewport::from_db(store_context.blueprint, viewport_state); + let viewport_blueprint = ViewportBlueprint::try_from_db(store_context.blueprint); + let mut viewport = Viewport::new(&viewport_blueprint, viewport_state); // If the blueprint is invalid, reset it. if viewport.blueprint.is_invalid() { @@ -141,8 +143,7 @@ impl AppState { .values() .flat_map(|space_view| { space_view.queries.iter().map(|query| { - let state = viewport.state.read(); - let props = state.space_view_props(space_view.id); + let props = viewport.state.space_view_props(space_view.id); let resolver = query.build_resolver(space_view.id, props); ( query.id, @@ -190,8 +191,7 @@ impl AppState { .values() .flat_map(|space_view| { space_view.queries.iter().map(|query| { - let state = viewport.state.read(); - let props = state.space_view_props(space_view.id); + let props = viewport.state.space_view_props(space_view.id); let resolver = query.build_resolver(space_view.id, props); ( query.id, @@ -208,7 +208,12 @@ impl AppState { ctx.query_results = &updated_query_results; time_panel.show_panel(&ctx, ui, app_blueprint.time_panel_expanded); - selection_panel.show_panel(&ctx, ui, &viewport, app_blueprint.selection_panel_expanded); + selection_panel.show_panel( + &ctx, + ui, + &mut viewport, + app_blueprint.selection_panel_expanded, + ); let central_panel_frame = egui::Frame { fill: ui.style().visuals.panel_fill, @@ -247,7 +252,7 @@ impl AppState { ui.add_space(4.0); } - blueprint_panel_ui(&viewport.blueprint, &ctx, ui, &spaces_info); + blueprint_panel_ui(viewport.blueprint, &ctx, ui, &spaces_info); }, ); diff --git a/crates/re_viewer/src/ui/selection_panel.rs b/crates/re_viewer/src/ui/selection_panel.rs index 5b4ed7529248..58426f49fa94 100644 --- a/crates/re_viewer/src/ui/selection_panel.rs +++ b/crates/re_viewer/src/ui/selection_panel.rs @@ -39,7 +39,7 @@ impl SelectionPanel { &mut self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui, - viewport: &Viewport<'_>, + viewport: &mut Viewport<'_, '_>, expanded: bool, ) { let screen_width = ui.ctx().screen_rect().width(); @@ -72,7 +72,7 @@ impl SelectionPanel { if let Some(selection) = self.selection_state_ui.selection_ui( ctx.re_ui, ui, - &viewport.blueprint, + viewport.blueprint, &mut history, ) { ctx.selection_state() @@ -97,7 +97,12 @@ impl SelectionPanel { } #[allow(clippy::unused_self)] - fn contents(&mut self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui, viewport: &Viewport<'_>) { + fn contents( + &mut self, + ctx: &ViewerContext<'_>, + ui: &mut egui::Ui, + viewport: &mut Viewport<'_, '_>, + ) { re_tracing::profile_function!(); let query = ctx.current_query(); @@ -117,20 +122,15 @@ impl SelectionPanel { }; for (i, item) in selection.iter().enumerate() { ui.push_id(i, |ui| { - what_is_selected_ui(ui, ctx, &viewport.blueprint, item); + what_is_selected_ui(ui, ctx, viewport.blueprint, item); match item { Item::Container(tile_id) => { - container_top_level_properties(ui, ctx, &viewport.blueprint, tile_id); + container_top_level_properties(ui, ctx, viewport.blueprint, tile_id); } Item::SpaceView(space_view_id) => { - space_view_top_level_properties( - ui, - ctx, - &viewport.blueprint, - space_view_id, - ); + space_view_top_level_properties(ui, ctx, viewport.blueprint, space_view_id); } _ => {} @@ -512,7 +512,12 @@ fn has_blueprint_section(item: &Item) -> bool { } /// What is the blueprint stuff for this item? -fn blueprint_ui(ui: &mut egui::Ui, ctx: &ViewerContext<'_>, viewport: &Viewport<'_>, item: &Item) { +fn blueprint_ui( + ui: &mut egui::Ui, + ctx: &ViewerContext<'_>, + viewport: &mut Viewport<'_, '_>, + item: &Item, +) { match item { Item::SpaceView(space_view_id) => { ui.horizontal(|ui| { @@ -597,9 +602,7 @@ fn blueprint_ui(ui: &mut egui::Ui, ctx: &ViewerContext<'_>, viewport: &Viewport< if let Some(space_view) = viewport.blueprint.space_view(space_view_id) { let space_view_class = *space_view.class_identifier(); - let mut state = viewport.state.write(); - - let space_view_state = state.space_view_state_mut( + let space_view_state = viewport.state.space_view_state_mut( ctx.space_view_class_registry, space_view.id, space_view.class_identifier(), diff --git a/crates/re_viewport/src/viewport.rs b/crates/re_viewport/src/viewport.rs index 1210cad01ec5..6981e39867d3 100644 --- a/crates/re_viewport/src/viewport.rs +++ b/crates/re_viewport/src/viewport.rs @@ -8,7 +8,6 @@ use ahash::HashMap; use egui_tiles::Behavior as _; use once_cell::sync::Lazy; -use parking_lot::RwLock; use re_data_store::EntityPropertyMap; use re_data_ui::item_ui; @@ -22,8 +21,8 @@ use crate::{ space_view_entity_picker::SpaceViewEntityPicker, space_view_heuristics::default_created_space_views, space_view_highlights::highlights_for_space_view, - system_execution::execute_systems_for_space_views, viewport_blueprint::load_viewport_blueprint, - SpaceInfoCollection, SpaceViewBlueprint, ViewportBlueprint, + system_execution::execute_systems_for_space_views, SpaceInfoCollection, SpaceViewBlueprint, + ViewportBlueprint, }; // State for each `SpaceView` including both the auto properties and @@ -76,34 +75,26 @@ impl ViewportState { // ---------------------------------------------------------------------------- /// Defines the layout of the Viewport -pub struct Viewport<'a> { - // This is what me mutate during the frame. - pub blueprint: ViewportBlueprint, +pub struct Viewport<'a, 'b> { + pub blueprint: &'a ViewportBlueprint, - pub state: RwLock<&'a mut ViewportState>, + pub state: &'b mut ViewportState, } -impl<'a> Viewport<'a> { - pub fn from_db(blueprint_db: &re_data_store::StoreDb, state: &'a mut ViewportState) -> Self { +impl<'a, 'b> Viewport<'a, 'b> { + pub fn new(blueprint: &'a ViewportBlueprint, state: &'b mut ViewportState) -> Self { re_tracing::profile_function!(); - let blueprint = load_viewport_blueprint(blueprint_db); - - Self { - blueprint, - state: RwLock::new(state), - } + Self { blueprint, state } } - pub fn show_add_remove_entities_window(&self, space_view_id: SpaceViewId) { - self.state.write().space_view_entity_window = Some(SpaceViewEntityPicker { space_view_id }); + pub fn show_add_remove_entities_window(&mut self, space_view_id: SpaceViewId) { + self.state.space_view_entity_window = Some(SpaceViewEntityPicker { space_view_id }); } - pub fn viewport_ui(&self, ui: &mut egui::Ui, ctx: &'a ViewerContext<'_>) { + pub fn viewport_ui(&mut self, ui: &mut egui::Ui, ctx: &'a ViewerContext<'_>) { let Viewport { blueprint, state } = self; - let mut state = state.write(); - if let Some(window) = &mut state.space_view_entity_window { if let Some(space_view) = blueprint.space_views.get(&window.space_view_id) { if !window.ui(ctx, ui, space_view) { @@ -157,7 +148,7 @@ impl<'a> Viewport<'a> { re_tracing::profile_scope!("tree.ui"); let mut tab_viewer = TabViewer { - viewport_state: &mut state, + viewport_state: state, ctx, space_views: &blueprint.space_views, maximized: &mut maximized, @@ -195,16 +186,14 @@ impl<'a> Viewport<'a> { } } - pub fn on_frame_start(&self, ctx: &ViewerContext<'_>, spaces_info: &SpaceInfoCollection) { + pub fn on_frame_start(&mut self, ctx: &ViewerContext<'_>, spaces_info: &SpaceInfoCollection) { re_tracing::profile_function!(); - let mut state = self.state.write(); - for space_view in self.blueprint.space_views.values() { let PerSpaceViewState { auto_properties, space_view_state, - } = state.space_view_state_mut( + } = self.state.space_view_state_mut( ctx.space_view_class_registry, space_view.id, space_view.class_identifier(), diff --git a/crates/re_viewport/src/viewport_blueprint.rs b/crates/re_viewport/src/viewport_blueprint.rs index 5cbef21012b7..5ed75076b4bb 100644 --- a/crates/re_viewport/src/viewport_blueprint.rs +++ b/crates/re_viewport/src/viewport_blueprint.rs @@ -60,6 +60,96 @@ pub struct ViewportBlueprint { } impl ViewportBlueprint { + pub fn try_from_db(blueprint_db: &re_data_store::StoreDb) -> Self { + re_tracing::profile_function!(); + + let query = LatestAtQuery::latest(Timeline::default()); + + let arch = match query_archetype::( + blueprint_db.store(), + &query, + &VIEWPORT_PATH.into(), + ) + .and_then(|arch| arch.to_archetype()) + { + Ok(arch) => arch, + Err(re_query::QueryError::PrimaryNotFound(_)) => { + // Empty Store + Default::default() + } + Err(err) => { + if cfg!(debug_assertions) { + re_log::error!("Failed to load viewport blueprint: {err}."); + } else { + re_log::debug!("Failed to load viewport blueprint: {err}."); + } + Default::default() + } + }; + + let space_view_ids: Vec = + arch.space_views.0.iter().map(|id| (*id).into()).collect(); + + let space_views: BTreeMap = space_view_ids + .into_iter() + .filter_map(|space_view: SpaceViewId| { + SpaceViewBlueprint::try_from_db(&space_view.as_entity_path(), blueprint_db) + }) + .map(|sv| (sv.id, sv)) + .collect(); + + let auto_layout = arch.auto_layout.unwrap_or_default().0; + + let auto_space_views = arch.auto_space_views.map_or_else( + || { + // Only enable auto-space-views if this is the app-default blueprint + blueprint_db + .store_info() + .map_or(false, |ri| ri.is_app_default_blueprint()) + }, + |auto| auto.0, + ); + + let maximized = arch.maximized.and_then(|id| id.0.map(|id| id.into())); + + let tree = blueprint_db + .store() + .query_timeless_component_quiet::(&VIEWPORT_PATH.into()) + .map(|space_view| space_view.value) + .unwrap_or_default() + .0; + + ViewportBlueprint { + space_views, + tree, + maximized, + auto_layout, + auto_space_views, + deferred_tree_actions: Default::default(), + } + + // TODO(jleibs): Need to figure out if we have to re-enable support for + // auto-discovery of SpaceViews logged via the experimental blueprint APIs. + /* + let unknown_space_views: HashMap<_, _> = space_views + .iter() + .filter(|(k, _)| !viewport_layout.space_view_keys.contains(k)) + .map(|(k, v)| (*k, v.clone())) + .collect(); + */ + + // TODO(jleibs): It seems we shouldn't call this until later, after we've created + // the snapshot. Doing this here means we are mutating the state before it goes + // into the snapshot. For example, even if there's no visibility in the + // store, this will end up with default-visibility, which then *won't* be saved back. + // TODO(jleibs): what to do about auto-discovery? + /* + for (_, view) in unknown_space_views { + viewport.add_space_view(view); + } + */ + } + /// Determine whether all views in a blueprint are invalid. /// /// This most commonly happens due to a change in struct definition that @@ -339,98 +429,6 @@ where // ---------------------------------------------------------------------------- -pub fn load_viewport_blueprint(blueprint_db: &re_data_store::StoreDb) -> ViewportBlueprint { - re_tracing::profile_function!(); - - let query = LatestAtQuery::latest(Timeline::default()); - - let arch = match query_archetype::( - blueprint_db.store(), - &query, - &VIEWPORT_PATH.into(), - ) - .and_then(|arch| arch.to_archetype()) - { - Ok(arch) => arch, - Err(re_query::QueryError::PrimaryNotFound(_)) => { - // Empty Store - Default::default() - } - Err(err) => { - if cfg!(debug_assertions) { - re_log::error!("Failed to load viewport blueprint: {err}."); - } else { - re_log::debug!("Failed to load viewport blueprint: {err}."); - } - Default::default() - } - }; - - let space_view_ids: Vec = - arch.space_views.0.iter().map(|id| (*id).into()).collect(); - - let space_views: BTreeMap = space_view_ids - .into_iter() - .filter_map(|space_view: SpaceViewId| { - SpaceViewBlueprint::try_from_db(&space_view.as_entity_path(), blueprint_db) - }) - .map(|sv| (sv.id, sv)) - .collect(); - - let auto_layout = arch.auto_layout.unwrap_or_default().0; - - let auto_space_views = arch.auto_space_views.map_or_else( - || { - // Only enable auto-space-views if this is the app-default blueprint - blueprint_db - .store_info() - .map_or(false, |ri| ri.is_app_default_blueprint()) - }, - |auto| auto.0, - ); - - let maximized = arch.maximized.and_then(|id| id.0.map(|id| id.into())); - - let tree = blueprint_db - .store() - .query_timeless_component_quiet::(&VIEWPORT_PATH.into()) - .map(|space_view| space_view.value) - .unwrap_or_default() - .0; - - ViewportBlueprint { - space_views, - tree, - maximized, - auto_layout, - auto_space_views, - deferred_tree_actions: Default::default(), - } - - // TODO(jleibs): Need to figure out if we have to re-enable support for - // auto-discovery of SpaceViews logged via the experimental blueprint APIs. - /* - let unknown_space_views: HashMap<_, _> = space_views - .iter() - .filter(|(k, _)| !viewport_layout.space_view_keys.contains(k)) - .map(|(k, v)| (*k, v.clone())) - .collect(); - */ - - // TODO(jleibs): It seems we shouldn't call this until later, after we've created - // the snapshot. Doing this here means we are mutating the state before it goes - // into the snapshot. For example, even if there's no visibility in the - // store, this will end up with default-visibility, which then *won't* be saved back. - // TODO(jleibs): what to do about auto-discovery? - /* - for (_, view) in unknown_space_views { - viewport.add_space_view(view); - } - */ -} - -// ---------------------------------------------------------------------------- - pub fn clear_space_view(deltas: &mut Vec, space_view_id: &SpaceViewId) { // TODO(jleibs): Seq instead of timeless? let timepoint = TimePoint::timeless(); From 86f5c27fa27b3cf20c7e05beae054d3c19d95ed2 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 14 Dec 2023 19:53:54 +0100 Subject: [PATCH 05/26] Move tree / tree-actions into the Viewport --- Cargo.lock | 1 - .../archetypes/viewport_blueprint.fbs | 6 +- crates/re_viewer/src/app.rs | 1 + crates/re_viewer/src/app_state.rs | 2 +- crates/re_viewer/src/ui/blueprint_panel.rs | 8 +- crates/re_viewer/src/ui/selection_panel.rs | 2 +- crates/re_viewport/Cargo.toml | 1 - .../archetypes/viewport_blueprint.rs | 63 ++++---- crates/re_viewport/src/viewport.rs | 71 ++++++--- crates/re_viewport/src/viewport_blueprint.rs | 110 ++++--------- .../re_viewport/src/viewport_blueprint_ui.rs | 148 +++++++++--------- 11 files changed, 200 insertions(+), 213 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3cff1a46550e..e5c320456326 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5267,7 +5267,6 @@ dependencies = [ "itertools 0.11.0", "nohash-hasher", "once_cell", - "parking_lot 0.12.1", "rayon", "re_arrow_store", "re_data_store", diff --git a/crates/re_types/definitions/rerun/blueprint/archetypes/viewport_blueprint.fbs b/crates/re_types/definitions/rerun/blueprint/archetypes/viewport_blueprint.fbs index ae3f10714f4a..00bc5911d179 100644 --- a/crates/re_types/definitions/rerun/blueprint/archetypes/viewport_blueprint.fbs +++ b/crates/re_types/definitions/rerun/blueprint/archetypes/viewport_blueprint.fbs @@ -19,11 +19,11 @@ table ViewportBlueprint ( /// All of the space-views that belong to the viewport. space_views: rerun.blueprint.components.IncludedSpaceViews ("attr.rerun.component_required", order: 1000); - /// The layout of the space-views - layout: rerun.blueprint.components.ViewportLayout ("attr.rerun.component_required", order: 2000); - // --- Optional --- + /// The layout of the space-views + layout: rerun.blueprint.components.ViewportLayout ("attr.rerun.component_optional", nullable, order: 2000); + /// Show one tab as maximized? maximized: rerun.blueprint.components.SpaceViewMaximized ("attr.rerun.component_optional", nullable, order: 3000); diff --git a/crates/re_viewer/src/app.rs b/crates/re_viewer/src/app.rs index b8a9854a5af3..e52329902981 100644 --- a/crates/re_viewer/src/app.rs +++ b/crates/re_viewer/src/app.rs @@ -368,6 +368,7 @@ impl App { SystemCommand::ResetBlueprint => { // By clearing the blueprint it will be re-populated with the defaults // at the beginning of the next frame. + re_log::debug!("Reset blueprint"); store_hub.clear_blueprint(); } SystemCommand::UpdateBlueprint(blueprint_id, updates) => { diff --git a/crates/re_viewer/src/app_state.rs b/crates/re_viewer/src/app_state.rs index c262e37034dc..728a624771f7 100644 --- a/crates/re_viewer/src/app_state.rs +++ b/crates/re_viewer/src/app_state.rs @@ -252,7 +252,7 @@ impl AppState { ui.add_space(4.0); } - blueprint_panel_ui(viewport.blueprint, &ctx, ui, &spaces_info); + blueprint_panel_ui(&mut viewport, &ctx, ui, &spaces_info); }, ); diff --git a/crates/re_viewer/src/ui/blueprint_panel.rs b/crates/re_viewer/src/ui/blueprint_panel.rs index f946d3a0e32c..ab893237f8b0 100644 --- a/crates/re_viewer/src/ui/blueprint_panel.rs +++ b/crates/re_viewer/src/ui/blueprint_panel.rs @@ -1,9 +1,9 @@ use re_viewer_context::{SystemCommandSender as _, ViewerContext}; -use re_viewport::{SpaceInfoCollection, ViewportBlueprint}; +use re_viewport::{SpaceInfoCollection, Viewport}; /// Show the Blueprint section of the left panel based on the current [`ViewportBlueprint`] pub fn blueprint_panel_ui( - blueprint: &ViewportBlueprint, + viewport: &mut Viewport<'_, '_>, ctx: &ViewerContext<'_>, ui: &mut egui::Ui, spaces_info: &SpaceInfoCollection, @@ -14,7 +14,7 @@ pub fn blueprint_panel_ui( "Blueprint", Some("The Blueprint is where you can configure the Rerun Viewer"), |ui| { - blueprint.add_new_spaceview_button_ui(ctx, ui, spaces_info); + viewport.add_new_spaceview_button_ui(ctx, ui, spaces_info); reset_blueprint_button_ui(ctx, ui); }, ); @@ -22,7 +22,7 @@ pub fn blueprint_panel_ui( // This call is excluded from `panel_content` because it has a ScrollArea, which should not be // inset. Instead, it calls panel_content itself inside the ScrollArea. - blueprint.tree_ui(ctx, ui); + viewport.tree_ui(ctx, ui); } fn reset_blueprint_button_ui(ctx: &ViewerContext<'_>, ui: &mut egui::Ui) { diff --git a/crates/re_viewer/src/ui/selection_panel.rs b/crates/re_viewer/src/ui/selection_panel.rs index 58426f49fa94..aad511fcdb3f 100644 --- a/crates/re_viewer/src/ui/selection_panel.rs +++ b/crates/re_viewer/src/ui/selection_panel.rs @@ -538,7 +538,7 @@ fn blueprint_ui( if let Some(space_view) = viewport.blueprint.space_view(space_view_id) { let mut new_space_view = space_view.clone(); new_space_view.id = SpaceViewId::random(); - viewport.blueprint.add_space_view(new_space_view, ctx); + viewport.blueprint.add_space_views(std::iter::once(new_space_view), ctx, &mut viewport.deferred_tree_actions); viewport.blueprint.mark_user_interaction(ctx); } } diff --git a/crates/re_viewport/Cargo.toml b/crates/re_viewport/Cargo.toml index d873246e2216..53c5385a7e98 100644 --- a/crates/re_viewport/Cargo.toml +++ b/crates/re_viewport/Cargo.toml @@ -45,7 +45,6 @@ image = { workspace = true, default-features = false, features = ["png"] } itertools.workspace = true nohash-hasher.workspace = true once_cell.workspace = true -parking_lot.workspace = true rayon.workspace = true rmp-serde.workspace = true tinyvec.workspace = true diff --git a/crates/re_viewport/src/blueprint/archetypes/viewport_blueprint.rs b/crates/re_viewport/src/blueprint/archetypes/viewport_blueprint.rs index 685539af5f5b..b43e2a73404b 100644 --- a/crates/re_viewport/src/blueprint/archetypes/viewport_blueprint.rs +++ b/crates/re_viewport/src/blueprint/archetypes/viewport_blueprint.rs @@ -28,7 +28,7 @@ pub struct ViewportBlueprint { pub space_views: crate::blueprint::components::IncludedSpaceViews, /// The layout of the space-views - pub layout: crate::blueprint::components::ViewportLayout, + pub layout: Option, /// Show one tab as maximized? pub maximized: Option, @@ -42,23 +42,19 @@ pub struct ViewportBlueprint { pub auto_space_views: Option, } -static REQUIRED_COMPONENTS: once_cell::sync::Lazy<[ComponentName; 2usize]> = - once_cell::sync::Lazy::new(|| { - [ - "rerun.blueprint.components.IncludedSpaceViews".into(), - "rerun.blueprint.components.ViewportLayout".into(), - ] - }); +static REQUIRED_COMPONENTS: once_cell::sync::Lazy<[ComponentName; 1usize]> = + once_cell::sync::Lazy::new(|| ["rerun.blueprint.components.IncludedSpaceViews".into()]); static RECOMMENDED_COMPONENTS: once_cell::sync::Lazy<[ComponentName; 1usize]> = once_cell::sync::Lazy::new(|| ["rerun.blueprint.components.ViewportBlueprintIndicator".into()]); -static OPTIONAL_COMPONENTS: once_cell::sync::Lazy<[ComponentName; 4usize]> = +static OPTIONAL_COMPONENTS: once_cell::sync::Lazy<[ComponentName; 5usize]> = once_cell::sync::Lazy::new(|| { [ "rerun.blueprint.components.AutoLayout".into(), "rerun.blueprint.components.AutoSpaceViews".into(), "rerun.blueprint.components.SpaceViewMaximized".into(), + "rerun.blueprint.components.ViewportLayout".into(), "rerun.components.InstanceKey".into(), ] }); @@ -67,11 +63,11 @@ static ALL_COMPONENTS: once_cell::sync::Lazy<[ComponentName; 7usize]> = once_cell::sync::Lazy::new(|| { [ "rerun.blueprint.components.IncludedSpaceViews".into(), - "rerun.blueprint.components.ViewportLayout".into(), "rerun.blueprint.components.ViewportBlueprintIndicator".into(), "rerun.blueprint.components.AutoLayout".into(), "rerun.blueprint.components.AutoSpaceViews".into(), "rerun.blueprint.components.SpaceViewMaximized".into(), + "rerun.blueprint.components.ViewportLayout".into(), "rerun.components.InstanceKey".into(), ] }); @@ -140,19 +136,20 @@ impl ::re_types_core::Archetype for ViewportBlueprint { .ok_or_else(DeserializationError::missing_data) .with_context("rerun.blueprint.archetypes.ViewportBlueprint#space_views")? }; - let layout = { - let array = arrays_by_name - .get("rerun.blueprint.components.ViewportLayout") - .ok_or_else(DeserializationError::missing_data) - .with_context("rerun.blueprint.archetypes.ViewportBlueprint#layout")?; - ::from_arrow_opt(&**array) - .with_context("rerun.blueprint.archetypes.ViewportBlueprint#layout")? - .into_iter() - .next() - .flatten() - .ok_or_else(DeserializationError::missing_data) - .with_context("rerun.blueprint.archetypes.ViewportBlueprint#layout")? - }; + let layout = + if let Some(array) = arrays_by_name.get("rerun.blueprint.components.ViewportLayout") { + Some({ + ::from_arrow_opt(&**array) + .with_context("rerun.blueprint.archetypes.ViewportBlueprint#layout")? + .into_iter() + .next() + .flatten() + .ok_or_else(DeserializationError::missing_data) + .with_context("rerun.blueprint.archetypes.ViewportBlueprint#layout")? + }) + } else { + None + }; let maximized = if let Some(array) = arrays_by_name.get("rerun.blueprint.components.SpaceViewMaximized") { @@ -214,7 +211,9 @@ impl ::re_types_core::AsComponents for ViewportBlueprint { [ Some(Self::indicator()), Some((&self.space_views as &dyn ComponentBatch).into()), - Some((&self.layout as &dyn ComponentBatch).into()), + self.layout + .as_ref() + .map(|comp| (comp as &dyn ComponentBatch).into()), self.maximized .as_ref() .map(|comp| (comp as &dyn ComponentBatch).into()), @@ -237,19 +236,25 @@ impl ::re_types_core::AsComponents for ViewportBlueprint { } impl ViewportBlueprint { - pub fn new( - space_views: impl Into, - layout: impl Into, - ) -> Self { + pub fn new(space_views: impl Into) -> Self { Self { space_views: space_views.into(), - layout: layout.into(), + layout: None, maximized: None, auto_layout: None, auto_space_views: None, } } + #[inline] + pub fn with_layout( + mut self, + layout: impl Into, + ) -> Self { + self.layout = Some(layout.into()); + self + } + #[inline] pub fn with_maximized( mut self, diff --git a/crates/re_viewport/src/viewport.rs b/crates/re_viewport/src/viewport.rs index 6981e39867d3..432f3c7d2cb0 100644 --- a/crates/re_viewport/src/viewport.rs +++ b/crates/re_viewport/src/viewport.rs @@ -72,20 +72,51 @@ impl ViewportState { } } +// We delay any modifications to the tree until the end of the frame, +// so that we don't iterate over something while modifying it. +#[derive(Clone, Default)] +pub struct TreeActions { + pub reset: bool, + pub create: Vec, + pub focus_tab: Option, + pub remove: Vec, +} + // ---------------------------------------------------------------------------- /// Defines the layout of the Viewport pub struct Viewport<'a, 'b> { + /// The blueprint that drives this viewport. This is the source of truth from the store + /// for this frame. pub blueprint: &'a ViewportBlueprint, + /// The persistent state of the viewport that is not saved to the store but otherwises + /// persis frame-to-frame. pub state: &'b mut ViewportState, + + /// The [`egui_tiles::Tree`] tree that actually manages blueprint layout. This tree needs + /// to be mutable for things like drag-and-drop and is ultimately saved back to the store. + /// at the end of the frame if edited. + pub tree: egui_tiles::Tree, + + /// Actions to perform at the end of the frame. + /// + /// We delay any modifications to the tree until the end of the frame, + /// so that we don't mutate something while inspecitng it. + //TODO(jleibs): Can we use the SystemCommandSender for this, too? + pub deferred_tree_actions: TreeActions, } impl<'a, 'b> Viewport<'a, 'b> { pub fn new(blueprint: &'a ViewportBlueprint, state: &'b mut ViewportState) -> Self { re_tracing::profile_function!(); - Self { blueprint, state } + Self { + blueprint, + state, + tree: blueprint.tree.clone(), + deferred_tree_actions: Default::default(), + } } pub fn show_add_remove_entities_window(&mut self, space_view_id: SpaceViewId) { @@ -93,7 +124,9 @@ impl<'a, 'b> Viewport<'a, 'b> { } pub fn viewport_ui(&mut self, ui: &mut egui::Ui, ctx: &'a ViewerContext<'_>) { - let Viewport { blueprint, state } = self; + let Viewport { + blueprint, state, .. + } = self; if let Some(window) = &mut state.space_view_entity_window { if let Some(space_view) = blueprint.space_views.get(&window.space_view_id) { @@ -123,19 +156,19 @@ impl<'a, 'b> Viewport<'a, 'b> { } } + let mut maximized_tree; + // TODO(jleibs): This tree won't have the edits from `viewport_blueprint_ui`. // Maybe we should route that all the way through to here and only save it once. - let mut tree = if let Some(space_view_id) = blueprint.maximized { + let tree = if let Some(space_view_id) = blueprint.maximized { let mut tiles = egui_tiles::Tiles::default(); let root = tiles.insert_pane(space_view_id); - egui_tiles::Tree::new("viewport_tree", root, tiles) + maximized_tree = egui_tiles::Tree::new("viewport_tree", root, tiles); + &mut maximized_tree } else { - blueprint.tree.clone() + &mut self.tree }; - // Snapshot the tree so we can save it if it changes - let tree_snapshot = tree.clone(); - let executed_systems_per_space_view = execute_systems_for_space_views( ctx, std::mem::take(&mut state.space_views_displayed_last_frame), @@ -169,21 +202,19 @@ impl<'a, 'b> Viewport<'a, 'b> { ); } - ViewportBlueprint::set_auto_layout(false, ctx); + blueprint.set_auto_layout(false, ctx); } state.space_views_displayed_last_frame = tab_viewer.space_views_displayed_current_frame; }); - // TODO(jleibs): These edits happen independently of those made by `viewport_blueprint_ui`. - // If both edit there will be a conflict and some will get missed. - if tree != tree_snapshot && blueprint.maximized.is_none() { - ViewportBlueprint::set_tree(tree, ctx); - } - if maximized != blueprint.maximized { - ViewportBlueprint::set_maximized(maximized, ctx); + self.blueprint.set_maximized(maximized, ctx); } + + // Finally, save any edits to the blueprint tree + // This is a no-op if the tree hasn't changed. + self.blueprint.set_tree(&self.tree, ctx); } pub fn on_frame_start(&mut self, ctx: &ViewerContext<'_>, spaces_info: &SpaceInfoCollection) { @@ -211,8 +242,12 @@ impl<'a, 'b> Viewport<'a, 'b> { new_space_views.push(space_view_candidate); } } - self.blueprint - .add_multi_space_view(new_space_views.into_iter(), ctx); + + self.blueprint.add_space_views( + new_space_views.into_iter(), + ctx, + &mut self.deferred_tree_actions, + ); } } diff --git a/crates/re_viewport/src/viewport_blueprint.rs b/crates/re_viewport/src/viewport_blueprint.rs index 5ed75076b4bb..2cd1ed275c1c 100644 --- a/crates/re_viewport/src/viewport_blueprint.rs +++ b/crates/re_viewport/src/viewport_blueprint.rs @@ -1,6 +1,5 @@ use std::collections::BTreeMap; -use parking_lot::Mutex; use re_arrow_store::LatestAtQuery; use re_data_store::EntityPath; use re_log_types::{DataRow, RowId, TimePoint, Timeline}; @@ -15,21 +14,12 @@ use crate::{ AutoLayout, AutoSpaceViews, IncludedSpaceViews, SpaceViewMaximized, ViewportLayout, }, space_view::SpaceViewBlueprint, + viewport::TreeActions, VIEWPORT_PATH, }; // ---------------------------------------------------------------------------- -// We delay any modifications to the tree until the end of the frame, -// so that we don't iterate over something while modifying it. -#[derive(Clone, Default)] -pub(crate) struct TreeActions { - pub reset: bool, - pub create: Vec, - pub focus_tab: Option, - pub remove: Vec, -} - /// Describes the layout and contents of the Viewport Panel. pub struct ViewportBlueprint { /// Where the space views are stored. @@ -50,13 +40,6 @@ pub struct ViewportBlueprint { /// Whether or not space views should be created automatically. pub auto_space_views: bool, - - /// Actions to perform at the end of the frame. - /// - /// We delay any modifications to the tree until the end of the frame, - /// so that we don't mutate something while inspecitng it. - //TODO(jleibs): Can we use the SystemCommandSender for this, too? - pub(crate) deferred_tree_actions: Mutex, } impl ViewportBlueprint { @@ -125,7 +108,6 @@ impl ViewportBlueprint { maximized, auto_layout, auto_space_views, - deferred_tree_actions: Default::default(), } // TODO(jleibs): Need to figure out if we have to re-enable support for @@ -258,54 +240,16 @@ impl ViewportBlueprint { save_single_component(&VIEWPORT_PATH.into(), component, ctx); } - pub fn add_space_view( - &self, - mut space_view: SpaceViewBlueprint, - ctx: &ViewerContext<'_>, - ) -> SpaceViewId { - let space_view_id = space_view.id; - - // Find a unique name for the space view - let mut candidate_name = space_view.display_name.clone(); - let mut append_count = 1; - let unique_name = 'outer: loop { - for view in &self.space_views { - if candidate_name == view.1.display_name { - append_count += 1; - candidate_name = format!("{} ({})", space_view.display_name, append_count); - - continue 'outer; - } - } - break candidate_name; - }; - - space_view.display_name = unique_name; - - // Save the space view to the store - space_view.save_full(ctx); - - // Update the space-view ids: - let component = IncludedSpaceViews( - self.space_views - .keys() - .map(|id| (*id).into()) - .chain(std::iter::once(space_view_id.into())) - .collect(), - ); - save_single_component(&VIEWPORT_PATH.into(), component, ctx); - - self.deferred_tree_actions.lock().create.push(space_view_id); - - space_view_id - } - - pub fn add_multi_space_view( + /// Add a set of space views to the viewport. + /// NOTE: Calling this more than once per frame will result in lost data. + // TODO(jleibs): Better safety check here. + pub fn add_space_views( &self, space_views: impl Iterator, ctx: &ViewerContext<'_>, + tree_actions: &mut TreeActions, ) { - let mut new_ids: Vec<_> = self.space_views.keys().cloned().collect(); + let mut new_ids: Vec<_> = vec![]; for mut space_view in space_views { let space_view_id = space_view.id; @@ -329,15 +273,21 @@ impl ViewportBlueprint { // Save the space view to the store space_view.save_full(ctx); - new_ids.push(space_view_id); // Update the space-view ids: - - self.deferred_tree_actions.lock().create.push(space_view_id); + new_ids.push(space_view_id); } - let component = IncludedSpaceViews(new_ids.into_iter().map(|id| id.into()).collect()); - save_single_component(&VIEWPORT_PATH.into(), component, ctx); + if !new_ids.is_empty() { + tree_actions.create.extend(new_ids.iter()); + + let updated_ids: Vec<_> = self.space_views.keys().chain(new_ids.iter()).collect(); + + let component = + IncludedSpaceViews(updated_ids.into_iter().map(|id| (*id).into()).collect()); + + save_single_component(&VIEWPORT_PATH.into(), component, ctx); + } } #[allow(clippy::unused_self)] @@ -363,19 +313,25 @@ impl ViewportBlueprint { .collect() } - pub fn set_auto_layout(value: bool, ctx: &ViewerContext<'_>) { - let component = AutoLayout(value); - save_single_component(&VIEWPORT_PATH.into(), component, ctx); + pub fn set_auto_layout(&self, value: bool, ctx: &ViewerContext<'_>) { + if self.auto_layout != value { + let component = AutoLayout(value); + save_single_component(&VIEWPORT_PATH.into(), component, ctx); + } } - pub fn set_maximized(space_view_id: Option, ctx: &ViewerContext<'_>) { - let component = SpaceViewMaximized(space_view_id.map(|id| id.into())); - save_single_component(&VIEWPORT_PATH.into(), component, ctx); + pub fn set_maximized(&self, space_view_id: Option, ctx: &ViewerContext<'_>) { + if self.maximized != space_view_id { + let component = SpaceViewMaximized(space_view_id.map(|id| id.into())); + save_single_component(&VIEWPORT_PATH.into(), component, ctx); + } } - pub fn set_tree(tree: egui_tiles::Tree, ctx: &ViewerContext<'_>) { - let component = ViewportLayout(tree); - save_single_component(&VIEWPORT_PATH.into(), component, ctx); + pub fn set_tree(&self, tree: &egui_tiles::Tree, ctx: &ViewerContext<'_>) { + if &self.tree != tree { + let component = ViewportLayout(tree.clone()); + save_single_component(&VIEWPORT_PATH.into(), component, ctx); + } } } diff --git a/crates/re_viewport/src/viewport_blueprint_ui.rs b/crates/re_viewport/src/viewport_blueprint_ui.rs index b58c8ce8ab46..6e72b50902e0 100644 --- a/crates/re_viewport/src/viewport_blueprint_ui.rs +++ b/crates/re_viewport/src/viewport_blueprint_ui.rs @@ -14,47 +14,45 @@ use re_viewer_context::{ use crate::{ space_view_heuristics::all_possible_space_views, SpaceInfoCollection, SpaceViewBlueprint, - ViewportBlueprint, + Viewport, }; -impl ViewportBlueprint { +impl Viewport<'_, '_> { /// Show the blueprint panel tree view. - pub fn tree_ui(&self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui) { + pub fn tree_ui(&mut self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui) { re_tracing::profile_function!(); - let mut tree = self.tree.clone(); - egui::ScrollArea::both() .id_source("blueprint_tree_scroll_area") .auto_shrink([true, false]) .show(ui, |ui| { ctx.re_ui.panel_content(ui, |_, ui| { if let Some(root) = self.tree.root() { - self.tile_ui(&mut tree, ctx, ui, root); + self.tile_ui(ctx, ui, root); } }); }); // At the end of the Tree-UI, we can safely apply deferred actions. - let mut deferred = self.deferred_tree_actions.lock(); - - let mut reset = std::mem::take(&mut deferred.reset); - let create = std::mem::take(&mut deferred.create); - let mut focus_tab = std::mem::take(&mut deferred.focus_tab); - let remove = std::mem::take(&mut deferred.remove); + let mut reset = std::mem::take(&mut self.deferred_tree_actions.reset); + let create = std::mem::take(&mut self.deferred_tree_actions.create); + let mut focus_tab = std::mem::take(&mut self.deferred_tree_actions.focus_tab); + let remove = std::mem::take(&mut self.deferred_tree_actions.remove); for space_view in &create { - if self.auto_layout { + if self.blueprint.auto_layout { // Re-run the auto-layout next frame: re_log::trace!( "Added a space view with no user edits yet - will re-run auto-layout" ); reset = true; - } else if let Some(root_id) = tree.root { - let tile_id = tree.tiles.insert_pane(*space_view); - if let Some(egui_tiles::Tile::Container(container)) = tree.tiles.get_mut(root_id) { + } else if let Some(root_id) = self.tree.root { + let tile_id = self.tree.tiles.insert_pane(*space_view); + if let Some(egui_tiles::Tile::Container(container)) = + self.tree.tiles.get_mut(root_id) + { re_log::trace!("Inserting new space view into root container"); container.add_child(tile_id); } else { @@ -69,7 +67,7 @@ impl ViewportBlueprint { } if let Some(focus_tab) = &focus_tab { - let found = tree.make_active(|tile| match tile { + let found = self.tree.make_active(|tile| match tile { egui_tiles::Tile::Pane(space_view_id) => space_view_id == focus_tab, egui_tiles::Tile::Container(_) => false, }); @@ -77,32 +75,29 @@ impl ViewportBlueprint { } for tile_id in remove { - for tile in tree.tiles.remove_recursively(tile_id) { + for tile in self.tree.tiles.remove_recursively(tile_id) { if let egui_tiles::Tile::Pane(space_view_id) = tile { - tree.tiles.remove(tile_id); - self.remove_space_view(&space_view_id, ctx); + self.tree.tiles.remove(tile_id); + self.blueprint.remove_space_view(&space_view_id, ctx); } } if Some(tile_id) == self.tree.root { - tree.root = None; + self.tree.root = None; } } - if tree.is_empty() && !self.space_views.is_empty() { + if self.tree.is_empty() && !self.blueprint.space_views.is_empty() { re_log::trace!("Tree is empty - will re-run auto-layout"); reset = true; } if reset { re_log::trace!("Resetting viewport tree"); - tree = super::auto_layout::tree_from_space_views( + self.tree = super::auto_layout::tree_from_space_views( ctx.space_view_class_registry, - &self.space_views, + &self.blueprint.space_views, ); - Self::set_tree(tree, ctx); - } else if tree != self.tree { - Self::set_tree(tree, ctx); } } @@ -112,34 +107,27 @@ impl ViewportBlueprint { 2 <= num_children && num_children <= 3 } - fn tile_ui( - &self, - tree: &mut egui_tiles::Tree, - ctx: &ViewerContext<'_>, - ui: &mut egui::Ui, - tile_id: egui_tiles::TileId, - ) { + fn tile_ui(&mut self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui, tile_id: egui_tiles::TileId) { // Temporarily remove the tile so we don't get borrow-checker fights: - let Some(mut tile) = tree.tiles.remove(tile_id) else { + let Some(mut tile) = self.tree.tiles.remove(tile_id) else { return; }; match &mut tile { egui_tiles::Tile::Container(container) => { - self.container_tree_ui(tree, ctx, ui, tile_id, container); + self.container_tree_ui(ctx, ui, tile_id, container); } egui_tiles::Tile::Pane(space_view_id) => { // A space view - self.space_view_entry_ui(tree, ctx, ui, tile_id, space_view_id); + self.space_view_entry_ui(ctx, ui, tile_id, space_view_id); } }; - tree.tiles.insert(tile_id, tile); + self.tree.tiles.insert(tile_id, tile); } fn container_tree_ui( - &self, - tree: &mut egui_tiles::Tree, + &mut self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui, tile_id: egui_tiles::TileId, @@ -150,8 +138,8 @@ impl ViewportBlueprint { // This means we won't be showing the visibility button of the parent container, // so if the child is made invisible, we should do the same for the parent. let child_is_visible = self.tree.is_visible(child_id); - tree.set_visible(tile_id, child_is_visible); - return self.tile_ui(tree, ctx, ui, child_id); + self.tree.set_visible(tile_id, child_is_visible); + return self.tile_ui(ctx, ui, child_id); } let item = Item::Container(tile_id); @@ -176,7 +164,7 @@ impl ViewportBlueprint { }) .show_collapsing(ui, ui.id().with(tile_id), default_open, |_, ui| { for &child in container.children() { - self.tile_ui(tree, ctx, ui, child); + self.tile_ui(ctx, ui, child); } }) .item_response; @@ -184,33 +172,32 @@ impl ViewportBlueprint { item_ui::select_hovered_on_click(ctx, &response, &[item]); if remove { - self.mark_user_interaction(ctx); - self.deferred_tree_actions.lock().remove.push(tile_id); + self.blueprint.mark_user_interaction(ctx); + self.deferred_tree_actions.remove.push(tile_id); } if visibility_changed { - if self.auto_layout { + if self.blueprint.auto_layout { re_log::trace!("Container visibility changed - will no longer auto-layout"); } // Keep `auto_space_views` enabled. - Self::set_auto_layout(false, ctx); + self.blueprint.set_auto_layout(false, ctx); - tree.set_visible(tile_id, visible); + self.tree.set_visible(tile_id, visible); } } fn space_view_entry_ui( - &self, - tree: &mut egui_tiles::Tree, + &mut self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui, tile_id: egui_tiles::TileId, space_view_id: &SpaceViewId, ) { - let Some(space_view) = self.space_views.get(space_view_id) else { + let Some(space_view) = self.blueprint.space_views.get(space_view_id) else { re_log::warn_once!("Bug: asked to show a ui for a Space View that doesn't exist"); - self.deferred_tree_actions.lock().remove.push(tile_id); + self.deferred_tree_actions.remove.push(tile_id); return; }; debug_assert_eq!(space_view.id, *space_view_id); @@ -245,7 +232,7 @@ impl ViewportBlueprint { let response = remove_button_ui(re_ui, ui, "Remove Space View from the Viewport"); if response.clicked() { - self.deferred_tree_actions.lock().remove.push(tile_id); + self.deferred_tree_actions.remove.push(tile_id); } response | vis_response @@ -271,20 +258,20 @@ impl ViewportBlueprint { .on_hover_text("Space View"); if response.clicked() { - self.deferred_tree_actions.lock().focus_tab = Some(space_view.id); + self.deferred_tree_actions.focus_tab = Some(space_view.id); } item_ui::select_hovered_on_click(ctx, &response, &[item]); if visibility_changed { - if self.auto_layout { + if self.blueprint.auto_layout { re_log::trace!("Space view visibility changed - will no longer auto-layout"); } // Keep `auto_space_views` enabled. - Self::set_auto_layout(false, ctx); + self.blueprint.set_auto_layout(false, ctx); - tree.set_visible(tile_id, visible); + self.tree.set_visible(tile_id, visible); } } @@ -453,7 +440,7 @@ impl ViewportBlueprint { } pub fn add_new_spaceview_button_ui( - &self, + &mut self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui, spaces_info: &SpaceInfoCollection, @@ -465,26 +452,31 @@ impl ViewportBlueprint { |ui| { ui.style_mut().wrap = Some(false); - let add_space_view_item = |ui: &mut egui::Ui, space_view: SpaceViewBlueprint| { - if ctx - .re_ui - .selectable_label_with_icon( - ui, - space_view.class(ctx.space_view_class_registry).icon(), - if space_view.space_origin.is_root() { - space_view.display_name.clone() - } else { - space_view.space_origin.to_string() - }, - false, - ) - .clicked() - { - ui.close_menu(); - let new_space_view_id = self.add_space_view(space_view, ctx); - ctx.set_single_selection(&Item::SpaceView(new_space_view_id)); - } - }; + let mut add_space_view_item = + |ui: &mut egui::Ui, space_view: SpaceViewBlueprint| { + if ctx + .re_ui + .selectable_label_with_icon( + ui, + space_view.class(ctx.space_view_class_registry).icon(), + if space_view.space_origin.is_root() { + space_view.display_name.clone() + } else { + space_view.space_origin.to_string() + }, + false, + ) + .clicked() + { + ui.close_menu(); + ctx.set_single_selection(&Item::SpaceView(space_view.id)); + self.blueprint.add_space_views( + std::iter::once(space_view), + ctx, + &mut self.deferred_tree_actions, + ); + } + }; // Space view options proposed by heuristics let mut possible_space_views = From f67ee39feb4dc979b987d1b55b6652d17eb47406 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 14 Dec 2023 19:54:33 +0100 Subject: [PATCH 06/26] cpp/py codegen --- .../blueprint/archetypes/viewport_blueprint.cpp | 4 ++-- .../blueprint/archetypes/viewport_blueprint.hpp | 16 ++++++++++------ .../blueprint/archetypes/viewport_blueprint.py | 9 +++++---- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/rerun_cpp/src/rerun/blueprint/archetypes/viewport_blueprint.cpp b/rerun_cpp/src/rerun/blueprint/archetypes/viewport_blueprint.cpp index db058f0dfecb..9a967352ce3f 100644 --- a/rerun_cpp/src/rerun/blueprint/archetypes/viewport_blueprint.cpp +++ b/rerun_cpp/src/rerun/blueprint/archetypes/viewport_blueprint.cpp @@ -21,8 +21,8 @@ namespace rerun { RR_RETURN_NOT_OK(result.error); cells.push_back(std::move(result.value)); } - { - auto result = DataCell::from_loggable(archetype.layout); + if (archetype.layout.has_value()) { + auto result = DataCell::from_loggable(archetype.layout.value()); RR_RETURN_NOT_OK(result.error); cells.push_back(std::move(result.value)); } diff --git a/rerun_cpp/src/rerun/blueprint/archetypes/viewport_blueprint.hpp b/rerun_cpp/src/rerun/blueprint/archetypes/viewport_blueprint.hpp index 6be8d85b29b2..10600134f7bf 100644 --- a/rerun_cpp/src/rerun/blueprint/archetypes/viewport_blueprint.hpp +++ b/rerun_cpp/src/rerun/blueprint/archetypes/viewport_blueprint.hpp @@ -26,7 +26,7 @@ namespace rerun::blueprint::archetypes { rerun::blueprint::components::IncludedSpaceViews space_views; /// The layout of the space-views - rerun::blueprint::components::ViewportLayout layout; + std::optional layout; /// Show one tab as maximized? std::optional maximized; @@ -50,11 +50,15 @@ namespace rerun::blueprint::archetypes { ViewportBlueprint() = default; ViewportBlueprint(ViewportBlueprint&& other) = default; - explicit ViewportBlueprint( - rerun::blueprint::components::IncludedSpaceViews _space_views, - rerun::blueprint::components::ViewportLayout _layout - ) - : space_views(std::move(_space_views)), layout(std::move(_layout)) {} + explicit ViewportBlueprint(rerun::blueprint::components::IncludedSpaceViews _space_views) + : space_views(std::move(_space_views)) {} + + /// The layout of the space-views + ViewportBlueprint with_layout(rerun::blueprint::components::ViewportLayout _layout) && { + layout = std::move(_layout); + // See: https://github.com/rerun-io/rerun/issues/4027 + RR_WITH_MAYBE_UNINITIALIZED_DISABLED(return std::move(*this);) + } /// Show one tab as maximized? ViewportBlueprint with_maximized(rerun::blueprint::components::SpaceViewMaximized _maximized diff --git a/rerun_py/rerun_sdk/rerun/blueprint/archetypes/viewport_blueprint.py b/rerun_py/rerun_sdk/rerun/blueprint/archetypes/viewport_blueprint.py index 82b519243b1e..09237654db12 100644 --- a/rerun_py/rerun_sdk/rerun/blueprint/archetypes/viewport_blueprint.py +++ b/rerun_py/rerun_sdk/rerun/blueprint/archetypes/viewport_blueprint.py @@ -24,8 +24,8 @@ class ViewportBlueprint(Archetype): def __init__( self: Any, space_views: components.IncludedSpaceViewsLike, - layout: components.ViewportLayoutLike, *, + layout: components.ViewportLayoutLike | None = None, maximized: datatypes.UuidLike | None = None, auto_layout: components.AutoLayoutLike | None = None, auto_space_views: components.AutoSpaceViewsLike | None = None, @@ -86,9 +86,10 @@ def _clear(cls) -> ViewportBlueprint: # # (Docstring intentionally commented out to hide this field from the docs) - layout: components.ViewportLayoutBatch = field( - metadata={"component": "required"}, - converter=components.ViewportLayoutBatch._required, # type: ignore[misc] + layout: components.ViewportLayoutBatch | None = field( + metadata={"component": "optional"}, + default=None, + converter=components.ViewportLayoutBatch._optional, # type: ignore[misc] ) # The layout of the space-views # From 08647738ac99dc95155f35864867a7ba282a5894 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 14 Dec 2023 20:01:55 +0100 Subject: [PATCH 07/26] Fix container editing now that we have a mutable tile-tree again --- crates/re_viewer/src/ui/selection_panel.rs | 24 +++++----------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/crates/re_viewer/src/ui/selection_panel.rs b/crates/re_viewer/src/ui/selection_panel.rs index aad511fcdb3f..2371a7e8fa7f 100644 --- a/crates/re_viewer/src/ui/selection_panel.rs +++ b/crates/re_viewer/src/ui/selection_panel.rs @@ -126,7 +126,7 @@ impl SelectionPanel { match item { Item::Container(tile_id) => { - container_top_level_properties(ui, ctx, viewport.blueprint, tile_id); + container_top_level_properties(ui, ctx, viewport, tile_id); } Item::SpaceView(space_view_id) => { @@ -407,11 +407,11 @@ fn space_view_top_level_properties( fn container_top_level_properties( ui: &mut egui::Ui, _ctx: &ViewerContext<'_>, - viewport: &ViewportBlueprint, + viewport: &mut Viewport<'_, '_>, tile_id: &egui_tiles::TileId, ) { // TODO(jleibs): fix container-editing - if let Some(Tile::Container(container)) = viewport.tree.tiles.get(*tile_id) { + if let Some(Tile::Container(container)) = viewport.tree.tiles.get_mut(*tile_id) { egui::Grid::new("container_top_level_properties") .num_columns(2) .show(ui, |ui| { @@ -446,11 +446,7 @@ fn container_top_level_properties( ); }); - // TODO(jleibs): fix container-editing - if container_kind != container.kind() { - re_log::warn!("TODO(jleibs): Fix container editing"); - } - //container.set_kind(container_kind); + container.set_kind(container_kind); ui.end_row(); @@ -470,30 +466,20 @@ fn container_top_level_properties( ui.style_mut().wrap = Some(false); ui.set_min_width(64.0); - // TODO(jleibs): Fix container-editing - ui.label(format!("Layout: {}", grid_layout_to_string(&grid.layout))); - /* ui.selectable_value( &mut grid.layout, GridLayout::Auto, grid_layout_to_string(&GridLayout::Auto), ); - */ + ui.separator(); for columns in 1..=grid.num_children() { - // TODO(jleibs): fix container-editing - ui.label(format!( - "Layout: {}", - grid_layout_to_string(&GridLayout::Columns(columns)) - )); - /* ui.selectable_value( &mut grid.layout, GridLayout::Columns(columns), grid_layout_to_string(&GridLayout::Columns(columns)), ); - */ } }); From 54bc7228a2968bab8c4f9085cb9b48275dc1783e Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 14 Dec 2023 20:09:04 +0100 Subject: [PATCH 08/26] No-op entities-detetermined-by-user --- crates/re_viewport/src/space_view.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/re_viewport/src/space_view.rs b/crates/re_viewport/src/space_view.rs index 9eb2e2e59143..9c9ea697aec9 100644 --- a/crates/re_viewport/src/space_view.rs +++ b/crates/re_viewport/src/space_view.rs @@ -196,8 +196,10 @@ impl SpaceViewBlueprint { } pub fn set_entity_determined_by_user(&self, ctx: &ViewerContext<'_>) { - let component = EntitiesDeterminedByUser(true); - save_single_component(&self.entity_path(), component, ctx); + if !self.entities_determined_by_user { + let component = EntitiesDeterminedByUser(true); + save_single_component(&self.entity_path(), component, ctx); + } } pub fn set_display_name(&self, name: String, ctx: &ViewerContext<'_>) { From b39fa0c61e4a9b9ec8995e7e7c386c3b785fb670 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 14 Dec 2023 20:14:13 +0100 Subject: [PATCH 09/26] Don't churn auto_layout / space_views on all interaction --- crates/re_viewport/src/viewport_blueprint.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/crates/re_viewport/src/viewport_blueprint.rs b/crates/re_viewport/src/viewport_blueprint.rs index 2cd1ed275c1c..8d7256e0f0ec 100644 --- a/crates/re_viewport/src/viewport_blueprint.rs +++ b/crates/re_viewport/src/viewport_blueprint.rs @@ -233,11 +233,8 @@ impl ViewportBlueprint { re_log::trace!("User edits - will no longer auto-layout"); } - let component = AutoLayout(false); - save_single_component(&VIEWPORT_PATH.into(), component, ctx); - - let component = AutoSpaceViews(false); - save_single_component(&VIEWPORT_PATH.into(), component, ctx); + self.set_auto_layout(false, ctx); + self.set_auto_space_views(false, ctx); } /// Add a set of space views to the viewport. @@ -320,6 +317,13 @@ impl ViewportBlueprint { } } + pub fn set_auto_space_views(&self, value: bool, ctx: &ViewerContext<'_>) { + if self.auto_layout != value { + let component = AutoSpaceViews(value); + save_single_component(&VIEWPORT_PATH.into(), component, ctx); + } + } + pub fn set_maximized(&self, space_view_id: Option, ctx: &ViewerContext<'_>) { if self.maximized != space_view_id { let component = SpaceViewMaximized(space_view_id.map(|id| id.into())); From 28d1a8f0dbcfd8ce584ec9667a6b107a02f8258e Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 14 Dec 2023 20:20:40 +0100 Subject: [PATCH 10/26] Has edits logic can go away --- crates/re_viewport/src/space_view.rs | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/crates/re_viewport/src/space_view.rs b/crates/re_viewport/src/space_view.rs index 9c9ea697aec9..a60f6c5a3e1c 100644 --- a/crates/re_viewport/src/space_view.rs +++ b/crates/re_viewport/src/space_view.rs @@ -1,4 +1,3 @@ -use ahash::HashSet; use re_arrow_store::LatestAtQuery; use re_data_store::{EntityPath, EntityProperties, StoreDb, TimeInt, VisibleHistory}; use re_data_store::{EntityPropertiesComponent, EntityPropertyMap}; @@ -41,28 +40,6 @@ pub struct SpaceViewBlueprint { pub entities_determined_by_user: bool, } -/// Determine whether this `SpaceViewBlueprint` has user-edits relative to another `SpaceViewBlueprint` -impl SpaceViewBlueprint { - pub fn has_edits(&self, other: &Self) -> bool { - let Self { - id, - display_name, - class_identifier, - space_origin, - queries, - entities_determined_by_user, - } = self; - - id != &other.id - || display_name != &other.display_name - || class_identifier != &other.class_identifier - || space_origin != &other.space_origin - || queries.iter().map(|q| q.id).collect::>() - != other.queries.iter().map(|q| q.id).collect::>() - || entities_determined_by_user != &other.entities_determined_by_user - } -} - impl SpaceViewBlueprint { pub fn new( space_view_class: SpaceViewClassIdentifier, From 6c757fafba7f06c392e296f70efdef9326e5e873 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 14 Dec 2023 21:43:27 +0100 Subject: [PATCH 11/26] Move the deferred tree action to a single place --- crates/re_viewer/src/app_state.rs | 3 + crates/re_viewport/src/viewport.rs | 80 ++++++++++++++++++- crates/re_viewport/src/viewport_blueprint.rs | 1 + .../re_viewport/src/viewport_blueprint_ui.rs | 67 ---------------- 4 files changed, 80 insertions(+), 71 deletions(-) diff --git a/crates/re_viewer/src/app_state.rs b/crates/re_viewer/src/app_state.rs index 728a624771f7..b2d7bb963954 100644 --- a/crates/re_viewer/src/app_state.rs +++ b/crates/re_viewer/src/app_state.rs @@ -275,6 +275,9 @@ impl AppState { }); }); + // Process deferred layout operationsand apply updates back to blueprint + viewport.update_and_sync_layout(&ctx); + { // We move the time at the very end of the frame, // so we have one frame to see the first data before we move the time. diff --git a/crates/re_viewport/src/viewport.rs b/crates/re_viewport/src/viewport.rs index 432f3c7d2cb0..1b4d592cf30b 100644 --- a/crates/re_viewport/src/viewport.rs +++ b/crates/re_viewport/src/viewport.rs @@ -211,10 +211,6 @@ impl<'a, 'b> Viewport<'a, 'b> { if maximized != blueprint.maximized { self.blueprint.set_maximized(maximized, ctx); } - - // Finally, save any edits to the blueprint tree - // This is a no-op if the tree hasn't changed. - self.blueprint.set_tree(&self.tree, ctx); } pub fn on_frame_start(&mut self, ctx: &ViewerContext<'_>, spaces_info: &SpaceInfoCollection) { @@ -285,6 +281,82 @@ impl<'a, 'b> Viewport<'a, 'b> { true } + /// Process any deferred `TreeActions` and then sync to store + pub fn update_and_sync_layout(&mut self, ctx: &ViewerContext<'_>) { + // At the end of the Tree-UI, we can safely apply deferred actions. + + let mut reset = std::mem::take(&mut self.deferred_tree_actions.reset); + let create = std::mem::take(&mut self.deferred_tree_actions.create); + let mut focus_tab = std::mem::take(&mut self.deferred_tree_actions.focus_tab); + let remove = std::mem::take(&mut self.deferred_tree_actions.remove); + + for space_view in &create { + if self.blueprint.auto_layout { + // Re-run the auto-layout next frame: + re_log::trace!( + "Added a space view with no user edits yet - will re-run auto-layout" + ); + + reset = true; + } else if let Some(root_id) = self.tree.root { + let tile_id = self.tree.tiles.insert_pane(*space_view); + if let Some(egui_tiles::Tile::Container(container)) = + self.tree.tiles.get_mut(root_id) + { + re_log::trace!("Inserting new space view into root container"); + container.add_child(tile_id); + } else { + re_log::trace!("Root was not a container - will re-run auto-layout"); + reset = true; + } + } else { + re_log::trace!("No root found - will re-run auto-layout"); + } + + focus_tab = Some(*space_view); + } + + if let Some(focus_tab) = &focus_tab { + let found = self.tree.make_active(|tile| match tile { + egui_tiles::Tile::Pane(space_view_id) => space_view_id == focus_tab, + egui_tiles::Tile::Container(_) => false, + }); + re_log::trace!("Found tab {focus_tab}: {found}"); + } + + for tile_id in remove { + for tile in self.tree.tiles.remove_recursively(tile_id) { + re_log::trace!("Removing tile {tile_id:?}"); + if let egui_tiles::Tile::Pane(space_view_id) = tile { + re_log::trace!("Removing space-view {space_view_id}"); + self.tree.tiles.remove(tile_id); + self.blueprint.remove_space_view(&space_view_id, ctx); + } + } + + if Some(tile_id) == self.tree.root { + self.tree.root = None; + } + } + + if self.tree.is_empty() && !self.blueprint.space_views.is_empty() { + re_log::trace!("Tree is empty - will re-run auto-layout"); + reset = true; + } + + if reset { + re_log::trace!("Resetting viewport tree"); + self.tree = super::auto_layout::tree_from_space_views( + ctx.space_view_class_registry, + &self.blueprint.space_views, + ); + } + + // Finally, save any edits to the blueprint tree + // This is a no-op if the tree hasn't changed. + self.blueprint.set_tree(&self.tree, ctx); + } + /// If `false`, the item is referring to data that is not present in this blueprint. #[inline] pub fn is_item_valid(&self, item: &Item) -> bool { diff --git a/crates/re_viewport/src/viewport_blueprint.rs b/crates/re_viewport/src/viewport_blueprint.rs index 8d7256e0f0ec..04756a3f1bea 100644 --- a/crates/re_viewport/src/viewport_blueprint.rs +++ b/crates/re_viewport/src/viewport_blueprint.rs @@ -333,6 +333,7 @@ impl ViewportBlueprint { pub fn set_tree(&self, tree: &egui_tiles::Tree, ctx: &ViewerContext<'_>) { if &self.tree != tree { + re_log::trace!("Updating the layout tree"); let component = ViewportLayout(tree.clone()); save_single_component(&VIEWPORT_PATH.into(), component, ctx); } diff --git a/crates/re_viewport/src/viewport_blueprint_ui.rs b/crates/re_viewport/src/viewport_blueprint_ui.rs index 6e72b50902e0..2290300faa50 100644 --- a/crates/re_viewport/src/viewport_blueprint_ui.rs +++ b/crates/re_viewport/src/viewport_blueprint_ui.rs @@ -32,73 +32,6 @@ impl Viewport<'_, '_> { } }); }); - - // At the end of the Tree-UI, we can safely apply deferred actions. - - let mut reset = std::mem::take(&mut self.deferred_tree_actions.reset); - let create = std::mem::take(&mut self.deferred_tree_actions.create); - let mut focus_tab = std::mem::take(&mut self.deferred_tree_actions.focus_tab); - let remove = std::mem::take(&mut self.deferred_tree_actions.remove); - - for space_view in &create { - if self.blueprint.auto_layout { - // Re-run the auto-layout next frame: - re_log::trace!( - "Added a space view with no user edits yet - will re-run auto-layout" - ); - - reset = true; - } else if let Some(root_id) = self.tree.root { - let tile_id = self.tree.tiles.insert_pane(*space_view); - if let Some(egui_tiles::Tile::Container(container)) = - self.tree.tiles.get_mut(root_id) - { - re_log::trace!("Inserting new space view into root container"); - container.add_child(tile_id); - } else { - re_log::trace!("Root was not a container - will re-run auto-layout"); - reset = true; - } - } else { - re_log::trace!("No root found - will re-run auto-layout"); - } - - focus_tab = Some(*space_view); - } - - if let Some(focus_tab) = &focus_tab { - let found = self.tree.make_active(|tile| match tile { - egui_tiles::Tile::Pane(space_view_id) => space_view_id == focus_tab, - egui_tiles::Tile::Container(_) => false, - }); - re_log::trace!("Found tab {focus_tab}: {found}"); - } - - for tile_id in remove { - for tile in self.tree.tiles.remove_recursively(tile_id) { - if let egui_tiles::Tile::Pane(space_view_id) = tile { - self.tree.tiles.remove(tile_id); - self.blueprint.remove_space_view(&space_view_id, ctx); - } - } - - if Some(tile_id) == self.tree.root { - self.tree.root = None; - } - } - - if self.tree.is_empty() && !self.blueprint.space_views.is_empty() { - re_log::trace!("Tree is empty - will re-run auto-layout"); - reset = true; - } - - if reset { - re_log::trace!("Resetting viewport tree"); - self.tree = super::auto_layout::tree_from_space_views( - ctx.space_view_class_registry, - &self.blueprint.space_views, - ); - } } /// If a group or spaceview has a total of this number of elements, show its subtree by default? From 8293a5641215e0a1c0941102ed29251b22b186ae Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 14 Dec 2023 22:07:46 +0100 Subject: [PATCH 12/26] Always reset at beginning of frame --- crates/re_viewer/src/app_state.rs | 8 ++++++-- crates/re_viewport/src/viewport.rs | 32 +++++++++++++++++++----------- 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/crates/re_viewer/src/app_state.rs b/crates/re_viewer/src/app_state.rs index b2d7bb963954..bcdeb7238bd2 100644 --- a/crates/re_viewer/src/app_state.rs +++ b/crates/re_viewer/src/app_state.rs @@ -106,7 +106,11 @@ impl AppState { } = self; let viewport_blueprint = ViewportBlueprint::try_from_db(store_context.blueprint); - let mut viewport = Viewport::new(&viewport_blueprint, viewport_state); + let mut viewport = Viewport::new( + &viewport_blueprint, + viewport_state, + space_view_class_registry, + ); // If the blueprint is invalid, reset it. if viewport.blueprint.is_invalid() { @@ -275,7 +279,7 @@ impl AppState { }); }); - // Process deferred layout operationsand apply updates back to blueprint + // Process deferred layout operations and apply updates back to blueprint viewport.update_and_sync_layout(&ctx); { diff --git a/crates/re_viewport/src/viewport.rs b/crates/re_viewport/src/viewport.rs index 1b4d592cf30b..74fdddb2b20c 100644 --- a/crates/re_viewport/src/viewport.rs +++ b/crates/re_viewport/src/viewport.rs @@ -108,13 +108,27 @@ pub struct Viewport<'a, 'b> { } impl<'a, 'b> Viewport<'a, 'b> { - pub fn new(blueprint: &'a ViewportBlueprint, state: &'b mut ViewportState) -> Self { + pub fn new( + blueprint: &'a ViewportBlueprint, + state: &'b mut ViewportState, + space_view_class_registry: &SpaceViewClassRegistry, + ) -> Self { re_tracing::profile_function!(); + // If the blueprint tree is empty/missing we need to auto-layout. + let tree = if blueprint.tree.is_empty() && !blueprint.space_views.is_empty() { + super::auto_layout::tree_from_space_views( + space_view_class_registry, + &blueprint.space_views, + ) + } else { + blueprint.tree.clone() + }; + Self { blueprint, state, - tree: blueprint.tree.clone(), + tree, deferred_tree_actions: Default::default(), } } @@ -339,17 +353,11 @@ impl<'a, 'b> Viewport<'a, 'b> { } } - if self.tree.is_empty() && !self.blueprint.space_views.is_empty() { - re_log::trace!("Tree is empty - will re-run auto-layout"); - reset = true; - } - if reset { - re_log::trace!("Resetting viewport tree"); - self.tree = super::auto_layout::tree_from_space_views( - ctx.space_view_class_registry, - &self.blueprint.space_views, - ); + // We don't run auto-layout here since the new space views also haven't been + // written to the store yet. + re_log::trace!("Clearing the blueprint tree to force reset on the next frame"); + self.tree = egui_tiles::Tree::empty("viewport_tree"); } // Finally, save any edits to the blueprint tree From e8867de2f280e946878210cc4ff5c4daa5fb63d3 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 14 Dec 2023 22:34:57 +0100 Subject: [PATCH 13/26] Also clear queries --- .../re_space_view/src/data_query_blueprint.rs | 6 ++ .../src/blueprint_helpers.rs | 29 +++++++ crates/re_viewer_context/src/lib.rs | 1 + crates/re_viewport/src/space_view.rs | 18 +++- crates/re_viewport/src/viewport_blueprint.rs | 84 ++++--------------- 5 files changed, 64 insertions(+), 74 deletions(-) create mode 100644 crates/re_viewer_context/src/blueprint_helpers.rs diff --git a/crates/re_space_view/src/data_query_blueprint.rs b/crates/re_space_view/src/data_query_blueprint.rs index 8139b2f1e45f..143139e4de89 100644 --- a/crates/re_space_view/src/data_query_blueprint.rs +++ b/crates/re_space_view/src/data_query_blueprint.rs @@ -4,6 +4,7 @@ use re_data_store::{ EntityProperties, EntityPropertiesComponent, EntityPropertyMap, EntityTree, StoreDb, }; use re_log_types::{DataRow, EntityPath, EntityPathExpr, RowId, TimePoint}; +use re_types_core::archetypes::Clear; use re_viewer_context::{ DataQueryId, DataQueryResult, DataResult, DataResultHandle, DataResultNode, DataResultTree, EntitiesPerSystem, EntitiesPerSystemPerClass, SpaceViewClassIdentifier, SpaceViewId, @@ -77,6 +78,11 @@ impl DataQueryBlueprint { }) } + pub fn clear(&self, ctx: &ViewerContext<'_>) { + let clear = Clear::recursive(); + ctx.save_blueprint_component(&self.id.as_entity_path(), clear.is_recursive); + } + pub fn build_resolver<'a>( &self, container: SpaceViewId, diff --git a/crates/re_viewer_context/src/blueprint_helpers.rs b/crates/re_viewer_context/src/blueprint_helpers.rs new file mode 100644 index 000000000000..c66a2d6d61a0 --- /dev/null +++ b/crates/re_viewer_context/src/blueprint_helpers.rs @@ -0,0 +1,29 @@ +use re_log_types::{DataRow, EntityPath, RowId, TimePoint}; + +use crate::{SystemCommand, SystemCommandSender as _, ViewerContext}; + +impl ViewerContext<'_> { + /// Helper to save a component to the blueprint store. + pub fn save_blueprint_component<'a, C>(&self, entity_path: &EntityPath, component: C) + where + C: re_types::Component + Clone + 'a, + std::borrow::Cow<'a, C>: std::convert::From, + { + let timepoint = TimePoint::timeless(); + + let row = DataRow::from_cells1_sized( + RowId::new(), + entity_path.clone(), + timepoint.clone(), + 1, + [component], + ) + .unwrap(); // TODO(emilk): statically check that the component is a mono-component - then this cannot fail! + + self.command_sender + .send_system(SystemCommand::UpdateBlueprint( + self.store_context.blueprint.store_id().clone(), + vec![row], + )); + } +} diff --git a/crates/re_viewer_context/src/lib.rs b/crates/re_viewer_context/src/lib.rs index 34afff8d9759..7ae11c606a51 100644 --- a/crates/re_viewer_context/src/lib.rs +++ b/crates/re_viewer_context/src/lib.rs @@ -4,6 +4,7 @@ mod annotations; mod app_options; +mod blueprint_helpers; mod blueprint_id; mod caches; mod command_sender; diff --git a/crates/re_viewport/src/space_view.rs b/crates/re_viewport/src/space_view.rs index a60f6c5a3e1c..8ff4a5dd5408 100644 --- a/crates/re_viewport/src/space_view.rs +++ b/crates/re_viewport/src/space_view.rs @@ -8,6 +8,7 @@ use re_renderer::ScreenshotProcessor; use re_space_view::{DataQueryBlueprint, ScreenshotMode}; use re_space_view_time_series::TimeSeriesSpaceView; use re_types::blueprint::components::{EntitiesDeterminedByUser, Name, SpaceViewOrigin}; +use re_types_core::archetypes::Clear; use re_viewer_context::{ DataQueryId, DataResult, DynSpaceViewClass, PerSystemDataResults, PerSystemEntities, SpaceViewClass, SpaceViewClassIdentifier, SpaceViewHighlights, SpaceViewId, SpaceViewState, @@ -16,7 +17,7 @@ use re_viewer_context::{ }; use crate::system_execution::create_and_run_space_view_systems; -use crate::viewport_blueprint::{add_delta_from_single_component, save_single_component}; +use crate::viewport_blueprint::add_delta_from_single_component; // ---------------------------------------------------------------------------- @@ -172,24 +173,33 @@ impl SpaceViewBlueprint { )); } + pub fn clear(&self, ctx: &ViewerContext<'_>) { + let clear = Clear::recursive(); + ctx.save_blueprint_component(&self.entity_path(), clear.is_recursive); + + for query in &self.queries { + query.clear(ctx); + } + } + pub fn set_entity_determined_by_user(&self, ctx: &ViewerContext<'_>) { if !self.entities_determined_by_user { let component = EntitiesDeterminedByUser(true); - save_single_component(&self.entity_path(), component, ctx); + ctx.save_blueprint_component(&self.entity_path(), component); } } pub fn set_display_name(&self, name: String, ctx: &ViewerContext<'_>) { if name != self.display_name { let component = Name(name.into()); - save_single_component(&self.entity_path(), component, ctx); + ctx.save_blueprint_component(&self.entity_path(), component); } } pub fn set_origin(&self, origin: &EntityPath, ctx: &ViewerContext<'_>) { if origin != &self.space_origin { let component = SpaceViewOrigin(origin.into()); - save_single_component(&self.entity_path(), component, ctx); + ctx.save_blueprint_component(&self.entity_path(), component); } } diff --git a/crates/re_viewport/src/viewport_blueprint.rs b/crates/re_viewport/src/viewport_blueprint.rs index 04756a3f1bea..3f1161e23a5a 100644 --- a/crates/re_viewport/src/viewport_blueprint.rs +++ b/crates/re_viewport/src/viewport_blueprint.rs @@ -4,10 +4,7 @@ use re_arrow_store::LatestAtQuery; use re_data_store::EntityPath; use re_log_types::{DataRow, RowId, TimePoint, Timeline}; use re_query::query_archetype; -use re_types_core::{archetypes::Clear, AsComponents as _}; -use re_viewer_context::{ - Item, SpaceViewClassIdentifier, SpaceViewId, SystemCommand, SystemCommandSender, ViewerContext, -}; +use re_viewer_context::{Item, SpaceViewClassIdentifier, SpaceViewId, ViewerContext}; use crate::{ blueprint::components::{ @@ -167,19 +164,17 @@ impl ViewportBlueprint { pub(crate) fn remove_space_view(&self, space_view_id: &SpaceViewId, ctx: &ViewerContext<'_>) { self.mark_user_interaction(ctx); - let timepoint = TimePoint::timeless(); - let mut deltas = vec![]; + // Remove the space view from the store + if let Some(space_view) = self.space_views.get(space_view_id) { + space_view.clear(ctx); + } + // If the space-view was maximized, clean it up if self.maximized == Some(*space_view_id) { - let component = SpaceViewMaximized(None); - add_delta_from_single_component( - &mut deltas, - &VIEWPORT_PATH.into(), - &timepoint, - component, - ); + self.set_maximized(None, ctx); } + // Filter the space-view from the included space-views let component = IncludedSpaceViews( self.space_views .keys() @@ -187,15 +182,7 @@ impl ViewportBlueprint { .map(|id| (*id).into()) .collect(), ); - add_delta_from_single_component(&mut deltas, &VIEWPORT_PATH.into(), &timepoint, component); - - clear_space_view(&mut deltas, space_view_id); - - ctx.command_sender - .send_system(SystemCommand::UpdateBlueprint( - ctx.store_context.blueprint.store_id().clone(), - deltas, - )); + ctx.save_blueprint_component(&VIEWPORT_PATH.into(), component); } /// If `false`, the item is referring to data that is not present in this blueprint. @@ -283,7 +270,7 @@ impl ViewportBlueprint { let component = IncludedSpaceViews(updated_ids.into_iter().map(|id| (*id).into()).collect()); - save_single_component(&VIEWPORT_PATH.into(), component, ctx); + ctx.save_blueprint_component(&VIEWPORT_PATH.into(), component); } } @@ -313,21 +300,21 @@ impl ViewportBlueprint { pub fn set_auto_layout(&self, value: bool, ctx: &ViewerContext<'_>) { if self.auto_layout != value { let component = AutoLayout(value); - save_single_component(&VIEWPORT_PATH.into(), component, ctx); + ctx.save_blueprint_component(&VIEWPORT_PATH.into(), component); } } pub fn set_auto_space_views(&self, value: bool, ctx: &ViewerContext<'_>) { if self.auto_layout != value { let component = AutoSpaceViews(value); - save_single_component(&VIEWPORT_PATH.into(), component, ctx); + ctx.save_blueprint_component(&VIEWPORT_PATH.into(), component); } } pub fn set_maximized(&self, space_view_id: Option, ctx: &ViewerContext<'_>) { if self.maximized != space_view_id { let component = SpaceViewMaximized(space_view_id.map(|id| id.into())); - save_single_component(&VIEWPORT_PATH.into(), component, ctx); + ctx.save_blueprint_component(&VIEWPORT_PATH.into(), component); } } @@ -335,7 +322,7 @@ impl ViewportBlueprint { if &self.tree != tree { re_log::trace!("Updating the layout tree"); let component = ViewportLayout(tree.clone()); - save_single_component(&VIEWPORT_PATH.into(), component, ctx); + ctx.save_blueprint_component(&VIEWPORT_PATH.into(), component); } } } @@ -363,46 +350,3 @@ pub fn add_delta_from_single_component<'a, C>( deltas.push(row); } - -// TODO(jleibs): Move this helper to a better location -pub fn save_single_component<'a, C>(entity_path: &EntityPath, component: C, ctx: &ViewerContext<'_>) -where - C: re_types::Component + Clone + 'a, - std::borrow::Cow<'a, C>: std::convert::From, -{ - let timepoint = TimePoint::timeless(); - - let row = DataRow::from_cells1_sized( - RowId::new(), - entity_path.clone(), - timepoint.clone(), - 1, - [component], - ) - .unwrap(); // TODO(emilk): statically check that the component is a mono-component - then this cannot fail! - - ctx.command_sender - .send_system(SystemCommand::UpdateBlueprint( - ctx.store_context.blueprint.store_id().clone(), - vec![row], - )); -} - -// ---------------------------------------------------------------------------- - -pub fn clear_space_view(deltas: &mut Vec, space_view_id: &SpaceViewId) { - // TODO(jleibs): Seq instead of timeless? - let timepoint = TimePoint::timeless(); - - if let Ok(row) = DataRow::from_component_batches( - RowId::new(), - timepoint, - space_view_id.as_entity_path(), - Clear::recursive() - .as_component_batches() - .iter() - .map(|b| b.as_ref()), - ) { - deltas.push(row); - } -} From 8f852f08439d445442027b49568595123796b075 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 14 Dec 2023 22:37:33 +0100 Subject: [PATCH 14/26] Rename --- crates/re_viewer/src/app_state.rs | 2 +- crates/re_viewport/src/viewport.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/re_viewer/src/app_state.rs b/crates/re_viewer/src/app_state.rs index bcdeb7238bd2..fad212e89915 100644 --- a/crates/re_viewer/src/app_state.rs +++ b/crates/re_viewer/src/app_state.rs @@ -280,7 +280,7 @@ impl AppState { }); // Process deferred layout operations and apply updates back to blueprint - viewport.update_and_sync_layout(&ctx); + viewport.update_and_sync_tile_tree_to_blueprint(&ctx); { // We move the time at the very end of the frame, diff --git a/crates/re_viewport/src/viewport.rs b/crates/re_viewport/src/viewport.rs index 74fdddb2b20c..17b55d7df84a 100644 --- a/crates/re_viewport/src/viewport.rs +++ b/crates/re_viewport/src/viewport.rs @@ -295,8 +295,8 @@ impl<'a, 'b> Viewport<'a, 'b> { true } - /// Process any deferred `TreeActions` and then sync to store - pub fn update_and_sync_layout(&mut self, ctx: &ViewerContext<'_>) { + /// Process any deferred `TreeActions` and then sync to blueprint + pub fn update_and_sync_tile_tree_to_blueprint(&mut self, ctx: &ViewerContext<'_>) { // At the end of the Tree-UI, we can safely apply deferred actions. let mut reset = std::mem::take(&mut self.deferred_tree_actions.reset); From 6190ffeadcd47862fc4fc679a3439a05d82d9e8a Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 14 Dec 2023 22:43:01 +0100 Subject: [PATCH 15/26] no need for a reset TreeAction --- crates/re_viewport/src/viewport.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/re_viewport/src/viewport.rs b/crates/re_viewport/src/viewport.rs index 17b55d7df84a..a414d15d2e9b 100644 --- a/crates/re_viewport/src/viewport.rs +++ b/crates/re_viewport/src/viewport.rs @@ -76,7 +76,6 @@ impl ViewportState { // so that we don't iterate over something while modifying it. #[derive(Clone, Default)] pub struct TreeActions { - pub reset: bool, pub create: Vec, pub focus_tab: Option, pub remove: Vec, @@ -299,7 +298,7 @@ impl<'a, 'b> Viewport<'a, 'b> { pub fn update_and_sync_tile_tree_to_blueprint(&mut self, ctx: &ViewerContext<'_>) { // At the end of the Tree-UI, we can safely apply deferred actions. - let mut reset = std::mem::take(&mut self.deferred_tree_actions.reset); + let mut reset = false; let create = std::mem::take(&mut self.deferred_tree_actions.create); let mut focus_tab = std::mem::take(&mut self.deferred_tree_actions.focus_tab); let remove = std::mem::take(&mut self.deferred_tree_actions.remove); From 2b8291eb3efc6881bf1e513698f7f15f2fe8f743 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 14 Dec 2023 22:46:17 +0100 Subject: [PATCH 16/26] no-op sets are no-op --- crates/re_viewport/src/viewport.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/re_viewport/src/viewport.rs b/crates/re_viewport/src/viewport.rs index a414d15d2e9b..82408681b220 100644 --- a/crates/re_viewport/src/viewport.rs +++ b/crates/re_viewport/src/viewport.rs @@ -221,9 +221,7 @@ impl<'a, 'b> Viewport<'a, 'b> { state.space_views_displayed_last_frame = tab_viewer.space_views_displayed_current_frame; }); - if maximized != blueprint.maximized { - self.blueprint.set_maximized(maximized, ctx); - } + self.blueprint.set_maximized(maximized, ctx); } pub fn on_frame_start(&mut self, ctx: &ViewerContext<'_>, spaces_info: &SpaceInfoCollection) { From 5c9da52ddad4aaec657020b9a4edbce4830cc67d Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Thu, 14 Dec 2023 22:47:35 +0100 Subject: [PATCH 17/26] Inline the set_ operations --- crates/re_viewport/src/space_view.rs | 3 +++ crates/re_viewport/src/viewport_blueprint.rs | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/crates/re_viewport/src/space_view.rs b/crates/re_viewport/src/space_view.rs index 8ff4a5dd5408..8b73b93d23cf 100644 --- a/crates/re_viewport/src/space_view.rs +++ b/crates/re_viewport/src/space_view.rs @@ -182,6 +182,7 @@ impl SpaceViewBlueprint { } } + #[inline] pub fn set_entity_determined_by_user(&self, ctx: &ViewerContext<'_>) { if !self.entities_determined_by_user { let component = EntitiesDeterminedByUser(true); @@ -189,6 +190,7 @@ impl SpaceViewBlueprint { } } + #[inline] pub fn set_display_name(&self, name: String, ctx: &ViewerContext<'_>) { if name != self.display_name { let component = Name(name.into()); @@ -196,6 +198,7 @@ impl SpaceViewBlueprint { } } + #[inline] pub fn set_origin(&self, origin: &EntityPath, ctx: &ViewerContext<'_>) { if origin != &self.space_origin { let component = SpaceViewOrigin(origin.into()); diff --git a/crates/re_viewport/src/viewport_blueprint.rs b/crates/re_viewport/src/viewport_blueprint.rs index 3f1161e23a5a..94fa2d5a79e1 100644 --- a/crates/re_viewport/src/viewport_blueprint.rs +++ b/crates/re_viewport/src/viewport_blueprint.rs @@ -297,6 +297,7 @@ impl ViewportBlueprint { .collect() } + #[inline] pub fn set_auto_layout(&self, value: bool, ctx: &ViewerContext<'_>) { if self.auto_layout != value { let component = AutoLayout(value); @@ -304,6 +305,7 @@ impl ViewportBlueprint { } } + #[inline] pub fn set_auto_space_views(&self, value: bool, ctx: &ViewerContext<'_>) { if self.auto_layout != value { let component = AutoSpaceViews(value); @@ -311,6 +313,7 @@ impl ViewportBlueprint { } } + #[inline] pub fn set_maximized(&self, space_view_id: Option, ctx: &ViewerContext<'_>) { if self.maximized != space_view_id { let component = SpaceViewMaximized(space_view_id.map(|id| id.into())); @@ -318,6 +321,7 @@ impl ViewportBlueprint { } } + #[inline] pub fn set_tree(&self, tree: &egui_tiles::Tree, ctx: &ViewerContext<'_>) { if &self.tree != tree { re_log::trace!("Updating the layout tree"); From 0987ae20c8a76290f44147184455ef6d24cd87aa Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Fri, 15 Dec 2023 00:32:44 +0100 Subject: [PATCH 18/26] doc-link --- crates/re_viewer/src/ui/blueprint_panel.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/re_viewer/src/ui/blueprint_panel.rs b/crates/re_viewer/src/ui/blueprint_panel.rs index ab893237f8b0..9538c93290c3 100644 --- a/crates/re_viewer/src/ui/blueprint_panel.rs +++ b/crates/re_viewer/src/ui/blueprint_panel.rs @@ -1,7 +1,7 @@ use re_viewer_context::{SystemCommandSender as _, ViewerContext}; use re_viewport::{SpaceInfoCollection, Viewport}; -/// Show the Blueprint section of the left panel based on the current [`ViewportBlueprint`] +/// Show the Blueprint section of the left panel based on the current [`Viewport`] pub fn blueprint_panel_ui( viewport: &mut Viewport<'_, '_>, ctx: &ViewerContext<'_>, From 2b2b0db8d9f162140dbccfdef97eb070fd42bf6f Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Fri, 15 Dec 2023 14:08:34 +0100 Subject: [PATCH 19/26] When duplicating a SpaceViewBlueprint, also duplicate its Query (#4549) ### What - Resolves: https://github.com/rerun-io/rerun/issues/4456 The crux of the problem is cloning the SpaceView created a new SpaceView with the same DataQuery reference (which is where the property overrides are stored). Since cloning the internal IDs of a SpaceView or DataQuery can have unintended consequences, I removed Clone from these types and replaced it with a `duplicate` method with clearer semantics. Also had to clean up the heuristic path to get rid of the one otherwise unnecessary clone. ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested the web demo (if applicable): * Using newly built examples: [app.rerun.io](https://app.rerun.io/pr/4549/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/4549/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [app.rerun.io](https://app.rerun.io/pr/4549/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json) * [x] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG - [PR Build Summary](https://build.rerun.io/pr/4549) - [Docs preview](https://rerun.io/preview/f128e543a4e06edee55f1a089b1b6a9837ca43b0/docs) - [Examples preview](https://rerun.io/preview/f128e543a4e06edee55f1a089b1b6a9837ca43b0/examples) - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) --- .../re_space_view/src/data_query_blueprint.rs | 18 ++++++- crates/re_viewer/src/ui/selection_panel.rs | 3 +- crates/re_viewport/src/space_view.rs | 22 ++++++++- .../re_viewport/src/space_view_heuristics.rs | 49 +++++++++++-------- 4 files changed, 67 insertions(+), 25 deletions(-) diff --git a/crates/re_space_view/src/data_query_blueprint.rs b/crates/re_space_view/src/data_query_blueprint.rs index 143139e4de89..e4fc06361dfc 100644 --- a/crates/re_space_view/src/data_query_blueprint.rs +++ b/crates/re_space_view/src/data_query_blueprint.rs @@ -29,7 +29,14 @@ use crate::{ /// The results of recursive expressions are only included if they are found within the [`EntityTree`] /// and for which there is a valid `ViewPart` system. This keeps recursive expressions from incorrectly /// picking up irrelevant data within the tree. -#[derive(Clone, PartialEq, Eq)] +/// +/// Note: [`DataQueryBlueprint`] doesn't implement Clone because it stores an internal +/// uuid used for identifying the path of its data in the blueprint store. It's ambiguous +/// whether the intent is for a clone to write to the same place. +/// +/// If you want a new space view otherwise identical to an existing one, use +/// [`DataQueryBlueprint::duplicate`]. +#[derive(PartialEq, Eq)] pub struct DataQueryBlueprint { pub id: DataQueryId, pub space_view_class_identifier: SpaceViewClassIdentifier, @@ -78,6 +85,15 @@ impl DataQueryBlueprint { }) } + /// Creates a new [`DataQueryBlueprint`] with a the same contents, but a different [`DataQueryId`] + pub fn duplicate(&self) -> Self { + Self { + id: DataQueryId::random(), + space_view_class_identifier: self.space_view_class_identifier, + expressions: self.expressions.clone(), + } + } + pub fn clear(&self, ctx: &ViewerContext<'_>) { let clear = Clear::recursive(); ctx.save_blueprint_component(&self.id.as_entity_path(), clear.is_recursive); diff --git a/crates/re_viewer/src/ui/selection_panel.rs b/crates/re_viewer/src/ui/selection_panel.rs index 2371a7e8fa7f..34bf83021913 100644 --- a/crates/re_viewer/src/ui/selection_panel.rs +++ b/crates/re_viewer/src/ui/selection_panel.rs @@ -522,8 +522,7 @@ fn blueprint_ui( .clicked() { if let Some(space_view) = viewport.blueprint.space_view(space_view_id) { - let mut new_space_view = space_view.clone(); - new_space_view.id = SpaceViewId::random(); + let new_space_view = space_view.duplicate(); viewport.blueprint.add_space_views(std::iter::once(new_space_view), ctx, &mut viewport.deferred_tree_actions); viewport.blueprint.mark_user_interaction(ctx); } diff --git a/crates/re_viewport/src/space_view.rs b/crates/re_viewport/src/space_view.rs index 8b73b93d23cf..3872813fd11d 100644 --- a/crates/re_viewport/src/space_view.rs +++ b/crates/re_viewport/src/space_view.rs @@ -22,7 +22,13 @@ use crate::viewport_blueprint::add_delta_from_single_component; // ---------------------------------------------------------------------------- /// A view of a space. -#[derive(Clone)] +/// +/// Note: [`SpaceViewBlueprint`] doesn't implement Clone because it stores an internal +/// uuid used for identifying the path of its data in the blueprint store. It's ambiguous +/// whether the intent is for a clone to write to the same place. +/// +/// If you want a new space view otherwise identical to an existing one, use +/// [`SpaceViewBlueprint::duplicate`]. pub struct SpaceViewBlueprint { pub id: SpaceViewId, pub display_name: String, @@ -173,6 +179,20 @@ impl SpaceViewBlueprint { )); } + /// Creates a new [`SpaceViewBlueprint`] with a the same contents, but a different [`SpaceViewId`] + /// + /// Also duplicates all of the queries in the space view. + pub fn duplicate(&self) -> Self { + Self { + id: SpaceViewId::random(), + display_name: self.display_name.clone(), + class_identifier: self.class_identifier, + space_origin: self.space_origin.clone(), + queries: self.queries.iter().map(|q| q.duplicate()).collect(), + entities_determined_by_user: self.entities_determined_by_user, + } + } + pub fn clear(&self, ctx: &ViewerContext<'_>) { let clear = Clear::recursive(); ctx.save_blueprint_component(&self.entity_path(), clear.is_recursive); diff --git a/crates/re_viewport/src/space_view_heuristics.rs b/crates/re_viewport/src/space_view_heuristics.rs index 79adb089dcab..c7b02a2282ed 100644 --- a/crates/re_viewport/src/space_view_heuristics.rs +++ b/crates/re_viewport/src/space_view_heuristics.rs @@ -311,35 +311,42 @@ pub fn default_created_space_views( // `AutoSpawnHeuristic::SpawnClassWithHighestScoreForRoot` means we're competing with other candidates for the same root. if let AutoSpawnHeuristic::SpawnClassWithHighestScoreForRoot(score) = spawn_heuristic { - let mut should_spawn_new = true; + // [`SpaceViewBlueprint`]s don't implement clone so wrap in an option so we can + // track whether or not we've consumed it. + let mut candidate_still_considered = Some(candidate); + for (prev_candidate, prev_spawn_heuristic) in &mut space_views { - if prev_candidate.space_origin == candidate.space_origin { - #[allow(clippy::match_same_arms)] - match prev_spawn_heuristic { - AutoSpawnHeuristic::SpawnClassWithHighestScoreForRoot(prev_score) => { - // If we're competing with a candidate for the same root, we either replace a lower score, or we yield. - should_spawn_new = false; - if *prev_score < score { - // Replace the previous candidate with this one. - *prev_candidate = candidate.clone(); - *prev_spawn_heuristic = spawn_heuristic; - } else { - // We have a lower score, so we don't spawn. + if let Some(candidate) = candidate_still_considered.take() { + if prev_candidate.space_origin == candidate.space_origin { + #[allow(clippy::match_same_arms)] + match prev_spawn_heuristic { + AutoSpawnHeuristic::SpawnClassWithHighestScoreForRoot(prev_score) => { + // If we're competing with a candidate for the same root, we either replace a lower score, or we yield. + if *prev_score < score { + // Replace the previous candidate with this one. + *prev_candidate = candidate; + *prev_spawn_heuristic = spawn_heuristic; + } + + // Either way we're done with this candidate. break; } - } - AutoSpawnHeuristic::AlwaysSpawn => { - // We can live side by side with always-spawn candidates. - } - AutoSpawnHeuristic::NeverSpawn => { - // Never spawn candidates should not be in the list, this is weird! - // But let's not fail on this since our heuristics are not perfect anyways. + AutoSpawnHeuristic::AlwaysSpawn => { + // We can live side by side with always-spawn candidates. + } + AutoSpawnHeuristic::NeverSpawn => { + // Never spawn candidates should not be in the list, this is weird! + // But let's not fail on this since our heuristics are not perfect anyways. + } } } + + // If we didn't hit the break condition, continue to consider the candidate + candidate_still_considered = Some(candidate); } } - if should_spawn_new { + if let Some(candidate) = candidate_still_considered { // Spatial views with images get extra treatment as well. if is_spatial_2d_class(candidate.class_identifier()) { #[derive(Hash, PartialEq, Eq)] From 4021e957927e2e6bfbb5e1dc9f9b0bd99bed4199 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Fri, 15 Dec 2023 16:06:04 +0100 Subject: [PATCH 20/26] PR cleanup --- crates/re_viewer/src/ui/selection_panel.rs | 1 - .../src/blueprint_helpers.rs | 22 +++++++++++-------- crates/re_viewport/src/viewport.rs | 9 +++++--- 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/crates/re_viewer/src/ui/selection_panel.rs b/crates/re_viewer/src/ui/selection_panel.rs index 34bf83021913..227656fa51b3 100644 --- a/crates/re_viewer/src/ui/selection_panel.rs +++ b/crates/re_viewer/src/ui/selection_panel.rs @@ -410,7 +410,6 @@ fn container_top_level_properties( viewport: &mut Viewport<'_, '_>, tile_id: &egui_tiles::TileId, ) { - // TODO(jleibs): fix container-editing if let Some(Tile::Container(container)) = viewport.tree.tiles.get_mut(*tile_id) { egui::Grid::new("container_top_level_properties") .num_columns(2) diff --git a/crates/re_viewer_context/src/blueprint_helpers.rs b/crates/re_viewer_context/src/blueprint_helpers.rs index c66a2d6d61a0..a16da5c5489b 100644 --- a/crates/re_viewer_context/src/blueprint_helpers.rs +++ b/crates/re_viewer_context/src/blueprint_helpers.rs @@ -11,19 +11,23 @@ impl ViewerContext<'_> { { let timepoint = TimePoint::timeless(); - let row = DataRow::from_cells1_sized( + match DataRow::from_cells1_sized( RowId::new(), entity_path.clone(), timepoint.clone(), 1, [component], - ) - .unwrap(); // TODO(emilk): statically check that the component is a mono-component - then this cannot fail! - - self.command_sender - .send_system(SystemCommand::UpdateBlueprint( - self.store_context.blueprint.store_id().clone(), - vec![row], - )); + ) { + Ok(row) => self + .command_sender + .send_system(SystemCommand::UpdateBlueprint( + self.store_context.blueprint.store_id().clone(), + vec![row], + )), + Err(err) => { + // TODO(emilk): statically check that the component is a mono-component - then this cannot fail! + re_log::error_once!("Failed to create DataRow for blueprint component: {}", err); + } + } } } diff --git a/crates/re_viewport/src/viewport.rs b/crates/re_viewport/src/viewport.rs index 82408681b220..e13f7337d155 100644 --- a/crates/re_viewport/src/viewport.rs +++ b/crates/re_viewport/src/viewport.rs @@ -297,9 +297,12 @@ impl<'a, 'b> Viewport<'a, 'b> { // At the end of the Tree-UI, we can safely apply deferred actions. let mut reset = false; - let create = std::mem::take(&mut self.deferred_tree_actions.create); - let mut focus_tab = std::mem::take(&mut self.deferred_tree_actions.focus_tab); - let remove = std::mem::take(&mut self.deferred_tree_actions.remove); + + let TreeActions { + create, + mut focus_tab, + remove, + } = std::mem::take(&mut self.deferred_tree_actions); for space_view in &create { if self.blueprint.auto_layout { From 5764be4b1fc6996f8a0bd496b36160bfea94c32d Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Fri, 15 Dec 2023 16:18:16 +0100 Subject: [PATCH 21/26] Clean up and document save methods --- .../re_space_view/src/data_query_blueprint.rs | 11 +++++--- crates/re_viewport/src/space_view.rs | 27 ++++++++++--------- crates/re_viewport/src/viewport_blueprint.rs | 4 +-- 3 files changed, 24 insertions(+), 18 deletions(-) diff --git a/crates/re_space_view/src/data_query_blueprint.rs b/crates/re_space_view/src/data_query_blueprint.rs index e4fc06361dfc..6bb025749efd 100644 --- a/crates/re_space_view/src/data_query_blueprint.rs +++ b/crates/re_space_view/src/data_query_blueprint.rs @@ -55,6 +55,10 @@ impl DataQueryBlueprint { pub const INDIVIDUAL_OVERRIDES_PREFIX: &'static str = "individual_overrides"; pub const RECURSIVE_OVERRIDES_PREFIX: &'static str = "recursive_overrides"; + /// Creates a new [`DataQueryBlueprint`]. + /// + /// This [`DataQueryBlueprint`] is ephemeral. It must be saved by calling + /// `save_to_blueprint_store` on the enclosing `SpaceViewBlueprint`. pub fn new( space_view_class_identifier: SpaceViewClassIdentifier, queries_entities: impl Iterator, @@ -66,18 +70,17 @@ impl DataQueryBlueprint { } } + /// Attempt to load a [`DataQueryBlueprint`] from the blueprint store. pub fn try_from_db( - path: &EntityPath, + id: DataQueryId, blueprint_db: &StoreDb, space_view_class_identifier: SpaceViewClassIdentifier, ) -> Option { let expressions = blueprint_db .store() - .query_timeless_component::(path) + .query_timeless_component::(&id.as_entity_path()) .map(|c| c.value)?; - let id = DataQueryId::from_entity_path(path); - Some(Self { id, space_view_class_identifier, diff --git a/crates/re_viewport/src/space_view.rs b/crates/re_viewport/src/space_view.rs index 03036214a57b..2f4e34d3ff7d 100644 --- a/crates/re_viewport/src/space_view.rs +++ b/crates/re_viewport/src/space_view.rs @@ -48,6 +48,10 @@ pub struct SpaceViewBlueprint { } impl SpaceViewBlueprint { + /// Creates a new [`SpaceViewBlueprint`] with a single [`DataQueryBlueprint`]. + /// + /// This [`SpaceViewBlueprint`] is ephemeral. If you want to make it permanent you + /// must call [`save_to_blueprint_store`]. pub fn new( space_view_class: SpaceViewClassIdentifier, space_view_class_display_name: &str, @@ -77,7 +81,8 @@ impl SpaceViewBlueprint { } } - pub fn try_from_db(path: &EntityPath, blueprint_db: &StoreDb) -> Option { + /// Attempt to load a [`SpaceViewBlueprint`] from the blueprint store. + pub fn try_from_db(id: SpaceViewId, blueprint_db: &StoreDb) -> Option { re_tracing::profile_function!(); let query = LatestAtQuery::latest(Timeline::default()); @@ -88,7 +93,7 @@ impl SpaceViewBlueprint { space_origin, entities_determined_by_user, contents, - } = query_archetype(blueprint_db.store(), &query, path) + } = query_archetype(blueprint_db.store(), &query, &id.as_entity_path()) .and_then(|arch| arch.to_archetype()) .map_err(|err| { if cfg!(debug_assertions) { @@ -100,8 +105,6 @@ impl SpaceViewBlueprint { }) .ok()?; - let id = SpaceViewId::from_entity_path(path); - let space_origin = space_origin.map_or_else(EntityPath::root, |origin| origin.0.into()); let class_identifier: SpaceViewClassIdentifier = class_identifier.0.as_str().into(); @@ -123,13 +126,7 @@ impl SpaceViewBlueprint { .0 .into_iter() .map(DataQueryId::from) - .filter_map(|id| { - DataQueryBlueprint::try_from_db( - &id.as_entity_path(), - blueprint_db, - class_identifier, - ) - }) + .filter_map(|id| DataQueryBlueprint::try_from_db(id, blueprint_db, class_identifier)) .collect(); let entities_determined_by_user = entities_determined_by_user.unwrap_or_default().0; @@ -144,7 +141,13 @@ impl SpaceViewBlueprint { }) } - pub fn save_full(&self, ctx: &ViewerContext<'_>) { + /// Persist the entire [`SpaceViewBlueprint`] to the blueprint store. + /// + /// This only needs to be called if the [`SpaceViewBlueprint`] was created with [`new`]. + /// + /// Otherwise, incremental calls to `set_` functions will write just the necessary component + /// update directly to the store. + pub fn save_to_blueprint_store(&self, ctx: &ViewerContext<'_>) { let timepoint = TimePoint::timeless(); let arch = re_types::blueprint::archetypes::SpaceViewBlueprint::new( diff --git a/crates/re_viewport/src/viewport_blueprint.rs b/crates/re_viewport/src/viewport_blueprint.rs index 94fa2d5a79e1..839902941310 100644 --- a/crates/re_viewport/src/viewport_blueprint.rs +++ b/crates/re_viewport/src/viewport_blueprint.rs @@ -73,7 +73,7 @@ impl ViewportBlueprint { let space_views: BTreeMap = space_view_ids .into_iter() .filter_map(|space_view: SpaceViewId| { - SpaceViewBlueprint::try_from_db(&space_view.as_entity_path(), blueprint_db) + SpaceViewBlueprint::try_from_db(space_view, blueprint_db) }) .map(|sv| (sv.id, sv)) .collect(); @@ -256,7 +256,7 @@ impl ViewportBlueprint { space_view.display_name = unique_name; // Save the space view to the store - space_view.save_full(ctx); + space_view.save_to_blueprint_store(ctx); // Update the space-view ids: new_ids.push(space_view_id); From 774a7636e365e3b958d4909b1bdc5da4a867b4f0 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Fri, 15 Dec 2023 16:20:19 +0100 Subject: [PATCH 22/26] typo --- crates/re_viewport/src/viewport.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/re_viewport/src/viewport.rs b/crates/re_viewport/src/viewport.rs index e13f7337d155..fb3b64584fa9 100644 --- a/crates/re_viewport/src/viewport.rs +++ b/crates/re_viewport/src/viewport.rs @@ -89,7 +89,7 @@ pub struct Viewport<'a, 'b> { /// for this frame. pub blueprint: &'a ViewportBlueprint, - /// The persistent state of the viewport that is not saved to the store but otherwises + /// The persistent state of the viewport that is not saved to the store but otherwise /// persis frame-to-frame. pub state: &'b mut ViewportState, From e05798a995fdd58f58a90997cbc1247857418d1d Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Fri, 15 Dec 2023 16:40:40 +0100 Subject: [PATCH 23/26] More comment cleanup --- crates/re_viewport/src/viewport.rs | 2 -- crates/re_viewport/src/viewport_blueprint.rs | 5 +++++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/re_viewport/src/viewport.rs b/crates/re_viewport/src/viewport.rs index fb3b64584fa9..d4c33240a346 100644 --- a/crates/re_viewport/src/viewport.rs +++ b/crates/re_viewport/src/viewport.rs @@ -171,8 +171,6 @@ impl<'a, 'b> Viewport<'a, 'b> { let mut maximized_tree; - // TODO(jleibs): This tree won't have the edits from `viewport_blueprint_ui`. - // Maybe we should route that all the way through to here and only save it once. let tree = if let Some(space_view_id) = blueprint.maximized { let mut tiles = egui_tiles::Tiles::default(); let root = tiles.insert_pane(space_view_id); diff --git a/crates/re_viewport/src/viewport_blueprint.rs b/crates/re_viewport/src/viewport_blueprint.rs index 839902941310..3418d7240564 100644 --- a/crates/re_viewport/src/viewport_blueprint.rs +++ b/crates/re_viewport/src/viewport_blueprint.rs @@ -225,7 +225,12 @@ impl ViewportBlueprint { } /// Add a set of space views to the viewport. + /// /// NOTE: Calling this more than once per frame will result in lost data. + /// Each call to `add_space_views` emits an updated list of [`IncludedSpaceViews`] + /// Built by taking the list of [`IncludedSpaceViews`] from the current frame + /// and adding the new space views to it. Since this the edit is not applied until + /// the end of frame the second call will see a stale version of the data. // TODO(jleibs): Better safety check here. pub fn add_space_views( &self, From 2d695ef4ff9bab80a122bcc156e521091740154c Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Fri, 15 Dec 2023 17:03:20 +0100 Subject: [PATCH 24/26] proper doc links --- crates/re_viewport/src/space_view.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/re_viewport/src/space_view.rs b/crates/re_viewport/src/space_view.rs index 2f4e34d3ff7d..0fa6d55a4b74 100644 --- a/crates/re_viewport/src/space_view.rs +++ b/crates/re_viewport/src/space_view.rs @@ -51,7 +51,7 @@ impl SpaceViewBlueprint { /// Creates a new [`SpaceViewBlueprint`] with a single [`DataQueryBlueprint`]. /// /// This [`SpaceViewBlueprint`] is ephemeral. If you want to make it permanent you - /// must call [`save_to_blueprint_store`]. + /// must call [`Self::save_to_blueprint_store`]. pub fn new( space_view_class: SpaceViewClassIdentifier, space_view_class_display_name: &str, @@ -143,7 +143,7 @@ impl SpaceViewBlueprint { /// Persist the entire [`SpaceViewBlueprint`] to the blueprint store. /// - /// This only needs to be called if the [`SpaceViewBlueprint`] was created with [`new`]. + /// This only needs to be called if the [`SpaceViewBlueprint`] was created with [`Self::new`]. /// /// Otherwise, incremental calls to `set_` functions will write just the necessary component /// update directly to the store. From b5a137156efceb5c0cdac9ecc95792eec38bbb12 Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Fri, 15 Dec 2023 17:31:08 +0100 Subject: [PATCH 25/26] Expose save API directly on query --- .../re_space_view/src/data_query_blueprint.rs | 10 ++++++++ crates/re_viewport/src/space_view.rs | 8 +------ crates/re_viewport/src/viewport_blueprint.rs | 24 ------------------- 3 files changed, 11 insertions(+), 31 deletions(-) diff --git a/crates/re_space_view/src/data_query_blueprint.rs b/crates/re_space_view/src/data_query_blueprint.rs index 6bb025749efd..d9c6cb2340a9 100644 --- a/crates/re_space_view/src/data_query_blueprint.rs +++ b/crates/re_space_view/src/data_query_blueprint.rs @@ -88,6 +88,16 @@ impl DataQueryBlueprint { }) } + /// Persist the entire [`DataQueryBlueprint`] to the blueprint store. + /// + /// This only needs to be called if the [`DataQueryBlueprint`] was created with [`Self::new`]. + /// + /// Otherwise, incremental calls to `set_` functions will write just the necessary component + /// update directly to the store. + pub fn save_to_blueprint_store(&self, ctx: &ViewerContext<'_>) { + ctx.save_blueprint_component(&self.id.as_entity_path(), self.expressions.clone()); + } + /// Creates a new [`DataQueryBlueprint`] with a the same contents, but a different [`DataQueryId`] pub fn duplicate(&self) -> Self { Self { diff --git a/crates/re_viewport/src/space_view.rs b/crates/re_viewport/src/space_view.rs index 0fa6d55a4b74..e99bf1bae77f 100644 --- a/crates/re_viewport/src/space_view.rs +++ b/crates/re_viewport/src/space_view.rs @@ -17,7 +17,6 @@ use re_viewer_context::{ }; use crate::system_execution::create_and_run_space_view_systems; -use crate::viewport_blueprint::add_delta_from_single_component; // ---------------------------------------------------------------------------- @@ -167,12 +166,7 @@ impl SpaceViewBlueprint { } for query in &self.queries { - add_delta_from_single_component( - &mut deltas, - &query.id.as_entity_path(), - &timepoint, - query.expressions.clone(), - ); + query.save_to_blueprint_store(ctx); } ctx.command_sender diff --git a/crates/re_viewport/src/viewport_blueprint.rs b/crates/re_viewport/src/viewport_blueprint.rs index 3418d7240564..3c450a55fb66 100644 --- a/crates/re_viewport/src/viewport_blueprint.rs +++ b/crates/re_viewport/src/viewport_blueprint.rs @@ -335,27 +335,3 @@ impl ViewportBlueprint { } } } - -// ---------------------------------------------------------------------------- - -// TODO(jleibs): Move this helper to a better location -pub fn add_delta_from_single_component<'a, C>( - deltas: &mut Vec, - entity_path: &EntityPath, - timepoint: &TimePoint, - component: C, -) where - C: re_types::Component + Clone + 'a, - std::borrow::Cow<'a, C>: std::convert::From, -{ - let row = DataRow::from_cells1_sized( - RowId::new(), - entity_path.clone(), - timepoint.clone(), - 1, - [component], - ) - .unwrap(); // TODO(emilk): statically check that the component is a mono-component - then this cannot fail! - - deltas.push(row); -} From db702308b1658fe691878c7976b445ba5623b72a Mon Sep 17 00:00:00 2001 From: Jeremy Leibs Date: Fri, 15 Dec 2023 17:45:38 +0100 Subject: [PATCH 26/26] lint --- crates/re_viewport/src/viewport_blueprint.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/re_viewport/src/viewport_blueprint.rs b/crates/re_viewport/src/viewport_blueprint.rs index 3c450a55fb66..165f3517e662 100644 --- a/crates/re_viewport/src/viewport_blueprint.rs +++ b/crates/re_viewport/src/viewport_blueprint.rs @@ -2,7 +2,7 @@ use std::collections::BTreeMap; use re_arrow_store::LatestAtQuery; use re_data_store::EntityPath; -use re_log_types::{DataRow, RowId, TimePoint, Timeline}; +use re_log_types::Timeline; use re_query::query_archetype; use re_viewer_context::{Item, SpaceViewClassIdentifier, SpaceViewId, ViewerContext};